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.

Sign up for GitHub

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

Re-engineer the kubeadm join logic. #56014

Merged
merged 2 commits into from
Nov 19, 2017
Merged

Re-engineer the kubeadm join logic. #56014

merged 2 commits into from
Nov 19, 2017

Conversation

xiangpengzhao
Copy link
Contributor

What this PR does / why we need it:

  • wait for the kubelet to create /etc/kubernetes/kubelet.conf
  • use those credentials to PATCH the node

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
ref: kubernetes/kubeadm#28 (comment)

Special notes for your reviewer:
/cc @kubernetes/sig-cluster-lifecycle-pr-reviews

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Nov 19, 2017
@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 19, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 19, 2017
luxas
luxas approved these changes Nov 19, 2017
View reviewed changes
Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

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

Looks good, small comments only

cmd/kubeadm/app/phases/kubelet/kubelet.go Outdated
@@ -69,6 +69,8 @@ func CreateBaseKubeletConfiguration(cfg *kubeadmapi.MasterConfiguration, client

// UpdateNodeWithConfigMap updates node ConfigSource with KubeletBaseConfigurationConfigMap
func UpdateNodeWithConfigMap(client clientset.Interface, nodeName string) error {
fmt.Println("[kubelet] Waiting for the node to update with remote base kubelet configuration...")
Copy link
Member

Choose a reason for hiding this comment

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

suggest: [kubelet] Using Dynamic Kubelet Config for node %q; config sourced from ConfigMap %q in namespace %s

Copy link
Member

Choose a reason for hiding this comment

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

and in CreateBaseKubeletConfiguration I'd like a
[kubelet] Uploading a ConfigMap %q in namespace %s with base configuration for the kubelets in the cluster

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

cmd/kubeadm/app/cmd/join.go Outdated
if err := kubeconfigutil.WriteToDisk(kubeconfigFile, cfg); err != nil {
return err
}
func getTLSBootstrappedClient() (clientset.Interface, error) {
Copy link
Member

Choose a reason for hiding this comment

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

add a comment here like // getTLSBootstrappedClient waits for the kubelet to perform the TLS bootstrap and then creates a k8s client of /etc/kubernetes/kubelet.conf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

cmd/kubeadm/app/cmd/join.go Outdated
// Loop on every falsy return. Return with an error if raised. Exit successfully if true is returned.
err := wait.PollImmediateInfinite(kubeadmconstants.APICallRetryInterval, func() (bool, error) {
if _, err := os.Stat(kubeletKubeConfig); err == nil {
return true, nil
Copy link
Member

Choose a reason for hiding this comment

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

nit: if we really want to shorten the logic here, we could skip the if clause and write return (err == nil), nil here instead.
What do you think is better / more readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the return (err == nil), nil one :) done.

@luxas luxas assigned luxas and unassigned lukemarsden Nov 19, 2017
@luxas luxas added this to the v1.9 milestone Nov 19, 2017
@luxas luxas added kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. status/approved-for-milestone labels Nov 19, 2017
@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request Current

@krousey @luxas @xiangpengzhao

Pull Request Labels
  • sig/cluster-lifecycle: Pull Request will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/bug: Fixes a bug discovered during the current release.
Help

@xiangpengzhao
Copy link
Contributor Author

@luxas inline comments addressed. PTAL. Thanks!

luxas
luxas reviewed Nov 19, 2017
View reviewed changes
Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

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

Note that you can use fmt.Printf here instead

cmd/kubeadm/app/phases/kubelet/kubelet.go Outdated
@@ -39,6 +39,9 @@ import (

// CreateBaseKubeletConfiguration creates base kubelet configuration for dynamic kubelet configuration feature.
func CreateBaseKubeletConfiguration(cfg *kubeadmapi.MasterConfiguration, client clientset.Interface) error {
fmt.Println(fmt.Sprintf("[kubelet] Uploading a ConfigMap %q in namespace %s with base configuration for the kubelets in the cluster",
Copy link
Member

Choose a reason for hiding this comment

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

you can write this as fmt.Printf("[kubelet] Uploading a ConfigMap %q in namespace %s with base configuration for the kubelets in the cluster", kubeadmconstants.KubeletBaseConfigurationConfigMap, metav1.NamespaceSystem) instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! I don't know what I was thinking when I wrote this... 😭

cmd/kubeadm/app/phases/kubelet/kubelet.go Outdated
@@ -69,6 +72,9 @@ func CreateBaseKubeletConfiguration(cfg *kubeadmapi.MasterConfiguration, client

// UpdateNodeWithConfigMap updates node ConfigSource with KubeletBaseConfigurationConfigMap
func UpdateNodeWithConfigMap(client clientset.Interface, nodeName string) error {
fmt.Println(fmt.Sprintf("[kubelet] Using Dynamic Kubelet Config for node %q; config sourced from ConfigMap %q in namespace %s",
Copy link
Member

Choose a reason for hiding this comment

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

s/fmt.Println(fmt.Sprintf/fmt.Printf/

@xiangpengzhao
Copy link
Contributor Author

@luxas addressed. PTAL. Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2017
luxas
luxas approved these changes Nov 19, 2017
View reviewed changes
Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve no-issue

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: luxas, xiangpengzhao

Associated issue: 28

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 19, 2017
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit e89bbb5 into kubernetes:master Nov 19, 2017
@xiangpengzhao xiangpengzhao deleted the join-kubelet-conf branch November 19, 2017 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luxas luxas luxas approved these changes

Assignees

@krousey krousey

@luxas luxas

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. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.9
Development

Successfully merging this pull request may close these issues.
6 participants
@xiangpengzhao @k8s-github-robot @luxas @krousey @lukemarsden @k8s-ci-robot