-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[WIP] enable concurrent join of both worker and control-plane nodes #685
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: neolit123 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 |
/kind feature |
} else if len(secondaryControlPlanes) > 0 && len(workers) == 0 { | ||
startMsg += "control-plane nodes 🎮" | ||
} else { | ||
startMsg += "worker nodes 🚜" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me know if the messages / icons need tweaks.
I´m curious what´s going to be the job results on versions <1.14
I think that we have the
I use for CI environments |
we technically have to enable this feature in kind only for kubeadm versions >= 1.15.
yep, but unless i'm missing something it if set to 0 it does not wait. |
in my tests if etcd member addition magically aligns, things work fine without the latest changes in k/k. |
but i find it quite odd that all 12, 13 and 14 passed. |
hehehe, is too late for me, I just realized that the jobs in the CI only have one control plane 🙃 😫 EDIT
@neolit123 we are not testing secondary control planes in the CI, should we? |
[will come back to this, keeping low priority due to WIP..., @ me if you need something sooner.] |
5fe433f
to
3d7118e
Compare
rebased the PR. when doing the concurrent join it now takes <2minutes, for a 3x3 cluster, however i got it to flake 1 time out of 5 (or so), but i couldn't catch the debug output and the real cause. probably it needs better timeouts/retries for etcd. |
defer ctx.Status.End(false) | ||
|
||
// TODO(bentheelder): it's too bad we can't do this concurrently | ||
// (this is not safe currently) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is only safe on recent versions, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at least this change is needed, which was added in 1.15.1
kubernetes/kubernetes@bc74ac3#diff-5e4c5bba67c635568de70e0c424882c6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably ship this as being concurrent only for >= 1.15.1 then ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, i guess there has to be a version check and the old way of serial join has to be supported too.
i only got it to flake 1/10 times. still no logs. |
@neolit123: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your 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. |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
@neolit123: 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. |
let's get back to this once the required changes in etcd land. /close |
@neolit123: Closed this PR. In response to this:
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. |
(first commit fixes some typos)
concurrent join of CP nodes should be now supported in k/k master for kubeadm.
no test signal in kinder yet though.
this change works for me, but oddly, i don't see much improvements in performance by using
time kind create cluster...
with a3CPx3W
cluster. it's roughly 10 seconds reduced from 2 minutes 40 seconds without the change, but the numbers vary a lot for me.i'm noticing that once
kind create cluster
finishes most pods (e.g 24 of 26) are already running for this 6 node setup, which should not be the case - most of the pods should be still creating once the kind binary exists, unless this is desired?do we have sync points or "wait for node/pods ready" logic in kind?
cc @BenTheElder @aojea @fabriziopandini