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

Fix #9696 - apiserver outage when replacing or scaling control plane nodes #9701

Closed

Conversation

holmesb
Copy link
Contributor

@holmesb holmesb commented Jan 24, 2023

Fixed by bouncing apiserver static pods sequentially instead of all at once when there are etcd node changes. Retain the faster, old method for use by non-HA apiserver.

No longer change running apiserver static pods (and restart them) if only the order of etcd servers have changed.

/kind bug

What this PR does / why we need it:
Fixes loss of apiserver when are etcd nodes are scaled, or CP1 is replaced.

Which issue(s) this PR fixes:
Fixes #9696

Does this PR introduce a user-facing change?:
NONE

@k8s-ci-robot k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jan 24, 2023
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 24, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 24, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @holmesb. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 24, 2023
@k8s-ci-robot
Copy link
Contributor

Keywords which can automatically close issues and at(@) or hashtag(#) mentions are not allowed in commit messages.

The list of commits with invalid commit messages:

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. I understand the commands that are listed here.

@holmesb holmesb force-pushed the bh/api_server_outage_fix branch 2 times, most recently from 3b858f1 to d56fa4c Compare January 25, 2023 14:30
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 25, 2023
@holmesb
Copy link
Contributor Author

holmesb commented Jan 25, 2023

Fixed author\CLA.

@holmesb
Copy link
Contributor Author

holmesb commented Jan 25, 2023

CI has a "command-instead-of-module" Ansible lint error because I'm using sed & curl instead of lineinfile & uri. But we must query the local endpoint (127.0.0.1/healthz) before moving onto the next CP node (apiserver static pod). Can't use throttle at the block level, otherwise I'd couple a lineinfile with a uri task. Open to suggestions. Maybe I should split this into its own play, then I could use serial. Or can we just ignore this lint error?

@floryut
Copy link
Member

floryut commented Jan 25, 2023

CI has a "command-instead-of-module" Ansible lint error because I'm using sed & curl instead of lineinfile & uri. But we must query the local endpoint (127.0.0.1/healthz) before moving onto the next CP node (apiserver static pod). Can't use throttle at the block level, otherwise I'd couple a shell with a uri task. Open to suggestions. Maybe I should split this into its own play, then I could use serial. Or can we just ignore this lint error?

ignoring the error is fine for this case I'd say, as long as there is a valid reason it's fine for me

@holmesb
Copy link
Contributor Author

holmesb commented Feb 3, 2023

Any news with this one @cyclinder @jayonlau? Be good to avoid downtime everytime control plane nodes are changed. Are no breaking changes.

@holmesb
Copy link
Contributor Author

holmesb commented Feb 17, 2023

Any news with this pls @floryut ? K8s API server going offline every CP change isn't exactly "Production Ready". I'd like to avoid having to add this as a patch to Kubespray every release\build.

@floryut
Copy link
Member

floryut commented Feb 20, 2023

Any news with this pls @floryut ? K8s API server going offline every CP change isn't exactly "Production Ready". I'd like to avoid having to add this as a patch to Kubespray every release\build.

@holmesb Sorry I was pretty much waiting for you to fix the CI not passing 😆
Can you take a look ?

@holmesb
Copy link
Contributor Author

holmesb commented Feb 20, 2023

The ansible lint message? I thought you said "ignoring the error is fine for this case I'd say". We're already using sed & curl elsewhere in this repo, so I can't envisage any issues.

@floryut
Copy link
Member

floryut commented Feb 20, 2023

The ansible lint message? I thought you said "ignoring the error is fine for this case I'd say". We're already using sed & curl elsewhere in this repo, so I can't envisage any issues.

Yes, ignoring the error, meaning adding a noqa in the code on this specific line 😄

@holmesb holmesb force-pushed the bh/api_server_outage_fix branch 2 times, most recently from 07f3f96 to 5800a7c Compare February 20, 2023 22:06
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 20, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed 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. labels Feb 20, 2023
@holmesb holmesb force-pushed the bh/api_server_outage_fix branch from 5800a7c to 6d1a5c1 Compare February 20, 2023 22:15
@k8s-ci-robot k8s-ci-robot removed the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Feb 20, 2023
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 20, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: holmesb
Once this PR has been reviewed and has the lgtm label, please assign luckysb for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 20, 2023
@holmesb holmesb force-pushed the bh/api_server_outage_fix branch 2 times, most recently from 6470788 to 0f65697 Compare February 21, 2023 11:36
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 21, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 21, 2023
@holmesb holmesb force-pushed the bh/api_server_outage_fix branch from edbfc11 to bad2f15 Compare February 21, 2023 16:39
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 21, 2023
@holmesb
Copy link
Contributor Author

holmesb commented Feb 22, 2023

Passing CI now @floryut

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 23, 2023
@jcpunk
Copy link
Contributor

jcpunk commented May 31, 2023

Can you rebase off head?

… of all at once when there are etcd node changes. Retain the faster, old method for use by non-HA apiserver.

No longer change running apiserver static pod if only the order of etcd servers have changed.
@holmesb holmesb force-pushed the bh/api_server_outage_fix branch from bad2f15 to 646bc92 Compare June 5, 2023 09:05
@k8s-ci-robot
Copy link
Contributor

Keywords which can automatically close issues and at(@) mentions are not allowed in the title of a Pull Request.

You can edit the title by writing /retitle in a comment.

When GitHub merges a Pull Request, the title is included in the merge commit. To avoid invalid keywords in the merge commit, please edit the title of the PR.

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. I understand the commands that are listed here.

@holmesb
Copy link
Contributor Author

holmesb commented Jun 7, 2023

Can you rebase off head?

Done @jcpunk

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle rotten
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 7, 2023
@VannTen
Copy link
Contributor

VannTen commented Nov 26, 2023

IMO, we should just revert #8253 instead. It's documented here that scale.yml it not intended for control-plane components, and you should use cluster/upgrade-cluster.
Reverting that PR would mean the api-server conf change is handled during the normal playbook upgrade path one at a time, and would avoid the mentioned down-time.

@VannTen
Copy link
Contributor

VannTen commented Dec 21, 2023

I think this has been fixed by the linked PR above, so we'll close that.
Feel free to reopen if still needed.
Thanks for the work regardless o/
/close

@k8s-ci-robot
Copy link
Contributor

@VannTen: Closed this PR.

In response to this:

I think this has been fixed by the linked PR above, so we'll close that.
Feel free to reopen if still needed.
Thanks for the work regardless o/
/close

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

PR needs rebase.

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 the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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.

apiserver outage when replacing or scaling control plane nodes
6 participants