e2e-k8s.sh drop CLUSTER_LOG_FORMAT and add versioned kubeadm config patches #4046

BenTheElder · 2025-11-03T22:38:22Z

x-ref kubernetes/test-infra#35846 #3847

k8s-ci-robot · 2025-11-03T22:38:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder

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:

~~OWNERS~~ [BenTheElder]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

BenTheElder · 2025-11-03T23:08:34Z

/retest

BenTheElder · 2025-11-04T02:43:24Z

/retest

BenTheElder · 2025-11-04T03:17:48Z

/retest

BenTheElder · 2025-11-04T16:25:42Z

/retest

…atches

BenTheElder · 2025-11-04T23:00:04Z

Bit concerned that the non-default logging flags are loadbearing but that's not new to this PR ...

pohly · 2025-11-05T06:54:21Z

hack/ci/e2e-k8s.sh

 kubeadmConfigPatches:
+# v1beta4 for the future (v1.35.0+ ?)
+# https://github.com/kubernetes-sigs/kind/issues/3847
+# TODO: drop v1beta3 when we no longer need versions that use it


Can you add some comments about container-log-max-files and container-log-max-size? As we learned, they are required by at least one job. This is valuable information that we should preserve for the next person who needs to edit this file. Something like:

Suggested change

# TODO: drop v1beta3 when we no longer need versions that use it

# TODO: drop v1beta3 when we no longer need versions that use it

#

# container-log-max-files and container-log-max-size affect overall

# performance and resource requirements of jobs. Extra care is

# necessary before changing them.

Hmm, I suppose, but this script is frequently copied wholesale as the basis for new scripts / jobs where that may not be relevant at all. I'm currently working on cleaning up many of those scripts and some of them don't set these already. I think the real issue is this job is barely succeeding with insufficient resources and/or noisy neighbor issues.

This may or may not be true next time it is edited even for the one job, but we that is why we run an extensive set of critical jobs based on this script to any PR to this repo.

Either way, a follow up goal is to eliminate these non-default values, it appears they were desired in the context of testing structured logging but got added to all configurations.

Perhaps a more detailed comment explaining that you should probably drop this from any derivative scripts, but I doubt on the other hand anyone will read it.

it appears they were desired in the context of testing structured logging

Now I am curious, because I don't think they were added for that.

The settings got added in #3774 because of #3772, on behalf of @aojea. ~~Then later it was updated in https://github.com/kubernetes-sigs/kind/pull/3774/files.~~ Different commit, same PR.

I've not been involved in any of this, so I lack context. You had some concerns about not testing defaults.

My takeaway from this is (once again) that explaining changes in GitHub PRs is not sufficient. It must be documented in the code for the next person who has to make changes to the file. It might even be the same person.

I'll have more time to dig and update later

I added a warning

Warning looks good, thanks!

Now only https://github.com/kubernetes-sigs/kind/pull/4046/files#r2501870491 is open as far as I can tell.

BenTheElder · 2025-11-06T22:57:18Z

/cc @neolit123 @pacoxu

neolit123

extra args diff lgtm
/lgtm
/hold

neolit123 · 2025-11-07T06:43:53Z

hack/ci/e2e-k8s.sh

+  kind: ClusterConfiguration
+  apiVersion: kubeadm.k8s.io/v1beta4
+  metadata:
+    name: config


note that running strict validation will report this. there is no objectmeta on the kubeadm kinds.

IIRC this is optionally used to match the object in the patch runtime and stripped out before reaching kubeadm to avoid this

kind/pkg/cluster/internal/create/actions/config/config.go

Lines 244 to 255 in b2b7266

// trims out the metadata.name we put in the config for kustomize matching,

// kubeadm will complain about this otherwise

func removeMetadata(kustomized string) string {

return strings.Replace(

kustomized,

`metadata:

name: config

`,

"",

-1,

)

}

IIRC kustomize required these fields to match patches, so we have the code to post-process dropping them, but kind's own patch runtime doesn't require this IIRC (we should keep the post-process bit for compatibility)

Yeah, our internal simplified patch runtime only matches on kind/version, it doesn't support having multiple named objects anyhow, so we can just drop this here but also all the examples ... I will leave the examples for another PR

(will also leave strict validation to another PR)

k8s-ci-robot · 2025-11-07T23:31:16Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 3, 2025

k8s-ci-robot requested review from aojea and stmcginnis November 3, 2025 22:38

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 3, 2025

BenTheElder force-pushed the no-log-format branch from c46b15a to d530442 Compare November 3, 2025 22:42

BenTheElder mentioned this pull request Nov 3, 2025

WIP: kubeadm v1beta4 #4040

Open

BenTheElder mentioned this pull request Nov 4, 2025

prepare kind configs for kubeadm v1beta4 kubernetes/kubernetes#135016

Merged

pohly mentioned this pull request Nov 4, 2025

add limited support for version aware containerd config patching #4042

Open

e2e-k8s.sh drop CLUSTER_LOG_FORMAT and add versioned kubeadm config p…

9d469f5

…atches

BenTheElder force-pushed the no-log-format branch from d530442 to 9d469f5 Compare November 4, 2025 21:06

pohly reviewed Nov 5, 2025

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 6, 2025

k8s-ci-robot requested review from neolit123 and pacoxu November 6, 2025 22:57

neolit123 reviewed Nov 7, 2025

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 7, 2025

k8s-ci-robot assigned neolit123 Nov 7, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2025

add warning to container-log* flags

56265ef

BenTheElder force-pushed the no-log-format branch from 5c8126c to 56265ef Compare November 7, 2025 23:31

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2025

-# TODO: drop v1beta3 when we no longer need versions that use it
+# TODO: drop v1beta3 when we no longer need versions that use it
+#
+# container-log-max-files and container-log-max-size affect overall
+# performance and resource requirements of jobs. Extra care is
+# necessary before changing them.

	// trims out the metadata.name we put in the config for kustomize matching,
	// kubeadm will complain about this otherwise
	func removeMetadata(kustomized string) string {
	return strings.Replace(
	kustomized,
	`metadata:
	name: config
	`,
	"",
	-1,
	)
	}

e2e-k8s.sh drop CLUSTER_LOG_FORMAT and add versioned kubeadm config patches #4046

Are you sure you want to change the base?

e2e-k8s.sh drop CLUSTER_LOG_FORMAT and add versioned kubeadm config patches #4046

Uh oh!

Conversation

BenTheElder commented Nov 3, 2025

Uh oh!

k8s-ci-robot commented Nov 3, 2025

Uh oh!

BenTheElder commented Nov 3, 2025

Uh oh!

BenTheElder commented Nov 4, 2025

Uh oh!

BenTheElder commented Nov 4, 2025

Uh oh!

BenTheElder commented Nov 4, 2025

Uh oh!

BenTheElder commented Nov 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pohly Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pohly Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenTheElder commented Nov 6, 2025

Uh oh!

neolit123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Nov 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pohly Nov 5, 2025 •

edited

Loading

pohly Nov 5, 2025 •

edited

Loading