Add the --wait flag to install #966

anthonydahanne · 2019-10-18T03:23:03Z

work in progress, not working yet
the idea to have the status call returns something is there, but I guess the "something" needs to be reworked

What this PR does / why we need it:
Attempt to solve #964

Fixes #964

anthonydahanne · 2019-10-18T03:24:07Z

gerred · 2019-10-18T18:37:33Z

pkg/kudoctl/cmd/install/install.go

+					return errors.Wrap(err, "getting plan status")
+				}
+				if status.Pending {
+					time.Sleep(2 * time.Second)


couple notes over this block:

I would consider replacing this with a for + select, and have a few channels you're selecting on. This will enable you to add timeouts in a nice, clean way. Eventually we might want to move this out to some Waiter package/func/interface as well.

Also, I'd think we'd want some sort of timeout, even if it's high (10 minutes?).

Check out the select block here to see a bit of what I mean on that:

https://gobyexample.com/timeouts

Where this gets interesting (and maybe we pair on Monday) is you'd need some sort of goroutine-based retry for actually fetching the planStatus.

actually, nevermind, this handles the client in the way we want it to. So we may just want to separate out the display and the logic into two separate functions. If you want to try your hand at that...that'd be a great next step before we do any refactoring into channel-based logic.

gerred · 2019-10-24T11:19:00Z

Hey @anthonydahanne following up on this. :) Can I help you in any way?

kensipe · 2019-11-20T19:52:00Z

@anthonydahanne could you rebase or merge latest master into this please? I'll work with you to get this merge in when we get on the latest. Thanks!!

anthonydahanne · 2019-11-22T02:11:20Z

hello @kensipe: tried to rebase with my conference fried brain, so, yeah, not super confident.

Since I submitted, a new install.go class was created under another namespace, and that makes the usage of PlanStatus, that requires a Settings object, kind of complicated?

Once again, maybe I don't make sense, I haven't even tested locally since I could not find quickly the instructions to rebuild and run Kudo...

gerred · 2020-01-06T16:37:27Z

Hey @anthonydahanne - great seeing you at KubeCon. Want to pair to finish and merge this?

kensipe · 2020-03-02T18:50:27Z

ping @anthonydahanne any interest in pairing or finishing this?

anthonydahanne · 2020-03-03T14:11:52Z

hey there! sorry for late reply!
Yes I'd love to. Please let me refresh my memory and ping you back!

kensipe · 2020-03-03T14:26:07Z

sweet!

anthonydahanne · 2020-03-07T21:15:35Z

all fine now!

~~having a quick look; I'm on master, I built the cli with make cli (I'm on Mac and using DockerForMac that comes with kubernetes 1.1.5.5 ) and when I do a~~

bin/kubectl-kudo init

~~the kudo-controller-manager is stuck on:~~

{"level":"info","ts":1583615409.944507,"logger":"entrypoint","msg":"KUDO Version: version.Info{GitVersion:\"0.7.2\", GitCommit:\"b48cef30\", BuildDate:\"2019-09-30T20:43:12Z\", GoVersion:\"go1.13.1\", Compiler:\"gc\", Platform:\"linux/amd64\"}"}
{"level":"info","ts":1583615409.9446228,"logger":"entrypoint","msg":"setting up manager"}
{"level":"info","ts":1583615410.3500917,"logger":"controller-runtime.metrics","msg":"metrics server is starting to listen","addr":":8080"}
{"level":"info","ts":1583615410.3505058,"logger":"entrypoint","msg":"Registering Components."}
{"level":"info","ts":1583615410.3505895,"logger":"entrypoint","msg":"setting up scheme"}
{"level":"info","ts":1583615410.3508122,"logger":"entrypoint","msg":"Setting up operator controller"}
{"level":"info","ts":1583615410.3512084,"logger":"controller-runtime.controller","msg":"Starting EventSource","controller":"operator","source":"kind source: /, Kind="}
{"level":"error","ts":1583615410.351352,"logger":"controller-runtime.source","msg":"if kind is a CRD, it should be installed before calling Start","kind":"Operator.kudo.dev","error":"no matches for kind \"Operator\" in version \"kudo.dev/v1alpha1\"","stacktrace":"github.com/go-logr/zapr.(*zapLogger).Error\n\t/Users/kensipe/projects/go/pkg/mod/github.com/go-logr/zapr@v0.1.0/zapr.go:128\nsigs.k8s.io/controller-runtime/pkg/source.(*Kind).Start\n\t/Users/kensipe/projects/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.2.0/pkg/source/source.go:88\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Watch\n\t/Users/kensipe/projects/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.2.0/pkg/internal/controller/controller.go:122\nsigs.k8s.io/controller-runtime/pkg/builder.(*Builder).doWatch\n\t/Users/kensipe/projects/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.2.0/pkg/builder/controller.go:162\nsigs.k8s.io/controller-runtime/pkg/builder.(*Builder).Build\n\t/Users/kensipe/projects/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.2.0/pkg/builder/controller.go:151\nsigs.k8s.io/controller-runtime/pkg/builder.(*Builder).Complete\n\t/Users/kensipe/projects/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.2.0/pkg/builder/controller.go:129\ngithub.com/kudobuilder/kudo/pkg/controller/operator.(*Reconciler).SetupWithManager\n\t/Users/kensipe/projects/go/src/github.com/kudobuilder/kudo/pkg/controller/operator/operator_controller.go:39\nmain.main\n\t/Users/kensipe/projects/go/src/github.com/kudobuilder/kudo/cmd/manager/main.go:60\nruntime.main\n\t/usr/local/Cellar/go/1.13.1/libexec/src/runtime/proc.go:203"}
{"level":"error","ts":1583615410.351561,"logger":"entrypoint","msg":"unable to register operator controller to the manager","error":"no matches for kind \"Operator\" in version \"kudo.dev/v1alpha1\"","stacktrace":"github.com/go-logr/zapr.(*zapLogger).Error\n\t/Users/kensipe/projects/go/pkg/mod/github.com/go-logr/zapr@v0.1.0/zapr.go:128\nmain.main\n\t/Users/kensipe/projects/go/src/github.com/kudobuilder/kudo/cmd/manager/main.go:62\nruntime.main\n\t/usr/local/Cellar/go/1.13.1/libexec/src/runtime/proc.go:203"}

