From b51cbb1d17ca680139d306b43a88c723e72a6103 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Mon, 24 Feb 2025 12:48:38 -0500 Subject: [PATCH] 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) }