PWX-30292: Migrating related CRDs to destination cluster with similar categories by diptiranjanpx · Pull Request #1494 · libopenstorage/stork

diptiranjanpx · 2023-08-31T12:21:16Z

Signed-Off-By: Diptiranjan

What type of PR is this?
improvement

What this PR does / why we need it:
Migrating related CRDs with similar categories as some apps activation requires CRDs other than their group.

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

Is a release note needed?:

Issue: ECK app installed with operator requires all related CRDs for starting up after migration.
User Impact: Elastic search app scaling up failed because of missing related CRD after migration.
Resolution: Related CRDs for a CR will also get migrated which will prevent the scaling up of ECK app after migration.

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

Test

In source:

[root@gejain-35-0 ~]# kubectl get crds | grep elastic
agents.agent.k8s.elastic.co                                  2023-08-22T09:27:51Z
apmservers.apm.k8s.elastic.co                                2023-08-22T09:27:51Z
beats.beat.k8s.elastic.co                                    2023-08-22T09:27:51Z
elasticmapsservers.maps.k8s.elastic.co                       2023-08-22T09:27:51Z
elasticsearchautoscalers.autoscaling.k8s.elastic.co          2023-08-22T09:27:51Z
elasticsearches.elasticsearch.k8s.elastic.co                 2023-08-22T09:27:51Z
enterprisesearches.enterprisesearch.k8s.elastic.co           2023-08-22T09:27:51Z
kibanas.kibana.k8s.elastic.co                                2023-08-22T09:27:51Z
logstashes.logstash.k8s.elastic.co                           2023-08-22T09:27:51Z
stackconfigpolicies.stackconfigpolicy.k8s.elastic.co         2023-08-22T09:27:51Z

[root@gejain-35-0 ~]# kubectl get elasticsearch -n elastic-system
NAME   HEALTH   NODES   VERSION   PHASE   AGE
es     green    1       8.9.1     Ready   6d2h


Without the fix , CRDs in dest cluster:

[root@gejain-37-0 ~]# kubectl get crds | grep elastic
elasticsearches.elasticsearch.k8s.elastic.co                 2023-08-25T09:11:27Z


With Fix:

After migrations in dest cluster:

[root@gejain-37-0 ~]# kubectl get crds | grep elastic
agents.agent.k8s.elastic.co                                  2023-08-31T11:43:35Z
apmservers.apm.k8s.elastic.co                                2023-08-31T11:43:30Z
beats.beat.k8s.elastic.co                                    2023-08-31T11:43:04Z
elasticmapsservers.maps.k8s.elastic.co                       2023-08-31T11:42:59Z
elasticsearchautoscalers.autoscaling.k8s.elastic.co          2023-08-31T11:43:09Z
elasticsearches.elasticsearch.k8s.elastic.co                 2023-08-25T09:11:27Z
enterprisesearches.enterprisesearch.k8s.elastic.co           2023-08-31T11:43:14Z
kibanas.kibana.k8s.elastic.co                                2023-08-31T11:43:24Z
logstashes.logstash.k8s.elastic.co                           2023-08-31T11:43:19Z
stackconfigpolicies.stackconfigpolicy.k8s.elastic.co         2023-08-31T11:42:53Z

After activating elastic search app in dest, 

[root@gejain-37-0 ~]# ./storkctl activate migrations -n elastic-system
Updated replicas for statefulset elastic-system/elastic-operator to 1
W0831 11:51:53.072898   14577 warnings.go:70] snapshot.storage.k8s.io/v1beta1 VolumeSnapshot is deprecated; use snapshot.storage.k8s.io/v1 VolumeSnapshot
[root@gejain-37-0 ~]# kubectl get po -n elastic-system
NAME                 READY   STATUS    RESTARTS   AGE
elastic-operator-0   1/1     Running   0          9m14s
es-es-default-0      1/1     Running   0          8m54s

[root@gejain-37-0 ~]# kubectl get elasticsearch -n elastic-system
NAME   HEALTH   NODES   VERSION   PHASE   AGE
es     green    1       8.9.1     Ready   18h

cnbu-jenkins · 2023-08-31T12:25:03Z

Can one of the admins verify this patch?

pp511

Core logic looks good to me. Some nits and comments to be addressed

pp511 · 2023-09-01T18:09:37Z

pkg/migration/controllers/migration.go

+	destClnt, err := apiextensionsclient.NewForConfig(remoteClient.remoteAdminConfig)
+	if err != nil {
+		return err
+	}
+


You don't need to call NewForConfig again here. You can just use remoteClient.adminClient as destClnt.

destClnt is *apiextensionsclient.Clientset whereas remoteClient.adminClient is *kubernetes.Clientset. Hence keeping it.

