feat(*): Add --wait flag #1820

thomastaylor312 · 2017-01-13T00:27:57Z

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 #1805

thomastaylor312 · 2017-01-13T00:29:16Z

pkg/kube/client_test.go

+	// 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


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?

thomastaylor312 · 2017-01-13T00:30:38Z

pkg/kube/client.go

+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) {


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

thomastaylor312 · 2017-01-13T00:32:16Z

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.

technosophos · 2017-01-13T18:49:45Z

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:

Push all of the manifests into the Kubernetes at once
If no errors happen there, then watch the created objects until they are all online or timeout.

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.

thomastaylor312 · 2017-01-13T20:54:47Z

@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.

adamreese · 2017-01-13T21:06:24Z

@thomastaylor312 #1823

technosophos · 2017-01-13T21:12:26Z

Oh, it does? I must be misreading what perform() does at the end.

thomastaylor312 · 2017-01-13T21:13:03Z

@technosophos I am not sure if my code overlaps with @adamreese's. Mine needs the objects (runtime.Object to be specific) that are created by those infos, but I am inexperienced with the k8s api libraries, so I could be wrong

thomastaylor312 · 2017-01-13T23:54:59Z

@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

thomastaylor312 · 2017-01-20T02:16:25Z

@technosophos and @adamreese this is ready for some more testing/review

bacongobbler · 2017-01-20T05:22:17Z

pkg/kube/client_test.go

 		t.Fatal(err)
 	}
-
+	// TODO: Find a way to test methods that use Client Set


should this code be removed, or is this a placeholder for things that should be tested but currently do not work?

The latter. @technosophos and @adamreese asked me to keep it in because this should be tested but can't be

bacongobbler · 2017-01-20T05:25:44Z

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 docs/?

thomastaylor312 · 2017-01-20T17:36:24Z

@bacongobbler Thanks for the review! I'll probably write that and push it up today.

paultiplady · 2017-01-24T16:40:03Z

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.

bacongobbler · 2017-01-24T16:53:36Z

@paultiplady that issue you're referencing 404s for me. Wrong copypasta?

paultiplady · 2017-01-24T18:02:44Z

Oops -- indeed, too many Github tabs open. Edited.

technosophos · 2017-01-24T20:44:57Z

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.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 13, 2017

thomastaylor312 commented Jan 13, 2017

View reviewed changes

thomastaylor312 force-pushed the feat/add_wait branch from aea2e27 to b4bf829 Compare January 13, 2017 00:31

thomastaylor312 mentioned this pull request Jan 13, 2017

create chart testing framework #1163

Closed

bacongobbler mentioned this pull request Jan 16, 2017

Feature request: helm install --wait #1805

Closed

feat(*): Add --wait flag

7ef9bb6

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

thomastaylor312 force-pushed the feat/add_wait branch from b4bf829 to 7ef9bb6 Compare January 20, 2017 02:10

bacongobbler reviewed Jan 20, 2017

View reviewed changes

Adds documentation for --wait and other new command line flags

5b11624

technosophos added the awaiting review label Jan 23, 2017

technosophos added lgtm and removed awaiting review labels Jan 26, 2017

thomastaylor312 merged commit e440a36 into helm:master Jan 26, 2017

technosophos added this to the 2.2.0 milestone Jan 26, 2017

mboersma mentioned this pull request Feb 15, 2017

Use helm install --wait flag instead of custom polling logic deis/jenkins-jobs#333

Closed

manics mentioned this pull request Feb 8, 2018

Pin helm version to v2.6.2 jupyterhub/zero-to-jupyterhub-k8s#486

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(*): Add --wait flag #1820

feat(*): Add --wait flag #1820

thomastaylor312 commented Jan 13, 2017

thomastaylor312 Jan 13, 2017

thomastaylor312 Jan 13, 2017

thomastaylor312 commented Jan 13, 2017

technosophos commented Jan 13, 2017

thomastaylor312 commented Jan 13, 2017 •

edited

adamreese commented Jan 13, 2017

technosophos commented Jan 13, 2017

thomastaylor312 commented Jan 13, 2017 •

edited

thomastaylor312 commented Jan 13, 2017

thomastaylor312 commented Jan 20, 2017

bacongobbler Jan 20, 2017

thomastaylor312 Jan 20, 2017

bacongobbler commented Jan 20, 2017 •

edited

thomastaylor312 commented Jan 20, 2017

paultiplady commented Jan 24, 2017 •

edited

bacongobbler commented Jan 24, 2017

paultiplady commented Jan 24, 2017

technosophos commented Jan 24, 2017

feat(*): Add --wait flag #1820

feat(*): Add --wait flag #1820

Conversation

thomastaylor312 commented Jan 13, 2017

thomastaylor312 Jan 13, 2017

Choose a reason for hiding this comment

thomastaylor312 Jan 13, 2017

Choose a reason for hiding this comment

thomastaylor312 commented Jan 13, 2017

technosophos commented Jan 13, 2017

thomastaylor312 commented Jan 13, 2017 • edited

adamreese commented Jan 13, 2017

technosophos commented Jan 13, 2017

thomastaylor312 commented Jan 13, 2017 • edited

thomastaylor312 commented Jan 13, 2017

thomastaylor312 commented Jan 20, 2017

bacongobbler Jan 20, 2017

Choose a reason for hiding this comment

thomastaylor312 Jan 20, 2017

Choose a reason for hiding this comment

bacongobbler commented Jan 20, 2017 • edited

thomastaylor312 commented Jan 20, 2017

paultiplady commented Jan 24, 2017 • edited

bacongobbler commented Jan 24, 2017

paultiplady commented Jan 24, 2017

technosophos commented Jan 24, 2017

thomastaylor312 commented Jan 13, 2017 •

edited

thomastaylor312 commented Jan 13, 2017 •

edited

bacongobbler commented Jan 20, 2017 •

edited

paultiplady commented Jan 24, 2017 •

edited