Adding Helm Parameter CI Tests by mdzraf · Pull Request #2868 · kubernetes-sigs/aws-ebs-csi-driver

mdzraf · 2026-02-17T18:09:18Z

What type of PR is this?

/kind feature

What is this PR about? / Why do we need it?

This PR adds individual tests for the various helm parameters that can be configured when deploying the driver.

How was this change tested?

Tested manually by running:

mkdir -p /tmp/e2e-results && export REPORT_DIR=/tmp/e2e-results  && make e2e/parameters-all

Also ran in CI under the existing single-az testing grid. It will have it's own test grid after this merges but put under single-az for reviewers to see results and JUnit outputs.

Does this PR introduce a user-facing change?

NONE

k8s-ci-robot · 2026-02-17T18:09:21Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

k8s-ci-robot · 2026-02-17T18:09:28Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

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

github-actions · 2026-02-17T18:11:26Z

Code Coverage Diff

This PR does not change the code coverage

mdzraf · 2026-02-17T18:23:14Z

/test pull-aws-ebs-csi-driver-e2e-single-az

mdzraf · 2026-02-17T18:27:14Z

/test pull-aws-ebs-csi-driver-verify

mdzraf · 2026-02-17T20:27:48Z

/test pull-aws-ebs-csi-driver-e2e-single-az
/test pull-aws-ebs-csi-driver-verify

mdzraf · 2026-02-17T20:33:54Z

hack/e2e/param-sets.sh

+param_set_legacy-compat() {
+  GINKGO_FOCUS="\[param:(useOldCSIDriver|legacyXFS)\]"
+  HELM_EXTRA_FLAGS="--set=useOldCSIDriver=true,node.legacyXFS=true"
+}
+
+param_set_selinux() {
+  GINKGO_FOCUS="\[param:selinux\]"
+  HELM_EXTRA_FLAGS="--set=node.selinux=true"
+}


These are not in the PARAMETERS_ALL test for now, I will see if they need a separate cluster or not.

mdzraf · 2026-02-17T20:53:24Z

/hold

Have to reverse single-az makefile target before merging

mdzraf · 2026-02-17T21:06:59Z

/test pull-aws-ebs-csi-driver-e2e-single-az
/test pull-aws-ebs-csi-driver-verify

mdzraf · 2026-02-18T14:21:07Z

Noticed some tests are being skipped, looking into it.

ElijahQuinones · 2026-02-18T19:59:37Z

hack/e2e/param-sets.sh

+#   standard            - Behavioral params (tagging, metrics, logging, storage classes, etc.)
+#   other               - Volume modification, volume attach limit, and metadata labeler
+#   debug               - debugLogs=true overrides individual logLevel settings
+#   infra               - Infrastructure/deployment params (resources, security, strategy, etc.)


Why is storage classes in standard and not in infra it is a cluster resource

Also why are we dividing these into categories at all ? If we end up running them all anyways

torredil · 2026-02-27T14:22:10Z

Makefile

+# THIS WILL BE REVERSED BEFORE MERGING. Only here because these tests do not 
+# yet have their own testgrid but reviewers need to see how JUnit output is after tests run for this PR. 
 .PHONY: e2e/single-az
 e2e/single-az: bin/helm bin/ginkgo
-	AWS_AVAILABILITY_ZONES=us-west-2a \
-	TEST_PATH=./tests/e2e/... \
-	GINKGO_FOCUS="\[ebs-csi-e2e\] \[single-az\]" \
-	GINKGO_PARALLEL=5 \
-	HELM_EXTRA_FLAGS="--set=controller.volumeModificationFeature.enabled=true,sidecars.provisioner.additionalArgs[0]='--feature-gates=VolumeAttributesClass=true',sidecars.resizer.additionalArgs[0]='--feature-gates=VolumeAttributesClass=true',node.enableMetrics=true" \
-	./hack/e2e/run.sh
+	./hack/e2e/param-sets.sh run-all 


Thanks for leaving a comment about it, I'm including it in the review so that we don't forget to revert this before the merge.

torredil · 2026-02-27T14:38:56Z

hack/e2e/param-sets.sh

+# Merge per-set JUnit XMLs into a single file with duplicate skipped tests removed.
+# Each Ginkgo run reports ALL specs (most as skipped), so the same skipped test appears
+# in every per-set file. This merges all results into one file, keeping non-skipped results
+# (passed/failed) over skipped duplicates, and emitting each skipped test only once.
+merge_junit_results() {
+  local report_dir="${REPORT_DIR:-/logs/artifacts}"
+  local output="${report_dir}/junit-params.xml"
+
+  python3 - "$report_dir" "$output" <<'PYEOF'
+import glob, sys, xml.etree.ElementTree as ET


I'd drop this merge script and let the JUnit files stand on their own.

Testgrid handles multiple files per job fine, and keeping them separate actually helps debugging since you can tell which param set a failure came from just by the filename. The skipped test noise is a not an issue,(the external test jobs already report 7000+ skips), but if we want to clean it up properly, the right move is putting these parameter tests in their own package so Ginkgo only discovers the specs we care about.

torredil · 2026-02-27T15:01:21Z

hack/e2e/param-sets.sh

+  fi
+}
+
+# Run all standard parameter sets sequentially


Why are param sets running sequentially? can we parallelize?

Another thing, most of these tests don't need a live cluster. Things like "does the controller deployment have 3 replicas" or "does the node daemonset have this toleration" are assertions about rendered kubernetes objects, not runtime behavior. We could test the vast majority with helm template + assertions on the rendered YAML.

torredil · 2026-02-27T15:12:47Z

hack/e2e/param-sets.sh

+param_set_standard() {
+  GINKGO_FOCUS="\[param:(extraCreateMetadata|k8sTagClusterId|extraVolumeTags|controllerMetrics|nodeMetrics|batching|defaultFsType|controllerLoggingFormat|nodeLoggingFormat|controllerLogLevel|nodeLogLevel|provisionerLogLevel|attacherLogLevel|snapshotterLogLevel|resizerLogLevel|nodeDriverRegistrarLogLevel|storageClasses|volumeSnapshotClasses|defaultStorageClass|snapshotterForceEnable|controllerUserAgentExtra|controllerEnablePrometheusAnnotations|nodeEnablePrometheusAnnotations|nodeKubeletPath|nodeTolerateAllTaints|controllerPodDisruptionBudget|provisionerLeaderElection|attacherLeaderElection|resizerLeaderElection|reservedVolumeAttachments|hostNetwork|nodeDisableMutation|nodeTerminationGracePeriod|nodeAllocatableUpdatePeriodSeconds)\]"
+  HELM_EXTRA_FLAGS="--set=controller.extraCreateMetadata=true,controller.k8sTagClusterId=e2e-param-test,controller.extraVolumeTags.TestKey=TestValue,controller.enableMetrics=true,node.enableMetrics=true,controller.batching=true,controller.defaultFsType=xfs,controller.loggingFormat=json,node.loggingFormat=json,controller.logLevel=5,node.logLevel=5,sidecars.provisioner.logLevel=5,sidecars.attacher.logLevel=5,sidecars.snapshotter.logLevel=5,sidecars.resizer.logLevel=5,sidecars.nodeDriverRegistrar.logLevel=5,defaultStorageClass.enabled=true,storageClasses[0].name=test-sc,storageClasses[0].parameters.type=gp3,volumeSnapshotClasses[0].name=test-vsc,volumeSnapshotClasses[0].deletionPolicy=Delete,sidecars.snapshotter.forceEnable=true,controller.userAgentExtra=e2e-test,controller.enablePrometheusAnnotations=true,node.enablePrometheusAnnotations=true,node.kubeletPath=/var/lib/kubelet,node.tolerateAllTaints=true,controller.podDisruptionBudget.enabled=true,sidecars.provisioner.leaderElection.enabled=true,sidecars.attacher.leaderElection.enabled=true,sidecars.resizer.leaderElection.enabled=true,node.reservedVolumeAttachments=2,node.hostNetwork=true,node.serviceAccount.disableMutation=true,node.terminationGracePeriodSeconds=60,nodeAllocatableUpdatePeriodSeconds=30"


Say someone adds a new helm value controller.foo=bar, here's what they have to do:

Write the go test with the right [param:foo] tag

Figure out which param set it belongs in (standard? infra? other? new one?)

Add foo to the GINKGO_FOCUS regex for that param set

Add controller.foo=bar to the HELM_EXTRA_FLAGS string for that param set

Hardcode "bar" in the go test assertion and hope it matches what they put in step 4

Hope they didn't typo anything in this massive 1000+ character strings

we need to simplify this, and have a single source of truth for what to deploy and what to assert. I suggest defining the test values once, in one place, such as by using a values yaml file per param set and have the go tests read expected values from it (or a shared const).

k8s-ci-robot requested review from ConnorJC3 and torredil February 17, 2026 18:09

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 17, 2026

mdzraf removed request for ConnorJC3 and torredil February 17, 2026 18:10

mdzraf changed the title ~~Adding Helm Parameter CI Tests~~ [WIP] Adding Helm Parameter CI Tests Feb 17, 2026

mdzraf force-pushed the AddHelmParamCITests branch from b3ad7d8 to b074ab6 Compare February 17, 2026 18:16

mdzraf force-pushed the AddHelmParamCITests branch from b074ab6 to 3334fde Compare February 17, 2026 20:27

mdzraf commented Feb 17, 2026

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 Feb 17, 2026

mdzraf force-pushed the AddHelmParamCITests branch from 3334fde to 976d61c Compare February 17, 2026 21:06

mdzraf changed the title ~~[WIP] Adding Helm Parameter CI Tests~~ Adding Helm Parameter CI Tests Feb 17, 2026

mdzraf marked this pull request as ready for review February 17, 2026 21:11

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 17, 2026

k8s-ci-robot requested review from AndrewSirenko and torredil February 17, 2026 21:11

ElijahQuinones reviewed Feb 18, 2026

View reviewed changes

mdzraf force-pushed the AddHelmParamCITests branch 2 times, most recently from 21ccf1e to 8437c58 Compare February 19, 2026 17:11

Adding Helm Parameter CI Tests

68a45c5

mdzraf force-pushed the AddHelmParamCITests branch from 8437c58 to 68a45c5 Compare February 19, 2026 17:40

torredil reviewed Feb 27, 2026

View reviewed changes

Conversation

mdzraf commented Feb 17, 2026

What type of PR is this?

What is this PR about? / Why do we need it?

How was this change tested?

Does this PR introduce a user-facing change?

Uh oh!

k8s-ci-robot commented Feb 17, 2026

Uh oh!

k8s-ci-robot commented Feb 17, 2026

Uh oh!

github-actions bot commented Feb 17, 2026

Code Coverage Diff

Uh oh!

mdzraf commented Feb 17, 2026

Uh oh!

mdzraf commented Feb 17, 2026

Uh oh!

mdzraf commented Feb 17, 2026

Uh oh!

mdzraf Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

mdzraf commented Feb 17, 2026

Uh oh!

mdzraf commented Feb 17, 2026

Uh oh!

mdzraf commented Feb 18, 2026

Uh oh!

ElijahQuinones Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

ElijahQuinones Feb 18, 2026 • 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!

torredil Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

torredil Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

torredil Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

torredil Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ElijahQuinones Feb 18, 2026 •

edited

Loading