✨ Initialize JoinConfiguration if missing #234
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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 |
k8s-ci-robot
added
approved
fabriziopandini
force-pushed
the
initialize-configobjects-if-missing
branch
from
b41e9c2
to
6d6589b
Compare
k8s-ci-robot
removed
the
needs-rebase
|
@fabriziopandini In our use cases, we use a template to generate bootstrap configs for all control plane machines. Hence, these configs all contains initconfiguration, clusterconfiguration and joinconfiguration. If adding the requirements in your PR, it will make the work of setting kubeadmconfig objects more manual. |
|
@accepting here a thread explaining why it was decided to make this change |
fabriziopandini
force-pushed
the
initialize-configobjects-if-missing
branch
from
6d6589b
to
d7fdceb
Compare
|
I'm really torn on this PR I'm leaning towards not being restrictive to allow more use cases, but I'm concerned about how people are using this. I don't think I have enough information about what is more common, using duplicated configs across all machines or being specific about which machine will be the initial control plane and which will be a secondary control plane. I'll talk to some people this week and see if I can get some consensus on this. Any additional thoughts are welcome. Please see the associated issue for more details. The TL;DR is: Should we allow users to supply all configs to every machine and let the controller figure it out? or should we be prescriptive to prevent accidental user errors and nice error messages. |
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
For this PR can we have just the initializing a join configuration if one doesn't exist and leave the other checks out for now? That way our behavior doesn't change and we can close #213. |
|
/retitle ✨ Initialize config objects if missing |
k8s-ci-robot
changed the title
|
|
||
| //In case of join, ClusterConfiguration and InitConfiguration should not be defined by users | ||
| if config.Spec.ClusterConfiguration != nil || config.Spec.InitConfiguration != nil { | ||
| return ctrl.Result{}, errors.New("Control plane already exists for the cluster, KubeadmConfig objects with ClusterConfiguration or InitConfiguration can not be processed at this stage") |
There was a problem hiding this comment.
It might be good to also emit an event here as well.
There was a problem hiding this comment.
If we return an error here, it means the KubeadmConfig will be reconciled again using backoff. We probably don't want to return an error.
There was a problem hiding this comment.
I asked @fabriziopandini to not make these changes as they are controversial and reduce this PR to fix the original issue of #213
fabriziopandini
force-pushed
the
initialize-configobjects-if-missing
branch
from
d7fdceb
to
4de256f
Compare
|
@chuckha
|
fabriziopandini
changed the title
| log.Info("Creating default JoinConfiguration") | ||
| config.Spec.JoinConfiguration = &kubeadmv1beta1.JoinConfiguration{} | ||
| if util.IsControlPlaneMachine(machine) { | ||
| config.Spec.JoinConfiguration.ControlPlane = &kubeadmv1beta1.JoinControlPlane{} |
There was a problem hiding this comment.
Shouldn't this be dependent on if it is a ControlPlane instance or a Worker Node instance?
There was a problem hiding this comment.
i think you missed line 270's if check
k8s-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #213