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

Check the failed phases either when uploading the snapshot in E2E testing #4162

Merged
merged 1 commit into from Nov 10, 2021

Conversation

ywk253100
Copy link
Contributor

When the snapshot uploading is failed, it should not be treat as completed and continue.
This commit covers both the phases of in progress and failed when uploading snapshot with vSphere plugin

Signed-off-by: Wenkai Yin(尹文开) yinw@vmware.com

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #(issue)

Please indicate you've done the following:

@ywk253100 ywk253100 added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Sep 19, 2021
Copy link
Contributor

@zubron zubron left a comment

Choose a reason for hiding this comment

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

This is a good spot, thanks! We should return from the function earlier though if we know the snapshot has failed rather than relying on the timeout to signal an error.

phase == "Snapshotted" ||
phase == "Uploading" {
// if the phase isn't "Uploaded", the uploading is either in progress or failed(will retry, if the retrying cannot recover, will timeout in the final)
if phase != "Uploaded" {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should return an error in the case where the snapshot reached a failed terminal state as it's not possible to recover from that. By checking for Uploaded only, this function could poll for up to an hour for a snapshot that we know can't be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zubron I noticed that seems the uploading will be retry by the vSphere plugin, I'm not sure whether the failed uploading can be fixed by the retrying, so don't return the error here

Should we expect the retrying may fix the failed uploading or just return the 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, sorry - I didn't realize that it would retry the upload 🤦‍♀️ In that case, it's probably best to ignore my comment and check with @dsu-igeek :)

@zubron zubron self-requested a review September 19, 2021 15:14
…ting

When the snapshot uploading is failed, it should not be treat as completed and continue.
This commit covers both the phases of in progress and failed when uploading snapshot with vSphere plugin

Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com>
Copy link
Contributor

@dsu-igeek dsu-igeek left a comment

Choose a reason for hiding this comment

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

LGTM

@dsu-igeek dsu-igeek merged commit 27f3a6d into vmware-tanzu:main Nov 10, 2021
danfengliu pushed a commit to danfengliu/velero that referenced this pull request Jan 25, 2022
…ting (vmware-tanzu#4162)

When the snapshot uploading is failed, it should not be treat as completed and continue.
This commit covers both the phases of in progress and failed when uploading snapshot with vSphere plugin

Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com>
gyaozhou pushed a commit to gyaozhou/velero-read that referenced this pull request May 14, 2022
…ting (vmware-tanzu#4162)

When the snapshot uploading is failed, it should not be treat as completed and continue.
This commit covers both the phases of in progress and failed when uploading snapshot with vSphere plugin

Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-e2e-tests kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants