PB-7821: Automate the few cluster share testcases #2734

sgajawada-px · 2024-08-14T05:51:00Z

Testcases

Verify Cluster Unshare deletes Restore Objects from user.
Verify Cluster unshare when BackupSchedules are deleted.
Verify RestoreObjects are not deleted when unshare from Super Admin is executed.
Shared user can restore to shared cluster from backup created on owned cluster.

sheet: https://docs.google.com/spreadsheets/d/1YmxvyOkwUaEKdJiInNZ4C0diXEvhsfhD6n0brYlm5lI/edit?gid=1744627660#gid=1744627660

What this PR does / why we need it:
To automate the PX-backup cluster share FC testcases

Which issue(s) this PR fixes (optional)
Closes #PB-7821

Special notes for your reviewer:
https://jenkins.pwx.dev.purestorage.com/job/portworx-backup/job/system-tests/job/byoc/job/px-backup-on-demand-system-test-byoc/6321/
https://jenkins.pwx.dev.purestorage.com/job/portworx-backup/job/system-tests/job/byoc/job/px-backup-on-demand-system-test-byoc/6324/

TestCase: Verify RestoreObjects are not deleted when unshare from Super Admin is executed
Is failed as we have not done the implementation at px-backup

[2024-08-13T18:37:15.371Z] Summarizing 1 Failure:
[2024-08-13T18:37:15.372Z] [FAIL] {ClusterShare} [It] VerifyRestoreObjectsAreNotDeletedCreatedBySuperAdmin [ClusterUnShare]
[2024-08-13T18:37:15.372Z] /go/src/github.com/portworx/torpedo/pkg/log/log.go:333
[2024-08-13T18:37:15.372Z] Ran 4 of 135 Specs in 1698.978 seconds
[2024-08-13T18:37:15.372Z] FAIL! -- 3 Passed | 1 Failed | 0 Pending | 131 Skipped
[2024-08-13T18:37:15.372Z] --- FAIL: TestBasic (1700.14s)
[2024-08-13T18:37:15.372Z] FAIL

vsundarraj-px · 2024-08-14T06:35:06Z

#2729 is doing exactly same. Is this PR needed ?

kshithijiyer-px · 2024-08-14T07:04:02Z

tests/backup/backup_cluster_share_test.go

+	// This testcase verifies whether the restores created/owned by the user were deleted during the cluster un-share.
+	It("VerifyRestoreObjectsAreDeletedCreatedByNonSuperAdmin", func() {
+		//TODO: Need to update the testrail ID
+		StartPxBackupTorpedoTest("VerifyRestoreObjectsAreDeletedCreatedByNonSuperAdmin", "VerifyRestoreObjectsAreDeletedCreatedByNonSuperAdmin during the cluster unshare", nil, 0, Sgajawada, Q2FY25)


Please make and entry of your name in https://github.com/portworx/torpedo/blob/master/tests/backup_helper.go#L80

Also, please do not add any automation test without importing it into testrail. Testrail ID is mandatory

kshithijiyer-px · 2024-08-14T07:04:54Z

tests/backup/backup_cluster_share_test.go

+
+	})
+	// This testcase verifies whether the restores created/owned by the user with super-admin role were not deleted during the cluster un-share.
+	It("VerifyRestoreObjectsAreNotDeletedCreatedBySuperAdmin", func() {


@mkoppal-px can we go ahead with multiple it's or should we break down the code where we can have one it per describe?

As long as the aetos dashboard shows the correct data, we are good.

kshithijiyer-px · 2024-08-14T07:06:26Z

tests/backup/backup_cluster_share_test.go

+			_, err = Inst().Backup.InspectRestore(ctx, restoreInspectRequest)
+			log.FailOnError(err, "inspect restore %s", restoreName)
+		})
+


Ideally you would want to move cleanup in JustAfterEach

sgajawada-px · 2024-08-14T09:26:16Z

#2729 is doing exactly same. Is this PR needed ?

@ak-px PR includes the helper functions only. Here i have included testcase also.
Once that PR is merged i can use the share and un-share utils from it

dbinnal-px · 2024-08-16T05:37:54Z

tests/backup_helper.go

+	return err
+}
+
+// ShareCluster provides access to the mentioned groups or/add users


comment should be updated according to unshare functionality

dbinnal-px · 2024-08-16T05:48:04Z

tests/backup/backup_cluster_share_test.go

+		})
+
+	})
+	// This testcase verifies whether the restores created/owned by the user with super-admin role were not deleted during the cluster un-share.


Can we combine the above test case and this one, both looks similar apart from in the above test case we want to verify restores are deleted in case of normal user and in this we want to verify restores are not deleted in case of super admin. We can have these as two separate steps.

As discussed with @vsundarraj-px he mentioned it is better to maintain the isolation.

dbinnal-px · 2024-08-16T05:56:20Z

tests/backup/backup_cluster_share_test.go

