Skip to content

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

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.

Sign up for GitHub

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

Open
BenTheElder wants to merge 2 commits into
base: main
Choose a base branch
from
+61 −43
Open

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9d469f5
e2e-k8s.sh drop CLUSTER_LOG_FORMAT and add versioned kubeadm config p…
BenTheElder Nov 3, 2025
56265ef
add warning to container-log* flags
BenTheElder Nov 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 61 additions & 43 deletions hack/ci/e2e-k8s.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,47 +82,11 @@ build() {
export PATH="${PWD}/_output/bin:$PATH"
}

check_structured_log_support() {
case "${KUBE_VERSION}" in
v1.1[0-8].*)
echo "$1 is only supported on versions >= v1.19, got ${KUBE_VERSION}"
exit 1
;;
esac
}

# up a cluster with kind
create_cluster() {
# Grab the version of the cluster we're about to start
KUBE_VERSION="$(docker run --rm --entrypoint=cat "kindest/node:latest" /kind/version)"

# Default Log level for all components in test clusters
KIND_CLUSTER_LOG_LEVEL=${KIND_CLUSTER_LOG_LEVEL:-4}

# potentially enable --logging-format
CLUSTER_LOG_FORMAT=${CLUSTER_LOG_FORMAT:-}
scheduler_extra_args=" \"v\": \"${KIND_CLUSTER_LOG_LEVEL}\""
controllerManager_extra_args=" \"v\": \"${KIND_CLUSTER_LOG_LEVEL}\""
apiServer_extra_args=" \"v\": \"${KIND_CLUSTER_LOG_LEVEL}\""
if [ -n "$CLUSTER_LOG_FORMAT" ]; then
check_structured_log_support "CLUSTER_LOG_FORMAT"
scheduler_extra_args="${scheduler_extra_args}
\"logging-format\": \"${CLUSTER_LOG_FORMAT}\""
controllerManager_extra_args="${controllerManager_extra_args}
\"logging-format\": \"${CLUSTER_LOG_FORMAT}\""
apiServer_extra_args="${apiServer_extra_args}
\"logging-format\": \"${CLUSTER_LOG_FORMAT}\""
fi
kubelet_extra_args=" \"v\": \"${KIND_CLUSTER_LOG_LEVEL}\"
\"container-log-max-files\": \"10\"
\"container-log-max-size\": \"100Mi\""
KUBELET_LOG_FORMAT=${KUBELET_LOG_FORMAT:-$CLUSTER_LOG_FORMAT}
if [ -n "$KUBELET_LOG_FORMAT" ]; then
check_structured_log_support "KUBECTL_LOG_FORMAT"
kubelet_extra_args="${kubelet_extra_args}
\"logging-format\": \"${KUBELET_LOG_FORMAT}\""
fi

# JSON or YAML map injected into featureGates config
feature_gates="${FEATURE_GATES:-{\}}"
# --runtime-config argument value passed to the API server, again as a map
Expand All @@ -146,29 +110,83 @@ nodes:
featureGates: ${feature_gates}
runtimeConfig: ${runtime_config}
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
Copy link
Contributor

@pohly pohly Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@BenTheElder BenTheElder Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@BenTheElder BenTheElder Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@pohly pohly Nov 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@pohly pohly Nov 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@BenTheElder BenTheElder Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have more time to dig and update later

Copy link
Member Author

@BenTheElder BenTheElder Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a warning

Copy link
Contributor

@pohly pohly Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning looks good, thanks!

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

Copy link
Member Author

@BenTheElder BenTheElder Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must be missing something. What's open?

Copy link
Contributor

@pohly pohly Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember what this was and the link is dead now (thanks GitHub!). Let's assume it's resolved.

- |
kind: ClusterConfiguration
apiVersion: kubeadm.k8s.io/v1beta4
apiServer:
extraArgs:
- name: "v"
value: "${KIND_CLUSTER_LOG_LEVEL}"
controllerManager:
extraArgs:
- name: "v"
value: "${KIND_CLUSTER_LOG_LEVEL}"
scheduler:
extraArgs:
- name: "v"
value: "${KIND_CLUSTER_LOG_LEVEL}"
---
kind: InitConfiguration
apiVersion: kubeadm.k8s.io/v1beta4
nodeRegistration:
kubeletExtraArgs:
- name: "v"
value: "${KIND_CLUSTER_LOG_LEVEL}"
- name: "container-log-max-files"
value: "10"
- name: "container-log-max-size"
value: "100Mi"
---
kind: JoinConfiguration
apiVersion: kubeadm.k8s.io/v1beta4
nodeRegistration:
kubeletExtraArgs:
- name: "v"
value: "${KIND_CLUSTER_LOG_LEVEL}"
# Warning: these flags appear to be load bearing / impact performance
# See: https://github.com/kubernetes-sigs/kind/pull/4046
# Be careful when updating these.
# Most CI jobs should not need them, but some CI jobs might.
- name: "container-log-max-files"
value: "10"
- name: "container-log-max-size"
value: "100Mi"
# v1beta3 for v1.23.0 ... ?
- |
kind: ClusterConfiguration
metadata:
name: config
apiVersion: kubeadm.k8s.io/v1beta3
apiServer:
extraArgs:
${apiServer_extra_args}
"v": "${KIND_CLUSTER_LOG_LEVEL}"
controllerManager:
extraArgs:
${controllerManager_extra_args}
"v": "${KIND_CLUSTER_LOG_LEVEL}"
scheduler:
extraArgs:
${scheduler_extra_args}
"v": "${KIND_CLUSTER_LOG_LEVEL}"
---
kind: InitConfiguration
apiVersion: kubeadm.k8s.io/v1beta3
nodeRegistration:
kubeletExtraArgs:
${kubelet_extra_args}
"v": "${KIND_CLUSTER_LOG_LEVEL}"
"container-log-max-files": "10"
"container-log-max-size": "100Mi"
---
kind: JoinConfiguration
apiVersion: kubeadm.k8s.io/v1beta3
nodeRegistration:
kubeletExtraArgs:
${kubelet_extra_args}
"v": "${KIND_CLUSTER_LOG_LEVEL}"
# Warning: these flags appear to be load bearing / impact performance
# See: https://github.com/kubernetes-sigs/kind/pull/4046
# Be careful when updating these.
# Most CI jobs should not need them, but some CI jobs might.
"container-log-max-files": "10"
"container-log-max-size": "100Mi"
EOF
# NOTE: must match the number of workers above
NUM_NODES=2
Expand Down
Loading