-
Notifications
You must be signed in to change notification settings - Fork 7k
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
feat(*): Add --wait flag #1820
feat(*): Add --wait flag #1820
Conversation
// if err := c.Update("test", objBody(codec, &listB), objBody(codec, &listC), false, 300, true); err != nil { | ||
// t.Fatal(err) | ||
// } | ||
// //Test with a wait should fail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question for these tests. These cause a panic because the mock client doesn't have a ClientSet
method. Is there another way to test this?
func (c *Client) waitForResources(timeout time.Duration, objects []runtime.Object) error { | ||
log.Printf("beginning wait for resources with timeout of %v", timeout) | ||
client, _ := c.ClientSet() | ||
return wait.Poll(2*time.Second, timeout, func() (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do not like this method, I also have an idea to implement this using watch
and merging all of the watch channels that come out of it
aea2e27
to
b4bf829
Compare
I did as much of the unit tests as I could think of. If there are other ways we could test this, please let me know. |
Thanks! That was a big patch, and I know you did a lot of work. I have an idea that might make this better. If I understand this correctly, --wait will cause Tiller to wait between each resource installation. So if I install two Deployment objects, it will install one, block, then install the second. That could have some unintended consequences because it will bypass the Kubernetes resolution loop (where it spins up multiple things at once, and they eventually all come online). Charts that work fine today will break if the --wait flag is supplied. I think what we want to do instead is something like this:
To that end, @adamreese is doing a PR that will fix some of the internals on the kubernetes library so that you can easily get a list of objects that have been pushed in. When that is done (hopefully today), then can we see if there's a way to use those objects in a watch? Again, thanks. I know you've already put a lot of work into this. |
@technosophos: I would love to use that code that @adamreese is making. Is it already open? As for the behavior, it is doing as you described. It creates all resources and then waits. |
Oh, it does? I must be misreading what |
@technosophos I am not sure if my code overlaps with @adamreese's. Mine needs the objects ( |
@technosophos I'll rebase this against master once the initial review is done so I can make any requested changes and fix conflicts |
Adds `--wait` flag to helm that waits for all pods to reach a ready state, PVCs to be bound, and services to have IP addresses Closes helm#1805
b4bf829
to
7ef9bb6
Compare
@technosophos and @adamreese this is ready for some more testing/review |
t.Fatal(err) | ||
} | ||
|
||
// TODO: Find a way to test methods that use Client Set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this code be removed, or is this a placeholder for things that should be tested but currently do not work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latter. @technosophos and @adamreese asked me to keep it in because this should be tested but can't be
The code itself and the documentation (in the form of the CLI help usage) looks like it's all well thought out and well-written from an outsider's perspective. My only comment would be is there somewhere we can document this new feature flag in |
@bacongobbler Thanks for the review! I'll probably write that and push it up today. |
This feature will be very useful, thanks for putting this together. For completeness, note that this has been discussed in great detail and for quite some time on the core k8s repo: kubernetes/kubernetes#1899 The concept of 'readiness' is quite hard to pin down in a generic way. Hopefully Helm can carve off some low-hanging fruit. |
@paultiplady that issue you're referencing 404s for me. Wrong copypasta? |
Oops -- indeed, too many Github tabs open. Edited. |
We'll have to make sure to document this as a breaking change to the client library. Not a huge deal since in the 2.2 release we're breaking a few APIs in this package in order to make it work better. I just need to remember to actually say something in the release notes about it. |
Adds
--wait
flag to helm that waits for all pods to reach a readystate, PVCs to be bound, and services to have IP addresses
Closes #1805