kubeadm: Remove the support of configurable component configs #120788

chendave · 2023-09-21T05:28:54Z

kubeadm upgrade plan uses to support the configure of component configs(kubeproxy and kubelet) in a config file and then check if the version is supported or not, if it's not supported it will be marked as a unsupported version and require to manually upgrade the component.

This feature will make the upgrade config API much harder as this violates the non-mutation principle for upgrade, and we have seen it's quite problematic to do like this.

This change removes the support of configurable component configs for kubeadm upgrade plan, along with the removal, the logic to parse the config file to decide whether a manual upgrade for the component configs is needed is removed as well.

NOTE that API is not changed, i.e. ManualUpgradeRequired is not removed from ComponentConfigVersionState but it's no-op now.

split from: #114062

more context:
#114062 (comment)
#114062 (comment)

This is one step forward upgrade config API.

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Related: #114062

Special notes for your reviewer:

Does this PR introduce a user-facing change?

ACTION REQUIRED: stop accepting component configuration for kube-proxy and kubelet during `kubeadm upgrade plan --config`. This is a legacy behavior that is not well supported for upgrades and can be used only at the plan stage to determine if the configuration for these components stored in the cluster needs manual version  migration. In the future, kubeadm will attempt alternative component config migration approaches.

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

`kubeadm upgrade plan` uses to support the configure of component configs(kubeproxy and kubelet) in a config file and then check if the version is supported or not, if it's not supported it will be marked as a unsupported version and require to manually upgrade the component. This feature will make the upgrade config API much harder as this violates the no-mutation principle for upgrade, and we have seen it's quite problematic to do like this. This change removes the support of configurable component configs for `kubeadm upgrade plan`, along with the removal, the logic to parse the config file to decide whether a manual upgrade for the component configs is needed is removed as well. NOTE that API is not changed, i.e. `ManualUpgradeRequired` is not removed from `ComponentConfigVersionState` but it's no-op now. Signed-off-by: Dave Chen <[email protected]>

k8s-ci-robot · 2023-09-21T05:29:03Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

chendave · 2023-09-21T05:35:56Z

this change is marked as ACTION REQUIRED, even though I am not sure what action a end-user should take.

If end-user provide a config file with component configs, then it will be just ignored.

How about print some warning here, saying, this is no longer supported and remove the ACTION REQUIRED from release note?

neolit123

we should add action required on breaking changes or changes in user expectations.

in this case if they were using the --config the component config would have been updated in the cluster, right? we seem to be changing this behavior, so they must do it manullay now before upgrade.

ideally this should have been a v1beta4 change only. but it's likely complicated to do.

i think we should start erroring on Kind that is not upgradeconfiguration passed on upgrade.

chendave · 2023-09-21T06:09:00Z

the component config would have been updated in the cluster

IIUC, the only thing I can see is if the version is different from current value (defined kubelet = v1beta1, kubeproxy = v1alpha1), then asking for manually upgrade, other than that, I didn't see anything different.

it's in the phase of plan so no real action would be taken.

chendave · 2023-09-21T06:10:28Z

i think we should start erroring on Kind that is not upgradeconfiguration passed on upgrade.

+1, let me consolidate this logic into #114062

neolit123 · 2023-09-21T06:43:41Z

the component config would have been updated in the cluster

IIUC, the only thing I can see is if the version is different from current value (defined kubelet = v1beta1, kubeproxy = v1alpha1), then asking for manually upgrade, other than that, I didn't see anything different.

it's in the phase of plan so no real action would be taken.

that may be true. i have forgotten what logic was added.

neolit123 · 2023-09-21T06:54:54Z

ACTION REQUIRED: kubeadm will not support the configure of component configs for kubeadm upgrade plan, it will be just ignored if configured.

suggesting:

kubeadm: stop analyzing component config on "kubeadm upgrade". do not show warnings for outdated kube-proxy or kubelet configuration.

like you said, there is no action for the user to take here.
but if we stop accepting only upgrade config, that's an action required. so we have to explain it on a different pr.

maybe after the upgrade config pr merges, because that pr is already huge?

neolit123 · 2023-09-21T06:56:20Z

/approve
/hold

k8s-ci-robot · 2023-09-21T06:56:52Z

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

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

pacoxu · 2023-09-21T08:53:11Z

