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: update docs for removed UpgradeAddonsBeforeControlPlane feature gate #46274

Merged

Conversation

SataQiu
Copy link
Member

@SataQiu SataQiu commented May 8, 2024

kubeadm: update docs for removed UpgradeAddonsBeforeControlPlane feature gate
Ref: kubernetes/kubernetes#124715

@k8s-ci-robot k8s-ci-robot added this to the 1.31 milestone May 8, 2024
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. language/en Issues or PRs related to English language labels May 8, 2024
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label May 8, 2024
Copy link

netlify bot commented May 8, 2024

👷 Deploy Preview for kubernetes-io-vnext-staging processing.

Name Link
🔨 Latest commit 82348a6
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/6646bd9299c6d50008ebdb82

@SataQiu
Copy link
Member Author

SataQiu commented May 8, 2024

/cc @neolit123

Copy link

netlify bot commented May 8, 2024

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 82348a6
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/6646bd92fb33b00008079df3
😎 Deploy Preview https://deploy-preview-46274--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Comment on lines 222 to 232
: This is as a **disabled** feature gate that was introduced for Kubernetes v1.28, in order to allow reactivating a legacy
and deprecated behavior during cluster upgrade. For kubeadm versions prior to v1.28, kubeadm upgrades cluster addons (including
CoreDNS and kube-proxy) immediately during `kubeadm upgrade apply`, regardless of whether there are other control plane
instances that have not been upgraded. This may cause compatibility problems. Since v1.28, kubeadm defaults to a mode that
always checks whether all the control plane instances have been upgraded before starting to upgrade the addons. This behavior
is applied to both `kubeadm upgrade apply` and `kubeadm upgrade node`. kubeadm determines whether a control plane instance
has been upgraded by checking whether the image of the kube-apiserver Pod has been upgraded. You must perform control plane
instances upgrade sequentially or at least ensure that the last control plane instance upgrade is not started until all the
other control plane instances have been upgraded completely, and the addons upgrade will be performed after the last control plane
instance is upgraded. The deprecated `UpgradeAddonsBeforeControlPlane` feature gate gives you a chance to keep the old upgrade
behavior. You should not need this old behavior; if you do, you should consider changing your cluster or upgrade processes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, add all this text in to the main branch (which at time of writing documents v1.30), and then once it has merged, update a couple of lines to make it correct for v1.31

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks @sftim
Does that mean we can commit this to the main branch first? Will that affect the documentation content of v1.30? We expect this change to only be seen in v1.31 and later.

Copy link
Contributor

Choose a reason for hiding this comment

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

On the contrary, my take of this change is that the gate can be used on v1.28-v1.30.
It is something that we should have documented since v1.28 and then removed in v1.31.

How about we document this into v1.30 (main) branch and then backport to v1.28-v1.29. The update we then need for v1.31 would be deleting or drastically simplify this paragraph because the gate has been removed.

Copy link
Member

@neolit123 neolit123 May 16, 2024

Choose a reason for hiding this comment

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

the gate existed as "deprecated" since 1.28 up until 1.31 under "List of deprecated feature gates:". it started as a deprecated FG. see https://v1-28.docs.kubernetes.io/docs/reference/setup-tools/kubeadm/kubeadm-init/#feature-gates
i don't understand why this change should be backported or targeting anything other than dev-1.31.

for v1.31 would be deleting or drastically simplify this paragraph because the gate has been removed.

this can be done, but we haven't done such simplifications for prior removals of FGs.

Copy link
Contributor

Choose a reason for hiding this comment

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

A gate, even for one that is deprecated when first introduced, is still something a user can use. I misunderstood the whole changes proposed in this PR. Actually this PR is about moving rather than removing the description about the gate. That should be fine. I mean, we don't need to backport anything.

For v1.31, the "drastically simplified version" could be something like:

This gate has been removed. It was introduced in v1.28 as a deprecated feature and then removed in v1.31. For documentation on older versions, please switch to the corresponding website version.

I mean, we don't need to add more details for such a "born to be deprecated" feature in v1.31.

Copy link
Member

Choose a reason for hiding this comment

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

This gate has been removed. It was introduced in v1.28 as a deprecated feature and then removed in v1.31. For documentation on older versions, please switch to the corresponding website version.

I mean, we don't need to add more details for such a "born to be deprecated" feature in v1.31.

SGTM

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, updated. @neolit123 @tengqm

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.

LGTM minus one nit
@sftim i also think dev-1.31 is the right branch

@neolit123
Copy link
Member

/sig cluster-lifecycle

@k8s-ci-robot k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label May 16, 2024
@SataQiu SataQiu force-pushed the update-kubeadm-docs-20240508 branch from bca8c8c to 5c11a74 Compare May 16, 2024 06:23
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.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: c46f82aae88f0276d49af3318a623008e43aa312

…ure gate

Co-authored-by: Lubomir I. Ivanov <neolit123@gmail.com>
@SataQiu SataQiu force-pushed the update-kubeadm-docs-20240508 branch from 5c11a74 to 82348a6 Compare May 17, 2024 02:14
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2024
@tengqm
Copy link
Contributor

tengqm commented May 17, 2024

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tengqm

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 May 17, 2024
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.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 3620fd4fa161f99e60422e39104ab129e7083d93

@k8s-ci-robot k8s-ci-robot merged commit 6539d64 into kubernetes:dev-1.31 May 17, 2024
6 checks passed
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants