Sign Up
Log In
Log In
or
Sign Up
Places
All Projects
Status Monitor
Collapse sidebar
openSUSE:Leap:15.5:Update
kubevirt.31632
0003-cgroupsv2-reconstruct-device-allowlist.patch
Overview
Repositories
Revisions
Requests
Users
Attributes
Meta
File 0003-cgroupsv2-reconstruct-device-allowlist.patch of Package kubevirt.31632
From c39907f51b515abe552347c7fd96f499dfb1f2e3 Mon Sep 17 00:00:00 2001 From: Vasiliy Ulyanov <vulyanov@suse.de> Date: Tue, 22 Nov 2022 13:36:30 +0100 Subject: [PATCH 1/4] cgroup v2: Allow VMI's block volume devices When a block volume is non-hotpluggable (i.e. it is specified explicitly in the VMI spec), the device cgroup permissions are managed purely by Kubernetes and CRI. For v2, that means a BPF program is assigned to the POD's cgroup. However, when we manage hotplug volumes, we overwrite the BPF program to allow access to the new block device. The problem is that we do not know what the existing BPF program does, hence we just follow some assumptions about the 'default' devices that we need to allow (e.g. /dev/kvm and some others). We need to also consider the non-hotpluggable volumes, otherwise a VM with a block PVC or DV will fail to start if a hotplug volume is attached to it. Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de> Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> --- pkg/virt-handler/cgroup/BUILD.bazel | 2 + pkg/virt-handler/cgroup/cgroup.go | 20 ++++-- pkg/virt-handler/cgroup/cgroup_test.go | 7 +- pkg/virt-handler/cgroup/cgroup_v2_manager.go | 12 +++- pkg/virt-handler/cgroup/util.go | 70 ++++++++++++++++++++ 5 files changed, 100 insertions(+), 11 deletions(-) diff --git a/pkg/virt-handler/cgroup/BUILD.bazel b/pkg/virt-handler/cgroup/BUILD.bazel index 54e735c6c..6480a1ecc 100644 --- a/pkg/virt-handler/cgroup/BUILD.bazel +++ b/pkg/virt-handler/cgroup/BUILD.bazel @@ -12,6 +12,7 @@ go_library( importpath = "kubevirt.io/kubevirt/pkg/virt-handler/cgroup", visibility = ["//visibility:public"], deps = [ + "//pkg/safepath:go_default_library", "//pkg/util:go_default_library", "//pkg/virt-handler/isolation:go_default_library", "//staging/src/kubevirt.io/api/core/v1:go_default_library", @@ -23,6 +24,7 @@ go_library( "//vendor/github.com/opencontainers/runc/libcontainer/cgroups/fs2:go_default_library", "//vendor/github.com/opencontainers/runc/libcontainer/configs:go_default_library", "//vendor/github.com/opencontainers/runc/libcontainer/devices:go_default_library", + "//vendor/golang.org/x/sys/unix:go_default_library", ], ) diff --git a/pkg/virt-handler/cgroup/cgroup.go b/pkg/virt-handler/cgroup/cgroup.go index b0b28a046..d881458cc 100644 --- a/pkg/virt-handler/cgroup/cgroup.go +++ b/pkg/virt-handler/cgroup/cgroup.go @@ -30,6 +30,7 @@ import ( runc_cgroups "github.com/opencontainers/runc/libcontainer/cgroups" "github.com/opencontainers/runc/libcontainer/configs" + "github.com/opencontainers/runc/libcontainer/devices" v1 "kubevirt.io/api/core/v1" @@ -89,9 +90,9 @@ func managerPath(taskPath string) string { return retPath } -// NewManagerFromPid initializes a new cgroup manager from VMI's pid. +// newManagerFromPid initializes a new cgroup manager from VMI's pid. // The pid is expected to VMI's pid from the host's viewpoint. -func NewManagerFromPid(pid int) (manager Manager, err error) { +func newManagerFromPid(pid int, deviceRules []*devices.Rule) (manager Manager, err error) { const isRootless = false var version CgroupVersion @@ -102,9 +103,11 @@ func NewManagerFromPid(pid int) (manager Manager, err error) { } config := &configs.Cgroup{ - Path: HostCgroupBasePath, - Resources: &configs.Resources{}, - Rootless: isRootless, + Path: HostCgroupBasePath, + Resources: &configs.Resources{ + Devices: deviceRules, + }, + Rootless: isRootless, } if runc_cgroups.IsCgroup2UnifiedMode() { @@ -140,7 +143,12 @@ func NewManagerFromVM(vmi *v1.VirtualMachineInstance) (Manager, error) { return nil, err } - return NewManagerFromPid(isolationRes.Pid()) + vmiDeviceRules, err := generateDeviceRulesForVMI(vmi, isolationRes) + if err != nil { + return nil, err + } + + return newManagerFromPid(isolationRes.Pid(), vmiDeviceRules) } // GetGlobalCpuSetPath returns the CPU set of the main cgroup slice diff --git a/pkg/virt-handler/cgroup/cgroup_test.go b/pkg/virt-handler/cgroup/cgroup_test.go index 3f982b484..7ac19d33e 100644 --- a/pkg/virt-handler/cgroup/cgroup_test.go +++ b/pkg/virt-handler/cgroup/cgroup_test.go @@ -43,7 +43,7 @@ var _ = Describe("cgroup manager", func() { if version == V1 { return newCustomizedV1Manager(mockRuncCgroupManager, false, execVirtChrootFunc, getCurrentlyDefinedRulesFunc) } else { - return newCustomizedV2Manager(mockRuncCgroupManager, false, execVirtChrootFunc) + return newCustomizedV2Manager(mockRuncCgroupManager, false, nil, execVirtChrootFunc) } } @@ -85,10 +85,11 @@ var _ = Describe("cgroup manager", func() { Expect(rulesDefined).To(ContainElement(fakeRule), "defined rule is expected to exist") - for _, defaultRule := range GenerateDefaultDeviceRules() { + defaultDeviceRules := GenerateDefaultDeviceRules() + for _, defaultRule := range defaultDeviceRules { Expect(rulesDefined).To(ContainElement(defaultRule), "default rules are expected to be defined") } - + Expect(rulesDefined).To(HaveLen(len(defaultDeviceRules) + 1)) }, Entry("for v1", V1), Entry("for v2", V2), diff --git a/pkg/virt-handler/cgroup/cgroup_v2_manager.go b/pkg/virt-handler/cgroup/cgroup_v2_manager.go index 4b8bf63f0..989c004e8 100644 --- a/pkg/virt-handler/cgroup/cgroup_v2_manager.go +++ b/pkg/virt-handler/cgroup/cgroup_v2_manager.go @@ -22,6 +22,7 @@ type v2Manager struct { runc_cgroups.Manager dirPath string isRootless bool + deviceRules []*devices.Rule execVirtChroot execVirtChrootFunc } @@ -31,14 +32,20 @@ func newV2Manager(config *runc_configs.Cgroup, dirPath string) (Manager, error) return nil, err } - return newCustomizedV2Manager(runcManager, config.Rootless, execVirtChrootCgroups) + return newCustomizedV2Manager(runcManager, config.Rootless, config.Resources.Devices, execVirtChrootCgroups) } -func newCustomizedV2Manager(runcManager runc_cgroups.Manager, isRootless bool, execVirtChroot execVirtChrootFunc) (Manager, error) { +func newCustomizedV2Manager( + runcManager runc_cgroups.Manager, + isRootless bool, + deviceRules []*devices.Rule, + execVirtChroot execVirtChrootFunc, +) (Manager, error) { manager := v2Manager{ runcManager, runcManager.GetPaths()[""], isRootless, + deviceRules, execVirtChroot, } @@ -55,6 +62,7 @@ func (v *v2Manager) Set(r *runc_configs.Resources) error { //Add default rules resourcesToSet.Devices = append(resourcesToSet.Devices, GenerateDefaultDeviceRules()...) + resourcesToSet.Devices = append(resourcesToSet.Devices, v.deviceRules...) rulesToSet, err := addCurrentRules(rulesPerPid[v.dirPath], resourcesToSet.Devices) if err != nil { diff --git a/pkg/virt-handler/cgroup/util.go b/pkg/virt-handler/cgroup/util.go index f088dc4ce..511ea7ca2 100644 --- a/pkg/virt-handler/cgroup/util.go +++ b/pkg/virt-handler/cgroup/util.go @@ -4,21 +4,28 @@ import ( "bufio" "encoding/base64" "encoding/json" + "errors" "fmt" "os" "os/exec" "path/filepath" "strconv" "strings" + "syscall" "github.com/opencontainers/runc/libcontainer/cgroups" + "golang.org/x/sys/unix" "github.com/opencontainers/runc/libcontainer/devices" runc_cgroups "github.com/opencontainers/runc/libcontainer/cgroups" runc_configs "github.com/opencontainers/runc/libcontainer/configs" + v1 "kubevirt.io/api/core/v1" "kubevirt.io/client-go/log" + + "kubevirt.io/kubevirt/pkg/safepath" + "kubevirt.io/kubevirt/pkg/virt-handler/isolation" ) type CgroupVersion string @@ -84,6 +91,69 @@ func addCurrentRules(currentRules, newRules []*devices.Rule) ([]*devices.Rule, e return newRules, nil } +// This builds up the known persistent block devices allow list for a VMI (as in, hotplugged volumes are handled separately) +// This will be maintained and extended as new devices likely have to end up on this list as well +// For example - https://kubernetes.io/docs/concepts/scheduling-eviction/dynamic-resource-allocation/ +func generateDeviceRulesForVMI(vmi *v1.VirtualMachineInstance, isolationRes isolation.IsolationResult) ([]*devices.Rule, error) { + mountRoot, err := isolationRes.MountRoot() + if err != nil { + return nil, err + } + + var vmiDeviceRules []*devices.Rule + for _, volume := range vmi.Spec.Volumes { + switch { + case volume.VolumeSource.PersistentVolumeClaim != nil: + if volume.VolumeSource.PersistentVolumeClaim.Hotpluggable { + continue + } + case volume.VolumeSource.DataVolume != nil: + if volume.VolumeSource.DataVolume.Hotpluggable { + continue + } + case volume.VolumeSource.Ephemeral != nil: + default: + continue + } + path, err := safepath.JoinNoFollow(mountRoot, filepath.Join("dev", volume.Name)) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + continue + } + return nil, fmt.Errorf("failed to resolve path for volume %s: %v", volume.Name, err) + } + if deviceRule, err := newAllowedDeviceRule(path); err != nil { + return nil, fmt.Errorf("failed to create device rule for %s: %v", path, err) + } else if deviceRule != nil { + log.Log.V(loggingVerbosity).Infof("device rule for volume %s: %v", volume.Name, deviceRule) + vmiDeviceRules = append(vmiDeviceRules, deviceRule) + } + } + return vmiDeviceRules, nil +} + +func newAllowedDeviceRule(devicePath *safepath.Path) (*devices.Rule, error) { + fileInfo, err := safepath.StatAtNoFollow(devicePath) + if err != nil { + return nil, err + } + if (fileInfo.Mode() & os.ModeDevice) == 0 { + return nil, nil //not a device file + } + deviceType := devices.BlockDevice + if (fileInfo.Mode() & os.ModeCharDevice) != 0 { + deviceType = devices.CharDevice + } + stat := fileInfo.Sys().(*syscall.Stat_t) + return &devices.Rule{ + Type: deviceType, + Major: int64(unix.Major(stat.Rdev)), + Minor: int64(unix.Minor(stat.Rdev)), + Permissions: "rwm", + Allow: true, + }, nil +} + func GenerateDefaultDeviceRules() []*devices.Rule { if len(defaultDeviceRules) > 0 { // To avoid re-computing default device rules -- 2.42.1 From 8d6155cd942a29ec7453065f93f913991f877895 Mon Sep 17 00:00:00 2001 From: Vasiliy Ulyanov <vulyanov@suse.de> Date: Mon, 28 Nov 2022 09:24:12 +0100 Subject: [PATCH 2/4] Hotplug volume mounter: set cgroup manager as arg Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de> Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> --- pkg/virt-handler/BUILD.bazel | 1 + .../hotplug-disk/generated_mock_mount.go | 34 ++--- pkg/virt-handler/hotplug-disk/mount.go | 133 ++++++++---------- pkg/virt-handler/hotplug-disk/mount_test.go | 22 ++- pkg/virt-handler/vm.go | 40 ++++-- pkg/virt-handler/vm_test.go | 81 ++++++----- 6 files changed, 153 insertions(+), 158 deletions(-) diff --git a/pkg/virt-handler/BUILD.bazel b/pkg/virt-handler/BUILD.bazel index a6ae28ebe..9c59fcf30 100644 --- a/pkg/virt-handler/BUILD.bazel +++ b/pkg/virt-handler/BUILD.bazel @@ -99,6 +99,7 @@ go_test( "//pkg/virt-config:go_default_library", "//pkg/virt-controller/services:go_default_library", "//pkg/virt-handler/cache:go_default_library", + "//pkg/virt-handler/cgroup:go_default_library", "//pkg/virt-handler/cmd-client:go_default_library", "//pkg/virt-handler/container-disk:go_default_library", "//pkg/virt-handler/hotplug-disk:go_default_library", diff --git a/pkg/virt-handler/hotplug-disk/generated_mock_mount.go b/pkg/virt-handler/hotplug-disk/generated_mock_mount.go index ef1432f2f..1ed10bc9a 100644 --- a/pkg/virt-handler/hotplug-disk/generated_mock_mount.go +++ b/pkg/virt-handler/hotplug-disk/generated_mock_mount.go @@ -7,6 +7,8 @@ import ( gomock "github.com/golang/mock/gomock" types "k8s.io/apimachinery/pkg/types" v1 "kubevirt.io/api/core/v1" + + cgroup "kubevirt.io/kubevirt/pkg/virt-handler/cgroup" ) // Mock of VolumeMounter interface @@ -30,44 +32,44 @@ func (_m *MockVolumeMounter) EXPECT() *_MockVolumeMounterRecorder { return _m.recorder } -func (_m *MockVolumeMounter) Mount(vmi *v1.VirtualMachineInstance) error { - ret := _m.ctrl.Call(_m, "Mount", vmi) +func (_m *MockVolumeMounter) Mount(vmi *v1.VirtualMachineInstance, cgroupManager cgroup.Manager) error { + ret := _m.ctrl.Call(_m, "Mount", vmi, cgroupManager) ret0, _ := ret[0].(error) return ret0 } -func (_mr *_MockVolumeMounterRecorder) Mount(arg0 interface{}) *gomock.Call { - return _mr.mock.ctrl.RecordCall(_mr.mock, "Mount", arg0) +func (_mr *_MockVolumeMounterRecorder) Mount(arg0, arg1 interface{}) *gomock.Call { + return _mr.mock.ctrl.RecordCall(_mr.mock, "Mount", arg0, arg1) } -func (_m *MockVolumeMounter) MountFromPod(vmi *v1.VirtualMachineInstance, sourceUID types.UID) error { - ret := _m.ctrl.Call(_m, "MountFromPod", vmi, sourceUID) +func (_m *MockVolumeMounter) MountFromPod(vmi *v1.VirtualMachineInstance, sourceUID types.UID, cgroupManager cgroup.Manager) error { + ret := _m.ctrl.Call(_m, "MountFromPod", vmi, sourceUID, cgroupManager) ret0, _ := ret[0].(error) return ret0 } -func (_mr *_MockVolumeMounterRecorder) MountFromPod(arg0, arg1 interface{}) *gomock.Call { - return _mr.mock.ctrl.RecordCall(_mr.mock, "MountFromPod", arg0, arg1) +func (_mr *_MockVolumeMounterRecorder) MountFromPod(arg0, arg1, arg2 interface{}) *gomock.Call { + return _mr.mock.ctrl.RecordCall(_mr.mock, "MountFromPod", arg0, arg1, arg2) } -func (_m *MockVolumeMounter) Unmount(vmi *v1.VirtualMachineInstance) error { - ret := _m.ctrl.Call(_m, "Unmount", vmi) +func (_m *MockVolumeMounter) Unmount(vmi *v1.VirtualMachineInstance, cgroupManager cgroup.Manager) error { + ret := _m.ctrl.Call(_m, "Unmount", vmi, cgroupManager) ret0, _ := ret[0].(error) return ret0 } -func (_mr *_MockVolumeMounterRecorder) Unmount(arg0 interface{}) *gomock.Call { - return _mr.mock.ctrl.RecordCall(_mr.mock, "Unmount", arg0) +func (_mr *_MockVolumeMounterRecorder) Unmount(arg0, arg1 interface{}) *gomock.Call { + return _mr.mock.ctrl.RecordCall(_mr.mock, "Unmount", arg0, arg1) } -func (_m *MockVolumeMounter) UnmountAll(vmi *v1.VirtualMachineInstance) error { - ret := _m.ctrl.Call(_m, "UnmountAll", vmi) +func (_m *MockVolumeMounter) UnmountAll(vmi *v1.VirtualMachineInstance, cgroupManager cgroup.Manager) error { + ret := _m.ctrl.Call(_m, "UnmountAll", vmi, cgroupManager) ret0, _ := ret[0].(error) return ret0 } -func (_mr *_MockVolumeMounterRecorder) UnmountAll(arg0 interface{}) *gomock.Call { - return _mr.mock.ctrl.RecordCall(_mr.mock, "UnmountAll", arg0) +func (_mr *_MockVolumeMounterRecorder) UnmountAll(arg0, arg1 interface{}) *gomock.Call { + return _mr.mock.ctrl.RecordCall(_mr.mock, "UnmountAll", arg0, arg1) } func (_m *MockVolumeMounter) IsMounted(vmi *v1.VirtualMachineInstance, volume string, sourceUID types.UID) (bool, error) { diff --git a/pkg/virt-handler/hotplug-disk/mount.go b/pkg/virt-handler/hotplug-disk/mount.go index 0dce62c32..65534de8e 100644 --- a/pkg/virt-handler/hotplug-disk/mount.go +++ b/pkg/virt-handler/hotplug-disk/mount.go @@ -34,8 +34,7 @@ import ( //go:generate mockgen -source $GOFILE -package=$GOPACKAGE -destination=generated_mock_$GOFILE const ( - unableFindHotplugMountedDir = "unable to find hotplug mounted directories for vmi without uid" - failedToCreateCgroupManagerErrTemplate = "could not create cgroup manager. err: %v" + unableFindHotplugMountedDir = "unable to find hotplug mounted directories for vmi without uid" ) var ( @@ -118,10 +117,6 @@ var ( return isolation.NewSocketBasedIsolationDetector(path) } - getCgroupManager = func(vmi *v1.VirtualMachineInstance) (cgroup.Manager, error) { - return cgroup.NewManagerFromVM(vmi) - } - parentPathForMount = func( parent isolation.IsolationResult, child isolation.IsolationResult, @@ -143,12 +138,12 @@ type volumeMounter struct { // VolumeMounter is the interface used to mount and unmount volumes to/from a running virtlauncher pod. type VolumeMounter interface { // Mount any new volumes defined in the VMI - Mount(vmi *v1.VirtualMachineInstance) error - MountFromPod(vmi *v1.VirtualMachineInstance, sourceUID types.UID) error + Mount(vmi *v1.VirtualMachineInstance, cgroupManager cgroup.Manager) error + MountFromPod(vmi *v1.VirtualMachineInstance, sourceUID types.UID, cgroupManager cgroup.Manager) error // Unmount any volumes no longer defined in the VMI - Unmount(vmi *v1.VirtualMachineInstance) error + Unmount(vmi *v1.VirtualMachineInstance, cgroupManager cgroup.Manager) error //UnmountAll cleans up all hotplug volumes - UnmountAll(vmi *v1.VirtualMachineInstance) error + UnmountAll(vmi *v1.VirtualMachineInstance, cgroupManager cgroup.Manager) error //IsMounted returns if the volume is mounted or not. IsMounted(vmi *v1.VirtualMachineInstance, volume string, sourceUID types.UID) (bool, error) } @@ -302,13 +297,20 @@ func (m *volumeMounter) writePathToMountRecord(path string, vmi *v1.VirtualMachi return nil } -func (m *volumeMounter) mountHotplugVolume(vmi *v1.VirtualMachineInstance, volumeName string, sourceUID types.UID, record *vmiMountTargetRecord, mountDirectory bool) error { +func (m *volumeMounter) mountHotplugVolume( + vmi *v1.VirtualMachineInstance, + volumeName string, + sourceUID types.UID, + record *vmiMountTargetRecord, + mountDirectory bool, + cgroupManager cgroup.Manager, +) error { logger := log.DefaultLogger() logger.V(4).Infof("Hotplug check volume name: %s", volumeName) if sourceUID != types.UID("") { if m.isBlockVolume(&vmi.Status, volumeName) { logger.V(4).Infof("Mounting block volume: %s", volumeName) - if err := m.mountBlockHotplugVolume(vmi, volumeName, sourceUID, record); err != nil { + if err := m.mountBlockHotplugVolume(vmi, volumeName, sourceUID, record, cgroupManager); err != nil { if !errors.Is(err, os.ErrNotExist) { return fmt.Errorf("failed to mount block hotplug volume %s: %v", volumeName, err) } @@ -325,15 +327,15 @@ func (m *volumeMounter) mountHotplugVolume(vmi *v1.VirtualMachineInstance, volum return nil } -func (m *volumeMounter) Mount(vmi *v1.VirtualMachineInstance) error { - return m.mountFromPod(vmi, types.UID("")) +func (m *volumeMounter) Mount(vmi *v1.VirtualMachineInstance, cgroupManager cgroup.Manager) error { + return m.mountFromPod(vmi, types.UID(""), cgroupManager) } -func (m *volumeMounter) MountFromPod(vmi *v1.VirtualMachineInstance, sourceUID types.UID) error { - return m.mountFromPod(vmi, sourceUID) +func (m *volumeMounter) MountFromPod(vmi *v1.VirtualMachineInstance, sourceUID types.UID, cgroupManager cgroup.Manager) error { + return m.mountFromPod(vmi, sourceUID, cgroupManager) } -func (m *volumeMounter) mountFromPod(vmi *v1.VirtualMachineInstance, sourceUID types.UID) error { +func (m *volumeMounter) mountFromPod(vmi *v1.VirtualMachineInstance, sourceUID types.UID, cgroupManager cgroup.Manager) error { record, err := m.getMountTargetRecord(vmi) if err != nil { return err @@ -350,7 +352,7 @@ func (m *volumeMounter) mountFromPod(vmi *v1.VirtualMachineInstance, sourceUID t if sourceUID == types.UID("") { sourceUID = volumeStatus.HotplugVolume.AttachPodUID } - if err := m.mountHotplugVolume(vmi, volumeStatus.Name, sourceUID, record, mountDirectory); err != nil { + if err := m.mountHotplugVolume(vmi, volumeStatus.Name, sourceUID, record, mountDirectory, cgroupManager); err != nil { return err } } @@ -378,7 +380,13 @@ func (m *volumeMounter) isBlockVolume(vmiStatus *v1.VirtualMachineInstanceStatus return false } -func (m *volumeMounter) mountBlockHotplugVolume(vmi *v1.VirtualMachineInstance, volume string, sourceUID types.UID, record *vmiMountTargetRecord) error { +func (m *volumeMounter) mountBlockHotplugVolume( + vmi *v1.VirtualMachineInstance, + volume string, + sourceUID types.UID, + record *vmiMountTargetRecord, + cgroupManager cgroup.Manager, +) error { virtlauncherUID := m.findVirtlauncherUID(vmi) if virtlauncherUID == "" { // This is not the node the pod is running on. @@ -389,11 +397,6 @@ func (m *volumeMounter) mountBlockHotplugVolume(vmi *v1.VirtualMachineInstance, return err } - cgroupsManager, err := getCgroupManager(vmi) - if err != nil { - return fmt.Errorf(failedToCreateCgroupManagerErrTemplate, err) - } - if _, err := safepath.JoinNoFollow(targetPath, volume); errors.Is(err, os.ErrNotExist) { dev, permissions, err := m.getSourceMajorMinor(sourceUID, volume) if err != nil { @@ -422,36 +425,18 @@ func (m *volumeMounter) mountBlockHotplugVolume(vmi *v1.VirtualMachineInstance, return fmt.Errorf("target device %v exists but it is not a block device", devicePath) } - isMigrationInProgress := vmi.Status.MigrationState != nil && !vmi.Status.MigrationState.Completed - volumeNotReady := !m.volumeStatusReady(volume, vmi) - - if isMigrationInProgress || volumeNotReady { - dev, _, err := m.getSourceMajorMinor(sourceUID, volume) - if err != nil { - return err - } - // allow block devices - if err := m.allowBlockMajorMinor(dev, cgroupsManager); err != nil { - return err - } + dev, _, err := m.getBlockFileMajorMinor(devicePath, statDevice) + if err != nil { + return err + } + // allow block devices + if err := m.allowBlockMajorMinor(dev, cgroupManager); err != nil { + return err } return m.ownershipManager.SetFileOwnership(devicePath) } -func (m *volumeMounter) volumeStatusReady(volumeName string, vmi *v1.VirtualMachineInstance) bool { - for _, volumeStatus := range vmi.Status.VolumeStatus { - if volumeStatus.Name == volumeName && volumeStatus.HotplugVolume != nil { - if volumeStatus.Phase != v1.VolumeReady { - log.DefaultLogger().V(4).Infof("Volume %s is not ready, but the target block device exists", volumeName) - } - return volumeStatus.Phase == v1.VolumeReady - } - } - // This should never happen, it should always find the volume status in the VMI. - return true -} - func (m *volumeMounter) getSourceMajorMinor(sourceUID types.UID, volumeName string) (uint64, os.FileMode, error) { basePath, err := deviceBasePath(sourceUID) if err != nil { @@ -473,17 +458,15 @@ func (m *volumeMounter) getBlockFileMajorMinor(devicePath *safepath.Path, getter return info.Rdev, fileInfo.Mode(), nil } -func (m *volumeMounter) removeBlockMajorMinor(dev uint64, manager cgroup.Manager) error { - return m.updateBlockMajorMinor(dev, false, manager) +func (m *volumeMounter) removeBlockMajorMinor(dev uint64, cgroupManager cgroup.Manager) error { + return m.updateBlockMajorMinor(dev, false, cgroupManager) } -func (m *volumeMounter) allowBlockMajorMinor(dev uint64, manager cgroup.Manager) error { - return m.updateBlockMajorMinor(dev, true, manager) +func (m *volumeMounter) allowBlockMajorMinor(dev uint64, cgroupManager cgroup.Manager) error { + return m.updateBlockMajorMinor(dev, true, cgroupManager) } -func (m *volumeMounter) updateBlockMajorMinor(dev uint64, allow bool, manager cgroup.Manager) error { - - var err error +func (m *volumeMounter) updateBlockMajorMinor(dev uint64, allow bool, cgroupManager cgroup.Manager) error { deviceRule := &devices.Rule{ Type: devices.BlockDevice, Major: int64(unix.Major(dev)), @@ -492,14 +475,18 @@ func (m *volumeMounter) updateBlockMajorMinor(dev uint64, allow bool, manager cg Allow: allow, } - err = manager.Set(&configs.Resources{ + if cgroupManager == nil { + return fmt.Errorf("failed to apply device rule %+v: cgroup manager is nil", *deviceRule) + } + + err := cgroupManager.Set(&configs.Resources{ Devices: []*devices.Rule{deviceRule}, }) if err != nil { - log.Log.Infof("cgroup %s had failed to set device rule. error: %v. rule: %+v", manager.GetCgroupVersion(), err, *deviceRule) + log.Log.Errorf("cgroup %s had failed to set device rule. error: %v. rule: %+v", cgroupManager.GetCgroupVersion(), err, *deviceRule) } else { - log.Log.Infof("cgroup %s device rule is set successfully. rule: %+v", manager.GetCgroupVersion(), *deviceRule) + log.Log.Infof("cgroup %s device rule is set successfully. rule: %+v", cgroupManager.GetCgroupVersion(), *deviceRule) } return err @@ -632,7 +619,7 @@ func (m *volumeMounter) getSourcePodFilePath(sourceUID types.UID, vmi *v1.Virtua } // Unmount unmounts all hotplug disk that are no longer part of the VMI -func (m *volumeMounter) Unmount(vmi *v1.VirtualMachineInstance) error { +func (m *volumeMounter) Unmount(vmi *v1.VirtualMachineInstance, cgroupManager cgroup.Manager) error { if vmi.UID != "" { record, err := m.getMountTargetRecord(vmi) if err != nil { @@ -704,7 +691,7 @@ func (m *volumeMounter) Unmount(vmi *v1.VirtualMachineInstance) error { if blockDevice, err := isBlockDevice(diskPath); err != nil { return err } else if blockDevice { - if err := m.unmountBlockHotplugVolumes(diskPath, vmi); err != nil { + if err := m.unmountBlockHotplugVolumes(diskPath, cgroupManager); err != nil { return err } } else if err := m.unmountFileSystemHotplugVolumes(diskPath); err != nil { @@ -745,12 +732,7 @@ func (m *volumeMounter) unmountFileSystemHotplugVolumes(diskPath *safepath.Path) return nil } -func (m *volumeMounter) unmountBlockHotplugVolumes(diskPath *safepath.Path, vmi *v1.VirtualMachineInstance) error { - cgroupsManager, err := getCgroupManager(vmi) - if err != nil { - return fmt.Errorf(failedToCreateCgroupManagerErrTemplate, err) - } - +func (m *volumeMounter) unmountBlockHotplugVolumes(diskPath *safepath.Path, cgroupManager cgroup.Manager) error { // Get major and minor so we can deny the container. dev, _, err := m.getBlockFileMajorMinor(diskPath, statDevice) if err != nil { @@ -760,14 +742,11 @@ func (m *volumeMounter) unmountBlockHotplugVolumes(diskPath *safepath.Path, vmi if err := safepath.UnlinkAtNoFollow(diskPath); err != nil { return err } - if err := m.removeBlockMajorMinor(dev, cgroupsManager); err != nil { - return err - } - return nil + return m.removeBlockMajorMinor(dev, cgroupManager) } // UnmountAll unmounts all hotplug disks of a given VMI. -func (m *volumeMounter) UnmountAll(vmi *v1.VirtualMachineInstance) error { +func (m *volumeMounter) UnmountAll(vmi *v1.VirtualMachineInstance, cgroupManager cgroup.Manager) error { if vmi.UID != "" { logger := log.DefaultLogger().Object(vmi) logger.Info("Cleaning up remaining hotplug volumes") @@ -787,20 +766,20 @@ func (m *volumeMounter) UnmountAll(vmi *v1.VirtualMachineInstance) error { logger.Infof("Device %v is not mounted anymore, continuing.", entry.TargetFile) continue } - logger.Infof("Unable to unmount volume at path %s: %v", entry.TargetFile, err) + logger.Warningf("Unable to unmount volume at path %s: %v", entry.TargetFile, err) continue } diskPath.Close() if isBlock, err := isBlockDevice(diskPath.Path()); err != nil { - logger.Infof("Unable to remove block device at path %s: %v", diskPath, err) + logger.Warningf("Unable to unmount volume at path %s: %v", diskPath, err) } else if isBlock { - if err := m.unmountBlockHotplugVolumes(diskPath.Path(), vmi); err != nil { - logger.Infof("Unable to remove block device at path %s: %v", diskPath, err) + if err := m.unmountBlockHotplugVolumes(diskPath.Path(), cgroupManager); err != nil { + logger.Warningf("Unable to remove block device at path %s: %v", diskPath, err) // Don't return error, try next. } } else { if err := m.unmountFileSystemHotplugVolumes(diskPath.Path()); err != nil { - logger.Infof("Unable to unmount volume at path %s: %v", diskPath, err) + logger.Warningf("Unable to unmount volume at path %s: %v", diskPath, err) // Don't return error, try next. } } diff --git a/pkg/virt-handler/hotplug-disk/mount_test.go b/pkg/virt-handler/hotplug-disk/mount_test.go index 9737eaa09..b1a180656 100644 --- a/pkg/virt-handler/hotplug-disk/mount_test.go +++ b/pkg/virt-handler/hotplug-disk/mount_test.go @@ -106,10 +106,6 @@ var _ = Describe("HotplugVolume", func() { rule1.Allow == rule2.Allow } - getCgroupManager = func(_ *v1.VirtualMachineInstance) (cgroup.Manager, error) { - return cgroupManagerMock, nil - } - cgroupMockSet := func(r *runc_configs.Resources) { if expectedCgroupRule == nil { return @@ -372,12 +368,12 @@ var _ = Describe("HotplugVolume", func() { By("Mounting and validating expected rule is set") setExpectedCgroupRuns(2) expectCgroupRule(devices.BlockDevice, 482, 64, true) - err = m.mountBlockHotplugVolume(vmi, "testvolume", blockSourcePodUID, record) + err = m.mountBlockHotplugVolume(vmi, "testvolume", blockSourcePodUID, record, cgroupManagerMock) Expect(err).ToNot(HaveOccurred()) By("Unmounting, we verify the reverse process happens") expectCgroupRule(devices.BlockDevice, 482, 64, false) - err = m.unmountBlockHotplugVolumes(deviceFile, vmi) + err = m.unmountBlockHotplugVolumes(deviceFile, cgroupManagerMock) Expect(err).ToNot(HaveOccurred()) }) @@ -453,7 +449,7 @@ var _ = Describe("HotplugVolume", func() { By("Mounting and validating expected rule is set") setExpectedCgroupRuns(1) expectCgroupRule(devices.BlockDevice, 482, 64, false) - err = m.unmountBlockHotplugVolumes(deviceFileName, vmi) + err = m.unmountBlockHotplugVolumes(deviceFileName, cgroupManagerMock) Expect(err).ToNot(HaveOccurred()) }) @@ -464,7 +460,7 @@ var _ = Describe("HotplugVolume", func() { deviceFileName, err := newFile(tempDir, "devicefile") Expect(err).ToNot(HaveOccurred()) os.Remove(unsafepath.UnsafeAbsolute(deviceFileName.Raw())) - err = m.unmountBlockHotplugVolumes(deviceFileName, vmi) + err = m.unmountBlockHotplugVolumes(deviceFileName, cgroupManagerMock) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("no such file or directory")) }) @@ -837,7 +833,7 @@ var _ = Describe("HotplugVolume", func() { return nil }) - err = m.Mount(vmi) + err = m.Mount(vmi, cgroupManagerMock) Expect(err).ToNot(HaveOccurred()) By("Verifying there are 2 records in tempDir/1234") record := &vmiMountTargetRecord{ @@ -867,7 +863,7 @@ var _ = Describe("HotplugVolume", func() { Name: "permanent", }) vmi.Status.VolumeStatus = volumeStatuses - err = m.Unmount(vmi) + err = m.Unmount(vmi, cgroupManagerMock) Expect(err).ToNot(HaveOccurred()) _, err = os.ReadFile(filepath.Join(tempDir, string(vmi.UID))) Expect(err).To(HaveOccurred(), "record file still exists %s", filepath.Join(tempDir, string(vmi.UID))) @@ -883,7 +879,7 @@ var _ = Describe("HotplugVolume", func() { Name: "permanent", }) vmi.Status.VolumeStatus = volumeStatuses - Expect(m.Mount(vmi)).To(Succeed()) + Expect(m.Mount(vmi, cgroupManagerMock)).To(Succeed()) }) It("unmountAll should cleanup regardless of vmi volumestatuses", func() { @@ -961,7 +957,7 @@ var _ = Describe("HotplugVolume", func() { return nil }) - err = m.Mount(vmi) + err = m.Mount(vmi, cgroupManagerMock) Expect(err).ToNot(HaveOccurred()) By("Verifying there are 2 records in tempDir/1234") @@ -985,7 +981,7 @@ var _ = Describe("HotplugVolume", func() { Expect(err).ToNot(HaveOccurred()) Expect(capturedPaths).To(ContainElements(expectedPaths)) - err = m.UnmountAll(vmi) + err = m.UnmountAll(vmi, cgroupManagerMock) Expect(err).ToNot(HaveOccurred()) _, err = os.ReadFile(filepath.Join(tempDir, string(vmi.UID))) Expect(err).To(HaveOccurred(), "record file still exists %s", filepath.Join(tempDir, string(vmi.UID))) diff --git a/pkg/virt-handler/vm.go b/pkg/virt-handler/vm.go index 7b4e50def..38c1762c8 100644 --- a/pkg/virt-handler/vm.go +++ b/pkg/virt-handler/vm.go @@ -181,6 +181,10 @@ var PasswordRelatedGuestAgentCommands = []string{ "guest-set-user-password", } +var getCgroupManager = func(vmi *v1.VirtualMachineInstance) (cgroup.Manager, error) { + return cgroup.NewManagerFromVM(vmi) +} + func NewController( recorder record.EventRecorder, clientset kubecli.KubevirtClient, @@ -2072,7 +2076,11 @@ func (d *VirtualMachineController) processVmCleanup(vmi *v1.VirtualMachineInstan if err := d.containerDiskMounter.Unmount(vmi); err != nil { return err } - if err := d.hotplugVolumeMounter.UnmountAll(vmi); err != nil { + + // UnmountAll does the cleanup on the "best effort" basis: it is + // safe to pass a nil cgroupManager. + cgroupManager, _ := getCgroupManager(vmi) + if err := d.hotplugVolumeMounter.UnmountAll(vmi, cgroupManager); err != nil { return err } @@ -2689,7 +2697,11 @@ func (d *VirtualMachineController) vmUpdateHelperMigrationTarget(origVMI *v1.Vir // Mount hotplug disks if attachmentPodUID := vmi.Status.MigrationState.TargetAttachmentPodUID; attachmentPodUID != types.UID("") { - if err := d.hotplugVolumeMounter.MountFromPod(vmi, attachmentPodUID); err != nil { + cgroupManager, err := getCgroupManager(vmi) + if err != nil { + return err + } + if err := d.hotplugVolumeMounter.MountFromPod(vmi, attachmentPodUID, cgroupManager); err != nil { return fmt.Errorf("failed to mount hotplug volumes: %v", err) } } @@ -2799,14 +2811,8 @@ func (d *VirtualMachineController) affinePitThread(vmi *v1.VirtualMachineInstanc return unix.SchedSetaffinity(pitpid, &Mask) } -func (d *VirtualMachineController) configureHousekeepingCgroup(vmi *v1.VirtualMachineInstance) error { - cgroupManager, err := cgroup.NewManagerFromVM(vmi) - if err != nil { - return err - } - - err = cgroupManager.CreateChildCgroup("housekeeping", "cpuset") - if err != nil { +func (d *VirtualMachineController) configureHousekeepingCgroup(vmi *v1.VirtualMachineInstance, cgroupManager cgroup.Manager) error { + if err := cgroupManager.CreateChildCgroup("housekeeping", "cpuset"); err != nil { log.Log.Reason(err).Error("CreateChildCgroup ") return err } @@ -2888,6 +2894,10 @@ func (d *VirtualMachineController) vmUpdateHelperDefault(origVMI *v1.VirtualMach return err } + cgroupManager, err := getCgroupManager(vmi) + if err != nil { + return err + } var errorTolerantFeaturesError []error disksInfo := map[string]*containerdisk.DiskInfo{} if !vmi.IsRunning() && !vmi.IsFinal() { @@ -2907,7 +2917,7 @@ func (d *VirtualMachineController) vmUpdateHelperDefault(origVMI *v1.VirtualMach } // Try to mount hotplug volume if there is any during startup. - if err := d.hotplugVolumeMounter.Mount(vmi); err != nil { + if err := d.hotplugVolumeMounter.Mount(vmi, cgroupManager); err != nil { return err } @@ -2992,7 +3002,7 @@ func (d *VirtualMachineController) vmUpdateHelperDefault(origVMI *v1.VirtualMach log.Log.Object(vmi).Error(err.Error()) } - if err := d.hotplugVolumeMounter.Mount(vmi); err != nil { + if err := d.hotplugVolumeMounter.Mount(vmi, cgroupManager); err != nil { return err } @@ -3045,7 +3055,7 @@ func (d *VirtualMachineController) vmUpdateHelperDefault(origVMI *v1.VirtualMach } if vmi.IsCPUDedicated() && vmi.Spec.Domain.CPU.IsolateEmulatorThread { - err = d.configureHousekeepingCgroup(vmi) + err = d.configureHousekeepingCgroup(vmi, cgroupManager) if err != nil { return err } @@ -3068,7 +3078,7 @@ func (d *VirtualMachineController) vmUpdateHelperDefault(origVMI *v1.VirtualMach if vmi.IsRunning() { // Umount any disks no longer mounted - if err := d.hotplugVolumeMounter.Unmount(vmi); err != nil { + if err := d.hotplugVolumeMounter.Unmount(vmi, cgroupManager); err != nil { return err } } @@ -3373,7 +3383,7 @@ func (d *VirtualMachineController) claimDeviceOwnership(virtLauncherRootMount *s } func (d *VirtualMachineController) reportDedicatedCPUSetForMigratingVMI(vmi *v1.VirtualMachineInstance) error { - cgroupManager, err := cgroup.NewManagerFromVM(vmi) + cgroupManager, err := getCgroupManager(vmi) if err != nil { return err } diff --git a/pkg/virt-handler/vm_test.go b/pkg/virt-handler/vm_test.go index 540e88392..273d3f30c 100644 --- a/pkg/virt-handler/vm_test.go +++ b/pkg/virt-handler/vm_test.go @@ -47,6 +47,7 @@ import ( netcache "kubevirt.io/kubevirt/pkg/network/cache" neterrors "kubevirt.io/kubevirt/pkg/network/errors" "kubevirt.io/kubevirt/pkg/util" + "kubevirt.io/kubevirt/pkg/virt-handler/cgroup" container_disk "kubevirt.io/kubevirt/pkg/virt-handler/container-disk" hotplug_volume "kubevirt.io/kubevirt/pkg/virt-handler/hotplug-disk" @@ -105,6 +106,7 @@ var _ = Describe("VirtualMachineInstance", func() { var mockIsolationResult *isolation.MockIsolationResult var mockContainerDiskMounter *container_disk.MockMounter var mockHotplugVolumeMounter *hotplug_volume.MockVolumeMounter + var mockCgroupManager *cgroup.MockManager var vmiFeeder *testutils.VirtualMachineFeeder var domainFeeder *testutils.DomainFeeder @@ -128,6 +130,10 @@ var _ = Describe("VirtualMachineInstance", func() { var certDir string + getCgroupManager = func(_ *v1.VirtualMachineInstance) (cgroup.Manager, error) { + return mockCgroupManager, nil + } + BeforeEach(func() { diskutils.MockDefaultOwnershipManager() @@ -205,6 +211,7 @@ var _ = Describe("VirtualMachineInstance", func() { mockContainerDiskMounter = container_disk.NewMockMounter(ctrl) mockHotplugVolumeMounter = hotplug_volume.NewMockVolumeMounter(ctrl) + mockCgroupManager = cgroup.NewMockManager(ctrl) migrationProxy := migrationproxy.NewMigrationProxyManager(tlsConfig, tlsConfig, config) fakeDownwardMetricsManager := newFakeManager() @@ -381,7 +388,7 @@ var _ = Describe("VirtualMachineInstance", func() { Expect(exists).To(BeTrue()) mockQueue.Add(namespace + "/" + name) - mockHotplugVolumeMounter.EXPECT().UnmountAll(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().UnmountAll(gomock.Any(), mockCgroupManager).Return(nil) client.EXPECT().Close() controller.Execute() @@ -556,7 +563,7 @@ var _ = Describe("VirtualMachineInstance", func() { vmiFeeder.Add(vmi) domainFeeder.Add(domain) - mockHotplugVolumeMounter.EXPECT().UnmountAll(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().UnmountAll(gomock.Any(), mockCgroupManager).Return(nil) controller.Execute() Expect(mockQueue.Len()).To(Equal(0)) @@ -573,7 +580,7 @@ var _ = Describe("VirtualMachineInstance", func() { mockWatchdog.CreateFile(vmi) vmiFeeder.Add(vmi) - mockHotplugVolumeMounter.EXPECT().UnmountAll(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().UnmountAll(gomock.Any(), mockCgroupManager).Return(nil) client.EXPECT().Close() controller.Execute() Expect(mockQueue.Len()).To(Equal(0)) @@ -601,7 +608,7 @@ var _ = Describe("VirtualMachineInstance", func() { vmiFeeder.Add(vmi) client.EXPECT().SyncVirtualMachine(vmi, gomock.Any()) - mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any(), mockCgroupManager).Return(nil) controller.Execute() testutils.ExpectEvent(recorder, VMIDefined) @@ -677,7 +684,7 @@ var _ = Describe("VirtualMachineInstance", func() { Expect(options.VirtualMachineSMBios.Product).To(Equal(virtconfig.SmbiosConfigDefaultProduct)) Expect(options.VirtualMachineSMBios.Manufacturer).To(Equal(virtconfig.SmbiosConfigDefaultManufacturer)) }) - mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any(), mockCgroupManager).Return(nil) controller.Execute() testutils.ExpectEvent(recorder, VMIDefined) }) @@ -718,8 +725,8 @@ var _ = Describe("VirtualMachineInstance", func() { Expect(vmi.Status.Machine).To(Equal(updatedVMI.Status.Machine)) return vmi, nil }) - mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any()).Return(nil) - mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any(), mockCgroupManager).Return(nil) + mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any(), mockCgroupManager).Return(nil) controller.Execute() }) @@ -822,8 +829,8 @@ var _ = Describe("VirtualMachineInstance", func() { }) vmiInterface.EXPECT().Update(context.Background(), NewVMICondMatcher(*updatedVMI)) client.EXPECT().GetGuestInfo().Return(&v1.VirtualMachineInstanceGuestAgentInfo{}, nil) - mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any()).Return(nil) - mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any(), mockCgroupManager).Return(nil) + mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any(), mockCgroupManager).Return(nil) controller.Execute() }) @@ -875,8 +882,8 @@ var _ = Describe("VirtualMachineInstance", func() { Expect(options.VirtualMachineSMBios.Manufacturer).To(Equal(virtconfig.SmbiosConfigDefaultManufacturer)) }) client.EXPECT().GetGuestInfo().Return(&v1.VirtualMachineInstanceGuestAgentInfo{}, nil) - mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any()).Return(nil) - mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any(), mockCgroupManager).Return(nil) + mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any(), mockCgroupManager).Return(nil) controller.Execute() }) @@ -930,8 +937,8 @@ var _ = Describe("VirtualMachineInstance", func() { Expect(options.VirtualMachineSMBios.Manufacturer).To(Equal(virtconfig.SmbiosConfigDefaultManufacturer)) }) vmiInterface.EXPECT().Update(context.Background(), NewVMICondMatcher(*updatedVMI)) - mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any()).Return(nil) - mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any(), mockCgroupManager).Return(nil) + mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any(), mockCgroupManager).Return(nil) controller.Execute() }) @@ -970,8 +977,8 @@ var _ = Describe("VirtualMachineInstance", func() { client.EXPECT().SyncVirtualMachine(vmi, gomock.Any()) vmiInterface.EXPECT().Update(context.Background(), NewVMICondMatcher(*updatedVMI)) - mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any()).Return(nil) - mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any(), mockCgroupManager).Return(nil) + mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any(), mockCgroupManager).Return(nil) controller.Execute() @@ -1009,8 +1016,8 @@ var _ = Describe("VirtualMachineInstance", func() { domainFeeder.Add(domain) client.EXPECT().SyncVirtualMachine(vmi, gomock.Any()) - mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any()).Return(nil) - mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any(), mockCgroupManager).Return(nil) + mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any(), mockCgroupManager).Return(nil) controller.Execute() // should not make another event entry unless something changes @@ -1063,8 +1070,8 @@ var _ = Describe("VirtualMachineInstance", func() { client.EXPECT().SyncVirtualMachine(vmi, gomock.Any()) vmiInterface.EXPECT().Update(context.Background(), NewVMICondMatcher(*vmiCopy)) - mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any()).Return(nil) - mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any(), mockCgroupManager).Return(nil) + mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any(), mockCgroupManager).Return(nil) controller.Execute() expectEvent(string(v1.AccessCredentialsSyncFailed), true) @@ -1101,8 +1108,8 @@ var _ = Describe("VirtualMachineInstance", func() { domainFeeder.Add(domain) client.EXPECT().SyncVirtualMachine(vmi, gomock.Any()) - mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any()).Return(nil) - mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any(), mockCgroupManager).Return(nil) + mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any(), mockCgroupManager).Return(nil) vmiInterface.EXPECT().Update(context.Background(), NewVMICondMatcher(*updatedVMI)) controller.Execute() @@ -1123,8 +1130,8 @@ var _ = Describe("VirtualMachineInstance", func() { domainFeeder.Add(domain) client.EXPECT().SyncVirtualMachine(vmi, gomock.Any()) - mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any()).Return(nil) - mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any(), mockCgroupManager).Return(nil) + mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any(), mockCgroupManager).Return(nil) vmiInterface.EXPECT().Update(context.Background(), NewVMICondMatcher(*updatedVMI)) controller.Execute() @@ -1190,7 +1197,7 @@ var _ = Describe("VirtualMachineInstance", func() { vmiInterface.EXPECT().Update(context.Background(), gomock.Any()).Do(func(ctx context.Context, vmi *v1.VirtualMachineInstance) { Expect(vmi.Status.Phase).To(Equal(v1.Failed)) }) - mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any(), mockCgroupManager).Return(nil) controller.Execute() testutils.ExpectEvent(recorder, "failed to configure vmi network:") testutils.ExpectEvent(recorder, VMICrashed) @@ -1226,7 +1233,7 @@ var _ = Describe("VirtualMachineInstance", func() { Expect(options.VirtualMachineSMBios.Product).To(Equal(virtconfig.SmbiosConfigDefaultProduct)) Expect(options.VirtualMachineSMBios.Manufacturer).To(Equal(virtconfig.SmbiosConfigDefaultManufacturer)) }) - mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any(), mockCgroupManager).Return(nil) vmiInterface.EXPECT().Update(context.Background(), updatedVMI) controller.Execute() @@ -1300,7 +1307,7 @@ var _ = Describe("VirtualMachineInstance", func() { vmi.Status.Phase = v1.Scheduled vmiFeeder.Add(vmi) vmiInterface.EXPECT().Update(context.Background(), gomock.Any()) - mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any(), mockCgroupManager).Return(nil) client.EXPECT().SyncVirtualMachine(vmi, gomock.Any()) controller.Execute() @@ -1316,8 +1323,8 @@ var _ = Describe("VirtualMachineInstance", func() { vmiFeeder.Add(vmi) domainFeeder.Add(domain) vmiInterface.EXPECT().Update(context.Background(), gomock.Any()) - mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any()).Return(nil) - mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any(), mockCgroupManager).Return(nil) + mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any(), mockCgroupManager).Return(nil) client.EXPECT().SyncVirtualMachine(vmi, gomock.Any()) controller.Execute() @@ -1332,7 +1339,7 @@ var _ = Describe("VirtualMachineInstance", func() { vmiFeeder.Add(vmi) domainFeeder.Add(domain) vmiInterface.EXPECT().Update(context.Background(), gomock.Any()) - mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any()).Return(fmt.Errorf("Error")) + mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any(), mockCgroupManager).Return(fmt.Errorf("Error")) controller.Execute() testutils.ExpectEvent(recorder, v1.SyncFailed.String()) @@ -1346,7 +1353,7 @@ var _ = Describe("VirtualMachineInstance", func() { domain.Status.Status = api.Running vmiFeeder.Add(vmi) domainFeeder.Add(domain) - mockHotplugVolumeMounter.EXPECT().UnmountAll(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().UnmountAll(gomock.Any(), mockCgroupManager).Return(nil) client.EXPECT().Close() controller.processVmCleanup(vmi) }) @@ -1672,7 +1679,7 @@ var _ = Describe("VirtualMachineInstance", func() { vmi := api2.NewMinimalVMI("testvmi") vmi.Status.Phase = phase vmiFeeder.Add(vmi) - mockHotplugVolumeMounter.EXPECT().UnmountAll(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().UnmountAll(gomock.Any(), mockCgroupManager).Return(nil) controller.Execute() // expect no errors and no mock interactions Expect(mockQueue.NumRequeues("default/testvmi")).To(Equal(0)) @@ -1804,7 +1811,7 @@ var _ = Describe("VirtualMachineInstance", func() { updatedVmi := vmi.DeepCopy() updatedVmi.Status.MigrationState.TargetNodeAddress = controller.migrationIpAddress updatedVmi.Status.MigrationState.TargetDirectMigrationNodePorts = destSrcPorts - mockHotplugVolumeMounter.EXPECT().UnmountAll(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().UnmountAll(gomock.Any(), mockCgroupManager).Return(nil) client.EXPECT().Close() controller.Execute() @@ -1845,7 +1852,7 @@ var _ = Describe("VirtualMachineInstance", func() { Expect(err).NotTo(HaveOccurred()) defer socket.Close() - mockHotplugVolumeMounter.EXPECT().UnmountAll(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().UnmountAll(gomock.Any(), mockCgroupManager).Return(nil) client.EXPECT().Ping().Return(fmt.Errorf("disconnected")) client.EXPECT().Close() @@ -3055,8 +3062,8 @@ var _ = Describe("VirtualMachineInstance", func() { domainFeeder.Add(domain) client.EXPECT().SyncVirtualMachine(vmi, gomock.Any()) - mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any()).Return(nil) - mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any(), mockCgroupManager).Return(nil) + mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any(), mockCgroupManager).Return(nil) expectedMemory := resource.MustParse("512Ki") vmiInterface.EXPECT().Update(context.Background(), gomock.Any()).Do(func(ctx context.Context, arg interface{}) { @@ -3140,8 +3147,8 @@ var _ = Describe("VirtualMachineInstance", func() { domainFeeder.Add(domain) client.EXPECT().SyncVirtualMachine(vmi, gomock.Any()) - mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any()).Return(nil) - mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any()).Return(nil) + mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any(), mockCgroupManager).Return(nil) + mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any(), mockCgroupManager).Return(nil) vmiInterface.EXPECT().Update(context.Background(), gomock.Any()).Do(func(ctx context.Context, arg interface{}) { Expect(arg.(*v1.VirtualMachineInstance).Status.VolumeStatus).To(HaveLen(2)) for _, status := range arg.(*v1.VirtualMachineInstance).Status.VolumeStatus { -- 2.42.1 From 9f078facf0d0d03f5438dae85fa243c891efb0b1 Mon Sep 17 00:00:00 2001 From: Vasiliy Ulyanov <vulyanov@suse.de> Date: Mon, 28 Nov 2022 10:39:57 +0100 Subject: [PATCH 3/4] cgroup v2: Drop global rulesPerPid map Keep the existing device rules within the manager state. Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de> Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> --- pkg/virt-handler/cgroup/cgroup_v2_manager.go | 21 ++++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/pkg/virt-handler/cgroup/cgroup_v2_manager.go b/pkg/virt-handler/cgroup/cgroup_v2_manager.go index 989c004e8..fb874c654 100644 --- a/pkg/virt-handler/cgroup/cgroup_v2_manager.go +++ b/pkg/virt-handler/cgroup/cgroup_v2_manager.go @@ -16,8 +16,6 @@ import ( "kubevirt.io/kubevirt/pkg/util" ) -var rulesPerPid = make(map[string][]*devices.Rule) - type v2Manager struct { runc_cgroups.Manager dirPath string @@ -45,7 +43,7 @@ func newCustomizedV2Manager( runcManager, runcManager.GetPaths()[""], isRootless, - deviceRules, + append(deviceRules, GenerateDefaultDeviceRules()...), execVirtChroot, } @@ -60,19 +58,20 @@ func (v *v2Manager) Set(r *runc_configs.Resources) error { // We want to keep given resources untouched resourcesToSet := *r - //Add default rules - resourcesToSet.Devices = append(resourcesToSet.Devices, GenerateDefaultDeviceRules()...) - resourcesToSet.Devices = append(resourcesToSet.Devices, v.deviceRules...) - - rulesToSet, err := addCurrentRules(rulesPerPid[v.dirPath], resourcesToSet.Devices) + rulesToSet, err := addCurrentRules(v.deviceRules, resourcesToSet.Devices) if err != nil { return err } - rulesPerPid[v.dirPath] = rulesToSet + v.deviceRules = rulesToSet resourcesToSet.Devices = rulesToSet + for _, rule := range rulesToSet { + if rule == nil { + continue + } + log.Log.V(5).Infof("cgroupsv2 device allowlist: rule after appending current+new: type: %d permissions: %s allow: %t major: %d minor: %d", rule.Type, rule.Permissions, rule.Allow, rule.Major, rule.Minor) + } - err = v.execVirtChroot(&resourcesToSet, map[string]string{"": v.dirPath}, v.isRootless, v.GetCgroupVersion()) - return err + return v.execVirtChroot(&resourcesToSet, map[string]string{"": v.dirPath}, v.isRootless, v.GetCgroupVersion()) } func (v *v2Manager) GetCgroupVersion() CgroupVersion { -- 2.42.1 From 725e4d70f1454d229882e2790dfa263bbeed2334 Mon Sep 17 00:00:00 2001 From: Alex Kalenyuk <akalenyu@redhat.com> Date: Mon, 6 Nov 2023 13:24:34 +0200 Subject: [PATCH 4/4] tests: virt handler getting killed/live migration with hotplug and persistent disk Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> --- tests/storage/hotplug.go | 151 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 141 insertions(+), 10 deletions(-) diff --git a/tests/storage/hotplug.go b/tests/storage/hotplug.go index 172f13e74..c6c8de4e1 100644 --- a/tests/storage/hotplug.go +++ b/tests/storage/hotplug.go @@ -572,6 +572,119 @@ var _ = SIGDescribe("Hotplug", func() { ) }) + Context("Offline VM with a block volume", func() { + var ( + vm *v1.VirtualMachine + sc string + ) + + BeforeEach(func() { + var exists bool + + sc, exists = libstorage.GetRWXBlockStorageClass() + if !exists { + Skip("Skip test when RWXBlock storage class is not present") + } + + vmi, _ := tests.NewRandomVirtualMachineInstanceWithBlockDisk( + cd.DataVolumeImportUrlForContainerDisk(cd.ContainerDiskCirros), + testsuite.GetTestNamespace(nil), + corev1.ReadWriteMany, + ) + tests.AddUserData(vmi, "cloud-init", "#!/bin/bash\necho 'hello'\n") + + By("Creating VirtualMachine") + vm, err = virtClient.VirtualMachine(vmi.Namespace).Create(context.Background(), tests.NewRandomVirtualMachine(vmi, false)) + Expect(err).ToNot(HaveOccurred()) + }) + + DescribeTable("Should start with a hotplug block", func(addVolumeFunc addVolumeFunction) { + dv := createDataVolumeAndWaitForImport(sc, corev1.PersistentVolumeBlock) + + By("Adding a hotplug block volume") + addVolumeFunc(vm.Name, vm.Namespace, dv.Name, dv.Name, v1.DiskBusSCSI, false, "") + + By("Verifying the volume has been added to the template spec") + verifyVolumeAndDiskVMAdded(virtClient, vm, dv.Name) + + By("Starting the VM") + vm = tests.StartVirtualMachine(vm) + vmi, err := virtClient.VirtualMachineInstance(vm.Namespace).Get(context.Background(), vm.Name, &metav1.GetOptions{}) + Expect(err).ToNot(HaveOccurred()) + + By("Verifying the volume is attached and usable") + verifyVolumeAndDiskVMIAdded(virtClient, vmi, dv.Name) + verifyVolumeStatus(vmi, v1.VolumeReady, "", dv.Name) + getVmiConsoleAndLogin(vmi) + targets := verifyHotplugAttachedAndUseable(vmi, []string{dv.Name}) + Expect(targets).To(HaveLen(1)) + }, + Entry("DataVolume", addDVVolumeVM), + Entry("PersistentVolume", addPVCVolumeVM), + ) + + It("[Serial]Should preserve access to block devices if virt-handler crashes", Serial, func() { + blockDevices := []string{"/dev/disk0"} + + By("Adding a hotplug block volume") + dv := createDataVolumeAndWaitForImport(sc, corev1.PersistentVolumeBlock) + blockDevices = append(blockDevices, fmt.Sprintf("/var/run/kubevirt/hotplug-disks/%s", dv.Name)) + addDVVolumeVM(vm.Name, vm.Namespace, dv.Name, dv.Name, v1.DiskBusSCSI, false, "") + + By("Verifying the volume has been added to the template spec") + verifyVolumeAndDiskVMAdded(virtClient, vm, dv.Name) + + By("Starting the VM") + vm = tests.StartVirtualMachine(vm) + vmi, err := virtClient.VirtualMachineInstance(vm.Namespace).Get(context.Background(), vm.Name, &metav1.GetOptions{}) + Expect(err).ToNot(HaveOccurred()) + + By("Verifying the volume is attached and usable") + verifyVolumeAndDiskVMIAdded(virtClient, vmi, dv.Name) + verifyVolumeStatus(vmi, v1.VolumeReady, "", dv.Name) + getVmiConsoleAndLogin(vmi) + targets := verifyHotplugAttachedAndUseable(vmi, []string{dv.Name}) + Expect(targets).To(HaveLen(1)) + + By("Deleting virt-handler pod") + virtHandlerPod, err := libnode.GetVirtHandlerPod(virtClient, vmi.Status.NodeName) + Expect(err).ToNot(HaveOccurred()) + Eventually(func() bool { + err := virtClient.CoreV1(). + Pods(virtHandlerPod.GetObjectMeta().GetNamespace()). + Delete(context.Background(), virtHandlerPod.GetObjectMeta().GetName(), metav1.DeleteOptions{}) + return errors.IsNotFound(err) + }, 60*time.Second, 1*time.Second).Should(BeTrue(), "virt-handler pod is expected to be deleted") + + By("Waiting for virt-handler pod to restart") + Eventually(func() bool { + virtHandlerPod, err = libnode.GetVirtHandlerPod(virtClient, vmi.Status.NodeName) + return err == nil && virtHandlerPod.Status.Phase == corev1.PodRunning + }, 60*time.Second, 1*time.Second).Should(BeTrue(), "virt-handler pod is expected to be restarted") + + By("Adding another hotplug block volume") + dv = createDataVolumeAndWaitForImport(sc, corev1.PersistentVolumeBlock) + blockDevices = append(blockDevices, fmt.Sprintf("/var/run/kubevirt/hotplug-disks/%s", dv.Name)) + addDVVolumeVM(vm.Name, vm.Namespace, dv.Name, dv.Name, v1.DiskBusSCSI, false, "") + + By("Verifying the volume is attached and usable") + verifyVolumeAndDiskVMIAdded(virtClient, vmi, dv.Name) + verifyVolumeStatus(vmi, v1.VolumeReady, "", dv.Name) + getVmiConsoleAndLogin(vmi) + targets = verifyHotplugAttachedAndUseable(vmi, []string{dv.Name}) + Expect(targets).To(HaveLen(1)) + + By("Verifying the block devices are still accessible") + for _, dev := range blockDevices { + By(fmt.Sprintf("Verifying %s", dev)) + output := tests.RunCommandOnVmiPod(vmi, []string{ + "dd", fmt.Sprintf("if=%s", dev), "of=/dev/null", "bs=1", "count=1", "status=none", + }) + Expect(output).To(BeEmpty()) + } + }) + }) + Context("WFFC storage", func() { var ( vm *v1.VirtualMachine @@ -983,15 +1096,32 @@ var _ = SIGDescribe("Hotplug", func() { sc string numberOfMigrations int + hotplugLabelKey string sourceNode string targetNode string ) const ( - hotplugLabelKey = "kubevirt-test-migration-with-hotplug-disks" hotplugLabelValue = "true" ) + containerDiskVMIFunc := func() *v1.VirtualMachineInstance { + return libvmi.NewCirros( + libvmi.WithInterface(libvmi.InterfaceDeviceWithMasqueradeBinding()), + libvmi.WithNetwork(v1.DefaultPodNetwork()), + ) + } + persistentDiskVMIFunc := func() *v1.VirtualMachineInstance { + vmi, _ := tests.NewRandomVirtualMachineInstanceWithBlockDisk( + cd.DataVolumeImportUrlForContainerDisk(cd.ContainerDiskCirros), + testsuite.GetTestNamespace(nil), + corev1.ReadWriteMany, + ) + tests.AddUserData(vmi, "cloud-init", "#!/bin/bash\necho 'hello'\n") + + return vmi + } + BeforeEach(func() { exists := false sc, exists = libstorage.GetRWXBlockStorageClass() @@ -1024,14 +1154,8 @@ var _ = SIGDescribe("Hotplug", func() { } // Ensure the virt-launcher pod is scheduled on the chosen source node and then // migrated to the proper target. + hotplugLabelKey = fmt.Sprintf("%s-hotplug-disk-migration", testsuite.GetTestNamespace(nil)) libnode.AddLabelToNode(sourceNode, hotplugLabelKey, hotplugLabelValue) - vmi = libvmi.NewCirros( - libvmi.WithInterface(libvmi.InterfaceDeviceWithMasqueradeBinding()), - libvmi.WithNetwork(v1.DefaultPodNetwork()), - ) - vmi.Spec.NodeSelector = map[string]string{hotplugLabelKey: hotplugLabelValue} - vmi = tests.RunVMIAndExpectLaunch(vmi, 240) - libnode.AddLabelToNode(targetNode, hotplugLabelKey, hotplugLabelValue) }) AfterEach(func() { @@ -1040,7 +1164,11 @@ var _ = SIGDescribe("Hotplug", func() { libnode.RemoveLabelFromNode(targetNode, hotplugLabelKey) }) - It("should allow live migration with attached hotplug volumes", func() { + DescribeTable("should allow live migration with attached hotplug volumes", func(vmiFunc func() *v1.VirtualMachineInstance) { + vmi = vmiFunc() + vmi.Spec.NodeSelector = map[string]string{hotplugLabelKey: hotplugLabelValue} + vmi = tests.RunVMIAndExpectLaunch(vmi, 240) + libnode.AddLabelToNode(targetNode, hotplugLabelKey, hotplugLabelValue) volumeName := "testvolume" volumeMode := corev1.PersistentVolumeBlock addVolumeFunc := addDVVolumeVMI @@ -1105,7 +1233,10 @@ var _ = SIGDescribe("Hotplug", func() { Expect(err).ToNot(HaveOccurred()) verifyVolumeAndDiskVMIAdded(virtClient, vmi, volumeName) verifyVolumeStatus(vmi, v1.VolumeReady, "", volumeName) - }) + }, + Entry("containerDisk VMI", containerDiskVMIFunc), + Entry("persistent disk VMI", persistentDiskVMIFunc), + ) }) Context("disk mutating sidecar", func() { -- 2.42.1
Locations
Projects
Search
Status Monitor
Help
OpenBuildService.org
Documentation
API Documentation
Code of Conduct
Contact
Support
@OBShq
Terms
openSUSE Build Service is sponsored by
The Open Build Service is an
openSUSE project
.
Sign Up
Log In
Places
Places
All Projects
Status Monitor