pp511 · 2023-09-01T18:23:16Z

pkg/migration/controllers/migration.go

+//
+//	a. CRD matching with the required groups.
+//	b. CRDs which are related with the CRDs from above list (#a)
+func getRelatedCRDListWRTGroupAndComponents(client *apiextensionsclient.Clientset, ruleset *inflect.Ruleset, crdList *stork_api.ApplicationRegistrationList, resGroups map[string]string) []*stork_api.ApplicationRegistration {


nit.. Didn't you want to name the function getRelatedCRDListWRTGroupAndCategories instead of getRelatedCRDListWRTGroupAndComponents?

pp511 · 2023-09-01T18:24:15Z

pkg/migration/controllers/migration.go

+func getRelatedCRDListWRTGroupAndComponents(client *apiextensionsclient.Clientset, ruleset *inflect.Ruleset, crdList *stork_api.ApplicationRegistrationList, resGroups map[string]string) []*stork_api.ApplicationRegistration {
+	filteredCRDList := make([]*stork_api.ApplicationRegistration, 0)
+	reqCategoriesMap := make(map[string]bool)
+	//categoriesToCRDMap := make(map[string][]*stork_api.ApplicationRegistrationList)


nit.. Stray comment

pp511 · 2023-09-01T18:45:16Z

pkg/migration/controllers/migration.go

+				filteredCRDList = append(filteredCRDList, &crd)
+				continue
+			}
+			crdName := ruleset.Pluralize(strings.ToLower(v.Kind)) + "." + v.Group


nit..nit..I think we can move the statement in the below if condition.

pp511 · 2023-09-01T18:50:01Z

pkg/migration/controllers/migration.go

+// getRelatedCRDListWRTGroupAndComponents takes input of AppRegs
+// and find out all CRDs those need to be collected.
+// The CRD list should contain
+//


nit..Just rewording a bit
getRelatedCRDListWRTGroupAndCategories takes AppRegs as input and
finds out all the CRDs that need to be collected.
This CRD list should contain

Also remove the space after this line

codecov · 2023-09-02T14:43:24Z

Codecov Report

Patch coverage has no change and project coverage change: +0.05% 🎉

Comparison is base (b6e0370) 65.90% compared to head (1ae2a0a) 65.96%.

❗ Current head 1ae2a0a differs from pull request most recent head d478679. Consider uploading reports for the commit d478679 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1494      +/-   ##
==========================================
+ Coverage   65.90%   65.96%   +0.05%     
==========================================
  Files          43       43              
  Lines        5139     5139              
==========================================
+ Hits         3387     3390       +3     
+ Misses       1425     1423       -2     
+ Partials      327      326       -1

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pp511

Just one nit. Rest lgtm.
Just a general concern. We do not have any UT coverage for migration.go. I think we should plan to start adding UTs for the new code we are adding [if feasible]. For eg. We can start by adding UTs for getRelatedCRDListWRTGroupAndCategories here. Let's start by filing a ticket to track this and label it good-first-issue.

pp511 · 2023-09-02T18:10:28Z

pkg/migration/controllers/migration.go

+// getRelatedCRDListWRTGroupAndCategories takes AppRegs as input and
+// finds out all the CRDs that need to be collected.
 // The CRD list should contain
 //


nit..I don't think a blank line is required here.

make do-fmt is adding that line.

PWX-30292: Migrating related CRDs to destination cluster.

diptiranjanpx requested review from adityadani and pp511 August 31, 2023 12:21

diptiranjanpx force-pushed the PWX-30292 branch from e5b4b1c to 305f9a4 Compare August 31, 2023 12:21

diptiranjanpx force-pushed the PWX-30292 branch from 305f9a4 to fc86979 Compare September 1, 2023 07:11

pp511 requested changes Sep 1, 2023

View reviewed changes

pp511 approved these changes Sep 2, 2023

View reviewed changes

diptiranjanpx force-pushed the PWX-30292 branch from 1ae2a0a to ee8d6de Compare September 3, 2023 07:09

Signed-Off-By: Diptiranjan

d478679

PWX-30292: Migrating related CRDs to destination cluster.

diptiranjanpx force-pushed the PWX-30292 branch from ee8d6de to d478679 Compare September 3, 2023 16:54

diptiranjanpx merged commit 95d675d into master Sep 4, 2023

diptiranjanpx deleted the PWX-30292 branch May 27, 2024 08:26

Conversation

diptiranjanpx commented Aug 31, 2023

Uh oh!

cnbu-jenkins commented Aug 31, 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!

codecov bot commented Sep 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Sep 2, 2023 •

edited

Loading