kubeadm: move duplicated code into enforceRequirements() #74511

rojkov · 2019-02-25T09:01:09Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Move duplicated code into enforceRequirements() and to a new function getK8sVersionFromUserInput().

Also drop applyPlanFlags.ignorePreflightErrorsSet field which is not a command line option.

Special notes for your reviewer:
This is a part of the PR series splitting #73135.

XXX: This PR increases code coverage, but the new tests reveal some unexpected behaviour. The current logic of handling user input is that if a user specifies a ClusterConfiguration file without KubernetesVersion specified in it and no version is given as an argument then there's no failure. Because the function LoadInitConfigurationFromFile() returns a config with some default version already defined which is taken from the release server and is not known at test time.
Thus when a new release of K8s is finalized the tests will start to fail.

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2019-02-25T09:01:16Z

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.

bart0sh · 2019-02-25T09:04:08Z

/ok-to-test

bart0sh · 2019-02-25T10:28:06Z

/test pull-kubernetes-integration

rosti

Thanks @rojkov !
We need to remove the flaky tests though.

rosti · 2019-02-25T10:11:56Z

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

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


s/Infof/Info

rosti · 2019-02-25T10:18:47Z

cmd/kubeadm/app/cmd/upgrade/common_test.go

+			name:               "Valid config, but no version specified",
+			isVersionMandatory: true,
+			flags: &applyPlanFlags{
+				cfgPath: "testdata/cluster_config_with_version.yaml",


I am not a big a fan of using testdata files. In fact I was planning on nuking those at some point. Trouble is, that these occasionally get recycled by other tests, the actual test data is far apart from the test code and the test data itself does not mention what it's used to test.
I'd rather keep the content of the config in a const in this file (or even test case), write it out in a temp file and supply the temp file path to cfgPath.
But that's just me, I am fine if you choose to keep the testdata files.

Rewrote to create temp files with configs.

rosti · 2019-02-25T10:23:05Z

cmd/kubeadm/app/cmd/upgrade/common_test.go

+			// XXX: This is possibly wrong. The current logic is that if version is omitted
+			//      from both the config and arguments then take the latest version
+			//      from the release server. Which is unknown at test time.
+			expectedVersion: "v1.13.3",


This is a flaky test, it requires internet connection and changing every time we release a new version. Please, don't add this case.

I added this test case to illustrate a possible problem. I wasn't sure If it's expected that a user could use apply without providing any version and still get away with it.

Removed now.

rosti · 2019-02-25T10:45:31Z

cmd/kubeadm/app/cmd/upgrade/common_test.go

+			// XXX: This is possibly wrong. The current logic is that if version is omitted
+			//      from both the config and arguments then take the latest version
+			//      from the release server. Which is unknown at test time.
+			expectedVersion: "v1.13.3",


Again a flaky test case to be removed.

rosti · 2019-02-25T10:56:59Z

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

+			return "", err
+		}
+
+		if cfg.KubernetesVersion != "" {


The if statement does not make any difference if cfg.KubernetesVersion == "", because userVersion should be empty at that moment. Just leave userVersion = cfg.KubernetesVersion.

Thanks! Fixed.

yagonobre

Thanks @rojkov
/priority important-longterm

and to a new function getK8sVersionFromUserInput(). Also drop applyPlanFlags.ignorePreflightErrorsSet field which is not a command line option.

neolit123

/approve

thanks seems like a clean change.
leaving the LGTM to others.

we also need to start auditing 1.13 -> 1.14 upgrades...

k8s-ci-robot · 2019-02-27T19:37:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, rojkov

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~~ [neolit123]

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

bart0sh · 2019-02-28T09:06:26Z

/lgtm

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 25, 2019

k8s-ci-robot requested review from neolit123 and yagonobre February 25, 2019 09:02

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 Feb 25, 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 Feb 25, 2019

rojkov force-pushed the kubeadm-refactor-enforceRequirements branch from f780c2b to b11b7f7 Compare February 25, 2019 09:56

rosti suggested changes Feb 25, 2019

View reviewed changes

yagonobre reviewed Feb 25, 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 Feb 25, 2019

kubeadm: move duplicated code into enforceRequirements()

226843f

and to a new function getK8sVersionFromUserInput(). Also drop applyPlanFlags.ignorePreflightErrorsSet field which is not a command line option.

rojkov force-pushed the kubeadm-refactor-enforceRequirements branch from b11b7f7 to 226843f Compare February 25, 2019 14:28

neolit123 reviewed Feb 27, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 2019

k8s-ci-robot assigned bart0sh Feb 28, 2019

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

k8s-ci-robot merged commit 83fc13e into kubernetes:master Feb 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: move duplicated code into enforceRequirements() #74511

kubeadm: move duplicated code into enforceRequirements() #74511

rojkov commented Feb 25, 2019

k8s-ci-robot commented Feb 25, 2019

bart0sh commented Feb 25, 2019

bart0sh commented Feb 25, 2019

rosti left a comment

rosti Feb 25, 2019

rojkov Feb 25, 2019

rosti Feb 25, 2019

rojkov Feb 25, 2019

rosti Feb 25, 2019

rojkov Feb 25, 2019

rosti Feb 25, 2019

rosti Feb 25, 2019

rojkov Feb 25, 2019

yagonobre left a comment

neolit123 left a comment

k8s-ci-robot commented Feb 27, 2019

bart0sh commented Feb 28, 2019

kubeadm: move duplicated code into enforceRequirements() #74511

kubeadm: move duplicated code into enforceRequirements() #74511

Conversation

rojkov commented Feb 25, 2019

k8s-ci-robot commented Feb 25, 2019

bart0sh commented Feb 25, 2019

bart0sh commented Feb 25, 2019

rosti 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

yagonobre left a comment

Choose a reason for hiding this comment

neolit123 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 27, 2019

bart0sh commented Feb 28, 2019