From 49dabe56d04f2f63804a5cd0411d374d5c235143 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Thu, 20 Feb 2025 16:39:38 -0500 Subject: [PATCH 1/7] Monitor progress tracking for permission change --- pkg/kubelet/events/event.go | 1 + pkg/volume/configmap/configmap.go | 3 +- pkg/volume/csi/csi_mounter.go | 4 +- pkg/volume/downwardapi/downwardapi.go | 3 +- pkg/volume/emptydir/empty_dir.go | 6 +- pkg/volume/fc/disk_manager.go | 3 +- .../operationexecutor/operation_generator.go | 1 + pkg/volume/volume.go | 2 + pkg/volume/volume_linux.go | 102 ++++++++++++++++++ 9 files changed, 119 insertions(+), 6 deletions(-) diff --git a/pkg/kubelet/events/event.go b/pkg/kubelet/events/event.go index 0eaa82fc77f..526a67cdb2b 100644 --- a/pkg/kubelet/events/event.go +++ b/pkg/kubelet/events/event.go @@ -61,6 +61,7 @@ const ( VolumeResizeFailed = "VolumeResizeFailed" VolumeResizeSuccess = "VolumeResizeSuccessful" FileSystemResizeFailed = "FileSystemResizeFailed" + VolumePermissionChangeInProgress = "VolumePermissionChangeInProgress" FileSystemResizeSuccess = "FileSystemResizeSuccessful" FailedMapVolume = "FailedMapVolume" WarnAlreadyMountedVolume = "AlreadyMountedVolume" diff --git a/pkg/volume/configmap/configmap.go b/pkg/volume/configmap/configmap.go index 53d03bb70b4..8bbf50c4b59 100644 --- a/pkg/volume/configmap/configmap.go +++ b/pkg/volume/configmap/configmap.go @@ -246,7 +246,8 @@ func (b *configMapVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterA setPerms := func(_ string) error { // This may be the first time writing and new files get created outside the timestamp subdirectory: // change the permissions on the whole volume and not only in the timestamp directory. - return volume.SetVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) + ownerShipChanger := volume.NewVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) + return ownerShipChanger.ChangePermissions() } err = writer.Write(payload, setPerms) if err != nil { diff --git a/pkg/volume/csi/csi_mounter.go b/pkg/volume/csi/csi_mounter.go index b31a777c85a..4f024201955 100644 --- a/pkg/volume/csi/csi_mounter.go +++ b/pkg/volume/csi/csi_mounter.go @@ -335,7 +335,9 @@ func (c *csiMountMgr) SetUpAt(dir string, mounterArgs volume.MounterArgs) error // Driver doesn't support applying FSGroup. Kubelet must apply it instead. // fullPluginName helps to distinguish different driver from csi plugin - err := volume.SetVolumeOwnership(c, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(c.plugin, c.spec)) + ownershipChanger := volume.NewVolumeOwnership(c, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(c.plugin, c.spec)) + ownershipChanger.AddProgressNotifier(c.pod, mounterArgs.Recorder) + err = ownershipChanger.ChangePermissions() if err != nil { // At this point mount operation is successful: // 1. Since volume can not be used by the pod because of invalid permissions, we must return error diff --git a/pkg/volume/downwardapi/downwardapi.go b/pkg/volume/downwardapi/downwardapi.go index 4c544516a52..d3790b4cd56 100644 --- a/pkg/volume/downwardapi/downwardapi.go +++ b/pkg/volume/downwardapi/downwardapi.go @@ -217,7 +217,8 @@ func (b *downwardAPIVolumeMounter) SetUpAt(dir string, mounterArgs volume.Mounte setPerms := func(_ string) error { // This may be the first time writing and new files get created outside the timestamp subdirectory: // change the permissions on the whole volume and not only in the timestamp directory. - return volume.SetVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) + ownershipChanger := volume.NewVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) + return ownershipChanger.ChangePermissions() } err = writer.Write(data, setPerms) if err != nil { diff --git a/pkg/volume/emptydir/empty_dir.go b/pkg/volume/emptydir/empty_dir.go index ed2f206fb38..3c64e6af6d3 100644 --- a/pkg/volume/emptydir/empty_dir.go +++ b/pkg/volume/emptydir/empty_dir.go @@ -18,10 +18,11 @@ package emptydir import ( "fmt" - "k8s.io/kubernetes/pkg/kubelet/util/swap" "os" "path/filepath" + "k8s.io/kubernetes/pkg/kubelet/util/swap" + "k8s.io/klog/v2" "k8s.io/mount-utils" utilstrings "k8s.io/utils/strings" @@ -278,7 +279,8 @@ func (ed *emptyDir) SetUpAt(dir string, mounterArgs volume.MounterArgs) error { err = fmt.Errorf("unknown storage medium %q", ed.medium) } - volume.SetVolumeOwnership(ed, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(ed.plugin, nil)) + ownershipChanger := volume.NewVolumeOwnership(ed, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(ed.plugin, nil)) + ownershipChanger.ChangePermissions() // If setting up the quota fails, just log a message but don't actually error out. // We'll use the old du mechanism in this case, at least until we support diff --git a/pkg/volume/fc/disk_manager.go b/pkg/volume/fc/disk_manager.go index 02e15c4f85c..597d3c0a430 100644 --- a/pkg/volume/fc/disk_manager.go +++ b/pkg/volume/fc/disk_manager.go @@ -91,7 +91,8 @@ func diskSetUp(manager diskManager, b fcDiskMounter, volPath string, mounter mou } if !b.readOnly { - volume.SetVolumeOwnership(&b, volPath, fsGroup, fsGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) + ownershipChanger := volume.NewVolumeOwnership(&b, volPath, fsGroup, fsGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) + ownershipChanger.ChangePermissions() } return nil diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index 5523b61a5ec..0537edae52c 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -584,6 +584,7 @@ func (og *operationGenerator) GenerateMountVolumeFunc( FsGroup: fsGroup, DesiredSize: volumeToMount.DesiredSizeLimit, FSGroupChangePolicy: fsGroupChangePolicy, + Recorder: og.recorder, SELinuxLabel: volumeToMount.SELinuxLabel, }) // Update actual state of world diff --git a/pkg/volume/volume.go b/pkg/volume/volume.go index 70bf7decc5b..4b75dce00cf 100644 --- a/pkg/volume/volume.go +++ b/pkg/volume/volume.go @@ -23,6 +23,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/record" ) // Volume represents a directory used by pods or hosts on a node. All method @@ -130,6 +131,7 @@ type MounterArgs struct { FSGroupChangePolicy *v1.PodFSGroupChangePolicy DesiredSize *resource.Quantity SELinuxLabel string + Recorder record.EventRecorder } // Mounter interface provides methods to set up/mount the volume. diff --git a/pkg/volume/volume_linux.go b/pkg/volume/volume_linux.go index ec7f6da4bfe..a5277577d09 100644 --- a/pkg/volume/volume_linux.go +++ b/pkg/volume/volume_linux.go @@ -20,14 +20,19 @@ limitations under the License. package volume import ( + "context" + "fmt" "path/filepath" + "sync/atomic" "syscall" "os" "time" v1 "k8s.io/api/core/v1" + "k8s.io/client-go/tools/record" "k8s.io/klog/v2" + "k8s.io/kubernetes/pkg/kubelet/events" "k8s.io/kubernetes/pkg/volume/util/types" ) @@ -35,8 +40,105 @@ const ( rwMask = os.FileMode(0660) roMask = os.FileMode(0440) execMask = os.FileMode(0110) + + progressReportDuration = 60 * time.Second ) +type VolumeOwnership struct { + mounter Mounter + dir string + fsGroup *int64 + fsGroupChangePolicy *v1.PodFSGroupChangePolicy + completionCallback func(types.CompleteFuncParam) + + // for monitoring progress of permission change operation + pod *v1.Pod + fileCounter atomic.Int64 + recorder record.EventRecorder +} + +func NewVolumeOwnership(mounter Mounter, dir string, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy, completeFunc func(types.CompleteFuncParam)) *VolumeOwnership { + vo := &VolumeOwnership{ + mounter: mounter, + dir: dir, + fsGroup: fsGroup, + fsGroupChangePolicy: fsGroupChangePolicy, + completionCallback: completeFunc, + } + vo.fileCounter.Store(0) + return vo +} + +func (vo *VolumeOwnership) AddProgressNotifier(pod *v1.Pod, recorder record.EventRecorder) *VolumeOwnership { + vo.pod = pod + vo.recorder = recorder + return vo +} + +func (vo *VolumeOwnership) ChangePermissions() error { + if vo.fsGroup == nil { + return nil + } + + if skipPermissionChange(vo.mounter, vo.dir, vo.fsGroup, vo.fsGroupChangePolicy) { + klog.V(3).InfoS("Skipping permission and ownership change for volume", "path", vo.dir) + return nil + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + timer := time.AfterFunc(30*time.Second, func() { + vo.initiateProgressMonitor(ctx) + }) + defer timer.Stop() + + return vo.changePermissionsRecursively() +} + +func (vo *VolumeOwnership) initiateProgressMonitor(ctx context.Context) { + klog.Warningf("Setting volume ownership for %s and fsGroup set. If the volume has a lot of files then setting volume ownership could be slow, see https://github.com/kubernetes/kubernetes/issues/69699", vo.dir) + if vo.pod != nil { + go vo.monitorProgress(ctx) + } +} + +func (vo *VolumeOwnership) changePermissionsRecursively() error { + err := walkDeep(vo.dir, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + vo.fileCounter.Add(1) + return changeFilePermission(path, vo.fsGroup, vo.mounter.GetAttributes().ReadOnly, info) + }) + + if vo.completionCallback != nil { + vo.completionCallback(types.CompleteFuncParam{ + Err: &err, + }) + } + return err +} + +func (vo *VolumeOwnership) monitorProgress(ctx context.Context) { + ticker := time.NewTicker(progressReportDuration) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + vo.logWarning() + } + } +} + +func (vo *VolumeOwnership) logWarning() { + msg := fmt.Sprintf("Setting volume ownership for %s, processed %d files", vo.dir, vo.fileCounter.Load()) + klog.Warning(msg) + vo.recorder.Event(vo.pod, v1.EventTypeWarning, events.VolumePermissionChangeInProgress, msg) +} + // SetVolumeOwnership modifies the given volume to be owned by // fsGroup, and sets SetGid so that newly created files are owned by // fsGroup. If fsGroup is nil nothing is done. From 32752fe395a3ec10083228d5dec6d841415b681e Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Mon, 24 Feb 2025 12:08:09 -0500 Subject: [PATCH 2/7] Add recommendation for using OnRootMismatch --- pkg/volume/util/metrics.go | 2 ++ pkg/volume/volume_linux.go | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/volume/util/metrics.go b/pkg/volume/util/metrics.go index 0eb030b359e..ef4e80bdfec 100644 --- a/pkg/volume/util/metrics.go +++ b/pkg/volume/util/metrics.go @@ -25,6 +25,7 @@ import ( "google.golang.org/grpc/status" "k8s.io/component-base/metrics" "k8s.io/component-base/metrics/legacyregistry" + "k8s.io/klog/v2" "k8s.io/kubernetes/pkg/volume" "k8s.io/kubernetes/pkg/volume/util/types" ) @@ -102,6 +103,7 @@ func OperationCompleteHook(plugin, operationName string) func(types.CompleteFunc if c.Migrated != nil { migrated = *c.Migrated } + klog.Infof("foobar Operation %s took %f", operationName, timeTaken) StorageOperationMetric.WithLabelValues(plugin, operationName, status, strconv.FormatBool(migrated)).Observe(timeTaken) } return opComplete diff --git a/pkg/volume/volume_linux.go b/pkg/volume/volume_linux.go index a5277577d09..9da89ca41f4 100644 --- a/pkg/volume/volume_linux.go +++ b/pkg/volume/volume_linux.go @@ -121,6 +121,8 @@ func (vo *VolumeOwnership) changePermissionsRecursively() error { } func (vo *VolumeOwnership) monitorProgress(ctx context.Context) { + msg := fmt.Sprintf("Setting volume ownership for %s is taking longer than expected, consider using OnRootMismatch - https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#configure-volume-permission-and-ownership-change-policy-for-pods", vo.dir) + vo.recorder.Event(vo.pod, v1.EventTypeWarning, events.VolumePermissionChangeInProgress, msg) ticker := time.NewTicker(progressReportDuration) defer ticker.Stop() for { @@ -134,7 +136,7 @@ func (vo *VolumeOwnership) monitorProgress(ctx context.Context) { } func (vo *VolumeOwnership) logWarning() { - msg := fmt.Sprintf("Setting volume ownership for %s, processed %d files", vo.dir, vo.fileCounter.Load()) + msg := fmt.Sprintf("Setting volume ownership for %s, processed %d files.", vo.dir, vo.fileCounter.Load()) klog.Warning(msg) vo.recorder.Event(vo.pod, v1.EventTypeWarning, events.VolumePermissionChangeInProgress, msg) } From b51cbb1d17ca680139d306b43a88c723e72a6103 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Mon, 24 Feb 2025 12:48:38 -0500 Subject: [PATCH 3/7] Change plugin interfaces to use progress monitoring --- pkg/volume/flexvolume/mounter.go | 3 ++- pkg/volume/git_repo/git_repo.go | 4 ++-- pkg/volume/iscsi/disk_manager.go | 9 ++++++--- pkg/volume/iscsi/iscsi.go | 2 +- pkg/volume/local/local.go | 4 +++- pkg/volume/portworx/portworx.go | 4 +++- pkg/volume/projected/projected.go | 3 ++- pkg/volume/secret/secret.go | 3 ++- pkg/volume/util/metrics.go | 2 -- pkg/volume/volume_linux.go | 32 ------------------------------- pkg/volume/volume_linux_test.go | 6 ++++-- 11 files changed, 25 insertions(+), 47 deletions(-) diff --git a/pkg/volume/flexvolume/mounter.go b/pkg/volume/flexvolume/mounter.go index 3821af7e923..cd8b4860bbc 100644 --- a/pkg/volume/flexvolume/mounter.go +++ b/pkg/volume/flexvolume/mounter.go @@ -95,7 +95,8 @@ func (f *flexVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterArgs) if !f.readOnly { if f.plugin.capabilities.FSGroup { // fullPluginName helps to distinguish different driver from flex volume plugin - volume.SetVolumeOwnership(f, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(f.plugin, f.spec)) + ownershipChanger := volume.NewVolumeOwnership(f, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(f.plugin, f.spec)) + ownershipChanger.ChangePermissions() } } diff --git a/pkg/volume/git_repo/git_repo.go b/pkg/volume/git_repo/git_repo.go index 9211bf8bc6f..ad37d6e8bdb 100644 --- a/pkg/volume/git_repo/git_repo.go +++ b/pkg/volume/git_repo/git_repo.go @@ -229,8 +229,8 @@ func (b *gitRepoVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterArg return fmt.Errorf("failed to exec 'git reset --hard': %s: %v", output, err) } - volume.SetVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) - + ownershipChanger := volume.NewVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) + ownershipChanger.ChangePermissions() volumeutil.SetReady(b.getMetaDir()) return nil } diff --git a/pkg/volume/iscsi/disk_manager.go b/pkg/volume/iscsi/disk_manager.go index 6aa8652bd6b..397bae22f71 100644 --- a/pkg/volume/iscsi/disk_manager.go +++ b/pkg/volume/iscsi/disk_manager.go @@ -19,7 +19,6 @@ package iscsi import ( "os" - v1 "k8s.io/api/core/v1" "k8s.io/klog/v2" "k8s.io/mount-utils" @@ -42,7 +41,9 @@ type diskManager interface { // utility to mount a disk based filesystem // globalPDPath: global mount path like, /var/lib/kubelet/plugins/kubernetes.io/iscsi/{ifaceName}/{portal-some_iqn-lun-lun_id} // volPath: pod volume dir path like, /var/lib/kubelet/pods/{podUID}/volumes/kubernetes.io~iscsi/{volumeName} -func diskSetUp(manager diskManager, b iscsiDiskMounter, volPath string, mounter mount.Interface, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy) error { +func diskSetUp(manager diskManager, b iscsiDiskMounter, volPath string, mounter mount.Interface, mounterArgs volume.MounterArgs) error { + fsGroup := mounterArgs.FsGroup + fsGroupChangePolicy := mounterArgs.FSGroupChangePolicy notMnt, err := mounter.IsLikelyNotMountPoint(volPath) if err != nil && !os.IsNotExist(err) { klog.Errorf("cannot validate mountpoint: %s", volPath) @@ -96,7 +97,9 @@ func diskSetUp(manager diskManager, b iscsiDiskMounter, volPath string, mounter } if !b.readOnly { - volume.SetVolumeOwnership(&b, volPath, fsGroup, fsGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) + // This code requires larger refactor to monitor progress of ownership change + ownershipChanger := volume.NewVolumeOwnership(&b, volPath, fsGroup, fsGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) + ownershipChanger.ChangePermissions() } return nil diff --git a/pkg/volume/iscsi/iscsi.go b/pkg/volume/iscsi/iscsi.go index 9300d006491..0e94a0c84e8 100644 --- a/pkg/volume/iscsi/iscsi.go +++ b/pkg/volume/iscsi/iscsi.go @@ -377,7 +377,7 @@ func (b *iscsiDiskMounter) SetUp(mounterArgs volume.MounterArgs) error { func (b *iscsiDiskMounter) SetUpAt(dir string, mounterArgs volume.MounterArgs) error { // diskSetUp checks mountpoints and prevent repeated calls - err := diskSetUp(b.manager, *b, dir, b.mounter, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy) + err := diskSetUp(b.manager, *b, dir, b.mounter, mounterArgs) if err != nil { klog.Errorf("iscsi: failed to setup") } diff --git a/pkg/volume/local/local.go b/pkg/volume/local/local.go index 19ad6fb661b..4d78faefb5e 100644 --- a/pkg/volume/local/local.go +++ b/pkg/volume/local/local.go @@ -610,7 +610,9 @@ func (m *localVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterArgs) if !m.readOnly { // Volume owner will be written only once on the first volume mount if len(refs) == 0 { - return volume.SetVolumeOwnership(m, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(m.plugin, nil)) + ownershipChanger := volume.NewVolumeOwnership(m, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(m.plugin, nil)) + ownershipChanger.AddProgressNotifier(m.pod, mounterArgs.Recorder) + return ownershipChanger.ChangePermissions() } } return nil diff --git a/pkg/volume/portworx/portworx.go b/pkg/volume/portworx/portworx.go index baa623ab4b4..496db552925 100644 --- a/pkg/volume/portworx/portworx.go +++ b/pkg/volume/portworx/portworx.go @@ -331,7 +331,9 @@ func (b *portworxVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterAr return err } if !b.readOnly { - volume.SetVolumeOwnership(b, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) + // Since portworxVolume is in process of being removed from in-tree, we avoid larger refactor to add progress tracking for ownership operation + ownershipChanger := volume.NewVolumeOwnership(b, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) + ownershipChanger.ChangePermissions() } klog.Infof("Portworx Volume %s setup at %s", b.volumeID, dir) return nil diff --git a/pkg/volume/projected/projected.go b/pkg/volume/projected/projected.go index ae18a81af8f..291fc88e8ae 100644 --- a/pkg/volume/projected/projected.go +++ b/pkg/volume/projected/projected.go @@ -229,7 +229,8 @@ func (s *projectedVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterA setPerms := func(_ string) error { // This may be the first time writing and new files get created outside the timestamp subdirectory: // change the permissions on the whole volume and not only in the timestamp directory. - return volume.SetVolumeOwnership(s, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(s.plugin, nil)) + ownershipChanger := volume.NewVolumeOwnership(s, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(s.plugin, nil)) + return ownershipChanger.ChangePermissions() } err = writer.Write(data, setPerms) if err != nil { diff --git a/pkg/volume/secret/secret.go b/pkg/volume/secret/secret.go index 6daa2c47732..06cb178fdc5 100644 --- a/pkg/volume/secret/secret.go +++ b/pkg/volume/secret/secret.go @@ -242,7 +242,8 @@ func (b *secretVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterArgs setPerms := func(_ string) error { // This may be the first time writing and new files get created outside the timestamp subdirectory: // change the permissions on the whole volume and not only in the timestamp directory. - return volume.SetVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) + ownershipChanger := volume.NewVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) + return ownershipChanger.ChangePermissions() } err = writer.Write(payload, setPerms) if err != nil { diff --git a/pkg/volume/util/metrics.go b/pkg/volume/util/metrics.go index ef4e80bdfec..0eb030b359e 100644 --- a/pkg/volume/util/metrics.go +++ b/pkg/volume/util/metrics.go @@ -25,7 +25,6 @@ import ( "google.golang.org/grpc/status" "k8s.io/component-base/metrics" "k8s.io/component-base/metrics/legacyregistry" - "k8s.io/klog/v2" "k8s.io/kubernetes/pkg/volume" "k8s.io/kubernetes/pkg/volume/util/types" ) @@ -103,7 +102,6 @@ func OperationCompleteHook(plugin, operationName string) func(types.CompleteFunc if c.Migrated != nil { migrated = *c.Migrated } - klog.Infof("foobar Operation %s took %f", operationName, timeTaken) StorageOperationMetric.WithLabelValues(plugin, operationName, status, strconv.FormatBool(migrated)).Observe(timeTaken) } return opComplete diff --git a/pkg/volume/volume_linux.go b/pkg/volume/volume_linux.go index 9da89ca41f4..0ce3ba4ae04 100644 --- a/pkg/volume/volume_linux.go +++ b/pkg/volume/volume_linux.go @@ -141,38 +141,6 @@ func (vo *VolumeOwnership) logWarning() { vo.recorder.Event(vo.pod, v1.EventTypeWarning, events.VolumePermissionChangeInProgress, msg) } -// SetVolumeOwnership modifies the given volume to be owned by -// fsGroup, and sets SetGid so that newly created files are owned by -// fsGroup. If fsGroup is nil nothing is done. -func SetVolumeOwnership(mounter Mounter, dir string, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy, completeFunc func(types.CompleteFuncParam)) error { - if fsGroup == nil { - return nil - } - - timer := time.AfterFunc(30*time.Second, func() { - klog.Warningf("Setting volume ownership for %s and fsGroup set. If the volume has a lot of files then setting volume ownership could be slow, see https://github.com/kubernetes/kubernetes/issues/69699", dir) - }) - defer timer.Stop() - - if skipPermissionChange(mounter, dir, fsGroup, fsGroupChangePolicy) { - klog.V(3).InfoS("Skipping permission and ownership change for volume", "path", dir) - return nil - } - - err := walkDeep(dir, func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - return changeFilePermission(path, fsGroup, mounter.GetAttributes().ReadOnly, info) - }) - if completeFunc != nil { - completeFunc(types.CompleteFuncParam{ - Err: &err, - }) - } - return err -} - func changeFilePermission(filename string, fsGroup *int64, readonly bool, info os.FileInfo) error { err := os.Lchown(filename, -1, int(*fsGroup)) if err != nil { diff --git a/pkg/volume/volume_linux_test.go b/pkg/volume/volume_linux_test.go index 180e6d16465..5576fbbc7a4 100644 --- a/pkg/volume/volume_linux_test.go +++ b/pkg/volume/volume_linux_test.go @@ -303,7 +303,8 @@ func TestSetVolumeOwnershipMode(t *testing.T) { } mounter := &localFakeMounter{path: "FAKE_DIR_DOESNT_EXIST"} // SetVolumeOwnership() must rely on tmpDir - err = SetVolumeOwnership(mounter, tmpDir, &expectedGid, test.fsGroupChangePolicy, nil) + ownershipChanger := NewVolumeOwnership(mounter, tmpDir, &expectedGid, test.fsGroupChangePolicy, nil) + err = ownershipChanger.ChangePermissions() if err != nil { t.Errorf("for %s error changing ownership with: %v", test.description, err) } @@ -439,7 +440,8 @@ func TestSetVolumeOwnershipOwner(t *testing.T) { mounter := &localFakeMounter{path: tmpDir} always := v1.FSGroupChangeAlways - err = SetVolumeOwnership(mounter, tmpDir, test.fsGroup, &always, nil) + ownershipChanger := NewVolumeOwnership(mounter, tmpDir, test.fsGroup, &always, nil) + err = ownershipChanger.ChangePermissions() if err != nil { t.Errorf("for %s error changing ownership with: %v", test.description, err) } From fbce6bd6107a8794c51ed1cc1c451532998ca735 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Mon, 24 Feb 2025 16:16:05 -0500 Subject: [PATCH 4/7] Fix typecheck errors --- pkg/volume/volume.go | 15 +++++++++++++++ pkg/volume/volume_linux.go | 15 +-------------- pkg/volume/volume_unsupported.go | 14 +++++++++++--- 3 files changed, 27 insertions(+), 17 deletions(-) diff --git a/pkg/volume/volume.go b/pkg/volume/volume.go index 4b75dce00cf..f5f933e0c6a 100644 --- a/pkg/volume/volume.go +++ b/pkg/volume/volume.go @@ -17,6 +17,7 @@ limitations under the License. package volume import ( + "sync/atomic" "time" v1 "k8s.io/api/core/v1" @@ -24,6 +25,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" + volumetypes "k8s.io/kubernetes/pkg/volume/util/types" ) // Volume represents a directory used by pods or hosts on a node. All method @@ -134,6 +136,19 @@ type MounterArgs struct { Recorder record.EventRecorder } +type VolumeOwnership struct { + mounter Mounter + dir string + fsGroup *int64 + fsGroupChangePolicy *v1.PodFSGroupChangePolicy + completionCallback func(volumetypes.CompleteFuncParam) + + // for monitoring progress of permission change operation + pod *v1.Pod + fileCounter atomic.Int64 + recorder record.EventRecorder +} + // Mounter interface provides methods to set up/mount the volume. type Mounter interface { // Uses Interface to provide the path for Docker binds. diff --git a/pkg/volume/volume_linux.go b/pkg/volume/volume_linux.go index 0ce3ba4ae04..58a1edad6e2 100644 --- a/pkg/volume/volume_linux.go +++ b/pkg/volume/volume_linux.go @@ -23,7 +23,6 @@ import ( "context" "fmt" "path/filepath" - "sync/atomic" "syscall" "os" @@ -44,19 +43,7 @@ const ( progressReportDuration = 60 * time.Second ) -type VolumeOwnership struct { - mounter Mounter - dir string - fsGroup *int64 - fsGroupChangePolicy *v1.PodFSGroupChangePolicy - completionCallback func(types.CompleteFuncParam) - - // for monitoring progress of permission change operation - pod *v1.Pod - fileCounter atomic.Int64 - recorder record.EventRecorder -} - +// NewVolumeOwnership returns an interface that can be used to recursively change volume permissions and ownership func NewVolumeOwnership(mounter Mounter, dir string, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy, completeFunc func(types.CompleteFuncParam)) *VolumeOwnership { vo := &VolumeOwnership{ mounter: mounter, diff --git a/pkg/volume/volume_unsupported.go b/pkg/volume/volume_unsupported.go index 4cef01bad8d..cfedeaa3221 100644 --- a/pkg/volume/volume_unsupported.go +++ b/pkg/volume/volume_unsupported.go @@ -21,11 +21,19 @@ package volume import ( v1 "k8s.io/api/core/v1" + "k8s.io/client-go/tools/record" "k8s.io/kubernetes/pkg/volume/util/types" ) -// SetVolumeOwnership sets the ownership of a volume to the specified user and group. -// It typically modifies the user and group ownership of the volume's file system. -func SetVolumeOwnership(mounter Mounter, dir string, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy, completeFunc func(types.CompleteFuncParam)) error { +// NewVolumeOwnership returns an interface that can be used to recursively change volume permissions and ownership +func NewVolumeOwnership(mounter Mounter, dir string, fsGroup *int64, fsGroupChangePolicy *v1.PodFSGroupChangePolicy, completeFunc func(types.CompleteFuncParam)) *VolumeOwnership { + return nil +} + +func (vo *VolumeOwnership) AddProgressNotifier(pod *v1.Pod, recorder record.EventRecorder) *VolumeOwnership { + return vo +} + +func (vo *VolumeOwnership) ChangePermissions() error { return nil } From f7c179929735702a1e6d209837a67c76c182e88c Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Mon, 24 Feb 2025 16:21:11 -0500 Subject: [PATCH 5/7] Handle returned error as placeholder variables for now --- pkg/volume/emptydir/empty_dir.go | 2 +- pkg/volume/fc/disk_manager.go | 3 ++- pkg/volume/flexvolume/mounter.go | 2 +- pkg/volume/git_repo/git_repo.go | 3 ++- pkg/volume/iscsi/disk_manager.go | 2 +- pkg/volume/portworx/portworx.go | 2 +- 6 files changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/volume/emptydir/empty_dir.go b/pkg/volume/emptydir/empty_dir.go index 3c64e6af6d3..a9128f661fa 100644 --- a/pkg/volume/emptydir/empty_dir.go +++ b/pkg/volume/emptydir/empty_dir.go @@ -280,7 +280,7 @@ func (ed *emptyDir) SetUpAt(dir string, mounterArgs volume.MounterArgs) error { } ownershipChanger := volume.NewVolumeOwnership(ed, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(ed.plugin, nil)) - ownershipChanger.ChangePermissions() + _ = ownershipChanger.ChangePermissions() // If setting up the quota fails, just log a message but don't actually error out. // We'll use the old du mechanism in this case, at least until we support diff --git a/pkg/volume/fc/disk_manager.go b/pkg/volume/fc/disk_manager.go index 597d3c0a430..05b2df553ec 100644 --- a/pkg/volume/fc/disk_manager.go +++ b/pkg/volume/fc/disk_manager.go @@ -92,7 +92,8 @@ func diskSetUp(manager diskManager, b fcDiskMounter, volPath string, mounter mou if !b.readOnly { ownershipChanger := volume.NewVolumeOwnership(&b, volPath, fsGroup, fsGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) - ownershipChanger.ChangePermissions() + // TODO: Handle error returned here properly. + _ = ownershipChanger.ChangePermissions() } return nil diff --git a/pkg/volume/flexvolume/mounter.go b/pkg/volume/flexvolume/mounter.go index cd8b4860bbc..6b240887641 100644 --- a/pkg/volume/flexvolume/mounter.go +++ b/pkg/volume/flexvolume/mounter.go @@ -96,7 +96,7 @@ func (f *flexVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterArgs) if f.plugin.capabilities.FSGroup { // fullPluginName helps to distinguish different driver from flex volume plugin ownershipChanger := volume.NewVolumeOwnership(f, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(f.plugin, f.spec)) - ownershipChanger.ChangePermissions() + _ = ownershipChanger.ChangePermissions() } } diff --git a/pkg/volume/git_repo/git_repo.go b/pkg/volume/git_repo/git_repo.go index ad37d6e8bdb..3a4bf850f58 100644 --- a/pkg/volume/git_repo/git_repo.go +++ b/pkg/volume/git_repo/git_repo.go @@ -230,7 +230,8 @@ func (b *gitRepoVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterArg } ownershipChanger := volume.NewVolumeOwnership(b, dir, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) - ownershipChanger.ChangePermissions() + // We do not care about return value, this plugin is deprecated + _ = ownershipChanger.ChangePermissions() volumeutil.SetReady(b.getMetaDir()) return nil } diff --git a/pkg/volume/iscsi/disk_manager.go b/pkg/volume/iscsi/disk_manager.go index 397bae22f71..4f4ff341e71 100644 --- a/pkg/volume/iscsi/disk_manager.go +++ b/pkg/volume/iscsi/disk_manager.go @@ -99,7 +99,7 @@ func diskSetUp(manager diskManager, b iscsiDiskMounter, volPath string, mounter if !b.readOnly { // This code requires larger refactor to monitor progress of ownership change ownershipChanger := volume.NewVolumeOwnership(&b, volPath, fsGroup, fsGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) - ownershipChanger.ChangePermissions() + _ = ownershipChanger.ChangePermissions() } return nil diff --git a/pkg/volume/portworx/portworx.go b/pkg/volume/portworx/portworx.go index 496db552925..bc56928c517 100644 --- a/pkg/volume/portworx/portworx.go +++ b/pkg/volume/portworx/portworx.go @@ -333,7 +333,7 @@ func (b *portworxVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterAr if !b.readOnly { // Since portworxVolume is in process of being removed from in-tree, we avoid larger refactor to add progress tracking for ownership operation ownershipChanger := volume.NewVolumeOwnership(b, dir, mounterArgs.FsGroup, mounterArgs.FSGroupChangePolicy, util.FSGroupCompleteHook(b.plugin, nil)) - ownershipChanger.ChangePermissions() + _ = ownershipChanger.ChangePermissions() } klog.Infof("Portworx Volume %s setup at %s", b.volumeID, dir) return nil From b0dc96e71ec12b5e804cd9f407c84a690956ec70 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Wed, 26 Feb 2025 13:27:32 -0500 Subject: [PATCH 6/7] Add unit tests for progress tracking and remove fullpath from reporting --- pkg/volume/volume_linux.go | 28 +++++-- pkg/volume/volume_linux_test.go | 130 +++++++++++++++++++++++++++++++- 2 files changed, 152 insertions(+), 6 deletions(-) diff --git a/pkg/volume/volume_linux.go b/pkg/volume/volume_linux.go index 58a1edad6e2..9d023abfa05 100644 --- a/pkg/volume/volume_linux.go +++ b/pkg/volume/volume_linux.go @@ -23,6 +23,7 @@ import ( "context" "fmt" "path/filepath" + "strings" "syscall" "os" @@ -39,8 +40,14 @@ const ( rwMask = os.FileMode(0660) roMask = os.FileMode(0440) execMask = os.FileMode(0110) +) - progressReportDuration = 60 * time.Second +var ( + // function that will be used for changing file permissions on linux + // mainly stored here as a variable so as it can replaced in tests + filePermissionChangeFunc = changeFilePermission + progressReportDuration = 60 * time.Second + firstEventReportDuration = 30 * time.Second ) // NewVolumeOwnership returns an interface that can be used to recursively change volume permissions and ownership @@ -75,7 +82,7 @@ func (vo *VolumeOwnership) ChangePermissions() error { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - timer := time.AfterFunc(30*time.Second, func() { + timer := time.AfterFunc(firstEventReportDuration, func() { vo.initiateProgressMonitor(ctx) }) defer timer.Stop() @@ -96,7 +103,7 @@ func (vo *VolumeOwnership) changePermissionsRecursively() error { return err } vo.fileCounter.Add(1) - return changeFilePermission(path, vo.fsGroup, vo.mounter.GetAttributes().ReadOnly, info) + return filePermissionChangeFunc(path, vo.fsGroup, vo.mounter.GetAttributes().ReadOnly, info) }) if vo.completionCallback != nil { @@ -108,7 +115,8 @@ func (vo *VolumeOwnership) changePermissionsRecursively() error { } func (vo *VolumeOwnership) monitorProgress(ctx context.Context) { - msg := fmt.Sprintf("Setting volume ownership for %s is taking longer than expected, consider using OnRootMismatch - https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#configure-volume-permission-and-ownership-change-policy-for-pods", vo.dir) + dirName := getDirnameToReport(vo.dir, string(vo.pod.UID)) + msg := fmt.Sprintf("Setting volume ownership for %s is taking longer than expected, consider using OnRootMismatch - https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#configure-volume-permission-and-ownership-change-policy-for-pods", dirName) vo.recorder.Event(vo.pod, v1.EventTypeWarning, events.VolumePermissionChangeInProgress, msg) ticker := time.NewTicker(progressReportDuration) defer ticker.Stop() @@ -122,8 +130,18 @@ func (vo *VolumeOwnership) monitorProgress(ctx context.Context) { } } +// report everything after podUID in dir string, including podUID +func getDirnameToReport(dir, podUID string) string { + podUIDIndex := strings.Index(dir, podUID) + if podUIDIndex == -1 { + return dir + } + return dir[podUIDIndex:] +} + func (vo *VolumeOwnership) logWarning() { - msg := fmt.Sprintf("Setting volume ownership for %s, processed %d files.", vo.dir, vo.fileCounter.Load()) + dirName := getDirnameToReport(vo.dir, string(vo.pod.UID)) + msg := fmt.Sprintf("Setting volume ownership for %s, processed %d files.", dirName, vo.fileCounter.Load()) klog.Warning(msg) vo.recorder.Event(vo.pod, v1.EventTypeWarning, events.VolumePermissionChangeInProgress, msg) } diff --git a/pkg/volume/volume_linux_test.go b/pkg/volume/volume_linux_test.go index 5576fbbc7a4..6ef7c614d20 100644 --- a/pkg/volume/volume_linux_test.go +++ b/pkg/volume/volume_linux_test.go @@ -25,8 +25,11 @@ import ( "path/filepath" "syscall" "testing" + "time" v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/record" utiltesting "k8s.io/client-go/util/testing" ) @@ -286,6 +289,7 @@ func TestSetVolumeOwnershipMode(t *testing.T) { } defer os.RemoveAll(tmpDir) + info, err := os.Lstat(tmpDir) if err != nil { t.Fatalf("error reading permission of tmpdir: %v", err) @@ -296,7 +300,7 @@ func TestSetVolumeOwnershipMode(t *testing.T) { t.Fatalf("error reading permission stats for tmpdir: %s", tmpDir) } - var expectedGid int64 = int64(stat.Gid) + var expectedGid = int64(stat.Gid) err = test.setupFunc(tmpDir) if err != nil { t.Errorf("for %s error running setup with: %v", test.description, err) @@ -316,6 +320,130 @@ func TestSetVolumeOwnershipMode(t *testing.T) { } } +func TestProgressTracking(t *testing.T) { + alwaysApplyPolicy := v1.FSGroupChangeAlways + var expectedGID int64 = 9999 + var permissionSleepDuration = 5 * time.Millisecond + // how often to report the events + progressReportDuration = 200 * time.Millisecond + firstEventReportDuration = 50 * time.Millisecond + + filePermissionChangeFunc = func(filename string, fsGroup *int64, _ bool, _ os.FileInfo) error { + t.Logf("Calling file permission change for %s with gid %d", filename, *fsGroup) + time.Sleep(permissionSleepDuration) + return nil + } + pod := &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod1", + UID: "pod1uid", + }, + Spec: v1.PodSpec{ + Volumes: []v1.Volume{ + { + Name: "volume-name", + VolumeSource: v1.VolumeSource{ + GCEPersistentDisk: &v1.GCEPersistentDiskVolumeSource{ + PDName: "fake-device1", + }, + }, + }, + }, + }, + } + + tests := []struct { + name string + filePermissionChangeTimeDuration time.Duration + totalWaitTime time.Duration + currentPod *v1.Pod + expectedEvents []string + }{ + { + name: "When permission change finishes quickly, no events should be logged", + filePermissionChangeTimeDuration: 30 * time.Millisecond, + totalWaitTime: 1 * time.Second, + currentPod: pod, + expectedEvents: []string{}, + }, + { + name: "When no pod is specified, no events should be logged", + filePermissionChangeTimeDuration: 300 * time.Millisecond, + totalWaitTime: 1 * time.Second, + currentPod: nil, + expectedEvents: []string{}, + }, + { + name: "When permission change takes loo long and pod is specified", + filePermissionChangeTimeDuration: 300 * time.Millisecond, + totalWaitTime: 1 * time.Second, + currentPod: pod, + expectedEvents: []string{ + "Warning VolumePermissionChangeInProgress Setting volume ownership for pod1uid/volumes/faketype is taking longer than expected, consider using OnRootMismatch - https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#configure-volume-permission-and-ownership-change-policy-for-pods", + "Warning VolumePermissionChangeInProgress Setting volume ownership for pod1uid/volumes/faketype, processed 1 files.", + }, + }, + } + + for i := range tests { + tc := tests[i] + t.Run(tc.name, func(t *testing.T) { + tmpDir, err := utiltesting.MkTmpdir("volume_linux_ownership") + if err != nil { + t.Fatalf("error creating temp dir: %v", err) + } + podUID := "placeholder" + if tc.currentPod != nil { + podUID = string(tc.currentPod.UID) + } + volumePath := filepath.Join(tmpDir, podUID, "volumes", "faketype") + err = os.MkdirAll(volumePath, 0770) + if err != nil { + t.Fatalf("error creating volumePath %s: %v", volumePath, err) + } + defer func() { + err := os.RemoveAll(tmpDir) + if err != nil { + t.Fatalf("error removing tmpDir %s: %v", tmpDir, err) + } + }() + + mounter := &localFakeMounter{path: "FAKE_DIR_DOESNT_EXIST"} // SetVolumeOwnership() must rely on tmpDir + + fakeRecorder := record.NewFakeRecorder(100) + recordedEvents := []string{} + + // Set how long file permission change takes + permissionSleepDuration = tc.filePermissionChangeTimeDuration + + ownershipChanger := NewVolumeOwnership(mounter, volumePath, &expectedGID, &alwaysApplyPolicy, nil) + if tc.currentPod != nil { + ownershipChanger.AddProgressNotifier(tc.currentPod, fakeRecorder) + } + err = ownershipChanger.ChangePermissions() + if err != nil { + t.Errorf("unexpected error: %+v", err) + } + time.Sleep(tc.totalWaitTime) + actualEventCount := len(fakeRecorder.Events) + if len(tc.expectedEvents) == 0 && actualEventCount != len(tc.expectedEvents) { + t.Errorf("expected 0 events got %d", actualEventCount) + } + + for range actualEventCount { + event := <-fakeRecorder.Events + recordedEvents = append(recordedEvents, event) + } + + for i, event := range tc.expectedEvents { + if event != recordedEvents[i] { + t.Errorf("expected event %d to be %s, got: %s", i, event, recordedEvents[i]) + } + } + }) + } +} + // verifyDirectoryPermission checks if given path has directory permissions // that is expected by k8s. If returns true if it does otherwise false func verifyDirectoryPermission(path string, readonly bool) bool { From 94f3b552bf31d6bbbd9042f3edb3abc078626cd7 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Wed, 26 Feb 2025 20:57:53 -0500 Subject: [PATCH 7/7] Fix linter warnings --- pkg/volume/volume_linux_test.go | 38 ++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/pkg/volume/volume_linux_test.go b/pkg/volume/volume_linux_test.go index 6ef7c614d20..171b7c63f00 100644 --- a/pkg/volume/volume_linux_test.go +++ b/pkg/volume/volume_linux_test.go @@ -126,8 +126,12 @@ func TestSkipPermissionChange(t *testing.T) { if err != nil { t.Fatalf("error creating temp dir: %v", err) } - - defer os.RemoveAll(tmpDir) + defer func() { + err := os.RemoveAll(tmpDir) + if err != nil { + t.Fatalf("error removing tmpDir %s: %v", tmpDir, err) + } + }() info, err := os.Lstat(tmpDir) if err != nil { @@ -141,12 +145,12 @@ func TestSkipPermissionChange(t *testing.T) { gid := stat.Gid - var expectedGid int64 + var expectedGID int64 if test.gidOwnerMatch { - expectedGid = int64(gid) + expectedGID = int64(gid) } else { - expectedGid = int64(gid + 3000) + expectedGID = int64(gid + 3000) } mask := rwMask @@ -169,7 +173,7 @@ func TestSkipPermissionChange(t *testing.T) { } mounter := &localFakeMounter{path: tmpDir} - ok = skipPermissionChange(mounter, tmpDir, &expectedGid, test.fsGroupChangePolicy) + ok = skipPermissionChange(mounter, tmpDir, &expectedGID, test.fsGroupChangePolicy) if ok != test.skipPermssion { t.Errorf("for %s expected skipPermission to be %v got %v", test.description, test.skipPermssion, ok) } @@ -288,7 +292,12 @@ func TestSetVolumeOwnershipMode(t *testing.T) { t.Fatalf("error creating temp dir: %v", err) } - defer os.RemoveAll(tmpDir) + defer func() { + err := os.RemoveAll(tmpDir) + if err != nil { + t.Fatalf("error removing tmpDir %s: %v", tmpDir, err) + } + }() info, err := os.Lstat(tmpDir) if err != nil { @@ -300,14 +309,14 @@ func TestSetVolumeOwnershipMode(t *testing.T) { t.Fatalf("error reading permission stats for tmpdir: %s", tmpDir) } - var expectedGid = int64(stat.Gid) + var expectedGID = int64(stat.Gid) err = test.setupFunc(tmpDir) if err != nil { t.Errorf("for %s error running setup with: %v", test.description, err) } mounter := &localFakeMounter{path: "FAKE_DIR_DOESNT_EXIST"} // SetVolumeOwnership() must rely on tmpDir - ownershipChanger := NewVolumeOwnership(mounter, tmpDir, &expectedGid, test.fsGroupChangePolicy, nil) + ownershipChanger := NewVolumeOwnership(mounter, tmpDir, &expectedGID, test.fsGroupChangePolicy, nil) err = ownershipChanger.ChangePermissions() if err != nil { t.Errorf("for %s error changing ownership with: %v", test.description, err) @@ -475,7 +484,7 @@ func TestSetVolumeOwnershipOwner(t *testing.T) { if currentUid != 0 { t.Skip("running as non-root") } - currentGid := os.Getgid() + currentGID := os.Getgid() tests := []struct { description string @@ -497,7 +506,7 @@ func TestSetVolumeOwnershipOwner(t *testing.T) { }, assertFunc: func(path string) error { filename := filepath.Join(path, "file.txt") - if !verifyFileOwner(filename, currentUid, currentGid) { + if !verifyFileOwner(filename, currentUid, currentGID) { return fmt.Errorf("invalid owner on %s", filename) } return nil @@ -559,7 +568,12 @@ func TestSetVolumeOwnershipOwner(t *testing.T) { t.Fatalf("error creating temp dir: %v", err) } - defer os.RemoveAll(tmpDir) + defer func() { + err := os.RemoveAll(tmpDir) + if err != nil { + t.Fatalf("error removing tmpDir %s: %v", tmpDir, err) + } + }() err = test.setupFunc(tmpDir) if err != nil {