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.

Sign up for GitHub

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

Jump to bottom

Make CSI snapshot creation timeout configurable for backup and schedule. #5104

Merged
merged 2 commits into from
Aug 8, 2022
Merged

Make CSI snapshot creation timeout configurable for backup and schedule. #5104

merged 2 commits into from
Aug 8, 2022

Conversation

blackpiglet
Copy link
Contributor

Signed-off-by: Xun Jiang [email protected]

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #5048

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

@blackpiglet blackpiglet self-assigned this Jul 8, 2022
@blackpiglet blackpiglet force-pushed the 5048-CSI-snapshot-timeout-configurable branch from 2b3fc31 to 630106b Compare July 8, 2022 01:31
@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2022 •
edited
Loading

Codecov Report

Merging #5104 (bee69db) into main (cea5e7f) will increase coverage by 0.04%.
The diff coverage is 37.50%.

@@            Coverage Diff             @@
##             main    #5104      +/-   ##
==========================================
+ Coverage   41.54%   41.58%   +0.04%     
==========================================
  Files         215      215              
  Lines       18625    18637      +12     
==========================================
+ Hits         7738     7751      +13     
  Misses      10308    10308              
+ Partials      579      578       -1
Impacted Files Coverage Δ
pkg/builder/backup_builder.go 0.00% <0.00%> (ø)
pkg/cmd/server/server.go 6.48% <0.00%> (-0.03%) ⬇️
pkg/controller/backup_controller.go 48.74% <50.00%> (+0.23%) ⬆️
pkg/cmd/cli/backup/create.go 22.32% <66.66%> (+0.25%) ⬆️
pkg/restore/restore.go 64.63% <0.00%> (+0.56%) ⬆️

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

@blackpiglet blackpiglet requested a review from reasonerjt July 11, 2022 02:53
shubham-pampattiwar
shubham-pampattiwar reviewed Jul 13, 2022
View reviewed changes
config/crd/v1/bases/velero.io_schedules.yaml Outdated
@@ -61,6 +61,11 @@ spec:
description: Template is the definition of the Backup to be run on
the provided schedule
properties:
csiSnapshotCreationTimeout:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add in the description that the default timeout is going to be 10 min ? Also, add in the docs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I will add the default value too.

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 added some description into site/content/docs/main/examples.md. I didn't find a document is suitable for the parameter. What do you think?

@blackpiglet blackpiglet force-pushed the 5048-CSI-snapshot-timeout-configurable branch 2 times, most recently from 5014083 to 4d30cde Compare July 13, 2022 04:13
shubham-pampattiwar
shubham-pampattiwar previously approved these changes Jul 13, 2022
View reviewed changes
Copy link
Collaborator

@shubham-pampattiwar shubham-pampattiwar left a comment

Choose a reason for hiding this comment

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

@blackpiglet Changes look sane to me, Thanks for the updates !

@drewboswell
Copy link

Is this going to make the cut for the next release? I'll use it a few minutes after it's published if yes.

🙏

@rohitss912
Copy link

Any ETA on when this change will make it to the release? We are seeing intermittent Partial Backup failures because AWS might be too slow to perform snapshots and we really would like have the timeout configurable.

@blackpiglet
Copy link
Contributor Author

blackpiglet commented Aug 2, 2022 •
edited
Loading

@rohitss912 @drewboswell
This PR is targeted at Velero v1.10. Currently, team is busy with Kopia integration developing and CAPI support feature. Since there is some use case blocked by this, we will make some progress, but you may need to use main branch to get this change.
Does it sound good for you?

@Lyndon-Li Lyndon-Li mentioned this pull request Aug 5, 2022
Closed
reasonerjt
reasonerjt previously approved these changes Aug 5, 2022
View reviewed changes
@blackpiglet blackpiglet force-pushed the 5048-CSI-snapshot-timeout-configurable branch from 4d30cde to 60460a2 Compare August 7, 2022 03:46
reasonerjt
reasonerjt previously approved these changes Aug 8, 2022
View reviewed changes
@reasonerjt reasonerjt added this to the v1.9.1 milestone Aug 8, 2022
@blackpiglet blackpiglet force-pushed the 5048-CSI-snapshot-timeout-configurable branch from bee69db to f8d9cfd Compare August 8, 2022 09:07
reasonerjt
reasonerjt approved these changes Aug 8, 2022
View reviewed changes
Copy link
Contributor

@reasonerjt reasonerjt left a comment

Choose a reason for hiding this comment

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

lgtm

@reasonerjt reasonerjt merged commit 6951875 into vmware-tanzu:main Aug 8, 2022
@blackpiglet blackpiglet deleted the 5048-CSI-snapshot-timeout-configurable branch October 15, 2022 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ywk253100 ywk253100 ywk253100 approved these changes

@reasonerjt reasonerjt reasonerjt approved these changes

@shubham-pampattiwar shubham-pampattiwar shubham-pampattiwar left review comments

@qiuming-best qiuming-best Awaiting requested review from qiuming-best

Assignees

@blackpiglet blackpiglet

Labels
Documentation has-changelog has-unit-tests
Projects
None yet
Milestone
v1.9.1
Development

Successfully merging this pull request may close these issues.

Make snapshot timeout configurable
7 participants
@blackpiglet @codecov-commenter @drewboswell @rohitss912 @reasonerjt @ywk253100 @shubham-pampattiwar