kubeadm: FeatureGate MergeCLIArgumentsWithConfig is added for ignorePreflightErrors #119946

chendave · 2023-08-15T09:29:34Z

Turn on FeatureGate MergeCLIArgumentsWithConfig to keep the legacy way of management of ignorePreflightErrors, which means the value defined by the flag ignore-preflight-errors will be merged with the value ignorePreflightErrors defined in the config file.

Otherwise, the value defined by the flag will replace the value from the config file if set.

/kind feature

ref: #113583 (comment)

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kubeadm: Turn on FeatureGate `MergeCLIArgumentsWithConfig` to merge the config from flag and config file, otherwise, If the flag `--ignore-preflight-errors` is set from CLI, then the value from config file will be ignored.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2023-08-15T09:29:37Z

Please note that we're already in Test Freeze for the release-1.28 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.28.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Tue Aug 15 04:36:57 UTC 2023.

cmd/kubeadm/app/apis/kubeadm/validation/validation.go

chendave · 2023-08-16T03:12:31Z

/hold

adding a feature gate.

neolit123 · 2023-08-20T14:36:34Z

/hold

adding a feature gate.

@chendave note how we added a deprecated FG here:
kubernetes/kubeadm#2346

we can use the same pattern IMO

cmd/kubeadm/app/features/features.go

chendave · 2023-08-31T07:21:20Z

/unhold

chendave · 2023-09-05T09:38:10Z

I must tweak a little bit around those related testcases.

@neolit123 @SataQiu @pacoxu pls review if you have time. I think all testcases should pass now.

neolit123

thanks for the update @chendave
this definitely needs more eyes.

the FG name and mechanism SGMT.
added a couple of questions related to side changes around join_*.

cmd/kubeadm/app/apis/kubeadm/validation/validation.go

neolit123 · 2023-09-05T10:06:36Z

cmd/kubeadm/app/cmd/join.go

+	initCfg := &kubeadmapi.InitConfiguration{}
+	// If the cfg is not valid config, e.g. this might be a fake config defined in testcase, then lazily fetch `InitConfiguration` until it's really used.
+	if tlsBootstrapCfg == nil {
+		if tlsBootstrapCfg, err = discovery.For(cfg); err == nil {
+			if initCfg, err = fetchInitConfigurationFromJoinConfiguration(cfg, tlsBootstrapCfg); err != nil {
+				return nil, err
+			}
+		}
+	}
+


hm, wouldn't this result in the init config being fetched always?
how do we know if this is a test config?

also why not add this below:

