kubeadm cleanup: master -> control-plane (cont.2) #74064

vanduc95 · 2019-02-14T08:02:41Z

This PR continuation of #73987

Does this PR introduce a user-facing change?:

NONE

/assign @neolit123

k8s-ci-robot · 2019-02-14T08:02:48Z

Hi @vanduc95. 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.

neolit123

thanks @vanduc95

neolit123 · 2019-02-14T14:11:29Z

cmd/kubeadm/app/phases/upgrade/postupgrade.go

-		// as we handle that ourselves in the markmaster phase
-		// TODO: Maybe we want to do that some time in the future, in order to remove some logic from the markmaster phase?
+		// Write env file with flags for the kubelet to use. We do not need to write the --register-with-taints for the control-plane,
+		// as we handle that ourselves in the markcontrolplane phase


mark-control-plane

neolit123 · 2019-02-14T14:11:37Z

cmd/kubeadm/app/phases/upgrade/postupgrade.go

-		// TODO: Maybe we want to do that some time in the future, in order to remove some logic from the markmaster phase?
+		// Write env file with flags for the kubelet to use. We do not need to write the --register-with-taints for the control-plane,
+		// as we handle that ourselves in the markcontrolplane phase
+		// TODO: Maybe we want to do that some time in the future, in order to remove some logic from the markcontrolplane phase?


mark-control-plane

neolit123 · 2019-02-14T14:14:22Z

cmd/kubeadm/app/util/config/cluster.go

