kubeadm: mark-control-plane phase #70625
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
the
needs-ok-to-test
|
@RA489: Reiterating the mentions to trigger a notification: In response to this:
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. |
k8s-ci-robot
added
cncf-cla: yes
|
/release-note-none |
k8s-ci-robot
added
release-note-none
|
/priority important-soon |
k8s-ci-robot
added
priority/important-soon
cmd/kubeadm/app/cmd/init.go
Outdated
| @@ -169,6 +168,8 @@ func NewCmdInit(out io.Writer) *cobra.Command { | |||
| initRunner.AppendPhase(phases.NewControlPlanePhase()) | |||
| initRunner.AppendPhase(phases.NewEtcdPhase()) | |||
| initRunner.AppendPhase(phases.NewWaitControlPlanePhase()) | |||
| initRunner.AppendPhase(phases.NewmarkcontrolplanePhase()) | |||
There was a problem hiding this comment.
NewmarkcontrolplanePhase -> NewMarkControlPlanePhase
| @@ -25,68 +25,65 @@ import ( | |||
| "k8s.io/kubernetes/cmd/kubeadm/app/cmd/options" | |||
| cmdutil "k8s.io/kubernetes/cmd/kubeadm/app/cmd/util" | |||
| kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants" | |||
| markmasterphase "k8s.io/kubernetes/cmd/kubeadm/app/phases/markmaster" | |||
| "k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow" | |||
| markcontrolplanephase "k8s.io/kubernetes/cmd/kubeadm/app/phases/markmaster" | |||
There was a problem hiding this comment.
we should rename the file to markcontrolplane to avoid the aliasing.
| kubeadm alpha phase mark-master | ||
|
|
||
| markcontrolplaneExample = normalizer.Examples(` | ||
| # Run master markcontrolplane checks using a config file. |
There was a problem hiding this comment.
Applies control-plane label and taint to the current node, functionally equivalent to what is executed by kubeadm init.
|
|
||
| markcontrolplaneExample = normalizer.Examples(` | ||
| # Run master markcontrolplane checks using a config file. | ||
| kubeadm init phase mark-controlplane --config config.yml |
|
|
||
| # Applies master label and taint to a specific node | ||
| kubeadm alpha phase mark-master --node-name myNode | ||
| kubeadm init phase mark-controlplane --node-name myNode |
| }, | ||
| fmt.Println("[markcontrolplane] running markcontrolplane checks") | ||
| var cfgPath string | ||
| kubeConfigFile := kubeadmconstants.GetAdminKubeConfigPath() |
There was a problem hiding this comment.
here we define the kubeconfig dir that also respects dry run:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/cmd/init.go#L366
you can also expose KubeConfigDir() for the data object in this file too.
this line would end up something like:
kubeConfigFile := filepath.Join(data.KubeConfig() , kubeadmconstants.AdminKubeConfigFileName)
| err = markmasterphase.MarkMaster(client, internalcfg.NodeRegistration.Name, internalcfg.NodeRegistration.Taints) | ||
| kubeadmutil.CheckErr(err) | ||
| }, | ||
| fmt.Println("[markcontrolplane] running markcontrolplane checks") |
There was a problem hiding this comment.
[markcontrolplane] running markcontrolplane checks
->
[mark-control-plane] Running checks
| fmt.Println("[markcontrolplane] running markcontrolplane checks") | ||
| var cfgPath string | ||
| kubeConfigFile := kubeadmconstants.GetAdminKubeConfigPath() | ||
| kubeConfigFile = cmdutil.FindExistingKubeConfig(kubeConfigFile) |
There was a problem hiding this comment.
note to self: seems to me like we are not doing this in other places, where we should be doing it.
possibly best to move it in init.go
| options.AddKubeConfigFlag(cmd.Flags(), &kubeConfigFile) | ||
| cmd.Flags().StringVar(&cfgPath, "config", cfgPath, "Path to kubeadm config file. WARNING: Usage of a configuration file is experimental") | ||
| cmd.Flags().StringVar(&cfg.NodeRegistration.Name, "node-name", cfg.NodeRegistration.Name, `The node name to which label and taints should apply`) | ||
| if err := markmasterphase.MarkMaster(client, internalcfg.NodeRegistration.Name, internalcfg.NodeRegistration.Taints); err != nil { |
There was a problem hiding this comment.
i think it's ok to rename the MarkMaster function to MarkControlPlane as part of this refector.
basically master->controlPlane everywhere.
| return cmd | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
please add new line at end of file.
|
/assign @fabriziopandini |
k8s-ci-robot
added
size/L
| // NewMarkControlPlanePhase creates a kubeadm workflow phase that implements mark-controlplane checks. | ||
| func NewMarkControlPlanePhase() workflow.Phase { | ||
| return workflow.Phase{ | ||
| Name: "markcontrolplane", |
There was a problem hiding this comment.
let's name it like so for consistency with other phases:
mark-control-plane
| func NewMarkControlPlanePhase() workflow.Phase { | ||
| return workflow.Phase{ | ||
| Name: "markcontrolplane", | ||
| Short: "Run master markcontrolplane checks", |
There was a problem hiding this comment.
Mark a node as a control-plane
| return workflow.Phase{ | ||
| Name: "markcontrolplane", | ||
| Short: "Run master markcontrolplane checks", | ||
| Long: "Run master markcontrolplane checks, applies master label.", |
There was a problem hiding this comment.
let's remove Long: here given Short fits on one line.
|
|
||
| fmt.Println("[mark-control-plane] Running checks") | ||
| var cfgPath string | ||
| kubeConfigFile := filepath.Join(data.KubeConfig() , kubeadmconstants.AdminKubeConfigFileName) |
There was a problem hiding this comment.
small whitespace problem.
./hack/update-gofmt.sh might help with these.
| return errors.New("mark-control-plane phase invoked with an invalid data struct") | ||
| } | ||
|
|
||
| fmt.Println("[mark-control-plane] Running checks") |
| // MarkMaster taints the master and sets the master label | ||
| func MarkMaster(client clientset.Interface, masterName string, taints []v1.Taint) error { | ||
| // MarkControlPlane taints the master and sets the master label | ||
| func MarkControlPlane(client clientset.Interface, masterName string, taints []v1.Taint) error { |
There was a problem hiding this comment.
masterName -> controlPlaneName
|
|
||
| fmt.Printf("[markmaster] Marking the node %s as master by adding the label \"%s=''\"\n", masterName, constants.LabelNodeRoleMaster) | ||
| fmt.Printf("[mark-control-plane] Marking the node %s as master by adding the label \"%s=''\"\n", masterName, constants.LabelNodeRoleMaster) |
There was a problem hiding this comment.
Marking the node %s as control-plane by adding the label
|
|
||
| if taints != nil && len(taints) > 0 { | ||
| taintStrs := []string{} | ||
| for _, taint := range taints { | ||
| taintStrs = append(taintStrs, taint.ToString()) | ||
| } | ||
| fmt.Printf("[markmaster] Marking the node %s as master by adding the taints %v\n", masterName, taintStrs) | ||
| fmt.Printf("[mark-control-plane] Marking the node %s as master by adding the taints %v\n", masterName, taintStrs) |
There was a problem hiding this comment.
Marking the node %s as a control-plane by adding the taints
|
/ok-to-test @RA489 as a helper, please make sure you have run these: and also include the ./docs changes after running: thanks! |
k8s-ci-robot
removed
the
needs-ok-to-test
There was a problem hiding this comment.
@RA489 thanks for this PR! Well done!
Please fix test, address few small nits and then ping me again after removing WIP
|
|
||
| var cfgPath string | ||
| kubeConfigFile := filepath.Join(data.KubeConfig(), kubeadmconstants.AdminKubeConfigFileName) | ||
| kubeConfigFile = cmdutil.FindExistingKubeConfig(kubeConfigFile) |
There was a problem hiding this comment.
The call to FindExistingKubeConfig should be removed and instead we should use data.Kubeconfig
@neolit123 the call of FindExistingKubeConfig should be removed by all phases, because it can make kubeadm use unexpected kubeconfig files while instead we need admin.conf.
There was a problem hiding this comment.
ok we had a discussion about this - @RA489 please remove the FindExistingKubeConfig call and use:
kubeConfigFile := data.KubeConfigPath()
| if err != nil { | ||
| return err | ||
| } | ||
| internalcfg, err := configutil.ConfigFileAndDefaultsToInternalConfig(cfgPath, data.Cfg()) |
There was a problem hiding this comment.
The call to ConfigFileAndDefaultsToInternalConfig is not necessary (it is already called by initData). Please remove
You can pass data.Cfg() directly to MarkControlPlane
| @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and | |||
| limitations under the License. | |||
| */ | |||
|
|
|||
| package markmaster | |||
| package markcontrolplane | |||
There was a problem hiding this comment.
Please kindly rename also file and folder
RA489
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
@RA489 I guess build is failing because hack/update-bazel.sh should be run after renaming files |
|
the previous phase just merged and init has a small conflict. |
| # Applies control-plane label and taint to the current node, functionally equivalent to what is executed by kubeadm init. | ||
| kubeadm init phase mark-control-plane --config config.yml | ||
|
|
||
| # Applies master label and taint to a specific node |
There was a problem hiding this comment.
Is it correct to change "master label" with "control-plane label"?
k8s-ci-robot
added
needs-rebase
|
@RA489 it seems another rebase is necessary to fix bazel-build errors... |
|
we need to merge this today, ideally. |
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
added
the
sig/scheduling
k8s-ci-robot
removed
the
needs-rebase
|
@RA489 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: RA489 If they are not already assigned, you can assign the PR to them by writing 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:
Approvers can indicate their approval by writing |
|
/close |
|
@neolit123: Closed this PR. In response to this:
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: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
What type of PR is this?
This PR implement mark-control-plane phase in the kubeadm init workflow.
/kind feature
What this PR does / why we need it:
Ref kubernetes/kubeadm#1163
/sig cluster-lifecycle
@kubernetes/sig-cluster-lifecycle-pr-reviews
Release note:
NONE