-
Notifications
You must be signed in to change notification settings - Fork 67
📖 Update default behavior for kubernetesVersion field in the readme #261
📖 Update default behavior for kubernetesVersion field in the readme #261
Conversation
[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 |
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
@@ -145,6 +146,9 @@ CABPK will fill in some values if they are left empty with sensible defaults: | |||
|
|||
> IMPORTANT! overriding above defaults could lead to broken Clusters. | |||
|
|||
[1] if both `clusterConfiguration.KubernetesVersion` and `Machine.Spec.Version` are empty, the latest Kubernetes | |||
version will be installed (as defined by the default kubeadm behavior). |
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'm not sure we can guarantee this... Since we are using pre-baked images the version deployed would require some type of defaulting in place on the infrastructure provider side to choose the right image...
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.
kubeadm's default is stable-1
for the control plane images ever since at least 1.13. It's different before (approximately) that version and specified a specific version, but do we really need to worry about older versions?
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.
My comment was more indicating that we cannot necessarily ensure a "Kubernetes Version" from the bootstrap provider. Especially since we expect the binaries on disk to already exist in the infrastructure providers.
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.
oh, yeah, I think that's a very fair point.
We can ensure the field is set, but we cannot guarantee you'll get that kubernetes version installed.
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.
hmm but i think it should mostly be ok if kubeadm gets configured with the right version...it could get a little weird spanning large minor versions, but generally i think we'll be ok here. Just in the edge cases it could be confusing. Such as a user specifies 1.10 but uses a kubeadm v1.16. kubeadmv1.16 init
looks VERY different from a kubeadmv1.10 init
which may or may not produce the right manifests and will most certainly produce the wrong systemd unit files.
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.
Between that and the uncertainty around lookup behavior per provider, I think I would rather force that the version be declared "somewhere" rather than having odd and inconsistent behavior otherwise.
What this PR does / why we need it:
This PR updates the readme file documenting the default behavior for the kubernetesVersion field