Interactive cli opt #4166

mrballcb · 2017-12-28T23:38:30Z

Sometimes when doing a kops rolling-update, I might want to ssh to a new master or new node to check that some component is configured properly, has started, or just in general look around to make sure it's behaving properly before it updates every instance in the cluster. Especially if I know a breaking change is being deployed, I'd like to have kops wait after performing the update on each host, not on a timer, but wait until I specifically allow it to continue.

This PR adds a kops cli option, --interactive, that pauses after each instance gets updated. It prints out a prompt:

Continue? (Y)es, (N)o, (A)lwaysYes: [Y]

...and waits for admin approval before continuing on to the next instance. Pressing "y" or "Y" will continue. The default is "Y"es, so pressing Enter will also continue. Pressing "n" or "N" could maybe be enhanced to print out some useful information (summary, where you stopped, etc) but for now it merely exits kops immediately with exitcode 3 so that it does not deploy any more rolling updates. Pressing "a" or "A" clears the Interactive flag and it will no longer prompt after updating each instance going forward.

Just builds, haven't tested yet.

mrballcb · 2017-12-28T23:39:38Z

There are a few things I don’t like and I feel warrant discussion:

Adding “strings” and “bufio” imports to the pkg/instancegroups/instancegroups.go file.
Output looks so drastically different from the surrounding glog.* functions, it’s kind of jarring.
I feel like it should be in a shared util module instead of directly in the file. (But I don’t see it being usable anywhere else, so what would be the gain?)
Implementation of “AlwaysYes” feels hackish, and pending my understanding of pointers as implemented in go, maybe downright wrong or unsafe.

mrballcb · 2017-12-29T01:01:49Z

/assign @chrislovecnm

chrislovecnm · 2018-01-05T18:42:04Z

/ok-to-test

chrislovecnm

Lots of comments for yah, and I know you are new to go, so I hope I provided enough detail. Ping me on slack if you have more questions or on #kops-dev

chrislovecnm · 2018-01-05T18:42:42Z

pkg/instancegroups/instancegroups.go

+	stop_prompting = false
+	reader := bufio.NewReader(os.Stdin)
+	glog.Infof("Pausing after finished %q", upgradedHost)
+	fmt.Printf("Continue? (Y)es, (N)o, (A)lwaysYes: [Y] ")


so should we just fmt.Printf? What looks better UX wise?

chrislovecnm · 2018-01-05T18:42:58Z

pkg/instancegroups/instancegroups.go

+	val = strings.ToLower(val)
+	switch val {
+	case "y", "", "\n":
+		glog.V(4).Infof("Continuing with next host (response %q)\n", val)


fmt.Printf?

I don't quite understand this comment. I was attempting to make a log message that only shows up for higher verbosity levels. Are you saying that you'd rather no output for this?

Now I'm wondering if I should just omit the test for "y" altogether. "n" means stop, "a" means no more prompts, anything else should be treated as "y". If you won't want any glog output, I can just omit that whole case "y" section and it should behave the same.

I ended up just completely getting rid of the case "y" section. Only [nN] or [aA] make something different happen (stop and no more prompting, respectively). Anything else is a default "y".

chrislovecnm · 2018-01-05T18:44:10Z

pkg/instancegroups/instancegroups.go

@@ -65,6 +67,32 @@ func NewRollingUpdateInstanceGroup(cloud fi.Cloud, cloudGroup *cloudinstances.Cl
 	}, nil
 }

+// User input routine copied from vendor/google.golang.org/api/examples/gmail.go
+func PromptInteractive(upgradedHost string) (stop_prompting bool) {


GoLang likes stopPrompting instead of stop_prompting :)

Also promptInteractive instead, no need to make it public, and https://blog.golang.org/godoc-documenting-go-code

The tldr; on the documentation is.

start with the func name. promptInteractive

write a sentence and end it with a period.

How about func promptInteractive(upgradedHost string) (stopPrompting bool, err error)? Then you would be able to return the error up the stack.

chrislovecnm · 2018-01-05T18:45:05Z

pkg/instancegroups/instancegroups.go

+	fmt.Printf("Continue? (Y)es, (N)o, (A)lwaysYes: [Y] ")
+	val := ""
+	var err error
+	if val, err = reader.ReadString('\n'); err != nil {


Can we use the endline option in golang that is platform independent? Eventually windows yay.

Switched from using bufio Reader to bufio Scanner.

chrislovecnm · 2018-01-05T18:47:21Z

pkg/instancegroups/instancegroups.go

@@ -65,6 +67,32 @@ func NewRollingUpdateInstanceGroup(cloud fi.Cloud, cloudGroup *cloudinstances.Cl
 	}, nil
 }

