PWX-33569: Creating objectstore locations in destination cluster in unidirectional clusterpair creation with storkctl #1501

diptiranjanpx · 2023-09-11T09:30:54Z

What type of PR is this?

Uncomment only one and also add the corresponding label in the PR:
bug

What this PR does / why we need it:

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

Is a release note needed?:

Issue: Creating clusterpair with --unidirectional fails as objectstore info was missing in destination cluster.
User Impact: Migration using the above clusterpair will fail.
Resolution: Creating objectstore info in destination cluster for migration to succeed.

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

Test
`

Creating clusterpair with unidirectional option.

➜ stork git:(PWX-33569) ✗ ./storkctl create clusterpair uni-cp-fw -n mongodb --src-kube-file /tmp/dipti5.config --dest-kube-file /tmp/dipti7.config --s3-access-key admin --s3-secret-key Password1 --s3-endpoint minio.pwx.dev.purestorage.com --s3-region us-east1 -p s3 -u
Destination portworx endpoint is 10.13.194.128:9001

Creating Secret and ObjectstoreLocation in source cluster in namespace mongodb...
ObjectstoreLocation uni-cp-fw created on source cluster in namespace mongodb

Creating Secret and ObjectstoreLocation in destination cluster in namespace mongodb...
ObjectstoreLocation uni-cp-fw created on destination cluster in namespace mongodb

Creating a cluster pair. Direction: Source -> Destination
ClusterPair uni-cp-fw created successfully. Direction Source -> Destination

In source:

➜ stork git:(PWX-33569) ✗ ./storkctl get clusterpair -n mongodb
NAME STORAGE-STATUS SCHEDULER-STATUS CREATED
uni-cp-fw Ready Ready 10 Sep 23 18:43 UTC
➜ stork git:(PWX-33569) ✗ km get backuplocations
NAME AGE
uni-cp-fw 74s
➜ stork git:(PWX-33569) ✗ km get secret uni-cp-fw
NAME TYPE DATA AGE
uni-cp-fw Opaque 8 81s

In dest:
➜ stork git:(PWX-33569) ✗ ./storkctl get clusterpair -n mongodb
No resources found.
➜ stork git:(PWX-33569) ✗ km get backuplocations
NAME AGE
uni-cp-fw 112s
➜ stork git:(PWX-33569) ✗ km get secret uni-cp-fw
NAME TYPE DATA AGE
uni-cp-fw Opaque 8 117s

Doing migration :

➜ stork git:(PWX-33569) ✗ ./storkctl get migrationschedule -n mongodb
NAME POLICYNAME CLUSTERPAIR SUSPEND LAST-SUCCESS-TIME LAST-SUCCESS-DURATION
mongo-rev every-15m dipti-mongodb true
mongodb-fp every-15m uni-cp-fw false 09 Sep 23 01:33 UTC 1m52s

➜ stork git:(PWX-33569) ✗ ./storkctl get migrations -n mongodb
NAME CLUSTERPAIR STAGE STATUS VOLUMES RESOURCES CREATED ELAPSED TOTAL BYTES TRANSFERRED
mongodb-fp-interval-2023-09-10-184626 uni-cp-fw Final Successful 6/6 29/29 10 Sep 23 18:46 UTC Volumes (1m30s) Resources (32s) 734998528

Creating cp with same name in dest cluster
➜ stork git:(23.7.2) ✗ ./storkctl create clusterpair uni-cp-fw -n mongodb --src-kube-file /tmp/dipti7.config --dest-kube-file /tmp/dipti5.config --s3-access-key admin --s3-secret-key Password1 --s3-endpoint minio.pwx.dev.purestorage.com --s3-region us-east1 -p s3 -u
Destination portworx endpoint is 10.13.192.154:9001

Creating Secret and ObjectstoreLocation in source cluster in namespace mongodb...
Error from server (AlreadyExists): secrets "uni-cp-fw" already exists

Creating CP with same name in dest cluster using “use-existing-backuplocation” flag

➜ stork git:(23.7.2) ✗ ./storkctl create clusterpair uni-cp-fw -n mongodb --src-kube-file /tmp/dipti7.config --dest-kube-file /tmp/dipti5.config --s3-access-key admin --s3-secret-key Password1 --s3-endpoint minio.pwx.dev.purestorage.com --s3-region us-east1 -p s3 -u --use-existing-backuplocation uni-cp-fw
Destination portworx endpoint is 10.13.192.154:9001
Creating a cluster pair. Direction: Source -> Destination
ClusterPair uni-cp-fw created successfully. Direction Source -> Destination

➜ stork git:(23.7.2) ✗ ./storkctl get clusterpair -n mongodb
NAME STORAGE-STATUS SCHEDULER-STATUS CREATED
uni-cp-fw Ready Ready 11 Sep 23 03:10 UTC

➜ stork git:(23.7.2) ✗ ./storkctl get migrations -n mongodb
NAME CLUSTERPAIR STAGE STATUS VOLUMES RESOURCES CREATED ELAPSED TOTAL BYTES TRANSFERRED
mongo-rev-interval-2023-09-11-032632 uni-cp-fw Final Successful 6/6 29/29 11 Sep 23 03:26 UTC Volumes (44s) Resources (1m2s) 842321920

Creating cp with different name in dest cluster
➜ stork git:(23.7.2) ✗ ./storkctl create clusterpair uni-cp-rev -n mongodb --src-kub
e-file /tmp/dipti7.config --dest-kube-file /tmp/dipti5.config --s3-access-key admin -
-s3-secret-key Password1 --s3-endpoint minio.pwx.dev.purestorage.com --s3-region us-e
ast1 -p s3 -u
Destination portworx endpoint is 10.13.192.154:9001

Creating Secret and ObjectstoreLocation in source cluster in namespace mongodb...
ObjectstoreLocation uni-cp-rev created on source cluster in namespace mongodb

Creating Secret and ObjectstoreLocation in destination cluster in namespace mongodb...
ObjectstoreLocation uni-cp-rev created on destination cluster in namespace mongodb

Creating a cluster pair. Direction: Source -> Destination
ClusterPair uni-cp-rev created successfully. Direction Source -> Destination

➜ stork git:(23.7.2) ✗ ./storkctl get migrations -n mongodb
NAME CLUSTERPAIR STAGE STATUS VOLUMES RESOURCES CREATED ELAPSED TOTAL BYTES TRANSFERRED
mongo-rev-interval-2023-09-11-055359 uni-cp-rev Final Successful 6/6 29/29 11 Sep 23 05:53 UTC Volumes (43s) Resources (25s) 854593536

Deleting clusterpair:

➜ stork git:(23.7.2) ✗ bin/linux/storkctl get clusterpair -n mongodb
NAME STORAGE-STATUS SCHEDULER-STATUS CREATED
uni-cp-rev Ready Ready 11 Sep 23 08:26 UTC
➜ stork git:(23.7.2) ✗ km delete clusterpair uni-cp-rev
clusterpair.stork.libopenstorage.org "uni-cp-rev" deleted
➜ stork git:(23.7.2) ✗
➜ stork git:(23.7.2) ✗ km get secrets | grep dipti
➜ stork git:(23.7.2) ✗ bin/linux/storkctl get clusterpair -n mongodb
No resources found.
➜ stork git:(23.7.2) ✗ km get backuplocations uni-cp-rev
Error from server (NotFound): backuplocations.stork.libopenstorage.org "uni-cp-rev" not found
➜ stork git:(23.7.2) ✗ km get secret uni-cp-rev
Error from server (NotFound): secrets "uni-cp-rev" not found

Creating a bidirectional cp:

➜ stork git:(23.7.2) ✗ bin/linux/storkctl create clusterpair bi-cp-mongo -n mongodb --src-kube-file /tmp/dipti5.config --dest-kube-file /tmp/dipti7.config --s3-access-key admin --s3-secret-key Password1 --s3-endpoint minio.pwx.dev.purestorage.com --s3-region us-east1 -p s3
Source portworx endpoint is 10.13.192.154:9001
Destination portworx endpoint is 10.13.194.128:9001

Creating Secret and ObjectstoreLocation in source cluster in namespace mongodb...
ObjectstoreLocation bi-cp-mongo created on source cluster in namespace mongodb

Creating Secret and ObjectstoreLocation in destination cluster in namespace mongodb...
ObjectstoreLocation bi-cp-mongo created on destination cluster in namespace mongodb

Creating a cluster pair. Direction: Source -> Destination
ClusterPair bi-cp-mongo created successfully. Direction Source -> Destination

Creating a cluster pair. Direction: Destination -> Source
Cluster pair bi-cp-mongo created successfully. Direction: Destination -> Source

In source:
➜ stork git:(23.7.2) ✗ km get clusterpair -A
NAMESPACE NAME AGE
mongodb bi-cp-mongo 27s
➜ stork git:(23.7.2) ✗ km get backuplocations bi-cp-mongo
NAME AGE
bi-cp-mongo 49s
➜ stork git:(23.7.2) ✗ km get secret bi-cp-mongo
NAME TYPE DATA AGE
bi-cp-mongo Opaque 8 57s

In dest:
➜ stork git:(23.7.2) ✗ km get clusterpair -A
NAMESPACE NAME AGE
mongodb bi-cp-mongo 82s
➜ stork git:(23.7.2) ✗ km get backuplocations bi-cp-mongo
NAME AGE
bi-cp-mongo 103s
➜ stork git:(23.7.2) ✗ km get secret bi-cp-mongo
NAME TYPE DATA AGE
bi-cp-mongo Opaque 8 107s

➜ stork git:(23.7.2) ✗ bin/linux/storkctl get migrations -n mongodb
NAME CLUSTERPAIR STAGE STATUS VOLUMES RESOURCES CREATED ELAPSED TOTAL BYTES TRANSFERRED
mongodb-fp-interval-2023-09-11-083836 bi-cp-mongo Final Successful 6/6 29/29 11 Sep 23 08:38 UTC Volumes (1m6s) Resources (1m9s) 854593536

`

cnbu-jenkins · 2023-09-11T09:34:03Z

Can one of the admins verify this patch?

adityadani

overall looks good. I have a few questions on the backupLocationName. Please address them before merging

adityadani · 2023-09-12T04:43:40Z

pkg/storkctl/clusterpair.go

-			conf, err = getConfig(dFile).ClientConfig()
+			storkops.Instance().SetConfig(sconf)
+			core.Instance().SetConfig(sconf)
+			srcClusterPair.Spec.Options[storkv1.BackupLocationResourceName] = finalBackupLocationName


You are using finalBackupLocationName interchangeably which is confusing. In certain cases you set the Options to backupLocation.Name and sometimes you use this variable.

I will remove this variable. Will modify the code, to overwrite the backuplocationName only in case of it is empty.

adityadani · 2023-09-12T04:44:30Z

pkg/storkctl/clusterpair.go

-			storkops.Instance().SetConfig(conf)
-			core.Instance().SetConfig(conf)
-			secret := &v1.Secret{
+			finalBackupLocationName := backupLocationName


The finalBackupLocationName is not used on line 446 when creating the BackupLocation CR?

finalBackupLocationName will be removed. The reason for not using that in case of creating backupLocation CR was because CR creation is only required when the user input backuplocation is not provided. In that case it is same as clusterpair name.

codecov · 2023-09-12T15:20:50Z

Codecov Report

Patch coverage: 2.48% and project coverage change: -0.68% ⚠️

Comparison is base (270029f) 65.90% compared to head (0045418) 65.23%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1501      +/-   ##
==========================================
- Coverage   65.90%   65.23%   -0.68%     
==========================================
  Files          43       43              
  Lines        5139     5175      +36     
==========================================
- Hits         3387     3376      -11     
- Misses       1425     1473      +48     
+ Partials      327      326       -1

Files Changed	Coverage Δ
pkg/storkctl/clusterpair.go	`19.05% <2.48%> (-2.89%)`	⬇️

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

pp511

Looks ok.
Some nitpicks.
One PWX which needs to be filed.
Reason for UT removal not clear.

pp511 · 2023-09-12T18:42:59Z

pkg/storkctl/clusterpair.go

@@ -490,14 +416,12 @@ func newCreateClusterPairCommand(cmdFactory Factory, ioStreams genericclioptions
 				destToken = token
 			}

-			srcClusterPair, err := generateClusterPair(clusterPairName, cmdFactory.GetNamespace(), dIP, dPort, destToken, dFile, projectMappingsStr, pxAuthSecretNamespaceDest, false, false)
+			sconf, err := getConfig(sFile).ClientConfig()


nit.. I think we can change it to sConf. Just like sFile.

pp511 · 2023-09-12T18:43:12Z

pkg/storkctl/clusterpair.go

-
-			// Create cluster-pair on source cluster
-			conf, err := getConfig(sFile).ClientConfig()
+			dconf, err := getConfig(dFile).ClientConfig()


pp511 · 2023-09-12T18:58:25Z

pkg/storkctl/clusterpair_test.go

-	cmdArgs = []string{"create", "clusterpair", "uni-pair1", "-n", "test", "--src-kube-file", srcConfig.Name(), "--dest-kube-file", destConfig.Name(), "-u"}
-	expected = "error: missing parameter \"provider\" - External objectstore provider needs to be either of azure, google, s3"
-	testCommon(t, cmdArgs, nil, expected, true)
-


Why are we removing this test?

Before this check , the px endpoint validation happens. And simulating that requires 2 different kubeconfigs, which I did not find any way to do. So this UT always fails.

pp511 · 2023-09-12T19:00:00Z

pkg/storkctl/clusterpair.go

+			if len(backupLocationName) > 0 {
+				err = verifyBackupLocationExistence(sFile, backupLocationName, cmdFactory.GetNamespace())
+				if err != nil {
+					util.CheckErr(fmt.Errorf("input backuplocation %s existence check failed with error in source cluster: %v", backupLocationName, err))


nit..backupLocation or BackupLocation(just like ObjectstoreLocation in other places)

changed to objectstorelocation

pp511 · 2023-09-12T19:00:18Z

pkg/storkctl/clusterpair.go

+				}
+				err = verifyBackupLocationExistence(dFile, backupLocationName, cmdFactory.GetNamespace())
+				if err != nil {
+					util.CheckErr(fmt.Errorf("input backuplocation %s existence check failed with error in destination cluster: %v", backupLocationName, err))


nit..backupLocation or BackupLocation

changed to objectstorelocation

My suggestion was to use camelCase or PascalCase to stay consistent with other places. We didn't have to rename this to objectstorelocation. I think this needs to change.

changed to objectstoreLocation as discussed.

pp511 · 2023-09-12T19:02:27Z

pkg/storkctl/clusterpair.go

+func verifyBackupLocationExistence(config string, blName string, namespace string) error {
+	conf, err := getConfig(config).ClientConfig()
+	if err != nil {
+		return err
+	}
+	storkOps, err := storkops.NewForConfig(conf)
+	if err != nil {
+		return err
+	}
+	bl, err := storkOps.GetBackupLocation(blName, namespace)
+	if err != nil {
+		return err
+	}
+	if len(bl.Location.SecretConfig) > 0 {
+		coreOps, err := core.NewForConfig(conf)
+		if err != nil {
+			return err
+		}
+		_, err = coreOps.GetSecret(bl.Location.SecretConfig, namespace)
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}


Even if one provides incorrect credentials to Object Store we pass this step and later fail during migration. Can you file a PWX to track this so a validation can be done here itself?

https://portworx.atlassian.net/browse/PWX-32852

…nidirectional clusterpair.

…nidirectional clusterpair creation with storkctl (#1501) * Additional changes for supporting "--use-existing-objectstorelocation".

diptiranjanpx requested review from adityadani and pp511 September 11, 2023 09:31

adityadani approved these changes Sep 12, 2023

View reviewed changes

diptiranjanpx force-pushed the PWX-33569 branch from 3382df8 to 03aa7ca Compare September 12, 2023 18:31

pp511 approved these changes Sep 12, 2023

View reviewed changes

diptiranjanpx force-pushed the PWX-33569 branch from 03aa7ca to 297b8fd Compare September 13, 2023 04:37

diptiranjanpx added 2 commits September 13, 2023 12:53

PWX-33569: Creating backuplocation in dest namespace while creating u…

bbc275c

…nidirectional clusterpair.

Additional changes for supporting "--use-existing-backuplocation".

3133788

diptiranjanpx force-pushed the PWX-33569 branch from 297b8fd to fa1f860 Compare September 13, 2023 07:23

Addressed the review comments.

0045418

diptiranjanpx force-pushed the PWX-33569 branch from fa1f860 to 0045418 Compare September 13, 2023 09:13

diptiranjanpx merged commit 86bbba1 into master Sep 13, 2023
2 of 4 checks passed

diptiranjanpx added a commit that referenced this pull request Sep 13, 2023

PWX-33569: Creating objectstore locations in destination cluster in u…

9bfc2d7

…nidirectional clusterpair creation with storkctl (#1501) * Additional changes for supporting "--use-existing-objectstorelocation".

diptiranjanpx added a commit that referenced this pull request Sep 14, 2023

PWX-33569: Creating objectstore locations in destination cluster in u…

0da10da

…nidirectional clusterpair creation with storkctl (#1501) * Additional changes for supporting "--use-existing-objectstorelocation".

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PWX-33569: Creating objectstore locations in destination cluster in unidirectional clusterpair creation with storkctl #1501

PWX-33569: Creating objectstore locations in destination cluster in unidirectional clusterpair creation with storkctl #1501

diptiranjanpx commented Sep 11, 2023

cnbu-jenkins commented Sep 11, 2023

adityadani left a comment

adityadani Sep 12, 2023

diptiranjanpx Sep 12, 2023

adityadani Sep 12, 2023

diptiranjanpx Sep 12, 2023

codecov bot commented Sep 12, 2023 •

edited

Loading

pp511 left a comment

pp511 Sep 12, 2023

diptiranjanpx Sep 13, 2023

pp511 Sep 12, 2023

diptiranjanpx Sep 13, 2023

pp511 Sep 12, 2023

diptiranjanpx Sep 13, 2023

pp511 Sep 12, 2023

diptiranjanpx Sep 13, 2023

pp511 Sep 12, 2023

diptiranjanpx Sep 13, 2023

pp511 Sep 13, 2023

diptiranjanpx Sep 13, 2023

pp511 Sep 12, 2023

diptiranjanpx Sep 13, 2023

PWX-33569: Creating objectstore locations in destination cluster in unidirectional clusterpair creation with storkctl #1501

PWX-33569: Creating objectstore locations in destination cluster in unidirectional clusterpair creation with storkctl #1501

Conversation

diptiranjanpx commented Sep 11, 2023

cnbu-jenkins commented Sep 11, 2023

adityadani left a 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

codecov bot commented Sep 12, 2023 • edited Loading

Codecov Report

pp511 left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 12, 2023 •

edited

Loading