PWX-31470: Clusterwide operator CR migration support. by diptiranjanpx · Pull Request #1451 · libopenstorage/stork

diptiranjanpx · 2023-07-13T04:00:59Z

What type of PR is this?
improvement

What this PR does / why we need it:
Stashing strategy for the CR content to a configmap so that after migration with startApplication: False the application automatically does not get started when it is managed by a clusterwide operator.

Avoid recreating the resources during migration if there is no change to the resource content.

Does this PR change a user-facing CRD or CLI?:

Is a release note needed?:

Issue: Applications controlled by CR automatically gets started after migration if the operator is already up and running in a different namespace.
User Impact: Apps in destination cluster gets started which is not as per expectation with startApplication: false
Resolution: Stashing strategy for the CR content to a configmap has been added so that only after activate migration the CR spec gets applied and app will start then only.

Does this change need to be cherry-picked to a release branch?:

Test
Have been updated in PWX-31470 & PWX-32807.

cnbu-jenkins · 2023-07-13T04:05:03Z

Can one of the admins verify this patch?

pp511

Looks ok. Some nits and one concern related to newOwnerUIDMapping fetch.

pp511 · 2023-07-13T18:08:30Z

pkg/apis/stork/v1alpha1/applicationregistration.go

+	// Clusterwide Operators will control the CRs in all namespaces
+	// StashStratergy option will make sure as part of migration the CR does not get applied
+	// if StashCR is enabled. The CR will be applied as part of app activation
+	StashStratergy StashStratergy `json:"stashStratergy"`


nit..typo. Strategy.

pp511 · 2023-07-13T18:08:50Z

pkg/apis/stork/v1alpha1/applicationregistration.go

+// StashStratergy to restrict CR applying during migration
+type StashStratergy struct {


nit..typo. Strategy.

pp511 · 2023-07-13T18:44:14Z

pkg/migration/controllers/migration.go

+				log.MigrationLog(migration).Infof("getting configmap details for stashing resource %s/%s", unstructured.GetKind(), unstructured.GetName())
+				unstructured, err = getStashedConfigMap(unstructured)
+				if err != nil {
+					log.MigrationLog(migration).Warnf("unable to get stashed configmap content for object %s/%s, error: %v", unstructured.GetKind(), unstructured.GetName(), err)


Do we want to move ahead here despite the error?

That function returns an error only in case of json marshal which is unlikely to fail. But we should continue in the for loop and avoid going ahead in this function.

We decided we will continue in the error case. This has not been addressed yet.

Instead of continue , addressed with a goto such that we can update the migration CR with error message.

pp511 · 2023-07-13T18:53:40Z

pkg/migration/controllers/migration.go

 	return pvToPVCMapping
 }
+
+func getStashCREnabledAppRegs(crdList stork_api.ApplicationRegistrationList) map[string]bool {


nit..Since the function is named getStashCREnabledAppRegs. So maybe we can return a list of AppReg for which StashCR is true. Also we won't have to validate true or false on the caller side. Or we should rename this to avoid confusion

Renamed it to "getAppRegsStashMap".
Returning a list of AppRegs and again checking whether for a crd if stash strategy is enabled is costly.

pp511 · 2023-07-13T19:01:20Z

pkg/migration/controllers/migration.go

+	configMap.SetGroupVersionKind(schema.GroupVersionKind{
+		Group:   "",
+		Version: "v1",
+		Kind:    "ConfigMap",
+	})


nit..I think it would be cleaner to not use unstructured cm here(as used "core v1 . ConfigMap{}) so we won't have to explicitly set gvk. We can convert it to unstructured when we are returning from the function

+1 on this - would be easier to just create a corev1.ConfigMap objet and then convert it to unstructered.

pp511 · 2023-07-13T19:10:47Z

pkg/resourceutils/updatereplicas.go

+		return
+	}
+	ruleset := resourcecollector.GetDefaultRuleSet()
+	// Create the CRs in the same namespace if those dont exist


nit..nit.. don't or do not :)

pp511 · 2023-07-13T19:14:00Z

pkg/resourceutils/updatereplicas.go

+			util.CheckErr(err)
+			continue
+		}
+		//var client k8sdynamic.ResourceInterface


nit.. remove the commended line

pp511 · 2023-07-13T19:35:34Z

pkg/resourceutils/updatereplicas.go

+		//resourceInterface := configClient.Resource()
+		//client = resourceInterface.Namespace(namespace)


nit..remove comments

pp511 · 2023-07-13T19:46:39Z

pkg/resourceutils/updatereplicas.go

+		}
+
+		// Get the CR
+		newParentResourceUnstructured, err := resourceClient.Get(context.TODO(), unstructuredObj.GetName(), metav1.GetOptions{})


