Introduce multi-stage upgrade by siggy · Pull Request #2723 · linkerd/linkerd2

siggy · 2019-04-19T20:35:24Z

linkerd install supports a 2-stage install process, linkerd upgrade
did not.

Add 2-stage support for linkerd upgrade. Also exercise multi-stage
functionality during upgrade integration tests.

Part of #2337

Signed-off-by: Andrew Seigner [email protected]

Depends on #2719

l5d-bot · 2019-04-19T21:00:14Z

Integration test results for b3eaf61: fail 😕
Log output: https://gist.github.com/6c564f077015749a076428fac6314900

alpeb

On a first pass, this is working as expected.

One small issue is that --from-manifests is being allowed in the first stage, because of it being separate from the main flagset.

I wonder how we can let users know when the need for linkerd upgrade config is needed, which would only be occasionally. First thing that occurs to me is hard-coding some boolean const alongside version.Version indicating that. Then linkerd upgrade control-plane would just issue a warning in stderr about it.

l5d-bot · 2019-04-23T22:43:54Z

Integration test results for e364a16: fail 😕
Log output: https://gist.github.com/ef4812ddaf0e7172a0487ca3300a39c5

l5d-bot · 2019-04-23T23:45:17Z

Integration test results for 79ea456: fail 😕
Log output: https://gist.github.com/c3dc980f8e0e299abe9afb001a92e20e

l5d-bot · 2019-04-24T02:49:21Z

Integration test results for 59f7f65: success 🎉
Log output: https://gist.github.com/63b77b487a735c25e3c451db5655e207

l5d-bot · 2019-04-24T23:55:16Z

Integration test results for 37f234e: fail 😕
Log output: https://gist.github.com/4f4fce69616d1a85b83624644ff616eb

dadjeibaah

This change looks good, had one clarifying question below.

dadjeibaah · 2019-04-24T23:51:00Z

cli/cmd/upgrade.go

+			if err != nil {
+				return err
+			}
+


Just noticed this now, when trying to test --from-manifests after @alpeb's comment. It seems, on line 79 of this file, that we are using a testing artifact to create a Kubernetes clientset. Is this intended? Wasn't clear on how its being used.

Good question! (Also I forgot about @alpeb's original question re: allowing linkerd config --from-manifests, have pushed a new commit to prevent that.)

Re: k8s.NewFakeClientSetsFromManifests on line 79, this is intended. Adding --from-manifests required reading k8s yaml from stdin (or files), and processing them in lieu of k8s resources returned from the k8s api. We already had some test code doing this to mock out k8s clients. The change in #2697 moved that test code into a new fake.go file, to support --from-manifests. Leveraging fake.go allows most of the upgrade code to be agnostic of where the k8s resources came from, since it's just using a k8s client.

l5d-bot · 2019-04-25T00:25:03Z

Integration test results for dd404b9: fail 😕
Log output: https://gist.github.com/4f8fce394536f8cf2e1b7b2c0d730637

ihcsim

Code change LGTM. Just a minor comment.

I will do some more testing on this branch.

Also, given what we saw today on how the CRD change affected the integration tests, I guess there is a chance that after running linkerd upgrade config, the control plane is left in limbo if the controller and other components aren't restarted, right?

ihcsim · 2019-04-25T03:07:08Z

cli/cmd/upgrade.go

 }