Probably, we should check https://github.com/kubernetes/enhancements/blob/0a6d6c888298c4381d3bedd2134a2e8ec098cab3/keps/sig-cluster-lifecycle/kubeadm/1381-component-config/README.md#kubeadm-upgrade-plan-changes that if the KEP needs some update.

neolit123 · 2023-09-21T08:58:18Z

Probably, we should check https://github.com/kubernetes/enhancements/blob/0a6d6c888298c4381d3bedd2134a2e8ec098cab3/keps/sig-cluster-lifecycle/kubeadm/1381-component-config/README.md#kubeadm-upgrade-plan-changes that if the KEP needs some update.

+1 @chendave PTAL and send PR with update there if needed.

neolit123 · 2023-09-28T08:24:31Z

/release-note-edit

ACTION REQUIRED: stop accepting component configuration for kube-proxy and kubelet during `kubeadm upgrade plan --config`. This is a legacy behavior that is not well supported for upgrades and can be used only at the plan stage to determine if the configuration for these components stored in the cluster needs manual version  migration. In the future, kubeadm will attempt alternative component config migration approaches.

neolit123 · 2023-09-28T08:25:49Z

^ @chendave please check if the updated release note makes sense for this PR.

still needs LGTM from another reviewer.

neolit123 · 2023-09-28T08:27:14Z

Probably, we should check https://github.com/kubernetes/enhancements/blob/0a6d6c888298c4381d3bedd2134a2e8ec098cab3/keps/sig-cluster-lifecycle/kubeadm/1381-component-config/README.md#kubeadm-upgrade-plan-changes that if the KEP needs some update.

+1 @chendave PTAL and send PR with update there if needed.

KEP changes are OK, but they are not urgent.
can be done retroactively.

neolit123 · 2023-09-28T08:28:45Z

^ @chendave please check if the updated release note makes sense for this PR.

still needs LGTM from another reviewer.

/cc @SataQiu
perhaps you remember when this CC logic was added for upgrades.

chendave · 2023-09-30T12:35:11Z

/release-note-edit

ACTION REQUIRED: stop accepting component configuration for kube-proxy and kubelet during `kubeadm upgrade plan --config`. This is a legacy behavior that is not well supported for upgrades and can be used only at the plan stage to determine if the configuration for these components stored in the cluster needs manual version  migration. In the future, kubeadm will attempt alternative component config migration approaches.

thanks @neolit123 for the update! this is not urgent, let's wait for a double check from @SataQiu or @pacoxu

chendave · 2023-09-30T13:05:01Z

/kind feature

chendave · 2023-09-30T13:07:02Z

/kind feature

tag it as feature might not appropriate, but I am not sure is there any better choice. :)

neolit123 · 2023-09-30T13:18:10Z

/kind feature

tag it as feature might not appropriate, but I am not is there any better choice. :)

/remove-kind feature
/kind deprecation

pacoxu · 2023-10-09T08:32:49Z

The behavior is to ignore or skip component configs(kubeproxy and kubelet) in --config.

The original is like below

The table below shows the current state of component configs as understood by this version of kubeadm.
Configs that have a "yes" mark in the "MANUAL UPGRADE REQUIRED" column require manual config upgrade or
resetting to kubeadm defaults before a successful upgrade can be performed. The version to manually
upgrade to is denoted in the "PREFERRED VERSION" column.

API GROUP                 CURRENT VERSION   PREFERRED VERSION   MANUAL UPGRADE REQUIRED
kubeproxy.config.k8s.io   v1alpha1          v1alpha1            no
kubelet.config.k8s.io     v1alpha2          v1beta1             yes

As #114062 (comment) discussed

this is not a common thing during upgrades. If we do not allow InitConfigiration on upgrade --config anymore, we are effectively breaking users that pass the component config +init configuration on upgrade.

/lgtm
Users may do the validation with kubeadm config validate --config before applying it to the configmap.

maybe after the upgrade config pr merges, because that pr is already huge?

@neolit123 Do you mean that this PR will be merged after #114062?

k8s-ci-robot · 2023-10-09T08:32:57Z

LGTM label has been added.

Git tree hash: 6787e93a52cd9fac26cf53145afdf85c75d51b76

neolit123 · 2023-10-09T10:34:24Z

Users may do the validation with kubeadm config validate --config before applying it to the configmap.