nit.. I think we should rename it and remove "parent". While reading the code it seemed like we are getting parentresource for the CR but it is not the case.

Right, renamed it to "newResourceUnstructured"

pp511 · 2023-07-13T19:49:46Z

pkg/resourceutils/updatereplicas.go

+		destPvcOwnersList := make([]metav1.OwnerReference, 0)
+		needUpdate := false
+		for _, pvcOwner := range pvc.GetOwnerReferences() {
+			if newOwnerUID, ok := newOwnerUIDMapping[fmt.Sprintf("%s-%s", pvcOwner.Name, pvcOwner.Kind)]; ok {


We are putting(name,namespace) as the key in newOwnerUIDMapping. But while fetching we are using (name, kind)

good catch, fixed it now. thanks.

adityadani · 2023-07-17T02:15:33Z

pkg/migration/controllers/migration.go

+	excludeSelectors := make(map[string]string)
+	if migration.Spec.ExcludeSelectors != nil {
+		excludeSelectors = migration.Spec.ExcludeSelectors
+		excludeSelectors["stash-cr"] = "true"


nit: lets define stash-cr as a constant

adityadani · 2023-07-17T02:18:23Z

pkg/migration/controllers/migration.go

 			var dynamicClient dynamic.ResourceInterface
 			ownerName := ""
 			for _, o := range objects {
+				gkv := o.GetObjectKind().GroupVersionKind()


adityadani · 2023-07-17T02:18:50Z

pkg/migration/controllers/migration.go

 			ownerName := ""
 			for _, o := range objects {
+				gkv := o.GetObjectKind().GroupVersionKind()
+				keyName := fmt.Sprintf("%v-%v-%v", gkv.Group, gkv.Kind, gkv.Version)


nit: may be write a helper function to get the key name for stashCRMap and use it in both the places.

adityadani · 2023-07-17T02:22:29Z

pkg/migration/controllers/migration.go

+						o.GetObjectKind().GroupVersionKind().GroupVersion().WithResource(resource.Name))
+				}
+			} else {
+				log.MigrationLog(migration).Infof("getting configmap details for stashing resource %s/%s", unstructured.GetKind(), unstructured.GetName())


nit: start log lines with upper-case

adityadani · 2023-07-17T02:29:15Z

pkg/migration/controllers/migration.go

+	}
+	// obj uid is not available and appending random uid will cause the next migration's get/delete of the configmap fail
+	// hence adding kind-name for cm name.
+	cmName := fmt.Sprintf("%s-%s", strings.ToLower(obj.GetKind()), obj.GetName())


Ideally this should also be a helper function under pkg/utils/utils.go

adityadani · 2023-07-17T02:29:52Z

pkg/migration/controllers/migration.go

+	configMap.SetGroupVersionKind(schema.GroupVersionKind{
+		Group:   "",
+		Version: "v1",
+		Kind:    "ConfigMap",
+	})


+1 on this - would be easier to just create a corev1.ConfigMap objet and then convert it to unstructered.

adityadani · 2023-07-17T02:32:06Z

pkg/migration/controllers/migration.go

+				log.MigrationLog(migration).Infof("getting configmap details for stashing resource %s/%s", unstructured.GetKind(), unstructured.GetName())
+				unstructured, err = getStashedConfigMap(unstructured)
+				if err != nil {
+					log.MigrationLog(migration).Warnf("unable to get stashed configmap content for object %s/%s, error: %v", unstructured.GetKind(), unstructured.GetName(), err)


That function returns an error only in case of json marshal which is unlikely to fail. But we should continue in the for loop and avoid going ahead in this function.

adityadani · 2023-07-17T02:33:46Z

pkg/migration/controllers/migration.go

+	configMap.Object["data"] = data
+	labels := map[string]string{"stash-cr": "true", "resource-kind": obj.GetKind()}
+	configMap.SetLabels(labels)
+	annotations := map[string]string{skipResourceAnnotation: "true", storkCreatedAnnotation: "true"}


We should also add the name of the object as a label

Name max char allowed is 253 while for label's value it is 63.
Will put a key with name then to keep the full name information. Is it ok , @adityadani ?

The CR name was added in configmap with a separate key.

adityadani · 2023-07-17T02:35:49Z

pkg/resourceutils/updatereplicas.go

+
+		// Create the CR
+		_, err = resourceClient.Create(context.TODO(), unstructuredObj, metav1.CreateOptions{})
+		if err != nil && !errors.IsAlreadyExists(err) {


If the error is already exists then should we log it on the CLI output?

Fixed with a message hinting the resource exists.

pp511

Mostly looks ok to me. Few comments and clarifications.

pp511 · 2023-08-18T03:54:22Z

pkg/apis/stork/v1alpha1/applicationregistration.go

 	Value string `json:"value"`
 }

+// StashStrategy to restrict CR applying during migration


nit. nit. CR applying -> applying CR

pp511 · 2023-08-18T03:56:48Z

pkg/apis/stork/v1alpha1/applicationregistration.go

+	// StashStrategy option will make sure as part of migration the CR does not get applied
+	// if StashCR is enabled. The CR will be applied as part of app activation


nit. nit...StashStrategy option if enabled will make sure CR does not get applied during migration.

pp511 · 2023-08-18T04:06:43Z

pkg/migration/controllers/migration.go

+		excludeSelectors[StashCRLabel] = "true"
+	} else {
+		excludeSelectors[StashCRLabel] = "true"


excludeSelectors[StashCRLabel] = "true" need not be inside if and else both. It can be outside these conditional statements.

pp511 · 2023-08-18T04:14:09Z

pkg/migration/controllers/migration.go

+						o.GetObjectKind().GroupVersionKind().GroupVersion().WithResource(resource.Name))
+				}
+			} else {
+				log.MigrationLog(migration).Infof("Getting configmap details for stashing resource %s/%s", unstructured.GetKind(), unstructured.GetName())


nit.. Why not print the version too?

Same thing in the next statement too

pp511 · 2023-08-18T04:16:18Z

pkg/migration/controllers/migration.go

+				log.MigrationLog(migration).Infof("getting configmap details for stashing resource %s/%s", unstructured.GetKind(), unstructured.GetName())
+				unstructured, err = getStashedConfigMap(unstructured)
+				if err != nil {
+					log.MigrationLog(migration).Warnf("unable to get stashed configmap content for object %s/%s, error: %v", unstructured.GetKind(), unstructured.GetName(), err)


We decided we will continue in the error case. This has not been addressed yet.

pp511 · 2023-08-18T04:22:22Z

pkg/migration/controllers/migration.go

+	}
+	// obj uid is not available and appending random uid will cause the next migration's get/delete of the configmap fail
+	// hence adding kind-name for cm name.
+	cmName := utils.GetStashedConfigMapName(strings.ToLower(obj.GetKind()), obj.GetName())


I think we should include version too in the cmName. What do you think?

Reverted this as version will contain "/" which is not allowed for configmap name.

pp511 · 2023-08-18T04:24:50Z

pkg/migration/controllers/migration.go

+			Namespace: obj.GetNamespace(),
+			Labels: map[string]string{
+				StashCRLabel:    "true",
+				"resource-kind": obj.GetKind(),


Why are we setting the resource-kind label here?

This has been kept for debugging purpose.

pp511 · 2023-08-18T17:00:48Z

pkg/migration/controllers/migration.go

+		Data: map[string]string{
+			StashedCMCRKey:       string(jsonData),
+			StashedCMOwnedPVCKey: string(ownedPVCsEncoded),
+			"name":               obj.GetName(),


nit..Why not have a constant for "name" too?

pp511 · 2023-08-18T17:20:54Z

pkg/migration/controllers/migration.go

+	return fmt.Sprintf("%v-%v-%v", group, kind, version)
+}
+
+func getStashedConfigMap(obj *unstructured.Unstructured, inputObjectHash uint64) (*unstructured.Unstructured, error) {


Ideally I would have expected core v1 . ConfigMap{} being returned from this function. But I see the you are trying to use the same worker to work for CM too. So I guess it's fine.

adityadani

Can you address the comments from the previous iteration?

adityadani · 2023-08-18T23:47:29Z

pkg/migration/controllers/migration.go

+					if _, ok := appRegsStashMap[keyName]; ok {
+						if appRegsStashMap[keyName] {


We don't need both the if conditions.

adityadani · 2023-08-18T23:49:29Z

pkg/migration/controllers/migration.go

+			if _, ok := appRegsStashMap[keyName]; ok {
+				stashCREnabled = appRegsStashMap[keyName]
+			}


Suggested change

if _, ok := appRegsStashMap[keyName]; ok {

stashCREnabled = appRegsStashMap[keyName]

}

stashCREnabled, _ = appRegsStashMap[keyName]

pp511

lgtm

adityadani · 2023-08-22T21:17:19Z

pkg/migration/controllers/migration.go

+	excludeSelectors := make(map[string]string)
+	excludeSelectors[StashCRLabel] = "true"
+	if migration.Spec.ExcludeSelectors != nil {
+		excludeSelectors = migration.Spec.ExcludeSelectors
+	}


You always want to add the StashCRLabel to the exclude selectors right? Then on line 614 you will be overriding it with whatever the user has provided.

Suggested change

excludeSelectors := make(map[string]string)

excludeSelectors[StashCRLabel] = "true"

if migration.Spec.ExcludeSelectors != nil {

excludeSelectors = migration.Spec.ExcludeSelectors

}

if migration.Spec.ExcludeSelectors != nil {

excludeSelectors = migration.Spec.ExcludeSelectors

} else {

excludeSelectors := make(map[string]string)

}

excludeSelectors[StashCRLabel] = "true"

you are right. now fixed it.

adityadani · 2023-08-22T21:18:59Z

pkg/utils/utils.go

 }
+
+func GetStashedConfigMapName(apiVersion string, objKind string, objName string) string {
+	cmName := fmt.Sprintf("%s-%s-%s", apiVersion, objKind, objName)


Should we add group as well here since you are anyway trimming it? Esp the opensource DB operators have conflicting Kinds.

PWX-32807: Compare hash objects of resources to avoid deletion and recreation in case of no change during the migration.

diptiranjanpx requested review from adityadani and pp511 July 13, 2023 04:00

diptiranjanpx force-pushed the PWX-31470 branch from 6827866 to 93338d6 Compare July 13, 2023 14:20

pp511 reviewed Jul 13, 2023

View reviewed changes

adityadani reviewed Jul 17, 2023

View reviewed changes

diptiranjanpx force-pushed the PWX-31470 branch 2 times, most recently from f5c263c to 11d9578 Compare July 18, 2023 07:36

diptiranjanpx force-pushed the PWX-31470 branch from 11d9578 to ea5a3a7 Compare August 17, 2023 11:32

pp511 requested changes Aug 18, 2023

View reviewed changes

adityadani reviewed Aug 19, 2023

View reviewed changes

diptiranjanpx force-pushed the PWX-31470 branch from ea5a3a7 to 49930f9 Compare August 21, 2023 09:48

pp511 approved these changes Aug 22, 2023

View reviewed changes

diptiranjanpx force-pushed the PWX-31470 branch from 49930f9 to 029ca70 Compare August 22, 2023 07:43

adityadani approved these changes Aug 22, 2023

View reviewed changes

diptiranjanpx force-pushed the PWX-31470 branch 3 times, most recently from 7415850 to 69b9990 Compare August 24, 2023 04:51

diptiranjanpx added enhancement documentation release-note Information about this change needs to be added to the release note labels Aug 25, 2023

diptiranjanpx force-pushed the PWX-31470 branch 2 times, most recently from e2069e1 to 70be3ea Compare August 26, 2023 01:49

PWX-31470: Clusterwide operator CR migration support.

eb92efa

PWX-32807: Compare hash objects of resources to avoid deletion and recreation in case of no change during the migration.

diptiranjanpx force-pushed the PWX-31470 branch 2 times, most recently from 70be3ea to 781a3c0 Compare August 29, 2023 02:16

diptiranjanpx merged commit 20c1b0d into master Aug 29, 2023

diptiranjanpx deleted the PWX-31470 branch May 27, 2024 08:25

		// StashStratergy to restrict CR applying during migration
		type StashStratergy struct {

		//resourceInterface := configClient.Resource()
		//client = resourceInterface.Namespace(namespace)

		// StashStrategy option will make sure as part of migration the CR does not get applied
		// if StashCR is enabled. The CR will be applied as part of app activation

		if _, ok := appRegsStashMap[keyName]; ok {
		if appRegsStashMap[keyName] {

Conversation

diptiranjanpx commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cnbu-jenkins commented Jul 13, 2023

Uh oh!

pp511 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

diptiranjanpx commented Jul 13, 2023 •

edited

Loading

diptiranjanpx Jul 17, 2023 •

edited

Loading