-
Notifications
You must be signed in to change notification settings - Fork 39.2k
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.
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
kubeadm: Move .NodeName and .CRISocket to a common sub-struct #64210
kubeadm: Move .NodeName and .CRISocket to a common sub-struct #64210
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nits then lgtm
|
||
// ExtraArgs passes through extra arguments to the kubelet. The arguments here are passed to the kubelet command line via the environment file | ||
// kubeadm writes at runtime for the kubelet to source. | ||
ExtraArgs map[string]string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is the Kubelet ExtraArgs it's not entirely clear from a developer UX.
For example:
NodeRegistrationOptions.ExtraArgs
vs.
KubeletRegistrationOptions.ExtraArgs or
NodeRegistrationOptions.KubeletExtraArgs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change this after we've discussed the naming with @fabriziopandini in the SIG meeting
|
||
// PatchNode tries to patch a node using the following client, executing patchFn for the actual mutating logic | ||
func PatchNode(client clientset.Interface, nodeName string, patchFn func(*v1.Node)) error { | ||
// Loop on every falsy return. Return with an error if raised. Exit successfully if true is returned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
false return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -186,3 +192,49 @@ func CreateOrUpdateClusterRoleBinding(client clientset.Interface, clusterRoleBin | |||
} | |||
return nil | |||
} | |||
|
|||
// PatchNode tries to patch a node using the following client, executing patchFn for the actual mutating logic | |||
func PatchNode(client clientset.Interface, nodeName string, patchFn func(*v1.Node)) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you patch b/c dynamic kubelet config is not default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this is used when e.g. setting the master label/taint, and generically modifying the Node object.
@@ -136,6 +129,27 @@ type API struct { | |||
BindPort int32 | |||
} | |||
|
|||
// NodeRegistrationOptions holds fields that relate to registering a new master or node to the cluster, either via "kubeadm init" or "kubeadm join" | |||
type NodeRegistrationOptions struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are considering convergence towards cluster API, the corresponding object is named machineSpec
.
WDYT? it is too early for starting to unify naming?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should stick with something custom like NodeRegistrationOptions
for now, mostly because of the ExtraArgs
field, assuming it's going to be replaced with a config map eventually (i.e. graduate out of using args?). at a later point we can start converging towards the MachineSpec
.
2c.
@@ -56,7 +56,8 @@ func ValidateMasterConfiguration(c *kubeadm.MasterConfiguration) field.ErrorList | |||
allErrs = append(allErrs, ValidateCertSANs(c.Etcd.ServerCertSANs, field.NewPath("etcd").Child("serverCertSANs"))...) | |||
allErrs = append(allErrs, ValidateCertSANs(c.Etcd.PeerCertSANs, field.NewPath("etcd").Child("peerCertSANs"))...) | |||
allErrs = append(allErrs, ValidateAbsolutePath(c.CertificatesDir, field.NewPath("certificatesDir"))...) | |||
allErrs = append(allErrs, ValidateNodeName(c.NodeName, field.NewPath("nodeName"))...) | |||
allErrs = append(allErrs, ValidateNodeName(c.NodeRegistration.Name, field.NewPath("nodeRegistration").Child("name"))...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about creating validateNodeRegistrationOptions
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS. It seems that ValidateNodeConfiguration
doesn't validate node name and crisocket... could you kindly fix it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is WIP, thanks for keeping me honest fixing this as well
// empty slice, i.e. `taints: {}` in the YAML file. This field is solely used for Node registration. | ||
Taints []v1.Taint | ||
|
||
// ExtraArgs passes through extra arguments to the kubelet. The arguments here are passed to the kubelet command line via the environment file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about adding a note that explain difference between extra args and KubeletConfiguration (machine specific vs cluster wide)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good point 👍
b66dfe3
to
8bcbc1e
Compare
As per slack discussion we move on with this PR as it is. Small bits will follow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Please open an issue to determine long term plan for translating <> machinespec.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini, luxas, timothysc 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 |
[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process @fabriziopandini @luxas @timothysc Pull Request Labels
|
/retest Review the full test history for this PR. Silence the bot with an |
Automatic merge from submit-queue (batch tested with PRs 64322, 64210, 64458, 64232, 64370). If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
Regroups some common fields for
kubeadm init
andkubeadm join
only used for the initial node registration.Lets the user specify ExtraArgs to the kubelet.
Now also runs the dynamic env file creation for
kubeadm join
.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes kubernetes/kubeadm#847
Follows-up #63887
Related to kubernetes/kubeadm#822
Special notes for your reviewer: WIP, but please review so we can finalize the direction of the PR
Release note:
@kubernetes/sig-cluster-lifecycle-pr-reviews @liztio