Change plugin interfaces to use progress monitoring

Author: gnufied

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()
 		}
 	}
 

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
 }

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

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")
 	}

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

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

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 {

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 {

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

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 {