WIP: Run e2e for multiple K8s versions - #218
Draft
sebastian-pf9 wants to merge 8 commits into
Draft
Conversation
sebastian-pf9
changed the title
Plus a generated pict matrix. Not wired into anything yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
.ci/generate-e2e-matrix.sh converts test/e2e/pict/generated-matrix.tsv into a GitHub Actions strategy.matrix.include JSON array, enriching each row with the concrete GINKGO_FOCUS/suite/env vars its Scenario implies so the workflow's test job stays a generic dispatcher instead of duplicating per-scenario logic 10 times. .github/workflows/e2e-matrix.yml wires it up: workflow_dispatch-only for now (not the default PR gate, so its CI-time/job-count impact can be measured first), a build job that builds the controller-manager image and agent .deb bundle once and shares them via artifacts across every matrix case (the same pattern e2e.yml already uses against build-controller-manager.yml/build-agent-bundle.yml, just within this one workflow since those two don't trigger on workflow_dispatch), and a 10-case test job (one per Scenario, from the model's own order-1 coverage) that runs make test-e2e or make test-packaging per row. PackagingDeb/PackagingRpm get their own matrix cases too, which needed a small Makefile change: test-packaging now accepts PACKAGING_GINKGO_FOCUS (mirroring test-e2e's existing GINKGO_FOCUS), threaded through via go test's -args -ginkgo.focus flag (verified Ginkgo v2 registers that flag for plain `go test`, not just the ginkgo CLI). Known inefficiency this introduces: the Rocky/byoh-node images now build twice (once per Packaging case) instead of once -- acceptable for this first gated, measurement-focused pass per the plan; worth revisiting once real numbers exist. No upgrade-target column exists in the model (UpgradeToVersion was dropped as 100%-determined) -- generate-e2e-matrix.sh hardcodes it to v1.31.2 for the two upgrade scenarios, matching those tests' own literal default, exactly where the model's comment said this decision belongs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Verified directly: Ginkgo v2's -ginkgo.focus reports "SUCCESS!" and exits 0 when it matches zero specs. This script hardcodes a Scenario -> GINKGO_FOCUS mapping; if a Describe() tag in a spec file is ever renamed without updating this script, the corresponding matrix case would silently run 0 specs and report green forever -- prompted by checking https://www.brokenpip3.com/posts/2024-25-02-ginkgo-github-actions-matrix/, whose own "focus values must be globally unique" gotcha is the same underlying fragility (GINKGO_FOCUS trusting a hand-maintained string against source that can drift out from under it), just the inverse failure direction. Now greps test/e2e/(packaging/)*.go for each generated focus value's literal text before emitting the matrix, refusing to generate one if anything's stale. Verified both directions: passes clean today, and fails loudly when a tag is deliberately renamed (reverted after confirming). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Running the old fixed e2e/packaging jobs on every PR is wasted CI spend while e2e-matrix.yml is being evaluated as their replacement -- both would otherwise run in parallel indefinitely with no plan to compare them on a schedule. Still dispatchable manually if needed; re-add the triggers if e2e-matrix.yml doesn't pan out. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…review workflow_dispatch can't dispatch a workflow that doesn't exist on the default branch yet -- no way to get a real execution of a brand-new workflow_dispatch workflow before it merges any other way. pull_request runs use the PR branch's own file content directly, so this gets a real signal on this exact PR without needing anything on main first, and without affecting any other already-open PR (each PR triggers off its own branch's workflow files). To revert before merge, restoring the intended permanent workflow_dispatch-only state. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
sebastian-pf9
force-pushed
the
claude/pr-6-pict-unified-model-5ba832
branch
from
August 21, 2026 07:41
e7aaf87 to
685a57d
Compare
Confirmed by actually running e2e-matrix.yml against this PR (with the temporary pull_request trigger): ByoHCtl, UpgradeCluster, and UpgradeClusterClass all failed identically, before a single spec ran -- "unexpected Kubernetes version format \"\"" in e2e_suite_test.go:275's SynchronizedBeforeSuite, which unconditionally builds a local k8s bundle for KUBERNETES_VERSION regardless of GINKGO_FOCUS. clusterctl's GetVariableOrEmpty (unlike this repo's own getEnvOrDefault, used for E2E_K8S_VERSION_FROM/_TO) treats an explicitly-empty env var as set rather than falling back to its default, so leaving kubernetes_version blank for those three rows actively broke the shared suite setup instead of no-op'ing. Now defaults kubernetes_version to v1.31.0 (this repo's own literal default, so behavior is unchanged from "not overridden at all") for any suite=="e2e" row whose Scenario doesn't otherwise need a specific version. suite=="packaging" rows are unaffected -- confirmed they already passed in the same run, since that suite has no SynchronizedBeforeSuite dependency at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GitHub auto-names a matrix job by concatenating every field in its object, so Join's job title leaked its GINKGO_FOCUS value verbatim -- "[PR-Blocking]", the pre-existing, unrelated Describe() tag from e2e_test.go (predates this PICT work; it's what the old default e2e.yml job used to select as its one PR-gating spec). In this new gated, optional matrix that reads as if the job were blocking something it isn't. generate-e2e-matrix.sh now computes a clean label per row (scenario, and the k8s version for suite=="e2e" rows), and the workflow's test job uses it via name:. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The CI run caught it: jq 1.6 (this repo's CI runners' version) fails to parse \$label as a --arg/variable reference at all -- confirmed in isolation, e.g. \`jq -n --arg label 1 '\$label'\` alone errors with "unexpected label, expecting IDENT" on jq 1.6, unrelated to whether the resulting object *key* is named "label" (that part was always fine, quoted or not -- the previous commit's fix was aimed at the wrong target). jq's \`label \$out | ...\`/\`break \$out\` control-flow keyword makes \$label itself unparseable, independent of context. Renamed the shell variable and --arg to job_label; the JSON field stays "label". Verified against jq 1.6 directly (via a plain ubuntu:22.04 container, matching what this repo's CI runners have) rather than only this Mac's jq, which didn't reproduce the bug at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Collaborator
Author
|
@srm6867 + @indradhanush : What do you think? Increasing the coverage is leading to quadratic time consumption. running via microsoft pict is reducing the amount of tests to run. See the e2e-matrix/test jobs here. I plan to reduce the number of jobs. Asking about the general approach |
indradhanush
left a comment
indradhanush
left a comment
Collaborator
There was a problem hiding this comment.
I like this direction. It seems like this becomes much easier to maintain over time.
Sidenotes:
- We'll eventually need to invest in formatting of the generated-matrix.tsv to be column aligned and sorted, but this is not a problem for now. Just noting because I had to do this locally to kind of understand what we want to do
- re:
I plan to reduce the number of jobs
More jobs is good right, because we can get a CI result quicker? Actual CI minutes used is still the same?
Comment on lines
+246
to
+253
| # /o:1 (each value covered at least once) instead of PICT's default /o:2 | ||
| # (every pair covered): each row is a full e2e run, and with only one | ||
| # real parameter besides Scenario today, pairwise coverage of exactly two | ||
| # parameters is the same thing as the full cross product -- no | ||
| # combinatorial saving over a naive nested loop, just more rows. Revisit | ||
| # once model.pict grows a third free parameter (e.g. OS, once a second | ||
| # real image exists), where pairwise actually starts saving cases. | ||
| PICT_OPTS = /o:1 |
Collaborator
There was a problem hiding this comment.
So maybe we run with /o:2 or /o:3 for commits merged to main but a leaner set for PRs? Is that how you're thinking of this?
Collaborator
Author
There was a problem hiding this comment.
for example, or /o:2 before releases.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.