kubeadm: refactor upgrade plan and apply commands #73135

rojkov · 2019-01-21T13:10:00Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
The change

removes code duplication in NewCmdApply() and NewCmdPlan() by moving their common logic into enforceRequirements();
makes flags provided by a user read-only structures and drops SetImplicitFlags();
avoids state maintenance for non-interactiveness in flags.nonInteractiveMode, but rather calculates the mode from immutable variables.

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2019-01-21T13:10:07Z

Hi @rojkov. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2019-01-21T13:12:35Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rojkov
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: fabriziopandini

If they are not already assigned, you can assign the PR to them by writing /assign @fabriziopandini in a comment when ready.

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

The pull request process is described here

Needs approval from an approver in each of these files:

cmd/kubeadm/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kad · 2019-01-21T13:27:06Z

/ok-to-test

rojkov · 2019-01-21T15:22:42Z

/test pull-kubernetes-e2e-kops-aws

yagonobre

Thanks @rojkov
/priority important-longterm

rosti · 2019-01-23T10:42:10Z

cmd/kubeadm/app/cmd/upgrade/common.go

 // enforceRequirements verifies that it's okay to upgrade and then returns the variables needed for the rest of the procedure
-func enforceRequirements(flags *applyPlanFlags, dryRun bool, newK8sVersion string) (*upgradeVariables, error) {
+func enforceRequirements(flags *applyPlanFlags, args []string, dryRun bool, versionIsMandatory bool) (clientset.Interface, upgrade.VersionGetter, *kubeadmapi.InitConfiguration, error) {


I am kind of reluctant to let the args slice propagate so far down the call stack. In my opinion, we should handle them as early as the run funcs of apply and plan.
This can be done in a separate helper func and then we can propagate the results of it to enforceRequirements.

Yes, makes sense. I've moved args handling in a separate function now.

rosti · 2019-01-23T14:33:18Z

cmd/kubeadm/app/cmd/upgrade/apply.go

 		return err
 	}

 	// Use normalized version string in all following code.
-	flags.newK8sVersionStr = upgradeVars.cfg.KubernetesVersion
-	k8sVer, err := version.ParseSemantic(flags.newK8sVersionStr)
+	newK8sVersion, err := version.ParseSemantic(cfg.KubernetesVersion)


I think, that cfg.KubernetesVersion is going to have the old version here, since it's taken from the ClusterConfiguration, that is in a config map in the cluster.

@rosti could you please elaborate a bit why this change is worse than the existing code?
In the old code flags.newK8sVersionStr is set one line above from the same source (upgradeVars.cfg.KubernetesVersion).

Sorry about that, I must have been blind when I was looking at it. Totally missed the line above that.

rojkov · 2019-01-29T14:57:36Z

/test pull-kubernetes-kubemark-e2e-gce-big

rosti · 2019-01-30T16:52:21Z

@rojkov Can you squash most (if not all) of your commits here into a more general role?
This needs more reviews as it's size/L and I may have missed something.

/assign @fabriziopandini @neolit123
/cc @yagonobre

neolit123

thanks for the cleanup @rojkov

unlike other projects, the k8s project usually doesn't follow the multi-commit per PR.
and we usually squash commits in a PR into one and focus a PR on a single change.
(i've seen exceptions).

with that in mind this PR probably could have been broken in 2-3 scoped single commit PRs.
having said that, i think it's OK to get this PR out of the way.
please, just keep that in mind for the future.
thanks.

LGTM
/lgtm
ping @fabriziopandini for the final /approve

rojkov · 2019-01-31T08:36:51Z

@rosti @neolit123 Thank you for review!
Indeed I was going to squash the commits after review, so I did it now and rebased.

Just in case the split commits can be found in https://github.com/rojkov/kubernetes/tree/kubeadm-refactor-enforceRequirements-split.

rosti · 2019-01-31T09:39:11Z

cmd/kubeadm/app/cmd/upgrade/common.go

+	}
+
+	// Ensure the user is root
+	klog.V(1).Infof("running preflight checks")


Please, change Infof to Infoln here.

rosti · 2019-01-31T09:44:33Z

Actually, I spotted a few Infofs in plan.go and apply.go, that are not touched by this PR. They are out of the scope of it, but need to be fixed.
@rojkov given, that you work in this area, can we count on you for a followup PR fixing those? If you don't have the time I'll create a help wanted issue.

Rremove code duplication in NewCmdApply() and NewCmdPlan() by moving their common logic into enforceRequirements() and getK8sVersionFromUserInput(); Make flags provided by a user read-only structures and drop SetImplicitFlags(). Avoid state maintenance for non-interactiveness in flags.nonInteractiveMode, but rather calculate the mode from immutable variables. Squashed commits: kubeadm: unhide the logic for non-interactiveness kubeadm: use non-formatting constructor for new error kubeadm: unhide version non-emptiness check kubeadm: drop applyFlags.newK8sVersion field since it's not a command line flag. kubeadm: drop applyFlags.newK8sVersionStr field since it's not a command line flag. kubeadm: simplify creating etcdClient kubeadm: move duplicated code into enforceRequirements() kubeadm: drop planFlags type kubeadm: move getting version from user's input to a separate function

rojkov · 2019-01-31T10:27:05Z

@rosti Sure, you can count on me. K8s is my day job, though I'm focused on hardware enabling.

I'll submit a followup PR for the Infofs.

yagonobre

Thanks @rojkov
/lgtm

timothysc · 2019-02-05T14:38:59Z

cmd/kubeadm/app/cmd/upgrade/apply.go

 	if err != nil {
-		return errors.Errorf("unable to parse normalized version %q as a semantic version", flags.newK8sVersionStr)
+		return errors.Errorf("unable to parse normalized version %q as a semantic version", cfg.KubernetesVersion)


I'd have to double check that the precedence order is correct, here.

timothysc · 2019-02-05T14:40:44Z

This should definitely be more than 1 PR per refactoring. I'll defer to @fabriziopandini on approval.

fabriziopandini

@rojkov thanks for this PR and sorry again for the delay on the review!

At first sight all the changes you are proposing seems reasonable, but I agree with @timothysc (and @neolit123) that probably it is better to break down all the different refactors in multiple PRs.

Please consider that ATM the CI signal for upgrades is not fully reliable, and the complexity of upgrades unfortunately already bite us in the past. Let's move carefully in this area (we are working on improving CI signal, but this will take some time).

PS. I see you are more and more active in kubeadm; I hope we will see you/hear your opinion at the kubeadm office hours soon!

rojkov · 2019-02-08T10:00:31Z

Submitted #73844 as a first PR in a series of split commits comprising this bigger PR.

fabriziopandini · 2019-02-11T17:16:15Z

/hold
as per comment above

k8s-ci-robot · 2019-02-17T01:30:20Z

@rojkov: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rojkov · 2019-02-28T14:21:25Z

Closing as the content of this PR has been merged from separate PRs referenced above.

neolit123 · 2019-02-28T16:11:48Z

thank you for the separate PRs @rojkov

fabriziopandini · 2019-02-28T16:36:01Z

Well done @rojkov

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 21, 2019

rojkov changed the title ~~Kubeadm refactor version big enforce requirements~~ kubeadm: refactor upgrade plan and apply commands Jan 21, 2019

k8s-ci-robot requested review from dixudx and kad January 21, 2019 13:12

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 21, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 21, 2019

yagonobre reviewed Jan 21, 2019

View reviewed changes

k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 21, 2019

rosti reviewed Jan 23, 2019

View reviewed changes

rojkov force-pushed the kubeadm-refactor-version-big-enforceRequirements branch from 16ccf2d to 6034f8a Compare January 24, 2019 09:20

k8s-ci-robot assigned fabriziopandini and neolit123 Jan 30, 2019

k8s-ci-robot requested a review from yagonobre January 30, 2019 16:52

neolit123 approved these changes Jan 30, 2019

View reviewed changes

rojkov force-pushed the kubeadm-refactor-version-big-enforceRequirements branch from 6034f8a to 089dc59 Compare January 31, 2019 08:31

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2019

rosti reviewed Jan 31, 2019

View reviewed changes

rojkov force-pushed the kubeadm-refactor-version-big-enforceRequirements branch from 089dc59 to 9cf1868 Compare January 31, 2019 10:25

yagonobre reviewed Feb 5, 2019

View reviewed changes

k8s-ci-robot assigned yagonobre Feb 5, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2019

timothysc reviewed Feb 5, 2019

View reviewed changes

fabriziopandini suggested changes Feb 6, 2019

View reviewed changes

rojkov mentioned this pull request Feb 8, 2019

kubeadm: restructure upgradeVariables #73844

Merged

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 11, 2019

rojkov mentioned this pull request Feb 15, 2019

kubeadm: unhide the logic for non-interactiveness #74131

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2019

This was referenced Feb 18, 2019

kubeadm: unhide version non-emptiness check #74199

Closed

kubeadm: drop applyFlags.newK8sVersion field #74212

Merged

kubeadm: drop applyFlags.newK8sVersionStr field #74256

Merged

kubeadm: simplify creating etcdClient #74400

Merged

rojkov mentioned this pull request Feb 25, 2019

kubeadm: move duplicated code into enforceRequirements() #74511

Merged

rojkov closed this Feb 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: refactor upgrade plan and apply commands #73135

kubeadm: refactor upgrade plan and apply commands #73135

rojkov commented Jan 21, 2019

k8s-ci-robot commented Jan 21, 2019

k8s-ci-robot commented Jan 21, 2019

kad commented Jan 21, 2019

rojkov commented Jan 21, 2019

yagonobre left a comment

rosti Jan 23, 2019

rojkov Jan 24, 2019

rosti Jan 23, 2019

rojkov Jan 24, 2019

rosti Jan 29, 2019

rojkov commented Jan 29, 2019

rosti commented Jan 30, 2019

neolit123 left a comment •

edited

rojkov commented Jan 31, 2019

rosti Jan 31, 2019

rojkov Jan 31, 2019

rosti commented Jan 31, 2019

rojkov commented Jan 31, 2019

yagonobre left a comment

timothysc Feb 5, 2019

timothysc commented Feb 5, 2019

fabriziopandini left a comment

rojkov commented Feb 8, 2019

fabriziopandini commented Feb 11, 2019

k8s-ci-robot commented Feb 17, 2019

rojkov commented Feb 28, 2019

neolit123 commented Feb 28, 2019

fabriziopandini commented Feb 28, 2019

kubeadm: refactor upgrade plan and apply commands #73135

kubeadm: refactor upgrade plan and apply commands #73135

Conversation

rojkov commented Jan 21, 2019

k8s-ci-robot commented Jan 21, 2019

k8s-ci-robot commented Jan 21, 2019

kad commented Jan 21, 2019

rojkov commented Jan 21, 2019

yagonobre 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

rojkov commented Jan 29, 2019

rosti commented Jan 30, 2019

neolit123 left a comment • edited

Choose a reason for hiding this comment

rojkov commented Jan 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosti commented Jan 31, 2019

rojkov commented Jan 31, 2019

yagonobre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothysc commented Feb 5, 2019

fabriziopandini left a comment

Choose a reason for hiding this comment

rojkov commented Feb 8, 2019

fabriziopandini commented Feb 11, 2019

k8s-ci-robot commented Feb 17, 2019

rojkov commented Feb 28, 2019

neolit123 commented Feb 28, 2019

fabriziopandini commented Feb 28, 2019

neolit123 left a comment •

edited