+// User input routine copied from vendor/google.golang.org/api/examples/gmail.go
+func PromptInteractive(upgradedHost string) (stop_prompting bool) {
+	stop_prompting = false


camel case please

chrislovecnm · 2018-01-05T18:48:02Z

pkg/instancegroups/instancegroups.go

+	val := ""
+	var err error
+	if val, err = reader.ReadString('\n'); err != nil {
+		glog.Fatalf("unable to interpret input: %v", err)


so instead of glog.Fatal please log and return an err. I think you are eating the error a bit here.

chrislovecnm · 2018-01-05T18:50:10Z

pkg/instancegroups/instancegroups.go

@@ -169,6 +197,13 @@ func (r *RollingUpdateInstanceGroup) RollingUpdate(rollingUpdateData *RollingUpd

 				glog.Warningf("Cluster validation failed after removing instance, proceeding since fail-on-validate is set to false: %v", err)
 			}
+			if rollingUpdateData.Interactive {
+				var stop_prompting bool = PromptInteractive(nodeName)


you should be able to do

stopPrompting, err := PromptInteractive(nodeName)

Test if err is nil, and if not return it up the stack.

Implemented in this manner.

chrislovecnm · 2018-01-05T18:51:46Z

Thanks for the PR!! @mrballcb lot of tweaks for yah to make it more Go and less Perl :) I know you mentioned Go looks a lot like Perl when we chatted.

explodes · 2018-01-19T04:06:52Z

pkg/instancegroups/instancegroups.go

+	stop_prompting = false
+	reader := bufio.NewReader(os.Stdin)
+	glog.Infof("Pausing after finished %q", upgradedHost)
+	fmt.Printf("Continue? (Y)es, (N)o, (A)lwaysYes: [Y] ")


fmt.Printf without any parameters here. fmt.Print will print without a newline

mrballcb · 2018-01-19T06:27:05Z

I took the current set of patches and applied it to the release-1.8 branch. I built and tested kops with these patches against a test cluster, and put the output of these into gists so you can see what the output looks like. This is normal verbosity level.

Answered [A]lwaysYes:
https://gist.github.com/mrballcb/c6ae28c7ef4754c36da6bd81055a8550

Answered default:
https://gist.github.com/mrballcb/7899624679cc45c16e6d396ece9e688e

Answerd [N]o:
https://gist.github.com/mrballcb/6733184bb4d5aebd8841e746f00a0030

chrislovecnm · 2018-01-26T18:54:47Z

/lgtm

k8s-ci-robot · 2018-01-26T18:54:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm, mrballcb

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [chrislovecnm]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

Todd Lyons added 3 commits December 28, 2017 10:54

Interactive cli arg framework

40eed60

Just builds, haven't tested yet.

Remove useless line

2f0d888

User input to continue/abort rolling update

73b29b6

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 28, 2017

Todd Lyons added 2 commits December 28, 2017 15:42

Lint fixes by make gofmt

7f7306d

Doc addition from make gen-cli-docs

c3c7f31

k8s-ci-robot assigned chrislovecnm Dec 29, 2017

mrballcb mentioned this pull request Dec 29, 2017

Add rolling-update interactive cli option #4164

Closed

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 5, 2018

chrislovecnm suggested changes Jan 5, 2018

View reviewed changes

explodes reviewed Jan 19, 2018

View reviewed changes

Maintainer recommended code/style updates

5c1b646

chrislovecnm approved these changes Jan 26, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 26, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 26, 2018

k8s-ci-robot merged commit 923118e into kubernetes:master Jan 26, 2018

Mikulas mentioned this pull request Aug 16, 2018

Fix interactive rolling update silently ignored #5642

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interactive cli opt #4166

Interactive cli opt #4166

mrballcb commented Dec 28, 2017

mrballcb commented Dec 28, 2017 •

edited

Loading

mrballcb commented Dec 29, 2017

chrislovecnm commented Jan 5, 2018

chrislovecnm left a comment

chrislovecnm Jan 5, 2018

chrislovecnm Jan 5, 2018

mrballcb Jan 19, 2018

mrballcb Jan 19, 2018

chrislovecnm Jan 5, 2018

chrislovecnm Jan 5, 2018

chrislovecnm Jan 5, 2018

chrislovecnm Jan 5, 2018

mrballcb Jan 19, 2018

chrislovecnm Jan 5, 2018

chrislovecnm Jan 5, 2018

chrislovecnm Jan 5, 2018

mrballcb Jan 19, 2018

chrislovecnm commented Jan 5, 2018

explodes Jan 19, 2018

mrballcb commented Jan 19, 2018

chrislovecnm commented Jan 26, 2018

k8s-ci-robot commented Jan 26, 2018

Interactive cli opt #4166

Interactive cli opt #4166

Conversation

mrballcb commented Dec 28, 2017

mrballcb commented Dec 28, 2017 • edited Loading

mrballcb commented Dec 29, 2017

chrislovecnm commented Jan 5, 2018

chrislovecnm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrislovecnm commented Jan 5, 2018

Choose a reason for hiding this comment

mrballcb commented Jan 19, 2018

chrislovecnm commented Jan 26, 2018

k8s-ci-robot commented Jan 26, 2018

mrballcb commented Dec 28, 2017 •

edited

Loading