kubeadm: graduate control plane join phase #73452

RA489 · 2019-01-29T06:44:16Z

What this PR does / why we need it:
Graduate control plane join phase

Which issue(s) this PR fixes:
Refs kubernetes/kubeadm#1204

Does this PR introduce a user-facing change?:

NONE
/kind feature

k8s-ci-robot · 2019-01-29T06:44:23Z

Hi @RA489. Thanks for your PR.

I'm waiting for a kubernetes 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.

RA489 · 2019-01-29T06:46:44Z

/release-note-none

yagonobre

Thanks @RA489
/ok-to-test
/priority important-soon

cmd/kubeadm/app/cmd/phases/controlplanejoin.go

neolit123

i tried reviewing this PR but github is telling me "Sorry, this diff is temporarily unavailable due to heavy server load." :)

the PR about the previous phase is about to merge soon.
please rebase this PR once that is done @RA489 thanks!
#73732

RA489 · 2019-02-07T13:43:16Z

@neolit123 rebased PTAL

cmd/kubeadm/app/cmd/join.go

neolit123

@RA489, i've added some comments.
this is progressing good.

cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go

neolit123 · 2019-02-08T14:24:51Z

cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go

+func NewControlPlaneJoinPhase() workflow.Phase {
+	return workflow.Phase{
+		Name:  "control-plane-join",
+		Short: "Prepares the machine to join a control plane.",


Joins a machine as a control plane instance

neolit123 · 2019-02-08T14:25:39Z

cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go

+}
+
+// NewControlPlaneJoinPhase creates a kubeadm workflow phase that implements marking the new node as master and update the cluster status
+// with information about current node


change to:

NewControlPlaneJoinPhase creates a kubeadm workflow phase that implements joining a machine as a control plane instance

neolit123 · 2019-02-08T14:26:09Z

cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go

+		Phases: []workflow.Phase{
+			{
+				Name:           "all",
+				Short:          "Generates all static Pod manifest files",


Joins a machine as a control plane instance

neolit123 · 2019-02-08T14:28:33Z

cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go

+			},
+			newControlPlaneJoinSubphase(kubeadmconstants.KubeAPIServer),
+			newControlPlaneJoinSubphase(kubeadmconstants.KubeControllerManager),
+			newControlPlaneJoinSubphase(kubeadmconstants.KubeScheduler),


@fabriziopandini are you OK with this ordering of sub-phases or should we create a "control-plane" subphase that holds these 3?

The creation of static manifest should not be part of control-plane-join, because already implemented in the control-plane-prepare phase (as documented in kubernetes/kubeadm#1204).
That means that the three sub phases above can go away toghether with the related code ( runControlPlaneJoinComponentSubphase method, controlPlanePhaseProperties struct, getPhaseDescription method)

@fabriziopandini removed the three subphases and its related codes

neolit123 · 2019-02-08T14:44:14Z

cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go

+	}
+}
+
+func newMarkControlPlaneSubphases() workflow.Phase {


newMarkControlPlaneSubphase without the s at the end?

neolit123 · 2019-02-08T14:44:29Z

cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go

+	return workflow.Phase{
+		Name:         "mark-control-plane",
+		Short:        "Mark a node as a control-plane",
+		Run:          runMarkControlPlanePhaseLocal,


try removing Local from the function name.

neolit123 · 2019-02-08T14:44:49Z

cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go

+	return nil
+}
+
+func runMarkControlPlanePhaseLocal(c workflow.RunData) error {


try removing Local from the name here.

neolit123 · 2019-02-08T14:45:46Z

cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go

+	return workflow.Phase{
+		Name:         controlPlanePhaseProperties[component].name,
+		Short:        controlPlanePhaseProperties[component].short,
+		Run:          runControlPlaneJoinSubphase(component),


rename this to runControlPlaneJoinComponentSubphase

removed because it is already implemented in control-plane-prepare phase

neolit123 · 2019-02-08T14:45:52Z

cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go

+	return nil
+}
+
+func runControlPlaneJoinSubphase(component string) func(c workflow.RunData) error {


rename this to runControlPlaneJoinComponentSubphase

removed because it is already implemented in control-plane-prepare phase

rosti

Thanks @RA489 !
This is a huge step in the right direction, but we need to figure a couple of details here.

rosti · 2019-02-11T13:44:25Z

cmd/kubeadm/app/cmd/join.go

-
-	return nil
+// KubeConfigPath returns the path to the kubeconfig file to use for connecting to Kubernetes
+func (j *joinData) KubeConfigPath() string {


Why do we need this one? Where is it used?

it is used in the function showControlPlaneJoinCommand in updated PR

rosti · 2019-02-11T13:46:00Z

cmd/kubeadm/app/cmd/join.go

-	return nil
+// KubeConfigPath returns the path to the kubeconfig file to use for connecting to Kubernetes
+func (j *joinData) KubeConfigPath() string {
+	j.kubeconfigPath = filepath.Join(kubeadmconstants.KubernetesDir, kubeadmconstants.AdminKubeConfigFileName)


If we are going to keep this func, let's at least not reinvent kubeadmconstants.GetAdminKubeConfigPath(). Also, I bet, that this needs to be conditioned and executed only if j.kubeconfigPath is empty.

rosti · 2019-02-11T14:07:46Z

cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go

+		}
+
+		// Skip if this is not a control plane
+		if data.Cfg().ControlPlane == nil {


Now this concerns me kind of indirectly. If I am not mistaken, data.Cfg().ControlPlane is going to be different from nil only if --experimental-control-plane is specified at the command line or it's filled in the config file. Hence, if the is no config file, the UX is going to be a bit odd, because we cannot execute the control-plane-join phase without the above flag at the command line.

The mandatory command line without config file would look something like this:

kubeadm join --experimental-control-plane phase control-plane-join ...

I believe, that we need to fix this.

@fabriziopandini @neolit123 WDYT?

Yes, this is the case, but as @fabriziopandini pointed out this is already taken care by the control plane prepare phase (and needs the flag if no config file is provided).

cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go

rosti · 2019-02-11T14:40:09Z

cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go

+		return errors.New("control-plane-join phase invoked with an invalid data struct")
+	}
+
+	kubeConfigFile := filepath.Join(kubeadmconstants.KubernetesDir, kubeadmconstants.AdminKubeConfigFileName)


Actually, it's even more complicated than that. The user may supply kubeconfig file in data.Cfg().Discovery.File.KubeConfigPath or via the --discovery-file command line switch. In that case we need to take those into account too.

@neolit123 @fabriziopandini WDYT?

fabriziopandini

@RA489 thanks for this PR!
The overall approach looks fine, but some changes are required before moving on with this effort:

creating manifest is out of the scope of this PR/this phases
all the code currently in Run should find a new home after refactor

If something is not clear, let me know!

fabriziopandini · 2019-02-11T14:54:35Z

cmd/kubeadm/app/cmd/join.go

-	return nil
+// KubeConfigPath returns the path to the kubeconfig file to use for connecting to Kubernetes
+func (j *joinData) KubeConfigPath() string {
+	j.kubeconfigPath = filepath.Join(kubeadmconstants.KubernetesDir, kubeadmconstants.AdminKubeConfigFileName)


Might be I'm missing something, but I don't understand why we should manage joinData. kubeconfigPath when this is always set to a constant value...

it is called in showControlPlaneJoinCommand function

might be i am missing something but i need it in showControlPlaneJoinCommand in updated PR

fabriziopandini · 2019-02-11T15:07:17Z

cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go

+			},
+			newControlPlaneJoinSubphase(kubeadmconstants.KubeAPIServer),
+			newControlPlaneJoinSubphase(kubeadmconstants.KubeControllerManager),
+			newControlPlaneJoinSubphase(kubeadmconstants.KubeScheduler),


The creation of static manifest should not be part of control-plane-join, because already implemented in the control-plane-prepare phase (as documented in kubernetes/kubeadm#1204).
That means that the three sub phases above can go away toghether with the related code ( runControlPlaneJoinComponentSubphase method, controlPlanePhaseProperties struct, getPhaseDescription method)

cmd/kubeadm/app/cmd/join.go

RA489 · 2019-02-12T12:36:59Z

@fabriziopandini updated the PR PTAL.

RA489 · 2019-02-13T09:04:47Z

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

fabriziopandini

@RA489 thanks for being so collaborative in addressing comments!
Few nits and one little misunderstanding to be addressed on the management of the final error, and then this is ready for lgtm

cmd/kubeadm/app/cmd/join.go

fabriziopandini · 2019-02-14T15:31:33Z

cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go

+		return errors.New("control-plane-join phase invoked with an invalid data struct")
+	}
+
+	kubeConfigFile := filepath.Join(kubeadmconstants.KubernetesDir, kubeadmconstants.AdminKubeConfigFileName)


I was forgetting the addition of the --kubeconfig flag, please change this into
kubeConfigFile := j.KubeConfigFile()
(the same apply to all the other subphases)

RA489 · 2019-02-14T16:00:02Z

@fabriziopandini Thanks for reviewing! I will update your suggestions ASAP.

RA489 · 2019-02-15T08:34:55Z

/test pull-kubernetes-e2e-gce

RA489 · 2019-02-15T14:40:05Z

@fabriziopandini updated the PR as per your suggestion. Please let me know if anything missed incorporating. Thanks!

RA489 · 2019-02-18T05:17:06Z

/test pull-kubernetes-integration

fabriziopandini

@RA489
I'm trying to test your changes but when I execute kubeadm join I got

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x13d2003]

goroutine 1 [running]:
k8s.io/kubernetes/cmd/kubeadm/app/cmd.NewCmdJoin.func1(0xc0001bef00, 0xc0000aaff0, 0x1, 0x5)
	cmd/kubeadm/app/cmd/join.go:165 +0xc3
k8s.io/kubernetes/vendor/github.com/spf13/cobra.(*Command).execute(0xc0001bef00, 0xc0000aafa0, 0x5, 0x5, 0xc0001bef00, 0xc0000aafa0)
	vendor/github.com/spf13/cobra/command.go:760 +0x2cc
k8s.io/kubernetes/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0xc0003a0280, 0xc0000caa00, 0xc0003a1180, 0xc00012c0a0)
	vendor/github.com/spf13/cobra/command.go:846 +0x2fd
k8s.io/kubernetes/vendor/github.com/spf13/cobra.(*Command).Execute(0xc0003a0280, 0xc00000c010, 0x1904b00)
	vendor/github.com/spf13/cobra/command.go:794 +0x2b
k8s.io/kubernetes/cmd/kubeadm/app.Run(0xc000084180, 0x960)
	cmd/kubeadm/app/kubeadm.go:50 +0x202
main.main()
	cmd/kubeadm/kubeadm.go:29 +0x33

Could you kindly check?

cmd/kubeadm/app/cmd/join.go

fabriziopandini

@RA489 Looks great!
I have tested both join worker and join control-plane with phases, thank you to all the people who contributed to this effort!

/approve
@neolit123 @ereslibre @rosti could you kindly give final check + lgtm

ereslibre

Thank you! Final nit to keep structs as small as possible with only the attributes they need, and almost ready for LGTM.

cmd/kubeadm/app/cmd/join.go

rosti

Thanks @RA489 !
We are on the final stretch here.

cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go

cmd/kubeadm/app/cmd/join.go

neolit123 · 2019-02-18T22:41:56Z

@RA489 please address the latest comments and mark the conversations as resolved (there is a button under each one).

if this PR can be merged this week, great, as this will give us more time for auditing.

fabriziopandini · 2019-02-19T14:07:31Z

All the comments seems to be addressed!
Let's get this merged and then we will eventually iterate after auditing as suggested by @neolit123
/approve
/lgtm

k8s-ci-robot · 2019-02-19T14:07:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, RA489

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]

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 requested review from luxas and neolit123 January 29, 2019 06:45

fabriziopandini mentioned this pull request Jan 31, 2019

Expose the kubeadm join workflow as phases kubernetes/kubeadm#1204

Closed

13 tasks

RA489 changed the title ~~WIP: kubeadm: graduate control plane join phase~~ kubeadm: graduate control plane join phase Feb 1, 2019

k8s-ci-robot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 1, 2019

yagonobre reviewed Feb 5, 2019

View reviewed changes

cmd/kubeadm/app/cmd/phases/controlplanejoin.go Outdated Show resolved Hide resolved

neolit123 reviewed Feb 6, 2019

View reviewed changes

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2019

neolit123 reviewed Feb 8, 2019

View reviewed changes

cmd/kubeadm/app/cmd/join.go Show resolved Hide resolved

neolit123 reviewed Feb 8, 2019

View reviewed changes

rosti reviewed Feb 11, 2019

View reviewed changes

fabriziopandini suggested changes Feb 11, 2019

View reviewed changes

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 12, 2019

fabriziopandini suggested changes Feb 14, 2019

View reviewed changes

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 17, 2019

fabriziopandini suggested changes Feb 18, 2019

View reviewed changes

fabriziopandini reviewed Feb 18, 2019

View reviewed changes

cmd/kubeadm/app/cmd/join.go Outdated Show resolved Hide resolved

fabriziopandini reviewed Feb 18, 2019

View reviewed changes

cmd/kubeadm/app/cmd/join.go Outdated Show resolved Hide resolved

fabriziopandini approved these changes Feb 18, 2019

View reviewed changes

ereslibre suggested changes Feb 18, 2019

View reviewed changes

cmd/kubeadm/app/cmd/join.go Outdated Show resolved Hide resolved

rosti reviewed Feb 18, 2019

View reviewed changes

cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go Outdated Show resolved Hide resolved

cmd/kubeadm/app/cmd/join.go Outdated Show resolved Hide resolved

cmd/kubeadm/app/cmd/join.go Outdated Show resolved Hide resolved

kubeadm: graduate control plane join phase

6bbed9f

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 19, 2019

k8s-ci-robot assigned fabriziopandini Feb 19, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 19, 2019

k8s-ci-robot merged commit 2911e86 into kubernetes:master Feb 19, 2019

BenTheElder mentioned this pull request Feb 20, 2019

kubernetes master branch is broken kubernetes-sigs/kind#321

Closed

neolit123 mentioned this pull request Aug 19, 2020

Regression - etcd datadir permissions not set on etcd grow kubernetes/kubeadm#2256

Closed

kubeadm: graduate control plane join phase #73452

kubeadm: graduate control plane join phase #73452

Conversation

RA489 commented Jan 29, 2019

k8s-ci-robot commented Jan 29, 2019

RA489 commented Jan 29, 2019

yagonobre left a comment

Choose a reason for hiding this comment

neolit123 left a comment

Choose a reason for hiding this comment

RA489 commented Feb 7, 2019

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

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

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

rosti 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

Choose a reason for hiding this comment

fabriziopandini left a comment • edited

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

RA489 commented Feb 12, 2019

RA489 commented Feb 13, 2019

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini Feb 14, 2019 • edited

Choose a reason for hiding this comment

RA489 commented Feb 14, 2019

RA489 commented Feb 15, 2019

RA489 commented Feb 15, 2019

RA489 commented Feb 18, 2019

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini left a comment

Choose a reason for hiding this comment

ereslibre left a comment

Choose a reason for hiding this comment

rosti left a comment

Choose a reason for hiding this comment

neolit123 commented Feb 18, 2019

fabriziopandini commented Feb 19, 2019 • edited

k8s-ci-robot commented Feb 19, 2019

fabriziopandini left a comment •

edited

fabriziopandini Feb 14, 2019 •

edited

fabriziopandini commented Feb 19, 2019 •

edited