if _, err := os.Stat(adminKubeConfigPath); err == nil && opt.controlPlane { // use the admin.conf as tlsBootstrapCfg, that is the kubeconfig file used for reading the kubeadm-config during discovery klog.V(1).Infof("[preflight] found %s. Use it for skipping discovery", adminKubeConfigPath) tlsBootstrapCfg, err = clientcmd.LoadFromFile(adminKubeConfigPath) if err != nil { return nil, errors.Wrapf(err, "Error loading %s", adminKubeConfigPath) } }

hm, wouldn't this result in the init config being fetched always?

we cannot get the FG defined from a init config if there is no such config.

how do we know if this is a test config?

if this is not a test config and an error is hit, then initCfg would be an empty config, then initCfg could be fetched again when it is needed.

also why not add this below:

do you mean fetching the config only it's a control plane && admin kubeconfig exist? sorry, I cannot get it, is there any benefit for us to move those code up?

hm, wouldn't this result in the init config being fetched always?

we cannot get the FG defined from a init config if there is no such config.

this is pain, is there anyway for us to workaround this?

do you mean fetching the config only it's a control plane && admin kubeconfig exist? sorry, I cannot get it, is there any benefit for us to move those code up?

no just putting the login closer to the block i posted here
#119946 (comment)

core organization wise

i don't think we are understanding each other very well here.
the above logic and comment in the code confused me, but i think you might want to ignore my question for now, because i'm doing N things at once and don't have enough time to have a look at this PR more today.

if you think it's the right thing to do, leave it like so and other reviewers will also have a look.

@neolit123 take your time, this is not urgent I think, thanks!

hm, wouldn't this result in the init config being fetched always?

this should be, the reason is we need to get the initCfg from cluster, so that we can get the FeatureGates from the initCfg.

here is the code:

ignorePreflightErrorsSet, err := validation.ValidateIgnorePreflightErrors(initCfg.FeatureGates, opt.ignorePreflightErrors, cfg.NodeRegistration.IgnorePreflightErrors)

Because of that, I add a parameter lazilyFetchInitCfg here to keep the original testcase unimpacted, the testcases there expected to lazily fetch a initConfiguration, I think that is because testcase should not interact with the cluster.

no just putting the login closer to the block i posted here

hmm, looks like I cannot move this piece of code up, because the code must under the cfg, I need the cfg to get the cfg.NodeRegistration.IgnorePreflightErrors as well.

cfg, err := configutil.LoadOrDefaultJoinConfiguration(opt.cfgPath, opt.externalcfg)

neolit123 · 2023-09-05T10:11:32Z

cmd/kubeadm/app/cmd/join_test.go

+			// set the controlPlane to true to bypass the discovery of a tlsBootstrapCfg from the JoinConfiguration, this could be opt-out by the flag defined in the testcases.
+			joinOptions.controlPlane = true


can you please elaborate on why this controlPlane field bypasses the discovery of a tlsBootstrapCfg? is it because the tlsBootstrapCfg local var will be nil if there is no admin.conf?
i got a bit lost on this one. maybe more details in the comment are needed.

overall, we want to bypass the logic to get the tlsBootstrapCfg from a cluster, unittest shouldn't do that (interact with a cluster to fetch something from a cluster).

https://github.com/kubernetes/kubernetes/pull/119946/files#diff-c6f6a594b0b45b48a444c274731731e10883c6cf9e2e66f8938aa28af5a57b27R436-R437

if tlsBootstrapCfg == nil { if tlsBootstrapCfg, err = discovery.For(cfg); err == nil {

set the controlplane will help to set tlsBootstrapCfg and hence skip this step.

Since I added a new parameter lazilyFetchInitCfg in the function of newJoinData, so those change are not needed anymore.

neolit123

/approve
/hold
/triage accepted

generally LGTM. needs another reviewer!

please double check the logic around:
https://github.com/kubernetes/kubernetes/pull/119946/files#r1315679568

k8s-ci-robot · 2023-10-12T14:35:03Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

chendave · 2023-10-12T14:49:58Z

@SataQiu @pacoxu Does this approach look good to you?

chendave · 2023-10-16T06:31:40Z

cmd/kubeadm/app/features/features.go

@@ -38,13 +38,19 @@ const (
 	EtcdLearnerMode = "EtcdLearnerMode"
 	// UpgradeAddonsBeforeControlPlane is expected to be in deprecated in v1.28 and will be removed in future release
 	UpgradeAddonsBeforeControlPlane = "UpgradeAddonsBeforeControlPlane"
+	// MergeCLIArgumentsWithConfig in deprecated in v1.29 and is expected to be removed in 1.30


…reflightErrors Turn on FeatureGate MergeCLIArgumentsWithConfig to keep the legacy way of management of ignorePreflightErrors, which means the value defined by the flag `ignore-preflight-errors` will be merged with the value `ignorePreflightErrors` defined in the config file. Otherwise, the value defined by the flag will replace the value from the config file if set. Signed-off-by: Dave Chen <dave.chen@arm.com>

pacoxu · 2023-10-16T06:42:07Z

/lgtm

k8s-ci-robot · 2023-10-16T06:42:14Z

LGTM label has been added.

Git tree hash: dc12011e34940d0cffc7ff92e2512eba2d97cc38

chendave · 2023-10-16T11:15:50Z

/unhold

thanks guys for the review!

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. labels Aug 15, 2023

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 15, 2023

k8s-ci-robot requested review from neolit123 and SataQiu August 15, 2023 09:29

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 15, 2023

SataQiu reviewed Aug 15, 2023

View reviewed changes

cmd/kubeadm/app/apis/kubeadm/validation/validation.go Show resolved Hide resolved

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

chendave force-pushed the ignore_preflight_error branch from c231254 to 6cbb7a3 Compare August 31, 2023 07:08

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 31, 2023

chendave changed the title ~~kubeadm: ignore-preflight-errors flag will replace the value from config file~~ ubeadm: FeatureGate MergeCLIArgumentsWithConfig is added for ignorePreflightErrors Aug 31, 2023

chendave changed the title ~~ubeadm: FeatureGate MergeCLIArgumentsWithConfig is added for ignorePreflightErrors~~ kubeadm: FeatureGate MergeCLIArgumentsWithConfig is added for ignorePreflightErrors Aug 31, 2023

chendave force-pushed the ignore_preflight_error branch from 6cbb7a3 to e64fa10 Compare August 31, 2023 07:18

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2023

chendave commented Aug 31, 2023

View reviewed changes

cmd/kubeadm/app/features/features.go Outdated Show resolved Hide resolved

chendave force-pushed the ignore_preflight_error branch from e64fa10 to 167fadb Compare August 31, 2023 07:20

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 31, 2023

chendave force-pushed the ignore_preflight_error branch 3 times, most recently from e3b9a4e to c6932e8 Compare August 31, 2023 08:59

neolit123 reviewed Sep 5, 2023

View reviewed changes

chendave force-pushed the ignore_preflight_error branch 2 times, most recently from 0d046d1 to 9a8b373 Compare September 5, 2023 11:48

neolit123 reviewed Sep 7, 2023

View reviewed changes

chendave force-pushed the ignore_preflight_error branch from 9a8b373 to f6ac3c9 Compare October 12, 2023 14:34

chendave commented Oct 16, 2023

View reviewed changes

chendave force-pushed the ignore_preflight_error branch from f6ac3c9 to 8009530 Compare October 16, 2023 06:37

k8s-ci-robot assigned pacoxu Oct 16, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2023

chendave mentioned this pull request Oct 16, 2023

Introduce of the deprecated FG: MergeCLIArgumentsWithConfig kubernetes/website#43510

Closed

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 16, 2023

k8s-ci-robot merged commit 3548ef0 into kubernetes:master Oct 16, 2023
15 checks passed

k8s-ci-robot added this to the v1.29 milestone Oct 16, 2023

chendave deleted the ignore_preflight_error branch October 17, 2023 01:48

chendave mentioned this pull request Oct 17, 2023

Introduce of the deprecated FG: MergeCLIArgumentsWithConfig kubernetes/website#43515

Merged

neolit123 mentioned this pull request Oct 17, 2023

[Failing Test] failing kinder jobs at latest that call kubeadm reset kubernetes/kubeadm#2943

Closed

chendave mentioned this pull request Oct 17, 2023

kubeadm: add new testcase to check the initConfiguration on worker nodes #121281

Closed

pacoxu mentioned this pull request Oct 17, 2023

Revert "kubeadm: FeatureGate MergeCLIArgumentsWithConfig is added for ignorePreflightErrors" #121290

Merged

chendave mentioned this pull request Oct 17, 2023

Revert "Introduce of the deprecated FG: MergeCLIArgumentsWithConfig" kubernetes/website#43530

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: FeatureGate MergeCLIArgumentsWithConfig is added for ignorePreflightErrors #119946

kubeadm: FeatureGate MergeCLIArgumentsWithConfig is added for ignorePreflightErrors #119946

chendave commented Aug 15, 2023 •

edited

k8s-ci-robot commented Aug 15, 2023

chendave commented Aug 16, 2023

neolit123 commented Aug 20, 2023

chendave commented Aug 31, 2023

chendave commented Sep 5, 2023

neolit123 left a comment

neolit123 Sep 5, 2023

chendave Sep 5, 2023

chendave Sep 5, 2023

neolit123 Sep 5, 2023

neolit123 Sep 5, 2023

chendave Sep 5, 2023 •

edited

chendave Oct 12, 2023

neolit123 Sep 5, 2023

chendave Sep 5, 2023

chendave Oct 12, 2023

neolit123 left a comment

k8s-ci-robot commented Oct 12, 2023

chendave commented Oct 12, 2023

chendave Oct 16, 2023

pacoxu commented Oct 16, 2023

k8s-ci-robot commented Oct 16, 2023

chendave commented Oct 16, 2023

		// set the controlPlane to true to bypass the discovery of a tlsBootstrapCfg from the JoinConfiguration, this could be opt-out by the flag defined in the testcases.
		joinOptions.controlPlane = true

kubeadm: FeatureGate MergeCLIArgumentsWithConfig is added for ignorePreflightErrors #119946

kubeadm: FeatureGate MergeCLIArgumentsWithConfig is added for ignorePreflightErrors #119946

Conversation

chendave commented Aug 15, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Aug 15, 2023

chendave commented Aug 16, 2023

neolit123 commented Aug 20, 2023

chendave commented Aug 31, 2023

chendave commented Sep 5, 2023

neolit123 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

chendave Sep 5, 2023 • edited

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

neolit123 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 12, 2023

chendave commented Oct 12, 2023

Choose a reason for hiding this comment

pacoxu commented Oct 16, 2023

k8s-ci-robot commented Oct 16, 2023

chendave commented Oct 16, 2023

chendave commented Aug 15, 2023 •

edited

chendave Sep 5, 2023 •

edited