@@ -59,7 +59,7 @@ func FetchInitConfigurationFromCluster(client clientset.Interface, w io.Writer,

 // getInitConfigurationFromCluster is separate only for testing purposes, don't call it directly, use FetchInitConfigurationFromCluster instead
 func getInitConfigurationFromCluster(kubeconfigDir string, client clientset.Interface, newControlPlane bool) (*kubeadmapi.InitConfiguration, error) {
-	// TODO: This code should support reading the MasterConfiguration key as well for backwards-compat
+	// TODO: This code should support reading the ClusterConfiguration key as well for backwards-compat


please remove this comment.
the MasterConfiguration key no longer exists in our code base.

neolit123 · 2019-02-14T14:16:55Z

/kind cleanup
/priority backlog
/ok-to-test

yagonobre

Thanks @vanduc95
/lgtm

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

cmd/kubeadm/app/constants/constants.go

@@ -143,7 +143,7 @@ const (
 	// the TLS bootstrap to get itself an unique credential


vanduc95 there are a lot more Master entries in this file.
we need to change code as well not only comments! :)

neolit123 · 2019-02-15T14:13:18Z

cmd/kubeadm/app/constants/constants.go

@@ -191,7 +191,7 @@ const (
 	// Default behaviour is 24 hours
 	DefaultTokenDuration = 24 * time.Hour

-	// LabelNodeRoleMaster specifies that a node is a master
+	// LabelNodeRoleMaster specifies that a node is a control-plane
 	// This is a duplicate definition of the constant in pkg/controller/service/service_controller.go
 	LabelNodeRoleMaster = "node-role.kubernetes.io/master"


this is an exception keep it like LabelNodeRoleMaster.
[1]

@vanduc95
i should have deleted the above comment next to deleting the similar one.

also i'm not sure whether we should keep this or change to LabelNodeRoleMaster at this point.
mostly leaning towards change.

but let's wait for more comments...

@rosti @kubernetes/sig-cluster-lifecycle-pr-reviews

We need to keep it like LabelNodeRoleMaster. There is currently a constant named in the same way and used for the same purpose in pkg/controller/service/service_controller.go and the idea is at some point to move that constant to a common place (where we won't import a huge package just for it).
Until then, it's best to keep the constant names in sync.

ok, keeping this like so.

neolit123 · 2019-02-15T14:14:25Z

cmd/kubeadm/app/constants/constants.go

@@ -350,18 +350,18 @@ const (
 	// DefaultAPIServerBindAddress is the default bind address for the API Server
 	DefaultAPIServerBindAddress = "0.0.0.0"

-	// MasterNumCPU is the number of CPUs required on master
+	// MasterNumCPU is the number of CPUs required on control-plane
 	MasterNumCPU = 2


ControlPlaneNumCPU

atoato88

If others are in favor of this, please take into account my review comments.

cmd/kubeadm/app/util/config/initconfiguration.go

vanduc95 · 2019-02-19T09:18:24Z

Thanks for your review @atoato88.

rosti

Thanks @vanduc95 !
We have to be careful with some constants though. They were copied locally to avoid importing large external packages and they must retain their old names as they are used throughout k8s sources.

rosti · 2019-02-19T13:19:14Z

cmd/kubeadm/app/constants/constants.go

 	// This is a duplicate definition of the constant in pkg/controller/service/service_controller.go
-	LabelNodeRoleMaster = "node-role.kubernetes.io/master"
+	LabelNodeRoleControlPlane = "node-role.kubernetes.io/master"


Don't change this one. It's a local copy of an external constant, that will eventually be removed and pointed again to the external reference.

rosti · 2019-02-19T13:20:51Z

cmd/kubeadm/app/constants/constants.go

 	// (i.e. bound to the cluster-admin ClusterRole)
-	MastersGroup = "system:masters"
+	ControlPlaneGroup = "system:masters"


This must be changed to SystemPrivilegedGroup

vanduc95 · 2019-02-20T02:17:11Z

Thanks @rosti !
This PR has been updated.

rosti

Thanks @vanduc95 !
/lgtm

neolit123

@vanduc95
please rebase to fix merge conflicts.
/approve

vanduc95 · 2019-02-21T01:27:09Z

Thanks @neolit123, I fixed merge conflicts.

neolit123 · 2019-02-21T01:36:26Z

cmd/kubeadm/app/preflight/checks.go

@@ -871,7 +871,7 @@ func (ncc NumCPUCheck) Check() (warnings, errorList []error) {
 	return warnings, errorList
 }

-// RunInitMasterChecks executes all individual, applicable to Master node checks.
+// RunInitMasterChecks executes all individual, applicable to master node checks.
 func RunInitMasterChecks(execer utilsexec.Interface, cfg *kubeadmapi.InitConfiguration, ignorePreflightErrors sets.String) error {


looks like we missed this one...
could you please change it too?
RunInitMasterChecks -> RunInitNodeChecks

yes @neolit123,
I just modified variables, constants and function names under kubeadm/app/constants package. In addition, I also modified in the files which have reference to constants in kubeadm/app/constants.
I will change other package in next PR. Thanks

ok, sounds good.
thanks for that!

/lgtm

Ok @neolit123, I changed :)

lol, I just updated PR :D

👍 let's wait to see if CI passes and will LGTM again.

neolit123 · 2019-02-21T01:36:53Z

cmd/kubeadm/app/preflight/checks.go

@@ -871,7 +871,7 @@ func (ncc NumCPUCheck) Check() (warnings, errorList []error) {
 	return warnings, errorList
 }

-// RunInitMasterChecks executes all individual, applicable to Master node checks.
+// RunInitMasterChecks executes all individual, applicable to master node checks.


master -> control-plane.

vanduc95 · 2019-02-21T06:22:55Z

/test pull-kubernetes-e2e-gce

neolit123

thanks @vanduc95

/lgtm
/approve

k8s-ci-robot · 2019-02-21T14:38:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, vanduc95

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~~ [neolit123]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

neolit123 · 2019-02-21T14:52:43Z

/retest

neolit123 · 2019-02-21T16:12:08Z

/retest

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Feb 14, 2019

k8s-ci-robot assigned neolit123 Feb 14, 2019

k8s-ci-robot requested review from kad and yagonobre February 14, 2019 08:03

neolit123 reviewed Feb 14, 2019

View reviewed changes

vanduc95 force-pushed the cleanup-kubeadm-cont.2-20190214 branch 2 times, most recently from f7b0f2a to f4eca14 Compare February 14, 2019 23:57

k8s-ci-robot assigned yagonobre Feb 14, 2019

yagonobre reviewed Feb 14, 2019

View reviewed changes

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

neolit123 reviewed Feb 15, 2019

View reviewed changes

atoato88 reviewed Feb 19, 2019

View reviewed changes

cmd/kubeadm/app/util/config/initconfiguration.go Outdated Show resolved Hide resolved

cmd/kubeadm/app/util/config/initconfiguration.go Outdated Show resolved Hide resolved

vanduc95 force-pushed the cleanup-kubeadm-cont.2-20190214 branch from 59895d6 to 0643b7e Compare February 19, 2019 09:14

rosti suggested changes Feb 19, 2019

View reviewed changes

vanduc95 force-pushed the cleanup-kubeadm-cont.2-20190214 branch from 0643b7e to 5d649ff Compare February 20, 2019 02:18

rosti approved these changes Feb 20, 2019

View reviewed changes

k8s-ci-robot assigned rosti Feb 20, 2019

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

neolit123 approved these changes Feb 20, 2019

View reviewed changes

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

vanduc95 force-pushed the cleanup-kubeadm-cont.2-20190214 branch from 5d649ff to 8640c17 Compare February 21, 2019 01:29

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

neolit123 reviewed Feb 21, 2019

View reviewed changes

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

vanduc95 force-pushed the cleanup-kubeadm-cont.2-20190214 branch from 8640c17 to 51eb01d Compare February 21, 2019 02:10

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

kubeadm cleanup: master -> control-plane (cont.2)

ae1ec88

vanduc95 force-pushed the cleanup-kubeadm-cont.2-20190214 branch from 51eb01d to ae1ec88 Compare February 21, 2019 03:04

neolit123 reviewed Feb 21, 2019

View reviewed changes

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

k8s-ci-robot merged commit 739af85 into kubernetes:master Feb 21, 2019

xichengliudui mentioned this pull request Apr 22, 2019

Change master to the control-plane knative/serving#3843

Closed

atoato88 mentioned this pull request Apr 26, 2019

REQUEST: New membership for atoato88 kubernetes/org#771

Closed

6 tasks

		@@ -143,7 +143,7 @@ const (
		// the TLS bootstrap to get itself an unique credential

kubeadm cleanup: master -> control-plane (cont.2) #74064

kubeadm cleanup: master -> control-plane (cont.2) #74064

Conversation

vanduc95 commented Feb 14, 2019

k8s-ci-robot commented Feb 14, 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

neolit123 commented Feb 14, 2019

yagonobre left a comment

Choose a reason for hiding this comment

neolit123 Feb 15, 2019 • edited

Choose a reason for hiding this comment

neolit123 Feb 15, 2019 • edited

Choose a reason for hiding this comment

neolit123 Feb 20, 2019 • 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

atoato88 left a comment

Choose a reason for hiding this comment

vanduc95 commented Feb 19, 2019

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

vanduc95 commented Feb 20, 2019

rosti left a comment

Choose a reason for hiding this comment

neolit123 left a comment

Choose a reason for hiding this comment

vanduc95 commented Feb 21, 2019

neolit123 Feb 21, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanduc95 commented Feb 21, 2019

neolit123 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 21, 2019

neolit123 commented Feb 21, 2019

neolit123 commented Feb 21, 2019

neolit123 Feb 15, 2019 •

edited

neolit123 Feb 15, 2019 •

edited

neolit123 Feb 20, 2019 •

edited

neolit123 Feb 21, 2019 •

edited