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: add docs for UpgradeAddonsBeforeControlPlane feature gate #41606

Conversation

SataQiu
Copy link
Member

@SataQiu SataQiu commented Jun 12, 2023

kubeadm: add docs for UpgradeAddonsBeforeControlPlane feature gate

Ref kubernetes/kubeadm#2346

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 12, 2023
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 12, 2023
@SataQiu SataQiu requested review from neolit123 and removed request for nate-double-u and mehabhalodiya June 12, 2023 15:11
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Jun 12, 2023
@SataQiu SataQiu requested a review from tengqm June 12, 2023 15:11
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.

/sig cluster-lifecycle

some minor nits but, lgtm
thanks

@k8s-ci-robot k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Jun 12, 2023
@neolit123
Copy link
Member

wants to merge 1 commit into kubernetes:main from

should be for the dev-1.28 branch instead

@k8s-ci-robot k8s-ci-robot added the language/ko Issues or PRs related to Korean language label Jun 12, 2023
@SataQiu SataQiu changed the base branch from main to dev-1.18 June 12, 2023 15:23
@SataQiu SataQiu changed the base branch from dev-1.18 to main June 12, 2023 15:23
@SataQiu SataQiu force-pushed the add-docs-for-UpgradeAddonsBeforeControlPlane branch from 54a51ec to 9c3c6bf Compare June 12, 2023 15:36
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 12, 2023
@SataQiu SataQiu changed the base branch from main to dev-1.28 June 12, 2023 15:37
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 12, 2023
@SataQiu SataQiu removed the language/ko Issues or PRs related to Korean language label Jun 12, 2023
@netlify
Copy link

netlify bot commented Jun 12, 2023

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 9c3c6bf
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/64873b9c5c02130008c2689b
😎 Deploy Preview https://deploy-preview-41606--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 settings.

Before v1.27 (including v1.27), kubeadm will upgrade the 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, which
may cause compatibility problems. Since v1.28, kubeadm will always checks whether all the control plane instances have
been upgraded before starting to upgrade the addons. You **MUST** perform control plane nodes upgrade sequentially, and
Copy link
Member

Choose a reason for hiding this comment

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

q: don't remember if i asked this. some users are known to not follow this rule today and they try to do parallel cp upgrades. will the logic we added in 1.28 support them or there are no guarantees - i.e. undefined behavior?

Copy link
Member Author

@SataQiu SataQiu Jun 13, 2023

Choose a reason for hiding this comment

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

@neolit123 Because the check is only performed during kubeadm upgrade apply/node, when the control plane nodes are upgrading in parallel, there may be a race that all the kubeadm upgrade processes assume that there are unupgraded control plane instances, this will cause the addons to miss performing the upgrade. Complete serial execution is not required, but in order to ensure that the addons can be successfully upgraded during the last cp node upgrade, users should ensure that all the other cp nodes have been upgraded before doing the last cp node upgrade. In the future, we can add a new separate phase and call it after all the cp nodes have been upgraded, such as kubeadm upgrade addons. Then gradually deprecate upgrading addons during the upgrade apply/node phase. But this will change the upgrade workflow and may take a long time.

Copy link
Member Author

@SataQiu SataQiu Jun 13, 2023

Choose a reason for hiding this comment

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

If parallel cp upgrade is necessary, the user needs to make additional checks to see if the addons have been successfully upgraded or always to call kubeadm upgrade node or kubeadm upgrade addons(in the future) on the last cp node again to ensure that the addons can be upgraded successfully. This should be safe because the upgrade operation is idempotent.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the clarification.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Hi. Here's how I'd change this - what do you think @SataQiu ?

@SataQiu SataQiu force-pushed the add-docs-for-UpgradeAddonsBeforeControlPlane branch from 9c3c6bf to c43c101 Compare June 13, 2023 10:57
@SataQiu
Copy link
Member Author

SataQiu commented Jun 13, 2023

thanks @sftim @neolit123 PTAL again!

@SataQiu SataQiu force-pushed the add-docs-for-UpgradeAddonsBeforeControlPlane branch from c43c101 to fe98320 Compare June 13, 2023 11:03
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 Jun 13, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 2829a9f7314fa1cd5e424518e595d7a4fd6c46a6

@SataQiu SataQiu requested a review from sftim June 14, 2023 14:40
@SataQiu
Copy link
Member Author

SataQiu commented Jun 17, 2023

hi @tengqm
Would you mind take a look?

@@ -160,6 +160,7 @@ Feature | Default | Alpha | Beta | GA
`PublicKeysECDSA` | `false` | 1.19 | - | -
`RootlessControlPlane` | `false` | 1.22 | - | -
`UnversionedKubeletConfigMap` | `true` | 1.22 | 1.23 | 1.25
`UpgradeAddonsBeforeControlPlane` | `false` | - | - | -
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this alpha, or beta?

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 @tengqm
Sorry for the late reply.

It isn't a new feature, it's more like a bugfix. So this feature gate has been marked deprecated since it was created.
So it's not in Alpha stage, and will never be Beta or GA.
For more context: kubernetes/kubernetes#116570 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, we should document this in the second table (gates for GA'ed or deprecated feature).

Copy link
Member Author

@SataQiu SataQiu Jun 21, 2023

Choose a reason for hiding this comment

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

Updated

Comment on lines 201 to 202
instances upgrade sequentially or at least ensure that the last control plane instance is upgraded after all the other control
plane instances have been upgraded, and the addons upgrade will be performed after the last control plane instance is upgraded.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure "that the last control plane instance is upgraded after all the other control
plane instances have been upgraded" ...

This is so verbose. Does this mean that we need to ensure all control plane instances are properly upgraded?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is because the control plane node upgrade needs to ensure the sequence. In a HA cluster, the upgrade of the last control plane node cannot be performed until all the other control plane nodes have been upgraded successfully.

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 is upgraded after all the other control
plane instances have been upgraded, 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the gate defaulted to true or false?
It is introduced as a deprecated feature, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the deprecated feature gate is defaulted to false.

instances upgrade sequentially or at least ensure that the last control plane instance is upgraded after all the other control
plane instances have been upgraded, 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 change your cluster or upgrade processes but please do not rely on it, as this
Copy link
Contributor

Choose a reason for hiding this comment

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

"please do not rely on it" ...
What does this "it" refer to?

Copy link
Member Author

Choose a reason for hiding this comment

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

"it" means the UpgradeAddonsBeforeControlPlane feature gate.

Copy link
Member Author

Choose a reason for hiding this comment

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

"please do not rely on it" has been removed now

@SataQiu SataQiu force-pushed the add-docs-for-UpgradeAddonsBeforeControlPlane branch from fe98320 to d5a2a69 Compare June 21, 2023 05:35
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2023
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 21, 2023
@tengqm
Copy link
Contributor

tengqm commented Jun 21, 2023

/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 Jun 21, 2023
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 Jun 22, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 1c39610de7f763532d027aefd8c517204bb076d6

@k8s-ci-robot k8s-ci-robot merged commit 30f8c53 into kubernetes:dev-1.28 Jun 22, 2023
@k8s-ci-robot k8s-ci-robot added this to the 1.28 milestone Jun 22, 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. 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.

5 participants