anthonydahanne · 2020-03-07T22:52:29Z

ok! memory refreshed: there's a cycle because I want to use plan package from util/kudo package (I want to check on the Status; if it's empty, it means the install done and I can return, otherwise it's pending and I wait)

It's not necessary to rely on package

ERRO Running error: buildssa: analysis skipped: errors in package: [/Users/anthony/workspaces/kudo/pkg/kudoctl/cmd/plan/plan_history.go:14:2: could not import github.com/kudobuilder/kudo/pkg/kudoctl/env (pkg/kudoctl/cmd/get/get.go:10:2: import cycle not allowed: import stack: [github.com/kudobuilder/kudo/cmd/kubectl-kudo github.com/kudobuilder/kudo/pkg/kudoctl/cmd github.com/kudobuilder/kudo/pkg/kudoctl/cmd/get github.com/kudobuilder/kudo/pkg/kudoctl/env github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo github.com/kudobuilder/kudo/pkg/kudoctl/cmd/plan github.com/kudobuilder/kudo/pkg/kudoctl/env]) /Users/anthony/workspaces/kudo/pkg/kudoctl/cmd/plan/plan_status.go:12:2: could not import github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo (pkg/kudoctl/env/environment.go:11:2: import cycle not allowed: import stack: [github.com/kudobuilder/kudo/cmd/kubectl-kudo github.com/kudobuilder/kudo/pkg/kudoctl/cmd github.com/kudobuilder/kudo/pkg/kudoctl/cmd/get github.com/kudobuilder/kudo/pkg/kudoctl/env github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo github.com/kudobuilder/kudo/pkg/kudoctl/cmd/plan github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo]) -: could not load export data: dependency "github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo" hasn't been loaded yet]

~~Please hit me up on Slack!~~

* get the status of the current plan, until it is completed then return Signed-off-by: Anthony Dahanne <anthony.dahanne@gmail.com>

runyontr · 2020-03-10T13:57:12Z

pkg/kudoctl/util/kudo/install.go

@@ -63,6 +64,26 @@ func InstallPackage(kc *Client, resources *packages.Resources, skipInstance bool
 		if _, err := kc.InstallInstanceObjToCluster(resources.Instance, namespace); err != nil {


I think I would rather the logic for this "wait" to be included in the client so that people using the client to install can leverage this. Eventually we would have the update call also have a "wait" option

I had to add my own "wait" function in the TF provider since there wasn't one in the client. Adding this wait like you've done won't let me eliminate the wait I have:

https://github.com/runyontr/terraform-provider-kudo/blob/master/kudo/resource_kudo_instance.go#L547-L576

kensipe · 2020-03-10T18:14:21Z

This can be tough on a new community contribution but there has been some evolution of thought here which sets a new acceptance criteria. I am willing to merge this as a good starting point and thank you for the contribution! Or we can see the full impl on this PR.

Here is what we are looking to do with this:

we need wait for more than just install. So we should have a separate function for wait on a plan.
the wait should be part of the kudo.Client allowing it to be used as a lib... for instance for terraform
It is likely we need to change the timeout for waits (I was initially thinking timeout was defined when acquiring a kudo.Client but I can see wait requiring it for each invocation.
the wait should work for an update, the code for terraform provide is a good start: https://github.com/runyontr/terraform-provider-kudo/blob/master/kudo/resource_kudo_instance.go#L547-L576. however it doesn't work well for update. It needs to check the meta.generation to verify that the plan actually started. Without the meta.generation it is possible that the "last plan" is "finished" and that is the wait func view of the world... which is incorrect.

To narrow the scope... we are not looking for wait to work with upgrade and update... just that the solution should have that in mind and provide a solution that can be leveraged.

kensipe · 2020-03-10T18:16:36Z

The assign at this point (to me) is as a shepard to @anthonydahanne

kensipe · 2020-03-10T21:42:04Z

The plan for this now after talking with anthony is to merge this in (with the value it brings) and take ownership of the new scope listed ^^. I want to hold until after the 0.11.0 release which is scheduled for tomorrow.

kensipe

lgtm for the initial commit. Follow on work has been identified in Issue #1418

gerred reviewed Oct 18, 2019

View reviewed changes

alenkacz added the Hacktoberfest label Oct 24, 2019

anthonydahanne force-pushed the issue964-install-wait-support branch from 5571bec to bd21ba2 Compare November 22, 2019 02:06

Fixes kudobuilder#964: Add the --wait flag to install

60b2e09

* get the status of the current plan, until it is completed then return Signed-off-by: Anthony Dahanne <anthony.dahanne@gmail.com>

anthonydahanne force-pushed the issue964-install-wait-support branch from bd21ba2 to 60b2e09 Compare March 10, 2020 13:23

anthonydahanne marked this pull request as ready for review March 10, 2020 13:28

anthonydahanne requested review from alenkacz, kensipe, nfnt and zen-dog as code owners March 10, 2020 13:28

runyontr reviewed Mar 10, 2020

View reviewed changes

kensipe self-assigned this Mar 10, 2020

kensipe added this to the 0.12.0 milestone Mar 10, 2020

kensipe added the do-not-merge/hold label Mar 10, 2020

kensipe mentioned this pull request Mar 11, 2020

Install --wait #1418

Closed

kensipe approved these changes Mar 11, 2020

View reviewed changes

kensipe merged commit 451d5c4 into kudobuilder:master Mar 11, 2020

kensipe added this to Done in KUDO Global via automation Mar 11, 2020

This was referenced Apr 14, 2020

Waiting For a Plan to Finish #1461

Merged

kudo update --wait is racy #1465

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the --wait flag to install #966

Add the --wait flag to install #966

anthonydahanne commented Oct 18, 2019

anthonydahanne commented Oct 18, 2019

gerred Oct 18, 2019

gerred Oct 18, 2019

gerred commented Oct 24, 2019

kensipe commented Nov 20, 2019

anthonydahanne commented Nov 22, 2019

gerred commented Jan 6, 2020

kensipe commented Mar 2, 2020

anthonydahanne commented Mar 3, 2020

kensipe commented Mar 3, 2020

anthonydahanne commented Mar 7, 2020 •

edited

Loading

anthonydahanne commented Mar 7, 2020 •

edited

Loading

runyontr Mar 10, 2020

runyontr Mar 10, 2020

kensipe commented Mar 10, 2020

kensipe commented Mar 10, 2020

kensipe commented Mar 10, 2020

kensipe left a comment

		@@ -63,6 +64,26 @@ func InstallPackage(kc Client, resources packages.Resources, skipInstance bool
		if _, err := kc.InstallInstanceObjToCluster(resources.Instance, namespace); err != nil {

Add the --wait flag to install #966

Add the --wait flag to install #966

Conversation

anthonydahanne commented Oct 18, 2019

anthonydahanne commented Oct 18, 2019

gerred Oct 18, 2019

Choose a reason for hiding this comment

gerred Oct 18, 2019

Choose a reason for hiding this comment

gerred commented Oct 24, 2019

kensipe commented Nov 20, 2019

anthonydahanne commented Nov 22, 2019

gerred commented Jan 6, 2020

kensipe commented Mar 2, 2020

anthonydahanne commented Mar 3, 2020

kensipe commented Mar 3, 2020

anthonydahanne commented Mar 7, 2020 • edited Loading

anthonydahanne commented Mar 7, 2020 • edited Loading

runyontr Mar 10, 2020

Choose a reason for hiding this comment

runyontr Mar 10, 2020

Choose a reason for hiding this comment

kensipe commented Mar 10, 2020

kensipe commented Mar 10, 2020

kensipe commented Mar 10, 2020

kensipe left a comment

Choose a reason for hiding this comment

anthonydahanne commented Mar 7, 2020 •

edited

Loading

anthonydahanne commented Mar 7, 2020 •

edited

Loading