✨ Add ExtraEnvs and ImagePullSerial to KubeadmConfig #10846
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
fabriziopandini
force-pushed
the
add-new-kubeadm-v1beta4-fields
branch
from
ef3a6e9 to
fccad65
Compare
fabriziopandini
force-pushed
the
add-new-kubeadm-v1beta4-fields
branch
from
fccad65 to
700138b
Compare
fabriziopandini
added
the
tide/merge-method-squash
fabriziopandini
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
Looks perfect, thx!! /lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: dff1454dc70fc4be1f941a9575422163f7186b92
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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
the
approved
|
|
||
| // ImagePullSerial specifies if image pulling performed by kubeadm must be done serially or in parallel. | ||
| // This option takes effect only on Kubernetes >=1.31.0. | ||
| // Default: true (defaulted in kubeadm) |
There was a problem hiding this comment.
Out of curiosity, why do we want the default to be true for this one?
There was a problem hiding this comment.
kubeadm defaults to true, I assume because it was the previous behavior
(cc @neolit123)
There was a problem hiding this comment.
yes, serial is the default in kubeadm and kubelet
| // Environment variables passed using ExtraEnvs will override any existing environment variables, or *_proxy environment variables that kubeadm adds by default. | ||
| // This option takes effect only on Kubernetes >=1.31.0. | ||
| // +optional | ||
| ExtraEnvs []EnvVar `json:"extraEnvs,omitempty"` |
There was a problem hiding this comment.
| ExtraEnvs []EnvVar `json:"extraEnvs,omitempty"` | |
| ExtraEnvs []corev1.EnvVar `json:"extraEnvs,omitempty"` |
Do we need the extra container?
There was a problem hiding this comment.
Good question. It aligns to what kubeadm did. Not sure why
(cc @neolit123)
There was a problem hiding this comment.
i actually don't remember why. will try to dig out comments.
seems there is no need for that.
should we change it pre-release 1.31?
There was a problem hiding this comment.
oh, that was a wild problem related to defaulting of corev1.ExtraEnvs using internal APIs
kubernetes/kubernetes#120561
https://github.com/kubernetes/kubernetes/pull/120561/files#diff-769d744db9aca1b60fb0f12e0c34fea8523740dccd24db5f9315b284ce1b1fefR211
kubernetes/kubeadm#2927
TL;DR a new custom wrapper struct was needed so that we can have a customer defaulter and not end up with generated defaulters that import internal k/k/pkg packages and causing weird behavior (that API machinery folks can't explain)
in CAPI you can maybe work around it, but for kubeadm we have to keep it.
There was a problem hiding this comment.
Having the same struct that kubeadm uses really helps to do comparisons cross versions (and we have many of them!)
So if there are no objections, I will keep this aligned
|
/hold for the two comments above, feel free to unhold when ready |
k8s-ci-robot
added
the
do-not-merge/hold
|
/test pull-cluster-api-e2e-main |
|
@vincepri, @neolit123 answered to both your questions (thanks!) |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
What this PR does / why we need it:
This PR adds ExtraEnvs and ImagePullSerial to KubeadmConfig; those fields have been added in kubeadm v1beta4 API, and thus they will be supported in Cluster with K8s >= 1.31
Which issue(s) this PR fixes:
Part of #10708
/area provider/bootstrap-kubeadm