Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kubeadm: Remove the support of configurable component configs #120788

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

chendave
Copy link
Member

@chendave chendave commented Sep 21, 2023

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 k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 21, 2023
@k8s-ci-robot
Copy link
Contributor

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.

@k8s-ci-robot k8s-ci-robot added 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. area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 21, 2023
@chendave
Copy link
Member Author

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?

Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

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
Copy link
Member Author

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

+1, let me consolidate this logic into #114062

@neolit123
Copy link
Member

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
Copy link
Member

neolit123 commented Sep 21, 2023

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
Copy link
Member

/approve
/hold

@k8s-ci-robot 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
Copy link
Contributor

[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:

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

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

@neolit123
Copy link
Member

/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
Copy link
Member

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

still needs LGTM from another reviewer.

@neolit123
Copy link
Member

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
Copy link
Member

^ @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
Copy link
Member Author

/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
Copy link
Member Author

/kind feature

@k8s-ci-robot 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
@chendave
Copy link
Member Author

chendave commented Sep 30, 2023

/kind feature

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

@neolit123
Copy link
Member

/kind feature

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

/remove-kind feature
/kind deprecation

@k8s-ci-robot 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
@pacoxu
Copy link
Member

pacoxu commented Oct 9, 2023

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 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
Copy link
Contributor

LGTM label has been added.

Git tree hash: 6787e93a52cd9fac26cf53145afdf85c75d51b76

@neolit123
Copy link
Member

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
Copy link
Member Author

chendave commented Oct 9, 2023

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
Copy link
Member

SataQiu commented Oct 9, 2023

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
Copy link
Member Author

/unhold

thanks all for the review

@k8s-ci-robot 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 k8s-ci-robot merged commit 1245828 into kubernetes:master Oct 11, 2023
14 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.29 milestone Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants