kubeadm graduate bootstrap-token phase #70727

yagonobre · 2018-11-07T04:20:15Z

What type of PR is this?
This PR graduates phase bootstrap-token in the kubeadm init workflow.
/kind feature

What this PR does / why we need it:
Ref kubernetes/kubeadm#1163

/sig cluster-lifecycle
/priority important-soon
/assign @timothysc @fabriziopandini @neolit123
@kubernetes/sig-cluster-lifecycle-pr-reviews

NONE

k8s-ci-robot · 2018-11-07T04:20:23Z

@yagonobre: Reiterating the mentions to trigger a notification:
@kubernetes/sig-cluster-lifecycle-pr-reviews

In response to this:

What type of PR is this?
This PR graduates phase bootstrap-token in the kubeadm init workflow.
/kind feature

What this PR does / why we need it:
Ref kubernetes/kubeadm#1163

/sig cluster-lifecycle
/priority important-soon
/assign @timothysc @fabriziopandini @neolit123
@kubernetes/sig-cluster-lifecycle-pr-reviews
NONE

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.

fabriziopandini · 2018-11-07T08:00:26Z

/ok-to-test

fabriziopandini

@yagonobre Thanks!
Please fix tests, address nits, remove WIP.
If I'm not wrong the error is related to some inconsistent behavior in DryRun, but I'm not 100% sure...

fabriziopandini · 2018-11-07T11:14:47Z

cmd/kubeadm/app/cmd/init.go

@@ -578,7 +532,8 @@ func runInit(i *initData, out io.Writer) error {
 	}

 	// Prints the join command, multiple times in case the user has multiple tokens
-	for _, token := range tokens {
+	for _, bt := range i.cfg.BootstrapTokens {


You can use initData.Tokens

fabriziopandini · 2018-11-07T11:15:19Z

cmd/kubeadm/app/cmd/phases/bootstraptoken.go

-		Short:   "Configures the node bootstrap process",
-		Aliases: []string{"clusterinfo"},
-		Long:    cmdutil.MacroCommandLongDescription,
+	tokens := []string{}


You can use initData.Tokens

fabriziopandini · 2018-11-07T11:26:32Z

cmd/kubeadm/app/cmd/phases/bootstraptoken.go

-		},
+	if !skipTokenPrint {
+		if len(tokens) == 1 {
+			glog.V(1).Infof("[bootstraptoken] using token: %s\n", tokens[0])


please use printf
[bootstraptoken] --> [bootstrap-token] (text in square breakets should match the phase name)
creating --> Creating (start all message with capital letter)

same for all the printf messages

@fabriziopandini why use printf instead of glog? I guess that all phases use glog.

@yagonobre we are using fmt.printf for things we want to notify to the user always (the default kubeadm output) and glog for info/debug message that can be triggered with the -v flag

However, this is a nit and if you prefer we can iterate also in following PRs.

fabriziopandini · 2018-11-07T11:27:35Z

cmd/kubeadm/app/cmd/phases/bootstraptoken.go

-			err = node.AutoApproveNodeCertificateRotation(client)
-			kubeadmutil.CheckErr(err)
-		},
+	// Create the default node bootstrap token


I think that this phase should give a user a message, otherwise this phase is almost silent about implemented actions. e.g.

fmt.Println("[bootstrap-token] Configuring bootstrap tokens, cluster-info ConfigMap, RBAC Roles")

extra space after ,

yagonobre · 2018-11-07T15:17:22Z

/test pull-kubernetes-integration

neolit123

@yagonobre thanks!
added some minor comments.

neolit123 · 2018-11-07T16:45:29Z

cmd/kubeadm/app/cmd/phases/bootstraptoken.go

-			err = node.AutoApproveNodeCertificateRotation(client)
-			kubeadmutil.CheckErr(err)
-		},
+	// Create the default node bootstrap token


extra space after ,

neolit123 · 2018-11-07T16:47:56Z

cmd/kubeadm/app/cmd/phases/bootstraptoken.go

-		fmt.Println(joinCommand)
+	// Create the cluster-info ConfigMap with the associated RBAC rules
+	if err := clusterinfophase.CreateBootstrapConfigMapIfNotExists(client, kubeconfigPath); err != nil {
+		return errors.Wrap(err, "error creating bootstrap configmap")


neolit123 · 2018-11-07T16:52:05Z

cmd/kubeadm/app/cmd/init.go

@@ -578,7 +532,7 @@ func runInit(i *initData, out io.Writer) error {
 	}

 	// Prints the join command, multiple times in case the user has multiple tokens
-	for _, token := range tokens {
+	for _, token := range i.Tokens() {
 		if err := printJoinCommand(out, adminKubeConfigPath, token, i.skipTokenPrint); err != nil {


we should move the printing of the join command outside of runInit().
this should happen right after the phase runner has executed all the phases.

I should address this on this pr? I guess that we have another step in the tracker issue for this.

sorry, indeed it was a separate step.
the scope of this PR is good.

neolit123 · 2018-11-07T16:55:10Z

cmd/kubeadm/app/cmd/phases/bootstraptoken.go

-		},
+	cfg := data.Cfg()
+	skipTokenPrint := data.SkipTokenPrint()
+	kubeconfigPath := data.KubeConfigPath()


i see them used only once?

possibly the intermediate vars are not needed here.
just use data.Cfg() , data.KubeConfigPath() and data.KubeConfigPath()

neolit123 · 2018-11-07T18:37:03Z

@yagonobre try running ./hack/make-rules/test-kubeadm-cmd.sh
to debug the failing integration tests.

yagonobre · 2018-11-07T18:50:38Z

@neolit123 I'll take a look a bit later.

fabriziopandini · 2018-11-08T09:18:23Z

/test pull-kubernetes-e2e-gce-100-performance

fabriziopandini · 2018-11-08T09:23:23Z

@yagonobre well done! let's try to get this merge asap
/approve
/lgtm

timothysc

/approve

k8s-ci-robot · 2018-11-09T00:08:58Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: fabriziopandini, timothysc, yagonobre

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:

~~cmd/kubeadm/OWNERS~~ [fabriziopandini,timothysc]
docs/OWNERS

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 assigned fabriziopandini Nov 7, 2018

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Nov 7, 2018

k8s-ci-robot assigned neolit123 Nov 7, 2018

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 7, 2018

k8s-ci-robot assigned timothysc Nov 7, 2018

k8s-ci-robot requested review from neolit123 and xiangpengzhao November 7, 2018 04:20

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/kubeadm labels Nov 7, 2018

yagonobre force-pushed the bootstrap-token branch from fbfe704 to 2b32280 Compare November 7, 2018 04:29

k8s-ci-robot added the kind/documentation Categorizes issue or PR as related to documentation. label Nov 7, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 7, 2018

neolit123 mentioned this pull request Nov 7, 2018

tracking issue for kubeadm init phases kubernetes/kubeadm#1163

Closed

17 tasks

fabriziopandini suggested changes Nov 7, 2018

View reviewed changes

neolit123 reviewed Nov 7, 2018

View reviewed changes

yagonobre force-pushed the bootstrap-token branch from 89b675b to 5339d78 Compare November 7, 2018 17:33

yagonobre force-pushed the bootstrap-token branch from 5339d78 to c752c96 Compare November 8, 2018 02:23

kubeadm graduate bootstrap-token phase

e743f0e

yagonobre force-pushed the bootstrap-token branch from c752c96 to e743f0e Compare November 8, 2018 03:01

yagonobre changed the title ~~[WIP] kubeadm graduate bootstrap-token phase~~ kubeadm graduate bootstrap-token phase Nov 8, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2018

timothysc approved these changes Nov 9, 2018

View reviewed changes

timothysc added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2018

k8s-ci-robot merged commit 34be549 into kubernetes:master Nov 9, 2018

yagonobre deleted the bootstrap-token branch November 9, 2018 03:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm graduate bootstrap-token phase #70727

kubeadm graduate bootstrap-token phase #70727

yagonobre commented Nov 7, 2018

k8s-ci-robot commented Nov 7, 2018

fabriziopandini commented Nov 7, 2018

fabriziopandini left a comment

fabriziopandini Nov 7, 2018

fabriziopandini Nov 7, 2018

fabriziopandini Nov 7, 2018

yagonobre Nov 7, 2018

fabriziopandini Nov 8, 2018 •

edited

Loading

fabriziopandini Nov 7, 2018

neolit123 Nov 7, 2018

yagonobre commented Nov 7, 2018

neolit123 left a comment

neolit123 Nov 7, 2018

neolit123 Nov 7, 2018

neolit123 Nov 7, 2018

yagonobre Nov 7, 2018

neolit123 Nov 7, 2018

neolit123 Nov 7, 2018

neolit123 commented Nov 7, 2018

yagonobre commented Nov 7, 2018

fabriziopandini commented Nov 8, 2018

fabriziopandini commented Nov 8, 2018

timothysc left a comment

k8s-ci-robot commented Nov 9, 2018

kubeadm graduate bootstrap-token phase #70727

kubeadm graduate bootstrap-token phase #70727

Conversation

yagonobre commented Nov 7, 2018

k8s-ci-robot commented Nov 7, 2018

fabriziopandini commented Nov 7, 2018

fabriziopandini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yagonobre commented Nov 7, 2018

neolit123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 commented Nov 7, 2018

yagonobre commented Nov 7, 2018

fabriziopandini commented Nov 8, 2018

fabriziopandini commented Nov 8, 2018

timothysc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 9, 2018

fabriziopandini Nov 8, 2018 •

edited

Loading