-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feature: support kubeadm.k8s.io/v1beta4 #3675
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ls-2018 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @ls-2018! |
Hi @ls-2018. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
@@ -480,6 +480,161 @@ conntrack: | |||
{{end}}{{end}} | |||
` | |||
|
|||
// ConfigTemplateBetaV4 is the kubeadm config template for API version v1beta4 | |||
const ConfigTemplateBetaV4 = `# config generated by kind | |||
apiVersion: kubeadm.k8s.io/v1beta4 |
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.
any changes we need to be aware of ...?
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.
Here are some of the changes, such as :
- When I redeploy the cluster using kind, I need to pass some env to apiserver, which is not supported by the current beta3 version, but is supported by beta4. Of course, the template I provide in pr may not cover all scenarios, which will need to be added later
- When trace is enabled on apiserver and authentication is required on the back end, some information needs to be specified in env
v1beta3
// ControlPlaneComponent holds settings common to control plane component of the cluster
type ControlPlaneComponent struct {
// ExtraArgs is an extra set of flags to pass to the control plane component.
// A key in this map is the flag name as it appears on the
// command line except without leading dash(es).
// TODO: This is temporary and ideally we would like to switch all components to
// use ComponentConfig + ConfigMaps.
// +optional
ExtraArgs map[string]string `json:"extraArgs,omitempty"`
// ExtraVolumes is an extra set of host volumes, mounted to the control plane component.
// +optional
ExtraVolumes []HostPathMount `json:"extraVolumes,omitempty"`
}
v1beta4
// ControlPlaneComponent holds settings common to control plane component of the cluster
type ControlPlaneComponent struct {
// ExtraArgs is an extra set of flags to pass to the control plane component.
// A key in this map is the flag name as it appears on the
// command line except without leading dash(es).
// TODO: This is temporary and ideally we would like to switch all components to
// use ComponentConfig + ConfigMaps.
// +optional
ExtraArgs map[string]string `json:"extraArgs,omitempty"`
// ExtraVolumes is an extra set of host volumes, mounted to the control plane component.
// +optional
ExtraVolumes []HostPathMount `json:"extraVolumes,omitempty"`
// ExtraEnvs is an extra set of environment variables to pass to the control plane component.
// Environment variables passed using ExtraEnvs will override any existing environment variables, or *_proxy environment variables that kubeadm adds by default.
// +optional
ExtraEnvs []corev1.EnvVar `json:"extraEnvs,omitempty"`
}
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.
When I redeploy the cluster using kind, I need to pass some env to apiserver, which is not supported by the current beta3 version, but is supported by beta4. Of course, the template I provide in pr may not cover all scenarios, which will need to be added later
There's no question that we will adopt v1beta4, my question is are there any breaking changes or other changes we should be aware of when adopting 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.
I didn't find anything else
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.
the above v1beta4 struct is not correct in terms of extra args for v1beta4. in the new versions they are a slice of key-value pairs. breaking changes are enumerated here:
https://groups.google.com/g/kubernetes-sig-cluster-lifecycle/c/9HgtTf0W1hk
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.
Changing extraArgs like this makes sense but a lot of kind configs will need to start targeting specific kubeadm versions when we roll this out because they've been patching extraArgs portably across versions as a subset of the config patched
@ls-2018 that includes
Line 97 in 00d659b
create_cluster() { |
which will be somewhat complicated.
node-labels: "{{ .NodeLabels }}" | ||
{{ if .InitSkipPhases -}} | ||
skipPhases: | ||
{{ range $phase := .InitSkipPhases -}} |
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.
#3693 Will this issue occur again?
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.
yeah, we need to fix this here too
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.
@dominicqi thank you for your notice.
I think we'll need to take a little bit more care when switching here, maybe we'll target kind switching to this for 1.32+ instead after e.g. our own scripts are updated to handle this ... (where possible we prefer to gate breaking changes against kubernetes versions users couldn't have been using yet) |
So we need to soon, but we can afford to think a bit about how to communicate the extraArgs change and rework various CI scripts (not just the one in this repo, we're likely to break a lot of other CI in the project when we turn this on, other subprojects have their own scripts) |
ccc723e
to
d3e1e52
Compare
I think that using v1beta4 should not affect the tools I previously wrote. They do not have any new features. If later tools need to test the new features of v1beta4, they can be tested through patches. I added a case, I'm not sure if everyone can accept this. |
the extra args change will certainly be disruptive. based on feedback we might keep v1beta3 for longer than 1 year. |
Signed-off-by: acejilam <[email protected]>
Do you have any good ideas about changing args from map to slice. |
if you have your v1beta3 config on disk as JSON/YAML the guidance is to use the |
Er, see https://github.com/kubernetes-sigs/kind/pull/3675/files#r1714482152 When we start generating for v1beta4 by default, any script attempting to patch extra args has to start patching the v1beta4 format. Also, previously those scripts could get away with not specifying the kubeadm config API version and letting the trivial patch target all versions. Now they will need to do version targeting patches. That includes the e2e script in this repo to start. /ok-to-test |
It's also a powerful change, and non-version-specific patches were just a shortcut, however kind will be responsible for when v1beta4 transition becomes required for its users. I think the currently pending release to go along with 1.31 has enough changes to absorb as-is and 1.31 just released so we'll probably have to move this to target 1.32 |
@ls-2018: 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-sigs/prow repository. I understand the commands that are listed here. |
@ls-2018 would it be better to rebase this onto 1.32.0-alpha.0 in order to fix the remaining tests? |
prow already rebases, the failures are not related to the versions
|
@ls-2018 with this information, could you fix the code or tests? |
Need to wait for the release