Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ls-2018
Copy link

@ls-2018 ls-2018 commented Jul 1, 2024

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ls-2018
Once this PR has been reviewed and has the lgtm label, please assign aojea for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 1, 2024
@k8s-ci-robot
Copy link
Contributor

Welcome @ls-2018!

It looks like this is your first PR to kubernetes-sigs/kind 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kind has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 1, 2024
@@ -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
Copy link
Member

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 ...?

Copy link
Author

@ls-2018 ls-2018 Jul 2, 2024

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"`
}

Copy link
Member

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.

Copy link
Author

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

Copy link
Member

@neolit123 neolit123 Jul 3, 2024

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

Copy link
Member

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

create_cluster() {

which will be somewhat complicated.

node-labels: "{{ .NodeLabels }}"
{{ if .InitSkipPhases -}}
skipPhases:
{{ range $phase := .InitSkipPhases -}}
Copy link
Contributor

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?

Copy link
Member

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

Copy link
Author

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.

@BenTheElder
Copy link
Member

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)

@BenTheElder
Copy link
Member

v1beta3 is now marked as deprecated but will continue to be supported until version 1.34 or later.

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)

@ls-2018 ls-2018 force-pushed the main branch 2 times, most recently from ccc723e to d3e1e52 Compare August 13, 2024 02:18
@ls-2018
Copy link
Author

ls-2018 commented Aug 13, 2024

@BenTheElder @neolit123

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.

@neolit123
Copy link
Member

v1beta3 is now marked as deprecated but will continue to be supported until version 1.34 or later.

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)

the extra args change will certainly be disruptive. based on feedback we might keep v1beta3 for longer than 1 year.

Signed-off-by: acejilam <acejilam@gmail.com>
@ls-2018
Copy link
Author

ls-2018 commented Aug 13, 2024

v1beta3 is now marked as deprecated but will continue to be supported until version 1.34 or later.

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)

the extra args change will certainly be disruptive. based on feedback we might keep v1beta3 for longer than 1 year.

Do you have any good ideas about changing args from map to slice.
@neolit123 , Is there any guidance available?

@neolit123
Copy link
Member

Do you have any good ideas about changing args from map to slice.
@neolit123 , Is there any guidance available?

if you have your v1beta3 config on disk as JSON/YAML the guidance is to use the kubeadm config migrate command to upgrade it to v1beta4.

@BenTheElder
Copy link
Member

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.

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

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 13, 2024
@BenTheElder
Copy link
Member

the extra args change will certainly be disruptive. based on feedback we might keep v1beta3 for longer than 1 year.

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

@k8s-ci-robot
Copy link
Contributor

@ls-2018: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kind-test 8c4b223 link true /test pull-kind-test
pull-kind-verify 8c4b223 link true /test pull-kind-verify
pull-kind-conformance-parallel-dual-stack-ipv4-ipv6 8c4b223 link true /test pull-kind-conformance-parallel-dual-stack-ipv4-ipv6
pull-kind-e2e-kubernetes 8c4b223 link true /test pull-kind-e2e-kubernetes
pull-kind-conformance-parallel-ipv6 8c4b223 link true /test pull-kind-conformance-parallel-ipv6
pull-kind-conformance-parallel-ga-only 8c4b223 link true /test pull-kind-conformance-parallel-ga-only
pull-kind-e2e-kubernetes-1-31 8c4b223 link true /test pull-kind-e2e-kubernetes-1-31

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.

@reneleonhardt
Copy link

@ls-2018 would it be better to rebase this onto 1.32.0-alpha.0 in order to fix the remaining tests?

@aojea
Copy link
Contributor

aojea commented Oct 3, 2024

prow already rebases, the failures are not related to the versions

pkg/cluster/internal/create/actions/config/config_test.go:89:2: expected ';', found g
WARN invalid TestEvent: FAIL	sigs.k8s.io/kind/pkg/cluster/internal/create/actions/config [setup failed]
bad output from test2json: FAIL	sigs.k8s.io/kind/pkg/cluster/internal/create/actions/config [setup failed]

@reneleonhardt
Copy link

@ls-2018 with this information, could you fix the code or tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants