Skip to content
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.

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

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

Merged
merged 3 commits into from
Sep 13, 2023

Conversation

diptiranjanpx
Copy link
Contributor

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
`

  1. 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

  1. 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

  1. 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

  1. 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

  1. 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

  1. 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
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link
Contributor

@adityadani adityadani left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

storkops.Instance().SetConfig(conf)
core.Instance().SetConfig(conf)
secret := &v1.Secret{
finalBackupLocationName := backupLocationName
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link

codecov bot commented Sep 12, 2023

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.

Copy link
Contributor

@pp511 pp511 left a comment

Choose a reason for hiding this comment

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

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

@@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


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

Choose a reason for hiding this comment

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

nit..dConf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines -144 to -147
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)

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to objectstorelocation

}
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))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit..backupLocation or BackupLocation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to objectstorelocation

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to objectstoreLocation as discussed.

Comment on lines +958 to +982
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
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@diptiranjanpx 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
…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
…nidirectional clusterpair creation with storkctl (#1501)


* Additional changes for supporting "--use-existing-objectstorelocation".
@diptiranjanpx diptiranjanpx deleted the PWX-33569 branch May 27, 2024 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants