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

velero install: wait for restic daemonset to be ready #1859

Merged
merged 2 commits into from
Sep 9, 2019

Conversation

skriss
Copy link
Contributor

@skriss skriss commented Sep 9, 2019

Signed-off-by: Steve Kriss [email protected]

Closes #1716

This is a copy-paste with modifications from the code that waits for the deployment to be ready. I looked again at the API docs for daemonsets, and I believe the readiness check I have makes the most sense -- comparing the number of expected scheduled pods to the number of "available" pods, which means pods on proper nodes that have been ready for spec.minReadySeconds. It seems like the Octant logic could falsely report "ready" if not all pods have been started yet.

@skriss skriss requested review from carlisia, nrb and prydonius September 9, 2019 17:59
@skriss skriss force-pushed the install-restic-wait branch from 9815dd3 to a5c63f9 Compare September 9, 2019 18:01
@@ -0,0 +1 @@
velero install: if `--use-restic` and `--wait` are specified, wait for restic daemonset to be ready
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to mention the timeout here?

I think we're up to a total of 2 minutes between the deployment and daemonset waits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure I can add that.

I didn't bother doing anything more complicated like waiting for both concurrently, as it would require more significant refactoring and it didn't seem super-important to be, but open to others' input on it.

Copy link
Contributor

@nrb nrb Sep 9, 2019

Choose a reason for hiding this comment

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

Yeah, I agree that separate checks with their own timeouts make sense. I'm not sure there's tons of benefit to combining the checks or moving them into concurrent goroutines.

Signed-off-by: Steve Kriss <[email protected]>
@skriss skriss force-pushed the install-restic-wait branch from a5c63f9 to e464dba Compare September 9, 2019 18:10
Copy link
Contributor

@prydonius prydonius left a comment

Choose a reason for hiding this comment

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

lgtm

@nrb nrb merged commit 1119006 into vmware-tanzu:master Sep 9, 2019
@skriss skriss deleted the install-restic-wait branch September 9, 2019 22:01
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.

Check restic daemonset for readiness on install
3 participants