Skip to content

Replace MutatorConfig with CreatePVCPrimeFn that allows custom creation of a PVC #201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Replace MutatorConfig with CreatePVCPrimeFn that allows custom creation of a PVC #201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 25 additions & 45 deletions populator-machinery/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ const (
reasonPopulateOperationFailed = "PopulateOperationFailed"
reasonPopulateOperationFinished = "PopulateOperationFinished"
reasonPVCPrimeCreationError = "PopulatorPVCPrimeCreationError"
reasonPVCPrimeMutatorError = "reasonPVCPrimeMutatorError"
reasonWaitForDataPopulationFinished = "PopulatorWaitForDataPopulationFinished"
reasonStorageClassCreationError = "PopulatorStorageClassCreationError"
reasonDataSourceNotFound = "PopulatorDataSourceNotFound"
Expand Down Expand Up @@ -117,7 +116,6 @@ type controller struct {
referenceGrantSynced cache.InformerSynced
podConfig *PodConfig
providerFunctionConfig *ProviderFunctionConfig
mutatorConfig *MutatorConfig
crossNamespace bool
providerMetricManager *ProviderMetricManager
}
Expand All @@ -142,9 +140,6 @@ type VolumePopulatorConfig struct {
// ProviderFunctionConfig is the configuration for invoking provider functions. Either PodConfig or ProviderFunctionConfig should
// be specified. PodConfig and ProviderFunctionConfig can't be provided at the same time
ProviderFunctionConfig *ProviderFunctionConfig
// MutatorConfig is the configuration for invoking mutator functions. You can specify your own mutator functions to modify the
// Kubernetes resources used for volume population
MutatorConfig *MutatorConfig
// ProviderMetricManager is the manager for provider specific metric handling
ProviderMetricManager *ProviderMetricManager
// CrossNamespace indicates if the populator supports data sources located in namespaces different than the PVC's namespace.
Expand All @@ -170,6 +165,9 @@ type PodConfig struct {
}

type ProviderFunctionConfig struct {
// CreatePVCPrimeFn is the provider specific PVCPRime creation function
// Returns a PVC object that should be created by the caller, otherwise an error if creation failed and should be retried.
CreatePVCPrimeFn func(context.Context, PopulatorParams) (*corev1.PersistentVolumeClaim, error)
// PopulateFn is the provider specific data population function
PopulateFn func(context.Context, PopulatorParams) error
// PopulateCompleteFn is the provider specific data population completeness check function, return true when data transfer gets completed
Expand All @@ -192,19 +190,6 @@ type PopulatorParams struct {
Recorder record.EventRecorder
}

type MutatorConfig struct {
// PvcPrimeMutator is the mutator function for pvcPrime. The function gets called to modify the PVC object before pvcPrime gets created.
PvcPrimeMutator func(PvcPrimeMutatorParams) (*corev1.PersistentVolumeClaim, error)
}

// PvcPrimeMutatorParams includes the parameters passing to the PvcPrimeMutator function
type PvcPrimeMutatorParams struct {
// PvcPrime is the temporary PVC created by volume populator
PvcPrime *corev1.PersistentVolumeClaim
// StorageClass is the original StorageClass Pvc refer to
StorageClass *storagev1.StorageClass
}

func RunController(masterURL, kubeconfig, imageName, httpEndpoint, metricsPath, namespace, prefix string,
gk schema.GroupKind, gvr schema.GroupVersionResource, mountPath, devicePath string,
populatorArgs func(bool, *unstructured.Unstructured) ([]string, error),
Expand Down Expand Up @@ -308,7 +293,6 @@ func RunControllerWithConfig(vpcfg VolumePopulatorConfig) {
referenceGrantSynced: referenceGrants.Informer().HasSynced,
podConfig: vpcfg.PodConfig,
providerFunctionConfig: vpcfg.ProviderFunctionConfig,
mutatorConfig: vpcfg.MutatorConfig,
crossNamespace: vpcfg.CrossNamespace,
providerMetricManager: vpcfg.ProviderMetricManager,
}
Expand Down Expand Up @@ -714,37 +698,33 @@ func (c *controller) syncPvc(ctx context.Context, key, pvcNamespace, pvcName str
// TODO: Handle PVC' update while the original PVC changed
// If the PVC is unbound and PVC' doesn't exist yet, create PVC'
if "" == pvc.Spec.VolumeName && pvcPrime == nil {
pvcPrime = &corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: pvcPrimeName,
Namespace: c.populatorNamespace,
},
Spec: corev1.PersistentVolumeClaimSpec{
AccessModes: pvc.Spec.AccessModes,
Resources: pvc.Spec.Resources,
StorageClassName: pvc.Spec.StorageClassName,
VolumeMode: pvc.Spec.VolumeMode,
},
}
if waitForFirstConsumer {
pvcPrime.Annotations = map[string]string{
annSelectedNode: nodeName,
}
}
if c.mutatorConfig != nil && c.mutatorConfig.PvcPrimeMutator != nil {
mp := PvcPrimeMutatorParams{
PvcPrime: pvcPrime,
StorageClass: storageClass,
}
pvcPrime, err = c.mutatorConfig.PvcPrimeMutator(mp)
if err != nil {
c.recorder.Eventf(pvc, corev1.EventTypeWarning, reasonPVCPrimeMutatorError, "Failed to mutate populator pvcPrime: %s", err)
if c.providerFunctionConfig != nil && c.providerFunctionConfig.CreatePVCPrimeFn != nil {
if pvcPrime, err = c.providerFunctionConfig.CreatePVCPrimeFn(ctx, *params); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing I'm not sure about is that there is a check of "" == pvcPrime.Spec.VolumeName[1]. Previous implementation was based on dynamic provisioning PV which will guarantee the check only get passed when the PVC' gets created. With CreatePVCPrimeFn if there is a static provisioned PV, is it possible that pvcPrime.Spec.VolumeName gets set, kubeClient.create.pvc' gets stuck for some reason, but the "" == pvcPrime.Spec.VolumeName pass?

[1]https://github.com/kubernetes-csi/lib-volume-populator/blob/master/populator-machinery/controller.go#L773

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be up to the client implementation. But, if the client uses static provisioning, the proper way to do this is to create a PV with a claimRef, and then a PVC without a volumeName set. The volume controller then binds the two, and sets volumeName.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a recommendation but not a requirement that users leave PVC.spec.volumeName unset.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flow probably should get added to the KEP or README later.

c.recorder.Eventf(pvc, corev1.EventTypeWarning, reasonPVCPrimeCreationError, "Failed to create PVC Prime: %s", err)
return err
}
if pvcPrime == nil {
c.recorder.Eventf(pvc, corev1.EventTypeWarning, reasonPVCPrimeMutatorError, "pvcPrime must not be nil")
c.recorder.Eventf(pvc, corev1.EventTypeWarning, reasonPVCPrimeCreationError, "pvcPrime must not be nil")
return fmt.Errorf("pvcPrime must not be nil")
}
} else {
pvcPrime = &corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: pvcPrimeName,
Namespace: c.populatorNamespace,
},
Spec: corev1.PersistentVolumeClaimSpec{
AccessModes: pvc.Spec.AccessModes,
Resources: pvc.Spec.Resources,
StorageClassName: pvc.Spec.StorageClassName,
VolumeMode: pvc.Spec.VolumeMode,
},
}
if waitForFirstConsumer {
pvcPrime.Annotations = map[string]string{
annSelectedNode: nodeName,
}
}
}
pvcPrime, err = c.kubeClient.CoreV1().PersistentVolumeClaims(c.populatorNamespace).Create(ctx, pvcPrime, metav1.CreateOptions{})
if err != nil {
Expand Down
88 changes: 41 additions & 47 deletions populator-machinery/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,14 @@ type testCase struct {
initialObjects []runtime.Object
// Args for the populator pod
populatorArgs func(b bool, u *unstructured.Unstructured) ([]string, error)
// Provider specific PVC prime creation function
pvcPrimeCreateFn func(context.Context, PopulatorParams) (*corev1.PersistentVolumeClaim, error)
// Provider specific data population function
populateFn func(context.Context, PopulatorParams) error
// Provider specific data population completeness check function, return true when data transfer gets completed.
populateCompleteFn func(context.Context, PopulatorParams) (bool, error)
// The original PVC gets deleted or not
pvcDeleted bool
// PvcPrimeMutator is the mutator function for pvcPrime
pvcPrimeMutator func(PvcPrimeMutatorParams) (*v1.PersistentVolumeClaim, error)
// Expected errors
expectedResult error
// Expected keys in the notifyMap
Expand Down Expand Up @@ -105,7 +105,7 @@ const (
testProvisioner = "test.provisioner"
testPopulationOperationStartFailed = "Test populate operation start failed"
testPopulateCompleteFailed = "Test populate operation complete failed"
testMutatePVCPrimeFailed = "Test mutate pvcPrime failed"
testPvcPrimeCreateFailed = "Test create pvcPrime failed"
dataSourceKey = "unstructured/" + testPvcNamespace + "/" + testDataSourceName
storageClassKey = "sc/" + testStorageClassName
podKey = "pod/" + testVpWorkingNamespace + "/" + testPodName
Expand Down Expand Up @@ -252,18 +252,18 @@ func populateCompleteSuccess(ctx context.Context, p PopulatorParams) (bool, erro
return true, nil
}

func pvcPrimeMutateAccessModeRWX(mp PvcPrimeMutatorParams) (*v1.PersistentVolumeClaim, error) {
accessMode := v1.ReadWriteMany
mp.PvcPrime.Spec.AccessModes[0] = accessMode
return mp.PvcPrime, nil
func pvcPrimeCreateError(ctx context.Context, p PopulatorParams) (*corev1.PersistentVolumeClaim, error) {
return nil, fmt.Errorf(testPvcPrimeCreateFailed)
}

func pvcPrimeMutateError(mp PvcPrimeMutatorParams) (*v1.PersistentVolumeClaim, error) {
return mp.PvcPrime, fmt.Errorf(testMutatePVCPrimeFailed)
func pvcPrimeCreateNil(ctx context.Context, p PopulatorParams) (*corev1.PersistentVolumeClaim, error) {
return nil, nil
}

func pvcPrimeMutatePVCPrimeNil(mp PvcPrimeMutatorParams) (*v1.PersistentVolumeClaim, error) {
return nil, nil
func pvcPrimeCreateSuccess(pvcPrime *corev1.PersistentVolumeClaim) func(ctx context.Context, p PopulatorParams) (*corev1.PersistentVolumeClaim, error) {
return func(ctx context.Context, p PopulatorParams) (*corev1.PersistentVolumeClaim, error) {
return pvcPrime, nil
}
}

func initTest(test testCase) (
Expand Down Expand Up @@ -310,19 +310,14 @@ func initTest(test testCase) (
}
}

var providerFunctionConfig *ProviderFunctionConfig
var providerFunctionConfig *ProviderFunctionConfig = &ProviderFunctionConfig{}
if test.populateFn != nil || test.populateCompleteFn != nil {
providerFunctionConfig = &ProviderFunctionConfig{
PopulateFn: test.populateFn,
PopulateCompleteFn: test.populateCompleteFn,
}
providerFunctionConfig.PopulateFn = test.populateFn
providerFunctionConfig.PopulateCompleteFn = test.populateCompleteFn
}

var mutatorConfig *MutatorConfig
if test.pvcPrimeMutator != nil {
mutatorConfig = &MutatorConfig{
PvcPrimeMutator: test.pvcPrimeMutator,
}
if test.pvcPrimeCreateFn != nil {
providerFunctionConfig.CreatePVCPrimeFn = test.pvcPrimeCreateFn
}

c := &controller{
Expand Down Expand Up @@ -350,7 +345,6 @@ func initTest(test testCase) (
referenceGrantSynced: referenceGrants.Informer().HasSynced,
podConfig: podConfig,
providerFunctionConfig: providerFunctionConfig,
mutatorConfig: mutatorConfig,
}
return c, pvcInformer, unstInformer, scInformer, podInformer, pvInformer
}
Expand All @@ -367,7 +361,7 @@ func compareResult(want error, got error) bool {

func compareNotifyMap(want []string, got map[string]*stringSet) error {
if len(want) != len(got) {
return fmt.Errorf("The number of keys expected is different from actual. Expect %v, got %v", len(want), len(got))
return fmt.Errorf("The number of keys expected is different from actual. Expect %v, got %v. Want %v: Got: %v", len(want), len(got), want, got)
}
for _, key := range want {
if got[key] == nil {
Expand Down Expand Up @@ -682,10 +676,10 @@ func TestSyncPvcWithPopulatorPod(t *testing.T) {
ust(),
sc(testStorageClassName, storagev1.VolumeBindingImmediate),
},
populatorArgs: populatorArgs,
pvcPrimeMutator: pvcPrimeMutateAccessModeRWX,
expectedResult: nil,
expectedKeys: []string{podKey, pvcPrimeKey},
populatorArgs: populatorArgs,
pvcPrimeCreateFn: pvcPrimeCreateSuccess(pvc(testPvcPrimeName, testVpWorkingNamespace, "", testStorageClassName, "", "", []string{}, nil, "", v1.ReadWriteMany)),
expectedResult: nil,
expectedKeys: []string{podKey, pvcPrimeKey},
expectedObjects: &vpObjects{
pvc: pvc(testPvcName, testPvcNamespace, "", testStorageClassName, "", testPvcUid, []string{pvFinalizer, vpFinalizer},
dsf(testApiGroup, testDatasourceKind, testDataSourceName, testPvcNamespace), "", v1.ReadWriteOnce),
Expand All @@ -710,10 +704,10 @@ func TestSyncPvcWithPopulatorPod(t *testing.T) {
ust(),
sc(testStorageClassName, storagev1.VolumeBindingImmediate),
},
populatorArgs: populatorArgs,
pvcPrimeMutator: pvcPrimeMutateError,
expectedResult: fmt.Errorf(testMutatePVCPrimeFailed),
expectedKeys: []string{podKey, pvcPrimeKey},
populatorArgs: populatorArgs,
pvcPrimeCreateFn: pvcPrimeCreateError,
expectedResult: fmt.Errorf(testPvcPrimeCreateFailed),
expectedKeys: []string{podKey, pvcPrimeKey},
expectedObjects: &vpObjects{
pvc: pvc(testPvcName, testPvcNamespace, "", testStorageClassName, "", testPvcUid, []string{pvFinalizer},
dsf(testApiGroup, testDatasourceKind, testDataSourceName, testPvcNamespace), "", v1.ReadWriteOnce),
Expand All @@ -730,10 +724,10 @@ func TestSyncPvcWithPopulatorPod(t *testing.T) {
ust(),
sc(testStorageClassName, storagev1.VolumeBindingImmediate),
},
populatorArgs: populatorArgs,
pvcPrimeMutator: pvcPrimeMutatePVCPrimeNil,
expectedResult: fmt.Errorf("pvcPrime must not be nil"),
expectedKeys: []string{podKey, pvcPrimeKey},
populatorArgs: populatorArgs,
pvcPrimeCreateFn: pvcPrimeCreateNil,
expectedResult: fmt.Errorf("pvcPrime must not be nil"),
expectedKeys: []string{podKey, pvcPrimeKey},
expectedObjects: &vpObjects{
pvc: pvc(testPvcName, testPvcNamespace, "", testStorageClassName, "", testPvcUid, []string{pvFinalizer},
dsf(testApiGroup, testDatasourceKind, testDataSourceName, testPvcNamespace), "", v1.ReadWriteOnce),
Expand Down Expand Up @@ -1076,10 +1070,10 @@ func TestSyncPvcWithProviderImplementation(t *testing.T) {
ust(),
sc(testStorageClassName, storagev1.VolumeBindingImmediate),
},
pvcPrimeMutator: pvcPrimeMutateAccessModeRWX,
populateFn: populateOperationStartError,
expectedResult: nil,
expectedKeys: []string{pvcPrimeKey},
pvcPrimeCreateFn: pvcPrimeCreateSuccess(pvc(testPvcPrimeName, testVpWorkingNamespace, "", testStorageClassName, "", "", []string{}, nil, "", v1.ReadWriteMany)),
populateFn: populateOperationStartError,
expectedResult: nil,
expectedKeys: []string{pvcPrimeKey},
expectedObjects: &vpObjects{
pvc: pvc(testPvcName, testPvcNamespace, "", testStorageClassName, "", testPvcUid, []string{pvFinalizer, vpFinalizer},
dsf(testApiGroup, testDatasourceKind, testDataSourceName, testPvcNamespace), "", v1.ReadWriteOnce),
Expand All @@ -1103,10 +1097,10 @@ func TestSyncPvcWithProviderImplementation(t *testing.T) {
ust(),
sc(testStorageClassName, storagev1.VolumeBindingImmediate),
},
pvcPrimeMutator: pvcPrimeMutateError,
populateFn: populateOperationStartError,
expectedResult: fmt.Errorf(testMutatePVCPrimeFailed),
expectedKeys: []string{pvcPrimeKey},
pvcPrimeCreateFn: pvcPrimeCreateError,
populateFn: populateOperationStartError,
expectedResult: fmt.Errorf(testPvcPrimeCreateFailed),
expectedKeys: []string{pvcPrimeKey},
expectedObjects: &vpObjects{
pvc: pvc(testPvcName, testPvcNamespace, "", testStorageClassName, "", testPvcUid, []string{pvFinalizer},
dsf(testApiGroup, testDatasourceKind, testDataSourceName, testPvcNamespace), "", v1.ReadWriteOnce),
Expand All @@ -1123,10 +1117,10 @@ func TestSyncPvcWithProviderImplementation(t *testing.T) {
ust(),
sc(testStorageClassName, storagev1.VolumeBindingImmediate),
},
pvcPrimeMutator: pvcPrimeMutatePVCPrimeNil,
populateFn: populateOperationStartError,
expectedResult: fmt.Errorf("pvcPrime must not be nil"),
expectedKeys: []string{pvcPrimeKey},
pvcPrimeCreateFn: pvcPrimeCreateNil,
populateFn: populateOperationStartError,
expectedResult: fmt.Errorf("pvcPrime must not be nil"),
expectedKeys: []string{pvcPrimeKey},
expectedObjects: &vpObjects{
pvc: pvc(testPvcName, testPvcNamespace, "", testStorageClassName, "", testPvcUid, []string{pvFinalizer},
dsf(testApiGroup, testDatasourceKind, testDataSourceName, testPvcNamespace), "", v1.ReadWriteOnce),
Expand Down