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

OADP-694 oadp-1.1: Add data mover to e2e #863

Merged
merged 10 commits into from
Nov 18, 2022

Conversation

kaovilai
Copy link
Member

Signed-off-by: Tiger Kaovilai tkaovila@redhat.com

override images

comment image overrides

Use 1.1 images + emcmulla/csi-plugin:latest

Signed-off-by: Tiger Kaovilai tkaovila@redhat.com

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

override images

comment image overrides

Use 1.1 images + emcmulla/csi-plugin:latest

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2022

Codecov Report

Base: 32.65% // Head: 32.65% // No change to project coverage 👍

Coverage data is based on head (f19a66c) compared to base (165e436).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##           oadp-1.1     #863   +/-   ##
=========================================
  Coverage     32.65%   32.65%           
=========================================
  Files            17       17           
  Lines          3237     3237           
=========================================
  Hits           1057     1057           
  Misses         2088     2088           
  Partials         92       92           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kaovilai
Copy link
Member Author

4.9 aws succeeded.

/retest

@kaovilai
Copy link
Member Author

one timed out on datamover
one timed out on non data mover entry

@kaovilai
Copy link
Member Author

/retest

2 similar comments
@kaovilai
Copy link
Member Author

/retest

@weshayutin
Copy link
Contributor

/retest

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
@kaovilai
Copy link
Member Author

/retest

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
@kaovilai
Copy link
Member Author

/retest

@kaovilai
Copy link
Member Author

/retest

@kaovilai
Copy link
Member Author

/test 4.10-operator-e2e-azure