-func (options *upgradeOptions) validateAndBuild(k kubernetes.Interface, flags *pflag.FlagSet) (*installValues, *pb.All, error) {
+func (options *upgradeOptions) validateAndBuild(stage string, k kubernetes.Interface, flags *pflag.FlagSet) (*installValues, *pb.All, error) {


I don't think stage is needed in this function; should be able to assign it to values after the call to validateAndBuild().

I considered this initially as it would have minimized the change and preserved validateAndBuild's signature. I opted to set stage in this function keep all the code that creates an installValues in one place. The alternative is that validateAndBuild creates a nearly-complete installValues that the caller must know to set stage after the fact.

alpeb · 2019-04-25T12:57:27Z

I verified --from-manifests is now only available in the second stage 👍

there is a chance that after running linkerd upgrade config, the control plane is left in limbo if the controller and other components aren't restarted

Probably an extra message in stderr alongside the existing "You're on your way to upgrading Linkerd!" could do the trick, like "Don't forget to run linkerd upgrade control-plane!" (in bold if possible?)

I wonder how we can let users know when the need for linkerd upgrade config is needed, which would only be occasionally. First thing that occurs to me is hard-coding some boolean const alongside version.Version indicating that. Then linkerd upgrade control-plane would just issue a warning in stderr about it.

A different take to ensure the second stage is ran after the first would be to add an annotation into linkerd's namespace with the version string. The second stage would retrieve the namespace and ensure it has the proper version. This could also apply to linkerd install, through a refactor of exitIfNamespaceDoesNotExist(). This also means both stages would always need to be run. An extra linkerd check could be added to ensure the namespace's version would match the control plane's.

ihcsim · 2019-04-25T17:36:29Z

Probably an extra message in stderr alongside the existing "You're on your way to upgrading Linkerd!" could do the trick, like "Don't forget to run linkerd upgrade control-plane!" (in bold if possible?)

Yeah, the scenario I was thinking about is hey, your control plane is broken, until you run linkerd upgrade control-plane and restart the controller

siggy · 2019-04-25T20:22:05Z

I agree there's more we could do here around version checking during an upgrade. Some of that will happen via multi-stage support in linkerd check, and I think most of it will happen as we consider versioning as part of #2671.

ihcsim

👍 Works on my machine.

`linkerd install` supports a 2-stage install process, `linkerd upgrade` did not. Add 2-stage support for `linkerd upgrade`. Also exercise multi-stage functionality during upgrade integration tests. Part of #2337 Signed-off-by: Andrew Seigner <[email protected]>

Signed-off-by: Andrew Seigner <[email protected]>

alpeb

This looks good to me 👍
I also verified the integrations tests are green, since it seems l5d-bot decided to take a break 🤖 🔥

l5d-bot · 2019-04-25T21:29:21Z

Integration test results for 7602ab8: success 🎉
Log output: https://gist.github.com/cf31fe038ee03b1bea2176f0219c6158

siggy added area/cli area/install labels Apr 19, 2019

siggy requested review from alpeb and ihcsim April 19, 2019 20:35

siggy self-assigned this Apr 19, 2019

alpeb mentioned this pull request Apr 19, 2019

New linkerd inject default and advanced modes #2721

Merged

alpeb reviewed Apr 22, 2019

View reviewed changes

siggy force-pushed the siggy/install-stages branch from e2fc912 to 3e7cf8d Compare April 23, 2019 17:49

siggy force-pushed the siggy/upgrade-stages branch from b3eaf61 to e364a16 Compare April 23, 2019 22:38

siggy changed the base branch from siggy/install-stages to master April 23, 2019 22:38

siggy force-pushed the siggy/upgrade-stages branch from e364a16 to 79ea456 Compare April 23, 2019 23:06

siggy force-pushed the siggy/upgrade-stages branch from 79ea456 to 59f7f65 Compare April 24, 2019 02:32

grampelberg mentioned this pull request Apr 24, 2019

Focus install by role and stage #2337

Closed

11 tasks

siggy force-pushed the siggy/upgrade-stages branch 2 times, most recently from f3d48ff to 37f234e Compare April 24, 2019 23:14

dadjeibaah reviewed Apr 24, 2019

View reviewed changes

ihcsim reviewed Apr 25, 2019

View reviewed changes

ihcsim approved these changes Apr 25, 2019

View reviewed changes

siggy added 3 commits April 25, 2019 13:59

review feedback: no manifests for config stage

fab0754

Signed-off-by: Andrew Seigner <[email protected]>

allow linkerd install config --ignore-cluster

a4a0b00

Signed-off-by: Andrew Seigner <[email protected]>

print upgrade control-plane message

7602ab8

Signed-off-by: Andrew Seigner <[email protected]>

siggy force-pushed the siggy/upgrade-stages branch from 5daa740 to 7602ab8 Compare April 25, 2019 21:00

alpeb approved these changes Apr 25, 2019

View reviewed changes

siggy merged commit 15ffd86 into master Apr 25, 2019

siggy deleted the siggy/upgrade-stages branch April 25, 2019 21:29

Conversation

siggy commented Apr 19, 2019

Uh oh!

l5d-bot commented Apr 19, 2019

Uh oh!

alpeb left a comment

Choose a reason for hiding this comment

Uh oh!

l5d-bot commented Apr 23, 2019

Uh oh!

l5d-bot commented Apr 23, 2019

Uh oh!

l5d-bot commented Apr 24, 2019

Uh oh!

l5d-bot commented Apr 24, 2019

Uh oh!

dadjeibaah left a comment

Choose a reason for hiding this comment

Uh oh!

dadjeibaah Apr 24, 2019

Choose a reason for hiding this comment

Uh oh!

siggy Apr 25, 2019

Choose a reason for hiding this comment

Uh oh!

l5d-bot commented Apr 25, 2019

Uh oh!

ihcsim left a comment

Choose a reason for hiding this comment

Uh oh!

ihcsim Apr 25, 2019

Choose a reason for hiding this comment

Uh oh!

siggy Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alpeb commented Apr 25, 2019

Uh oh!

ihcsim commented Apr 25, 2019

Uh oh!

siggy commented Apr 25, 2019

Uh oh!

ihcsim left a comment

Choose a reason for hiding this comment

Uh oh!

alpeb left a comment

Choose a reason for hiding this comment

Uh oh!

l5d-bot commented Apr 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

siggy Apr 25, 2019 •

edited

Loading