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

Jump to bottom

kubeadm join: wait for API endpoints #34703

Merged
merged 1 commit into from Oct 15, 2016
Merged

kubeadm join: wait for API endpoints #34703

merged 1 commit into from Oct 15, 2016

Conversation

taimir
Copy link
Contributor

@taimir taimir commented Oct 13, 2016 •
edited

What this PR does / why we need it: enhance kubeadm to allow for parallel provisioning of API endpoints and slave nodes, continued from #33543

Fixes: #33542

Special notes for your reviewer:

  • Introduces a concurrent retry mechanism for bootstrapping with a single API endpoint during kubeadm join (this was left out in Decouple master bootstrap from CSR in kubeadm #33543 so that it can be implemented in a separate PR). The polling of the discovery service API itself is yet to come.

@errordeveloper @pires

This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot ok to test" on its own line.

Regular contributors should join the org to skip this step.

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Oct 13, 2016
errordeveloper
errordeveloper reviewed Oct 13, 2016
View reviewed changes
cmd/kubeadm/app/node/bootstrap.go
defer wg.Done()
wait.Until(func() {
fmt.Printf("<node/bootstrap> trying to connect to endpoint %s\n", endpoint)
if err := clientSet.CoreClient.Get().AbsPath("version").Do().Error(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Would be handy to grab the version string and print it, don't you think? We might want to move this to second log-level, but for now just printing it in human-readable format would help folks who are trying --kubernetes-version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sure :) does this match what you had in mind about failing early?

Copy link
Member

Choose a reason for hiding this comment

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

Yep!

Copy link
Contributor Author

@taimir taimir Oct 13, 2016 •
edited

Choose a reason for hiding this comment

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

Let's see : to grab the version I would need to unmarshall the response from the simple GET on /version.

But the DiscoveryClient method ServerVersion()here actually does exactly that. I don't see a need to duplicate the code, except if you think that the Get().AbsPath('version') is somehow more explicit (or if by some chance the behavior of discoveryClient.ServerVersion() is subject to change in the future).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also the server version was used in the checkCertsAPI, so I had to refactor a bit ... Do you think it makes sense to actually have a checkAPIEndpoint() function that does both checks one after the other (checks the server version, then checks the certs API with a ServerGroups call, as is currently done in checkCertsAPI)?

Copy link
Member

Choose a reason for hiding this comment

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

Good idea, thanks 👍

Copy link
Contributor Author

@taimir taimir Oct 13, 2016 •
edited

Choose a reason for hiding this comment

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

Done, please see if it's ok :)

errordeveloper
errordeveloper reviewed Oct 13, 2016
View reviewed changes
Copy link
Member

@errordeveloper errordeveloper left a comment

Choose a reason for hiding this comment

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

Just one nit, LGTM overall 👍

pires
pires approved these changes Oct 13, 2016
View reviewed changes
Copy link
Contributor

@pires pires left a comment

Choose a reason for hiding this comment

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

LGTM

@taimir taimir mentioned this pull request Oct 13, 2016
Merged
errordeveloper
errordeveloper approved these changes Oct 13, 2016
View reviewed changes
Copy link
Member

@errordeveloper errordeveloper left a comment

Choose a reason for hiding this comment

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

LGTM!

@taimir taimir force-pushed the kubeadm branch 2 times, most recently from a36bcbc to 931c751 Compare October 13, 2016 13:40
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 13, 2016
@luxas luxas added release-note-none Denotes a PR that doesn't merit a release note. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed release-note-label-needed labels Oct 13, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 890e4ed69e2cf4155e36a466241b8bc5937c3d43. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@pires
Copy link
Contributor

pires commented Oct 13, 2016 •
edited

@taimir there seems to be an issue here:

cmd/kubeadm/app/node/bootstrap.go:75: range variable endpoint captured by func literal
cmd/kubeadm/app/node/bootstrap.go:76: range variable endpoint captured by func literal
cmd/kubeadm/app/node/bootstrap.go:81: range variable endpoint captured by func literal
cmd/kubeadm/app/node/bootstrap.go:86: range variable endpoint captured by func literal
exit status 1
Makefile:99: recipe for target 'verify' failed

@luxas luxas removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 13, 2016
@luxas
Copy link
Member

luxas commented Oct 13, 2016

@taimir Yes, please fix the verification test

@taimir
Copy link
Contributor Author

taimir commented Oct 13, 2016

Ah, I was AFK, sorry - sure thing, I'll fix it

@taimir
kubeadm join: wait for API endpoints
072259f 
 * Introduce a concurrent retry mechanism for bootstrapping
   with a single API endpoint
@pires
Copy link
Contributor

pires commented Oct 13, 2016

@luxas I think it's missing your lgtm.

@luxas luxas added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 14, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 3e9e507 into kubernetes:master Oct 15, 2016
@luxas luxas mentioned this pull request Oct 15, 2016
Closed
k8s-github-robot pushed a commit that referenced this pull request Oct 15, 2016
Merge pull request #34718 from taimir/kubeadm-disco-wait
7e6fda2 
Automatic merge from submit-queue

kubeadm join: polling discovery service API

**What this PR does / why we need it**: Enhance kubeadm to allow for parallel provisioning of API endpoints and slave nodes, in addition to #33543. This PR let's `kubeadm join` poll the discovery service API and retry connecting to it every couple of seconds. That way `kubeadm init` and `kubeadm join` can be executed in parallel.

**Fixes**: #33542

**Special notes for your reviewer**:

@pires @errordeveloper last part of the discussed changes, in addition to #33543 and #34703
@luxas luxas mentioned this pull request Oct 25, 2016
Closed
@mcapuccini mcapuccini mentioned this pull request Nov 20, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@errordeveloper errordeveloper errordeveloper approved these changes

@pires pires pires approved these changes

Assignees

@luxas luxas

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@taimir @k8s-ci-robot @pires @luxas @k8s-github-robot @errordeveloper @googlebot