@@ -186,7 +261,7 @@ var _ = Describe("AWS backup restore tests", func() {
Expect(err).ToNot(HaveOccurred())

// wait for backup to not be running
Eventually(IsBackupDone(dpaCR.Client, namespace, backupName), timeoutMultiplier*time.Minute*12, time.Second*10).Should(BeTrue())
Eventually(IsBackupDone(dpaCR.Client, namespace, backupName), timeoutMultiplier*time.Minute*20, time.Second*10).Should(BeTrue())
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is data mover test, would it be possible to also check if VSB is completed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Velero backup status should be reliable. VSB status should really only rarely be checked when things go wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's leave it out. Could add on failure. Follow up todo?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually VSB/VSR is already checked on failure

Copy link
Member Author

Choose a reason for hiding this comment

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

*logged on failure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good

// run restore
log.Printf("Creating restore %s for case %s", restoreName, brCase.Name)
restore, err := CreateRestoreFromBackup(dpaCR.Client, namespace, backupName, restoreName)
Expect(err).ToNot(HaveOccurred())
Eventually(IsRestoreDone(dpaCR.Client, namespace, restoreName), timeoutMultiplier*time.Minute*4, time.Second*10).Should(BeTrue())
Eventually(IsRestoreDone(dpaCR.Client, namespace, restoreName), timeoutMultiplier*time.Minute*60, time.Second*10).Should(BeTrue())
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here for VSR

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually VSB/VSR is already checked on failure

tests/e2e/backup_restore_suite_test.go Outdated Show resolved Hide resolved
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
@kaovilai
Copy link
Member Author

/test 4.9-operator-e2e-gcp

@kaovilai
Copy link
Member Author

/retest

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
@@ -171,14 +193,26 @@ func (v *DpaCustomResource) CreateOrUpdateWithRetries(spec *oadpv1alpha1.DataPro
)
for i := 0; i < retries; i++ {
if cr, err = v.Get(); apierrors.IsNotFound(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

was there repeated errors creating the dpa here or is this just an improvement?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ensuring the content of v.CustomResource that's all.

return buf.String(), nil
}

func GetDeploymentPodContainerLogs(namespace, deploymentName, containerName string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

oh snap.. am I reading the correctly? We have to verify the that restore worked w/ datamover by scraping the deployment log?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's used here in case of failure for dumping out logs

Copy link
Contributor

@weshayutin weshayutin left a comment

Choose a reason for hiding this comment

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

looks awesome but I do have some questions

Comment on lines +145 to +146
GinkgoWriter.Println("Printing volume-snapshot-mover deployment pod logs")
GinkgoWriter.Print(GetDeploymentPodContainerLogs(namespace, common.DataMover, common.DataMoverControllerContainer))
Copy link
Contributor

Choose a reason for hiding this comment

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

can we get some code comments as to why we're pulling the log

Copy link
Contributor

@weshayutin weshayutin left a comment

Choose a reason for hiding this comment

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

please add code comments as to why

Copy link
Contributor

@weshayutin weshayutin left a comment

Choose a reason for hiding this comment

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

spoke to Tiger about a few things, will land improvements in jira. Approved well done!

@openshift-ci
Copy link

openshift-ci bot commented Nov 18, 2022

@kaovilai: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@weshayutin weshayutin merged commit 19c4952 into openshift:oadp-1.1 Nov 18, 2022
@@ -186,7 +261,7 @@ var _ = Describe("AWS backup restore tests", func() {
Expect(err).ToNot(HaveOccurred())

// wait for backup to not be running
Eventually(IsBackupDone(dpaCR.Client, namespace, backupName), timeoutMultiplier*time.Minute*12, time.Second*10).Should(BeTrue())
Eventually(IsBackupDone(dpaCR.Client, namespace, backupName), timeoutMultiplier*time.Minute*20, time.Second*10).Should(BeTrue())
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good

err = dpaCR.Client.List(context.Background(), &pvcList, &client.ListOptions{Namespace: namespace})
Expect(err).NotTo(HaveOccurred())
GinkgoWriter.Printf("PVC oadp ns list %v\n", pvcList)

Copy link
Contributor

Choose a reason for hiding this comment

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

Worth listing volumeSnapshotContents? wdyt

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

@hhpatel14
Copy link
Contributor

/cherry-pick master

@openshift-cherrypick-robot
Copy link
Contributor

@hhpatel14: #863 failed to apply on top of branch "master":

Applying: Add data mover to e2e
Using index info to reconstruct a base tree...
M	Makefile
M	go.mod
M	go.sum
M	tests/e2e/backup_restore_suite_test.go
M	tests/e2e/lib/apps.go
M	tests/e2e/lib/dpa_helpers.go
Falling back to patching base and 3-way merge...
Auto-merging tests/e2e/lib/dpa_helpers.go
Auto-merging tests/e2e/lib/apps.go
Auto-merging tests/e2e/backup_restore_suite_test.go
CONFLICT (content): Merge conflict in tests/e2e/backup_restore_suite_test.go
Auto-merging go.sum
CONFLICT (content): Merge conflict in go.sum
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
Auto-merging Makefile
CONFLICT (content): Merge conflict in Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add data mover to e2e
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick master

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

hhpatel14 pushed a commit to hhpatel14/oadp-operator that referenced this pull request Apr 10, 2023
Co-authored-by: Tiger Kaovilai <tiger+github@redhat.com>
kaovilai added a commit that referenced this pull request Apr 13, 2023
* master: Add data mover to e2e cherrypick of #863

Co-authored-by: Tiger Kaovilai <tiger+github@redhat.com>

* workaround to fix existing PVC of the same name found..

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

* 1.2 update

* add restorePhase async plugin waiting phases

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

* Remove duplicate application installs, add CSIDataMover failure deletion handling, volsync channel stable-0.7

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

* Create app, and therefore namespace, before creating PVC into namespace

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

* FEntry typo

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

* enable volsync MoverSecurityContext

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

* Update mysql/mariadb pod securityContext to match Dockerfile USER for volsync to pick up.

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

* add vsmv1alpha1 to client scheme for failure cleanup

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

---------

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Co-authored-by: Tiger Kaovilai <tkaovila@redhat.com>
Co-authored-by: Tiger Kaovilai <tiger+github@redhat.com>
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/oadp-operator that referenced this pull request Apr 13, 2023
Co-authored-by: Tiger Kaovilai <tiger+github@redhat.com>
kaovilai added a commit that referenced this pull request Apr 13, 2023
* master: Add data mover to e2e cherrypick of #863

Co-authored-by: Tiger Kaovilai <tiger+github@redhat.com>

* workaround to fix existing PVC of the same name found..

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

* 1.2 update

* add restorePhase async plugin waiting phases

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

* Remove duplicate application installs, add CSIDataMover failure deletion handling, volsync channel stable-0.7

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

* Create app, and therefore namespace, before creating PVC into namespace

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

* FEntry typo

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

* enable volsync MoverSecurityContext

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

* Update mysql/mariadb pod securityContext to match Dockerfile USER for volsync to pick up.

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

* add vsmv1alpha1 to client scheme for failure cleanup

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

---------

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Co-authored-by: Tiger Kaovilai <tkaovila@redhat.com>
Co-authored-by: Tiger Kaovilai <tiger+github@redhat.com>
Co-authored-by: hhpatel14 <hitpatel@redhat.com>
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.

None yet

6 participants