+1

@neolit123 Do you mean that this PR will be merged after #114062?

this can merge before the upgrade config PR, as well. @chendave do as you prefer.

chendave · 2023-10-09T10:57:16Z

thanks @neolit123 and @pacoxu !

let's hold for more days in case @SataQiu want to comment.

I will push a change to update KEP as a follow up.

SataQiu · 2023-10-09T12:46:00Z

perhaps you remember when this CC logic was added for upgrades.

It was introduced by #88124.
Previously, this was used to give the user more information to help complete the upgrade smoothly.
Since it violates the non-mutation principle for upgrade, I'm ok with this change.

/lgtm

chendave · 2023-10-11T01:31:44Z

/unhold

thanks all for the review

k8s-ci-robot requested review from neolit123 and pacoxu September 21, 2023 05:29

neolit123 reviewed Sep 21, 2023

View reviewed changes

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2023

k8s-ci-robot requested a review from SataQiu September 28, 2023 08:28

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 30, 2023

k8s-ci-robot added kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Sep 30, 2023

k8s-ci-robot assigned pacoxu Oct 9, 2023

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

k8s-ci-robot assigned SataQiu Oct 9, 2023

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 11, 2023

k8s-ci-robot merged commit 1245828 into kubernetes:master Oct 11, 2023
14 checks passed

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

chendave mentioned this pull request Oct 11, 2023

kubeadm: implementation of UpgradeConfiguration API types #114062

Closed

3 tasks

sveinpj mentioned this pull request Apr 26, 2024

Upgrade AKS to 1.29 equinor/radix-platform#1318

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: Remove the support of configurable component configs #120788

kubeadm: Remove the support of configurable component configs #120788

chendave commented Sep 21, 2023 •

edited

k8s-ci-robot commented Sep 21, 2023

chendave commented Sep 21, 2023

neolit123 left a comment

chendave commented Sep 21, 2023

chendave commented Sep 21, 2023

neolit123 commented Sep 21, 2023

neolit123 commented Sep 21, 2023 •

edited

neolit123 commented Sep 21, 2023

k8s-ci-robot commented Sep 21, 2023

pacoxu commented Sep 21, 2023

neolit123 commented Sep 21, 2023

neolit123 commented Sep 28, 2023

neolit123 commented Sep 28, 2023

neolit123 commented Sep 28, 2023

neolit123 commented Sep 28, 2023

chendave commented Sep 30, 2023

chendave commented Sep 30, 2023

chendave commented Sep 30, 2023 •

edited

neolit123 commented Sep 30, 2023

pacoxu commented Oct 9, 2023 •

edited

k8s-ci-robot commented Oct 9, 2023

neolit123 commented Oct 9, 2023

chendave commented Oct 9, 2023

SataQiu commented Oct 9, 2023

chendave commented Oct 11, 2023

kubeadm: Remove the support of configurable component configs #120788

kubeadm: Remove the support of configurable component configs #120788

Conversation

chendave commented Sep 21, 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 Sep 21, 2023

chendave commented Sep 21, 2023

neolit123 left a comment

Choose a reason for hiding this comment

chendave commented Sep 21, 2023

chendave commented Sep 21, 2023

neolit123 commented Sep 21, 2023

neolit123 commented Sep 21, 2023 • edited

neolit123 commented Sep 21, 2023

k8s-ci-robot commented Sep 21, 2023

pacoxu commented Sep 21, 2023

neolit123 commented Sep 21, 2023

neolit123 commented Sep 28, 2023

neolit123 commented Sep 28, 2023

neolit123 commented Sep 28, 2023

neolit123 commented Sep 28, 2023

chendave commented Sep 30, 2023

chendave commented Sep 30, 2023

chendave commented Sep 30, 2023 • edited

neolit123 commented Sep 30, 2023

pacoxu commented Oct 9, 2023 • edited

k8s-ci-robot commented Oct 9, 2023

neolit123 commented Oct 9, 2023

chendave commented Oct 9, 2023

SataQiu commented Oct 9, 2023

chendave commented Oct 11, 2023

chendave commented Sep 21, 2023 •

edited

neolit123 commented Sep 21, 2023 •

edited

chendave commented Sep 30, 2023 •

edited

pacoxu commented Oct 9, 2023 •

edited