controllers: add a new controller to handle provider upgrades #3131

rewantsoni · 2025-04-03T16:51:49Z

For Provider Mode update to 4.19, we are creating a new controller StorageConsumerUpdateController. This will look at the existing consumers for which the clients are in DF 4.18 and process these consumers to create a configMap and then update the consumer spec to include all the default classes. Once all the changes are made we will add an annotation to these consumers to imply that these are processed, and we can proceed with the reconciliation of these consumers.

nb-ohad · 2025-04-03T20:29:51Z

@rewantsoni please add a meaningful description to the PR

controllers/storageconsumer/storageconsumer_upgrade_controller.go

nb-ohad · 2025-04-03T20:40:00Z

controllers/storageconsumer/storageconsumer_upgrade_controller.go

+func (r *StorageConsumerUpgradeReconciler) SetupWithManager(mgr ctrl.Manager) error {
+
+	operatorVersionPredicate := predicate.Funcs{
+		CreateFunc: func(e event.TypedCreateEvent[client.Object]) bool {
+			if e.Object == nil {
+				return false
+			}
+			obj := e.Object.(*ocsv1alpha1.StorageConsumer)
+			return strings.HasPrefix(obj.Status.Client.OperatorVersion, "4.18") &&
+				obj.GetAnnotations()[util.StorageConsumer418ProcessedAnnotationKey] != util.StorageConsumer418ProcessedAnnotationProcessed
+		},
+		UpdateFunc: func(e event.UpdateEvent) bool {
+			if e.ObjectOld == nil || e.ObjectNew == nil {
+				return false
+			}
+			newObj := e.ObjectNew.(*ocsv1alpha1.StorageConsumer)
+			return strings.HasPrefix(newObj.Status.Client.OperatorVersion, "4.18") &&
+				newObj.GetAnnotations()[util.StorageConsumer418ProcessedAnnotationKey] != util.StorageConsumer418ProcessedAnnotationProcessed
+		},
+	}


You are mixing usage of predicate.Funcs (untyped) with typed events (events.TypedCreateEvent). And then you use to use the type as client.Client which does not make sense because it is the type you get when you use the untyped versionevent.CreateEvent.

If you want the predicate to process client.Client in your handler use:

Suggested change

func (r *StorageConsumerUpgradeReconciler) SetupWithManager(mgr ctrl.Manager) error {

operatorVersionPredicate := predicate.Funcs{

CreateFunc: func(e event.TypedCreateEvent[client.Object]) bool {

if e.Object == nil {

return false

}

obj := e.Object.(*ocsv1alpha1.StorageConsumer)

return strings.HasPrefix(obj.Status.Client.OperatorVersion, "4.18") &&

obj.GetAnnotations()[util.StorageConsumer418ProcessedAnnotationKey] != util.StorageConsumer418ProcessedAnnotationProcessed

},

UpdateFunc: func(e event.UpdateEvent) bool {

if e.ObjectOld == nil || e.ObjectNew == nil {

return false

}

newObj := e.ObjectNew.(*ocsv1alpha1.StorageConsumer)

return strings.HasPrefix(newObj.Status.Client.OperatorVersion, "4.18") &&

newObj.GetAnnotations()[util.StorageConsumer418ProcessedAnnotationKey] != util.StorageConsumer418ProcessedAnnotationProcessed

},

}

func (r *StorageConsumerUpgradeReconciler) SetupWithManager(mgr ctrl.Manager) error {

operatorVersionPredicate := predicate.Funcs{

CreateFunc: func(e event.CreateEvent) bool {

if e.Object == nil {

return false

}

obj := e.Object.(*ocsv1alpha1.StorageConsumer)

return strings.HasPrefix(obj.Status.Client.OperatorVersion, "4.18") &&

obj.GetAnnotations()[util.StorageConsumer418ProcessedAnnotationKey] != util.StorageConsumer418ProcessedAnnotationProcessed

},

UpdateFunc: func(e event.UpdateEvent) bool {

if e.ObjectOld == nil || e.ObjectNew == nil {

return false

}

newObj := e.ObjectNew.(*ocsv1alpha1.StorageConsumer)

return strings.HasPrefix(newObj.Status.Client.OperatorVersion, "4.18") &&

newObj.GetAnnotations()[util.StorageConsumer418ProcessedAnnotationKey] != util.StorageConsumer418ProcessedAnnotationProcessed

},

}

But becuase you anyway assume an ocsv1alpha1.StorageConsumer entity by casting into it. Then preferably do

Suggested change

func (r *StorageConsumerUpgradeReconciler) SetupWithManager(mgr ctrl.Manager) error {

operatorVersionPredicate := predicate.Funcs{

CreateFunc: func(e event.TypedCreateEvent[client.Object]) bool {

if e.Object == nil {

return false

}

obj := e.Object.(*ocsv1alpha1.StorageConsumer)

return strings.HasPrefix(obj.Status.Client.OperatorVersion, "4.18") &&

obj.GetAnnotations()[util.StorageConsumer418ProcessedAnnotationKey] != util.StorageConsumer418ProcessedAnnotationProcessed

},

UpdateFunc: func(e event.UpdateEvent) bool {

if e.ObjectOld == nil || e.ObjectNew == nil {

return false

}

newObj := e.ObjectNew.(*ocsv1alpha1.StorageConsumer)

return strings.HasPrefix(newObj.Status.Client.OperatorVersion, "4.18") &&

newObj.GetAnnotations()[util.StorageConsumer418ProcessedAnnotationKey] != util.StorageConsumer418ProcessedAnnotationProcessed

},

}

func (r *StorageConsumerUpgradeReconciler) SetupWithManager(mgr ctrl.Manager) error {

operatorVersionPredicate := predicate.TypedFuncs[ocsv1alpha1.StorageConsumer]{

CreateFunc: func(e event.TypedCreateEvent[ocsv1alpha1.StorageConsumer]) bool {

return strings.HasPrefix(e.obj.Status.Client.OperatorVersion, "4.18") &&

obj.GetAnnotations()[util.StorageConsumer418ProcessedAnnotationKey] != util.StorageConsumer418ProcessedAnnotationProcessed

},

UpdateFunc: func(e event.TypedUpdateEvent[ocsv1alpha1.StorageConsumer]) bool {

return strings.HasPrefix(e.newObj.Status.Client.OperatorVersion, "4.18") &&

newObj.GetAnnotations()[util.StorageConsumer418ProcessedAnnotationKey] != util.StorageConsumer418ProcessedAnnotationProcessed

},

}

Getting this error:

controllers/storageconsumer/storageconsumer_upgrade_controller.go:60:62: cannot use oldUnProcessedConsumerPredicate (variable of struct type predicate.TypedFuncs["github.com/red-hat-storage/ocs-operator/api/v4/v1alpha1".StorageConsumer]) as predicate.Predicate value in argument to builder.WithPredicates: predicate.TypedFuncs["github.com/red-hat-storage/ocs-operator/api/v4/v1alpha1".StorageConsumer] does not implement predicate.TypedPredicate[client.Object] (wrong type for method Create) have Create(event.TypedCreateEvent["github.com/red-hat-storage/ocs-operator/api/v4/v1alpha1".StorageConsumer]) bool want Create(event.TypedCreateEvent[client.Object]) bool

nb-ohad · 2025-04-03T20:41:03Z

controllers/storageconsumer/storageconsumer_upgrade_controller.go

+			}
+			obj := e.Object.(*ocsv1alpha1.StorageConsumer)
+			return strings.HasPrefix(obj.Status.Client.OperatorVersion, "4.18") &&
+				obj.GetAnnotations()[util.StorageConsumer418ProcessedAnnotationKey] != util.StorageConsumer418ProcessedAnnotationProcessed


I don't get this filter. Why are you trying to filter based on an annotation? and even if you do it does not make sense to look at the value where the mare existence of the annotation is the thing that is important

controllers/storageconsumer/storageconsumer_upgrade_controller.go

nb-ohad · 2025-04-03T20:55:59Z

controllers/storageconsumer/storageconsumer_upgrade_controller.go

+			{Name: util.GenerateNameForCephBlockPoolSC(storageCluster)},
+			{Name: util.GenerateNameForCephFilesystemSC(storageCluster)},


Not related to this PR, but why the suffix SC and not StorageClass? the other Generate calls have the full type of the CR at the end: SnapshotClass and GropSnapshotClass

I agree, I will raise a fix to rename all generate to have StorageClass as suffix

nb-ohad · 2025-04-03T20:57:02Z

controllers/storageconsumer/storageconsumer_upgrade_controller.go

+			{Name: util.GenerateNameForGroupSnapshotClass(storageCluster, util.RbdGroupSnapshotter)},
+			{Name: util.GenerateNameForGroupSnapshotClass(storageCluster, util.CephfsGroupSnapshotter)},
+		}
+		util.AddAnnotation(storageConsumer, util.StorageConsumer418ProcessedAnnotationKey, util.StorageConsumer418ProcessedAnnotationProcessed)


There is no point in having a value for this annotation. Use "" or "true"

I think you decided to use "", but that's come with its own issue of have a need to get a secondary argument when accessing the annotation map to check for existence. While it is not a bad thing, it does make the code more cumbersome.

Let's use strconv.FormatBool(true) instead

controllers/storageconsumer/storageconsumer_upgrade_controller.go

controllers/util/storageconsumer.go

openshift-ci · 2025-04-04T05:07:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rewantsoni
Once this PR has been reviewed and has the lgtm label, please ask for approval from nb-ohad. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nb-ohad · 2025-04-04T11:40:31Z

controllers/storageconsumer/storageconsumer_upgrade_controller.go

+		// For Provider Mode we supported creating one rbdClaim, generating the clientProfile, rns name, secret names
+		// based on what we decided


How does a reader understand the sentence based on what we decided? With comments you should assume the reader have no outside context

nb-ohad · 2025-04-04T11:41:41Z

controllers/storageconsumer/storageconsumer_upgrade_controller.go

+		rbdStorageRequestHash := getStorageRequestName(string(storageConsumer.UID), rbdClaimName)
+		rbdNodeSecretName := storageClaimCephCsiSecretName("node", rbdStorageRequestHash)
+		rbdProvisionerSecretName := storageClaimCephCsiSecretName("provisioner", rbdStorageRequestHash)
+		md5Sum = md5.Sum([]byte(rbdStorageRequestHash))


don't use the same var name again in the same scope

nb-ohad · 2025-04-04T11:43:56Z

controllers/storageconsumer/storageconsumer_upgrade_controller.go

+	if err := r.Client.Update(ctx, storageConsumer); err != nil {
+		return reconcile.Result{}, err
+	}


There is no point in returning an error if the error you get back is not found

nb-ohad · 2025-04-04T11:48:05Z

controllers/storageconsumer/storageconsumer_upgrade_controller.go

+
+	consumerConfigMapName := fmt.Sprintf("storageconsumer-%v", util.FnvHash(storageConsumer.Name))
+
+	if storageConsumer.Spec.ResourceNameMappingConfigMap.Name == consumerConfigMapName {


You should not check for a specific value, but check for having a value.

nb-ohad · 2025-04-04T11:49:15Z

controllers/storageconsumer/storageconsumer_upgrade_controller.go

+		return reconcile.Result{}, nil
+	}
+
+	consumerConfigMapName := fmt.Sprintf("storageconsumer-%v", util.FnvHash(storageConsumer.Name))


Please calculate this near the creation code of the config map where it is needed

Signed-off-by: Rewant Soni <[email protected]>

openshift-ci · 2025-04-04T14:43:05Z

@rewantsoni: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/ci-bundle-ocs-operator-bundle	`f0343b7`	link	true	`/test ci-bundle-ocs-operator-bundle`
ci/prow/images	`f0343b7`	link	true	`/test images`
ci/prow/ocs-operator-bundle-e2e-aws	`f0343b7`	link	true	`/test ocs-operator-bundle-e2e-aws`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

rewantsoni force-pushed the consumer-update-controller branch from 21eb5de to 4bdf679 Compare April 3, 2025 17:05

nb-ohad requested changes Apr 3, 2025

View reviewed changes

openshift-ci bot assigned nb-ohad Apr 3, 2025

rewantsoni force-pushed the consumer-update-controller branch from 4bdf679 to 3eb6c0b Compare April 4, 2025 05:07

rewantsoni force-pushed the consumer-update-controller branch 5 times, most recently from 9455cd5 to a2e0228 Compare April 4, 2025 06:20

nb-ohad reviewed Apr 4, 2025

View reviewed changes

rewantsoni force-pushed the consumer-update-controller branch from a2e0228 to 354db75 Compare April 4, 2025 11:57

controllers: add a new controller to handle provider upgrades

f0343b7

Signed-off-by: Rewant Soni <[email protected]>

rewantsoni force-pushed the consumer-update-controller branch from 354db75 to f0343b7 Compare April 4, 2025 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

controllers: add a new controller to handle provider upgrades #3131

controllers: add a new controller to handle provider upgrades #3131

rewantsoni commented Apr 3, 2025 •

edited

Loading

nb-ohad commented Apr 3, 2025 •

edited

Loading

nb-ohad Apr 3, 2025

rewantsoni Apr 4, 2025

nb-ohad Apr 3, 2025

nb-ohad Apr 3, 2025

rewantsoni Apr 4, 2025

nb-ohad Apr 3, 2025

nb-ohad Apr 4, 2025

openshift-ci bot commented Apr 4, 2025

nb-ohad Apr 4, 2025

nb-ohad Apr 4, 2025

nb-ohad Apr 4, 2025

nb-ohad Apr 4, 2025

nb-ohad Apr 4, 2025

openshift-ci bot commented Apr 4, 2025

		{Name: util.GenerateNameForCephBlockPoolSC(storageCluster)},
		{Name: util.GenerateNameForCephFilesystemSC(storageCluster)},

		// For Provider Mode we supported creating one rbdClaim, generating the clientProfile, rns name, secret names
		// based on what we decided


		consumerConfigMapName := fmt.Sprintf("storageconsumer-%v", util.FnvHash(storageConsumer.Name))

		if storageConsumer.Spec.ResourceNameMappingConfigMap.Name == consumerConfigMapName {

controllers: add a new controller to handle provider upgrades #3131

Are you sure you want to change the base?

controllers: add a new controller to handle provider upgrades #3131

Conversation

rewantsoni commented Apr 3, 2025 • edited Loading

nb-ohad commented Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 4, 2025

rewantsoni commented Apr 3, 2025 •

edited

Loading

nb-ohad commented Apr 3, 2025 •

edited

Loading