+			restoreUID       string
+		)
+
+		Step("Get login context for the test users", func() {


Can we move this to beforeEach as I see we are repeating it.

Yes the Step is same but here we were fetching the context after adding the role.
Example:
VerifyRestoreObjectsAreDeletedCreatedByNonSuperAdmin - Uses infra admin role
VerifyRestoreObjectsAreNotDeletedCreatedBySuperAdmin - Uses the super admin role
Fetching the context should be after assigning the user role. So it cant be part of JustBeforeEach

dbinnal-px · 2024-08-16T05:57:30Z

tests/backup/backup_cluster_share_test.go

+			}
+		})
+
+		Step("Create a cluster object with User1 and Share with User2", func() {


Same here, we are repeating it.

It can't be part of JustBeforeEach because
VerifyRestoreCreateBySharedClusterUser is sharing a destination cluster and all the testcases were sharing the source cluster

dbinnal-px · 2024-08-16T05:58:18Z

tests/backup/backup_cluster_share_test.go

+			dash.VerifyFatal(err, nil, fmt.Sprintf("Verifying share of source [%s] cluster with %s user using %s ctx", SourceClusterName, testUsers[2].name, testUsers[1].name))
+		})
+
+		Step("Create backup location and cloud setting with User2", func() {


This one too we are repeating same lines of code

dbinnal-px · 2024-08-16T06:00:49Z

tests/backup/backup_cluster_share_test.go

+
+	})
+	// This testcase verifies the restore creation on the shared cluster by the shared user.
+	It("VerifyRestoreCreateBySharedClusterUser", func() {


Can we verify this case also as part of the first testcase you have written? Looks like it is just verifying restore creation which is part of first testcase

As discussed with @vsundarraj-px he mentioned it is better to maintain the isolation.

ak-px · 2024-08-16T08:51:31Z

tests/backup_helper.go

+	backupDriver := Inst().Backup
+	userIDs := make([]string, 0)
+
+	clusterUid, err := backupDriver.GetClusterUID(ctx, BackupOrgID, clusterName)


@sgajawada-px I'm passing clusterUid as an input parameter for both methods in my PR. Fetching clusterUid may cause issues if the context is admin and the admin has multiple clusters from users with the same name. If you are planning to rebase with my branch, it's better to pass clusterUid at the test case level.

mkoppal-px

Few things to note here:

Please submit one test case per PR. It alway better to have multiple small PRs rather than a one big PR. Easier to review and maintain.
Create sub directory under tests/backup called backup-functional and move the _test.go files there

vsundarraj-px · 2024-08-20T06:01:57Z

@mkoppal-px Not sure we can combine one Test case with one PR. We have created FC tasks based on object operation. For instance restore related test cases will be combined with 1 task.
Similarly, Backup and backup schedule related test cases will have one task again and each task will associate with 1 pr.

mkoppal-px · 2024-08-20T09:29:50Z

@mkoppal-px Not sure we can combine one Test case with one PR. We have created FC tasks based on object operation. For instance restore related test cases will be combined with 1 task. Similarly, Backup and backup schedule related test cases will have one task again and each task will associate with 1 pr.

We usually create one task per test case. It is easier to track and review.

- Verify Cluster Unshare deletes Restore Objects from user. - Verify Cluster unshare when BackupSchedules are deleted. - Verify RestoreObjects are not deleted when unshare from Super Admin is executed. - Shared user can restore to shared cluster from backup created on owned cluster.

sgajawada-px requested review from vpinisetti-px, snigdha-px, mkoppal-px, ak-px, pallav-px and vsundarraj-px August 14, 2024 05:51

sgajawada-px requested a review from a team as a code owner August 14, 2024 05:51

kshithijiyer-px reviewed Aug 14, 2024

View reviewed changes

vsundarraj-px approved these changes Aug 16, 2024

View reviewed changes

dbinnal-px reviewed Aug 16, 2024

View reviewed changes

ak-px reviewed Aug 16, 2024

View reviewed changes

mkoppal-px requested changes Aug 19, 2024

View reviewed changes

sgajawada-px force-pushed the PB-7821 branch 4 times, most recently from 1d07304 to 598fa2b Compare August 23, 2024 03:51

PB-7821: Address the review comments

546d2d8

sgajawada-px force-pushed the PB-7821 branch from 598fa2b to 546d2d8 Compare August 23, 2024 04:12

PB-7821: Testing changes

3067826

sgajawada-px force-pushed the PB-7821 branch from 74b5e39 to 3067826 Compare August 23, 2024 07:22

sgajawada-px added 2 commits August 27, 2024 14:53

Add Cluster UID when creating the backup schedule

ef0492d

PB-7821: create and delete the backup location by torpedo admin user

2ec809c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PB-7821: Automate the few cluster share testcases #2734

PB-7821: Automate the few cluster share testcases #2734

sgajawada-px commented Aug 14, 2024 •

edited

Loading

vsundarraj-px commented Aug 14, 2024

kshithijiyer-px Aug 14, 2024

mkoppal-px Aug 19, 2024

kshithijiyer-px Aug 14, 2024

mkoppal-px Aug 19, 2024

kshithijiyer-px Aug 14, 2024 •

edited

Loading

sgajawada-px commented Aug 14, 2024

dbinnal-px Aug 16, 2024

dbinnal-px Aug 16, 2024

sgajawada-px Aug 21, 2024

dbinnal-px Aug 16, 2024

sgajawada-px Aug 21, 2024

dbinnal-px Aug 16, 2024

sgajawada-px Aug 21, 2024

dbinnal-px Aug 16, 2024

sgajawada-px Aug 21, 2024

dbinnal-px Aug 16, 2024 •

edited

Loading

sgajawada-px Aug 21, 2024

ak-px Aug 16, 2024

mkoppal-px left a comment

vsundarraj-px commented Aug 20, 2024

mkoppal-px commented Aug 20, 2024

PB-7821: Automate the few cluster share testcases #2734

Are you sure you want to change the base?

PB-7821: Automate the few cluster share testcases #2734

Conversation

sgajawada-px commented Aug 14, 2024 • edited Loading

vsundarraj-px commented Aug 14, 2024

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

kshithijiyer-px Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

sgajawada-px commented Aug 14, 2024

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

dbinnal-px Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkoppal-px left a comment

Choose a reason for hiding this comment

vsundarraj-px commented Aug 20, 2024

mkoppal-px commented Aug 20, 2024

sgajawada-px commented Aug 14, 2024 •

edited

Loading

kshithijiyer-px Aug 14, 2024 •

edited

Loading

dbinnal-px Aug 16, 2024 •

edited

Loading