Repository navigation
[ci] Add upgrade test job to release pipeline - #2401
myasnikovdaniil wants to merge 3 commits into
Conversation
Adds automated upgrade testing that runs on every tag push alongside the existing release preparation. Tests the most common upgrade path: previous latest minor release -> current version. The upgrade test provisions a 3-node Talos cluster, installs the previous stable release via its installer chart (extracted from the git tag), deploys PostgreSQL with test data, upgrades to the current version, and validates that all HelmReleases reconcile, no pods are in CrashLoopBackOff, PersistentVolumes remain Bound, PostgreSQL data survives intact, and the Cozystack API stays available. New files: - hack/e2e-upgrade-cozystack.bats: 15-step BATS test script - packages/core/testing/Makefile: upgrade-cozystack target - .github/workflows/tags.yaml: upgrade-test job (parallel with changelog) Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new upgrade-test job and end-to-end Bats suite that checks out the release branch, downloads the draft release asset, provisions a sandbox, runs an automated upgrade validation against a previous minor release, collects/uploaded reports, and always attempts sandbox and workspace teardown. Changes
Sequence Diagram(s)sequenceDiagram
participant TagPush as Tag push
participant Actions as GitHub Actions
participant GH_API as GitHub API (gh)
participant Runner as Self-hosted Runner / Sandbox
participant MakeDocker as Docker / make inside sandbox
participant Artifacts as GitHub Artifacts
TagPush->>Actions: trigger tags workflow
Actions->>Actions: start prepare-release
Actions->>Actions: start upgrade-test job
Actions->>GH_API: generate GitHub App token / fetch draft release asset
GH_API-->>Actions: return asset (nocloud-amd64.raw.xz)
Actions->>Runner: checkout release-<version>, copy workspace, compute SANDBOX_NAME
Runner->>Runner: provision sandbox (make ... prepare-env) [retries up to 3]
Runner->>MakeDocker: run upgrade test (hack/cozytest.sh hack/e2e-upgrade-cozystack.bats)
MakeDocker-->>Runner: test results, cozyreport.tgz
Runner->>Artifacts: upload cozyreport.tgz
Runner->>Runner: teardown sandbox / cleanup (always)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Four fixes surfaced by manually running the upgrade on a long-lived cluster (dev10 → v1.3.0-rc.1): 1. Use `cozystack` as the helm release name (not `installer`) to match the install command documented on the website. 2. Pull the published OCI chart (oci://ghcr.io/cozystack/cozystack/cozy-installer --version X.Y.Z) instead of a local `packages/core/installer` path, so the release pipeline actually exercises the OCI artifact end users consume. 3. Compare pre/post snapshots in Phase 4 validation (HR-not-Ready, CrashLoopBackOff pods, unbound PVs) so pre-existing noise on non-pristine clusters doesn't mask real upgrade regressions. 4. New `Verify migration state after upgrade` test — asserts the cozystack-version configmap equals the chart's migrations.targetVersion, doesn't regress, and the migration job completed when a version bump was needed. Co-Authored-By: Claude Opus 4.7 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
dosubot
Bot
added
size/XL
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the release process by implementing automated upgrade testing. By simulating a real-world upgrade scenario from the previous stable minor release, the new pipeline job ensures that platform upgrades do not introduce regressions in workload stability, data integrity, or system health. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new end-to-end upgrade test suite in hack/e2e-upgrade-cozystack.bats and a corresponding Makefile target to automate the validation of Cozystack upgrades from the previous stable minor release. The test covers workload deployment, data integrity verification for PostgreSQL, and system health checks post-upgrade. Feedback focuses on improving the robustness of the test script, including ensuring the test fails explicitly when HelmReleases do not reconcile, adding checks for empty pod variables to provide clearer error messages, refining the regex for identifying stable releases, and addressing the use of magic numbers in resource count assertions.
| if kubectl get hr -A | grep -v " True " | grep -v NAME; then | ||
| kubectl get hr -A | ||
| echo "Some HelmReleases failed to reconcile (previous version)" >&2 | ||
| fi |
There was a problem hiding this comment.
The test currently prints an error message if HelmReleases fail to reconcile, but it does not actually fail the test. This could lead to false positives where the upgrade test proceeds despite a broken initial state. You should explicitly exit with a non-zero status here.
if kubectl get hr -A | grep -v " True " | grep -v NAME; then
kubectl get hr -A
echo "Some HelmReleases failed to reconcile (previous version)" >&2
exit 1
fi
There was a problem hiding this comment.
Addressed in ae8bbd6: added a STRICT_BASELINE env switch (defaults to 0). The tags.yaml upgrade-test step sets STRICT_BASELINE=1, so Phase 1 now fails fast on reconcile errors in CI (the sandbox is always pristine). Long-lived dev clusters stay lenient.
| pg_pod=$(kubectl get pods -n tenant-test \ | ||
| -l "cnpg.io/cluster=postgres-${name}" \ | ||
| --field-selector=status.phase=Running \ | ||
| -o jsonpath='{.items[0].metadata.name}') |
There was a problem hiding this comment.
If no running PostgreSQL pods are found, pg_pod will be empty, causing the subsequent kubectl exec to fail with an unclear error message. It is better to verify that a pod was actually found before attempting to use it.
pg_pod=$(kubectl get pods -n tenant-test \
-l "cnpg.io/cluster=postgres-${name}" \
--field-selector=status.phase=Running \
-o jsonpath='{.items[0].metadata.name}')
[ -n "$pg_pod" ] || { echo "PostgreSQL pod not found"; exit 1; }
There was a problem hiding this comment.
Done in ae8bbd6. Added [ -n "$pg_pod" ] || { echo "No running PostgreSQL pod found for postgres-${name}" >&2; exit 1; } right after the pod lookup.
| pg_pod=$(kubectl get pods -n tenant-test \ | ||
| -l "cnpg.io/cluster=postgres-${name}" \ | ||
| --field-selector=status.phase=Running \ | ||
| -o jsonpath='{.items[0].metadata.name}') |
There was a problem hiding this comment.
Similar to the pre-upgrade check, if no running PostgreSQL pods are found after the upgrade, pg_pod will be empty. Adding a check here ensures the test fails with a clear message if the workload didn't recover.
pg_pod=$(kubectl get pods -n tenant-test \
-l "cnpg.io/cluster=postgres-${name}" \
--field-selector=status.phase=Running \
-o jsonpath='{.items[0].metadata.name}')
[ -n "$pg_pod" ] || { echo "PostgreSQL pod not found after upgrade"; exit 1; }
There was a problem hiding this comment.
Same guard applied post-upgrade in ae8bbd6 — errors out cleanly with "No running PostgreSQL pod found for postgres-${name} after upgrade" instead of a cryptic kubectl exec failure.
| # Latest stable (non-prerelease) tag from the previous minor line | ||
| local prev_release | ||
| prev_release=$(git tag -l "v${major}.${prev_minor}.*" --sort=-v:refname \ | ||
| | grep -v -E '-(rc|alpha|beta)\.' | head -1) |
There was a problem hiding this comment.
The current regex for excluding pre-releases only covers rc, alpha, and beta. According to SemVer, any tag containing a hyphen is considered a pre-release. Using grep -v '-' would be more robust for identifying the latest stable release in the previous minor line.
| grep -v '-' | head -1)
There was a problem hiding this comment.
Good catch. Switched to grep -v -- '-' in ae8bbd6 — any SemVer pre-release (including future -pre, -preview, custom prefixes) is now excluded correctly.
| EOF | ||
|
|
||
| # Wait until HelmReleases appear & reconcile | ||
| timeout 180 sh -ec 'until [ $(kubectl get hr -A --no-headers 2>/dev/null | wc -l) -gt 10 ]; do sleep 1; done' |
There was a problem hiding this comment.
The magic number 10 for the minimum number of HelmReleases might be fragile if the default package set changes or if a different variant is used. Consider making this check more dynamic or documenting why 10 is the expected minimum for the isp-full variant.
There was a problem hiding this comment.
Added an inline comment in ae8bbd6 explaining that 10 is a conservative floor for "the operator started reconciling" — the isp-full variant produces many more HRs, and the exact count varies by variant, so a threshold is more robust than an exact match.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
hack/e2e-upgrade-cozystack.bats (1)
122-125: Minor:grep -v NAMEis unanchored.
grep -v NAMEwould also filter any future row whose namespace or release name happens to contain the substringNAME. Anchor it to the header row (grep -vE '^NAMESPACE\s') or reuse the same awk expression used for the baseline at line 334-335 for consistency. Not a bug today since this block only emits a warning and the authoritative check is the delta comparison post-upgrade.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-upgrade-cozystack.bats` around lines 122 - 125, The pipeline unanchors the header filter in the if-block that runs `kubectl get hr -A | grep -v " True " | grep -v NAME`; replace the unanchored `grep -v NAME` with an anchored header filter such as `grep -vE '^NAMESPACE\s'` (or reuse the same `awk` expression used elsewhere in this script for baseline filtering) so only the header row is excluded and namespace/release names containing "NAME" are not accidentally filtered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tags.yaml:
- Line 243: Add the custom runner label "oracle-vm-24cpu-96gb-x86-64" to the
actionlint configuration by appending it under the self-hosted-runner.labels key
in the repo's actionlint config so actionlint recognizes this self-hosted runner
label (ensure the label string exactly matches the one used in the workflow
runs-on field and save the change so CI lint jobs stop flagging it as unknown).
In `@hack/e2e-upgrade-cozystack.bats`:
- Around line 364-369: The helm upgrade command uses a too-short --timeout 2m
which can cause flakes when the pre-upgrade Job (cozystack-migration-hook) with
backoffLimit: 3 runs; update the helm upgrade invocation that calls "helm
upgrade cozystack ... --wait --timeout 2m" to use a longer timeout (e.g.
--timeout 10m) to match the post-upgrade HR wait and avoid spurious failures.
- Around line 341-343: The fallback for writing pre-cozystack-version doesn't
handle the case where kubectl exits 0 but returns empty stdout; change the logic
around the kubectl call that writes "$STATE_DIR/pre-cozystack-version" so an
empty result is treated as "0" as well (e.g., capture the kubectl output into a
variable or test for -z and write "0"), ensuring the file never ends up empty
before the integer comparisons that use that file; update the block with the
kubectl get cm ... jsonpath command and references to
"$STATE_DIR/pre-cozystack-version" accordingly.
---
Nitpick comments:
In `@hack/e2e-upgrade-cozystack.bats`:
- Around line 122-125: The pipeline unanchors the header filter in the if-block
that runs `kubectl get hr -A | grep -v " True " | grep -v NAME`; replace the
unanchored `grep -v NAME` with an anchored header filter such as `grep -vE
'^NAMESPACE\s'` (or reuse the same `awk` expression used elsewhere in this
script for baseline filtering) so only the header row is excluded and
namespace/release names containing "NAME" are not accidentally filtered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb422ae2-4d0e-4a61-b2f7-1dbfba6e2311
📒 Files selected for processing (3)
.github/workflows/tags.yamlhack/e2e-upgrade-cozystack.batspackages/core/testing/Makefile
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Feature works as described and scope is clean. Requesting changes on four business-level points below before merge, plus closing out the outstanding bot findings.
Note: CodeRabbit's --timeout 2m concern about the migration hook is misplaced — the pre-upgrade hook lives in the platform chart, not cozy-installer, so helm upgrade cozy-installer doesn't wait on it. The other bot findings (empty pg_pod guard, pre-release regex, empty configmap value, actionlint label) are valid and should be addressed.
| # Latest stable (non-prerelease) tag from the previous minor line | ||
| local prev_release | ||
| prev_release=$(git tag -l "v${major}.${prev_minor}.*" --sort=-v:refname \ | ||
| | grep -v -E '-(rc|alpha|beta)\.' | head -1) |
There was a problem hiding this comment.
Only minor → current is covered, patch → patch is not.
For tags on a maintenance branch (e.g. v0.43.2 on release-0.43), this resolves the baseline to v0.42.latest instead of v0.43.1. The real production upgrade path a user runs on a hotfix (v0.43.1 → v0.43.2) is never exercised.
Either extend find_previous_release to prefer the previous patch on the same minor when the tag sits on a release-X.Y branch, or document this limitation in the PR body / a comment at the top of the file so reviewers know what's covered.
There was a problem hiding this comment.
Decided not to exercise patch→patch here. Patch releases are by policy scoped to bug fixes with no schema or API changes — the interesting failure modes land on minor→current upgrades where new features and migration hooks show up. We can revisit if a patch ever ships something breaking, but I'd rather not pay the test-matrix cost for paths we deliberately keep low-risk.
| console.log(`PR already exists from ${head} to ${base}`); | ||
| } | ||
|
|
||
| upgrade-test: |
There was a problem hiding this comment.
A failed upgrade-test does not gate release publication.
prepare-release creates the draft release, uploads assets, tags the api submodule, and opens the release PR BEFORE this job runs. generate-changelog and update-website-docs declare needs: [prepare-release], not needs: [upgrade-test], so they proceed regardless of the upgrade-test outcome. A human must remember to check this job's status before promoting the draft.
That may be intentional (advisory gate), but it's worth either wiring generate-changelog / update-website-docs to also depend on upgrade-test, or calling out the advisory-by-design behavior in the PR body.
There was a problem hiding this comment.
Good point — wired it up in ae8bbd6. generate-changelog and update-website-docs now have upgrade-test in their needs: and && needs.upgrade-test.result == 'success' in their if:. A failing upgrade-test leaves the draft release in place and skips changelog/website PRs, so a human decides whether to promote.
| # Phase 2: Deploy workloads and seed test data | ||
| # =================================================================== | ||
|
|
||
| @test "Deploy PostgreSQL with test data" { |
There was a problem hiding this comment.
Only PostgreSQL is exercised.
The PR body says "representative workloads" but only one workload type is covered. An upgrade that silently breaks VMs, Kubernetes-in-Kubernetes, or other managed apps would still pass this test. Fine as a first iteration; worth stating explicitly in the PR body that broader workload coverage is a follow-up.
There was a problem hiding this comment.
Broadened in ae8bbd6: Phase 2 now also deploys MariaDB (seed+verify 3 rows, mirrors the PostgreSQL canary), Kafka (cluster+topic with UID check), VMInstance (UID + phase=Running check post-upgrade), and a tenant Kubernetes cluster via Kamaji (TenantControlPlane Ready + UID). PostgreSQL stays the deep data-integrity canary; the others verify the CRD/operator path survives the upgrade without drift.
|
|
||
| if kubectl get hr -A | grep -v " True " | grep -v NAME; then | ||
| kubectl get hr -A | ||
| echo "Some HelmReleases failed to reconcile (previous version)" >&2 |
There was a problem hiding this comment.
The warning-only behavior was justified in commit 2 as "tolerate pre-existing noise on long-lived clusters", but in CI the cluster is always pristine — a broken baseline there should fail fast, not proceed silently into Phase 2.
Consider an env switch (e.g. STRICT_BASELINE=1) so the CI run is strict while dev-cluster runs stay lenient.
There was a problem hiding this comment.
Agreed. Added STRICT_BASELINE env switch in ae8bbd6 — defaults to 0 (lenient on dev clusters), and tags.yaml passes STRICT_BASELINE=1 to the CI run so a broken baseline fails fast there. Same thread as the Gemini comment above.
Broaden workload coverage, tighten correctness, and wire the release gate so a failed upgrade-test actually blocks publication. * Phase 2 / Phase 4: add MariaDB (seed+verify rows), Kafka (cluster + topic survival via UID check), VMInstance (UID + phase=Running), and tenant Kubernetes cluster (TenantControlPlane Ready + UID). Keeps PostgreSQL data integrity as the deep canary. * STRICT_BASELINE env switch: Phase 1 reconcile check fails fast in CI (always pristine), stays lenient on long-lived dev clusters. tags.yaml sets STRICT_BASELINE=1. * Release gate: generate-changelog and update-website-docs now depend on upgrade-test. A failing job leaves the draft release unpromoted. * Correctness: empty-pod guards before kubectl exec, SemVer-correct pre-release filter (grep -v -- '-'), empty-stdout coalesce for the pre-upgrade cozystack-version read, 2m -> 10m upgrade timeout to cover the cozystack-migration-hook pre-upgrade Job. * Lint: register the oracle-vm-24cpu-96gb-x86-64 self-hosted runner label in .github/actionlint.yaml so actionlint stops flagging it. Signed-off-by: Myasnikov Daniil <[email protected]>
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Prior four points (STRICT_BASELINE, release-pipeline gating, broader workload coverage, patch-to-patch scoping) are all addressed in ae8bbd6 — thanks for the turnaround.
Two new blockers on HEAD:
- The four
comm -23 <(...) <(...)constructs in the bats file will fail under dash, which is the sandbox's/bin/sh. Reproduced —cozytest.shsources the file and dash syntax-errors before the first test runs. - The asset-download curl lacks
--fail, so any 4xx/5xx writes an HTML error body into the Talos image file.
Because generate-changelog and update-website-docs now depend on upgrade-test.result == 'success', a failing upgrade-test blocks release promotion entirely — worth fixing both before merge rather than after the first tag push.
Non-blocking observations inline.
| while [ $elapsed -lt $max_wait ]; do | ||
| local new_not_ready | ||
| new_not_ready=$(comm -23 \ | ||
| <(kubectl get hr -A --no-headers 2>/dev/null | awk '$4 != "True" {print $1"/"$2}' | sort) \ |
There was a problem hiding this comment.
Process substitution <(kubectl ...) is bash-only. hack/cozytest.sh has #!/bin/sh and sources the transformed bats file via . "$TMP_SH" (line 104). The sandbox image (packages/core/testing/images/e2e-sandbox/Dockerfile, FROM ubuntu:22.04) has /bin/sh -> dash, and dash parses the whole file at source time — it syntax-errors on the <(...) lines and no test ever runs. Reproduced in a vanilla ubuntu:22.04 container:
sh: 3: /tmp/t.sh: Syntax error: "(" unexpected
Same issue affects lines 673-674, 710-711, 725-726.
Because generate-changelog and update-website-docs now carry needs.upgrade-test.result == 'success', a permanently-failing upgrade-test blocks the whole release pipeline on every tag.
Two fixes:
- Switch
hack/cozytest.shshebang to#!/bin/bash(minimal). - Or rewrite the four blocks using temp files, e.g.:
kubectl get hr -A --no-headers 2>/dev/null \ | awk '$4 != "True" {print $1"/"$2}' | sort > "$STATE_DIR/cur-hr-not-ready" new_not_ready=$(comm -23 "$STATE_DIR/cur-hr-not-ready" "$pre_not_ready")
| echo "nocloud-amd64.raw.xz not found in draft release for ${RELEASE_TAG}" >&2 | ||
| exit 1 | ||
| fi | ||
| curl -sSL \ |
There was a problem hiding this comment.
curl -sSL -o _out/assets/nocloud-amd64.raw.xz "$ASSET_URL" returns exit 0 on 4xx/5xx and writes the error body into the output file. The upgrade test then consumes a corrupted image and fails downstream with an unrelated-looking error. Add --fail --retry 3 --retry-delay 5 so the job fails fast with a clear signal.
| pg_pod=$(kubectl get pods -n tenant-test \ | ||
| -l "cnpg.io/cluster=postgres-${name}" \ | ||
| --field-selector=status.phase=Running \ | ||
| -o jsonpath='{.items[0].metadata.name}') |
There was a problem hiding this comment.
Observation (non-blocking): with replicas: 2 the selector cnpg.io/cluster=postgres-${name} returns any CNPG pod, and psql -U postgres over the local socket talks to whatever instance landed in .items[0]. If that's a standby, the INSERT fails with cannot execute INSERT in a read-only transaction. The CI data point is small so the flake may look rare, but it's path-dependent and will surface eventually. Pin to the primary: -l "cnpg.io/cluster=postgres-${name},cnpg.io/instanceRole=primary".
| contents: write | ||
| pull-requests: write | ||
| if: needs.prepare-release.result == 'success' | ||
| if: needs.prepare-release.result == 'success' && needs.upgrade-test.result == 'success' |
There was a problem hiding this comment.
Observation (non-blocking): on the skip path (prepare-release.outputs.skip == 'true'), upgrade-test's if: causes it to be skipped, so its result is not 'success', and generate-changelog (and by the same logic update-website-docs) now also skips. The pre-PR behavior was to still generate the changelog on re-runs of already-published releases. If that regression is intentional, fine; otherwise add an OR branch like (needs.prepare-release.outputs.skip == 'true' || needs.upgrade-test.result == 'success').
| @test "Deploy tenant Kubernetes cluster" { | ||
| local name='upgrade-k8s' | ||
| local k8s_version | ||
| k8s_version=$(yq 'keys | sort_by(.) | .[-1]' packages/apps/kubernetes/files/versions.yaml) |
There was a problem hiding this comment.
Observation (non-blocking): yq 'keys | sort_by(.) | .[-1]' is a lexicographic sort, not a semver sort. Today it produces the correct answer because every key is v1.30-v1.35 (two-digit minor). A future v1.9 or v1.100 entry would silently pick the wrong version. Split on . and sort numerically, or post-process with sort -V after stripping the v.
| @test "Verify platform pods are healthy after upgrade" { | ||
| # Check that key system namespaces have no unhealthy pods | ||
| local failed=false | ||
| for ns in cozy-system cozy-fluxcd cozy-linstor cozy-metallb; do |
There was a problem hiding this comment.
Observation (non-blocking): the for ns in cozy-system cozy-fluxcd cozy-linstor cozy-metallb whitelist predates the expanded workload coverage. With MariaDB/Kafka/VMInstance/Kamaji now in the test, isp-full also has cozy-kubevirt, cozy-kamaji, cozy-kafka-operator, cozy-cluster-api, cozy-monitoring, etc. — pod regressions in those are invisible to this assertion. Iterate over kubectl get ns -o name | grep '^namespace/cozy-' instead.
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — head is unchanged since my previous review (ae8bbd6), so the two blockers I raised then are still open. A closer pass on the new release gating and the post-upgrade check surfaced three more. The prior four business points (STRICT_BASELINE, release-pipeline gating, broader workload coverage, patch-to-patch scoping) and the earlier bot findings (empty pg_pod guard, pre-release regex, empty-configmap coalesce, actionlint label) are all addressed.
Status of my prior CHANGES_REQUESTED: both blockers still open (B1, B2). The non-blocking inline observations from that review are also still present (carried below).
Blockers
B1 — Process substitution <(...) aborts the whole test file under dash.
File: hack/e2e-upgrade-cozystack.bats:657-658, 673-674, 710-711, 725-726
Evidence: hack/cozytest.sh is #!/bin/sh (line 1), runs set -eu (line 6), and sources the converted test file via . "$TMP_SH" (line 104). The sandbox image is FROM ubuntu:22.04 where /bin/sh is dash. dash parses the entire sourced file — including these comm -23 <(...) constructs inside the @test function bodies — at source time and syntax-errors (Syntax error: "(" unexpected) before a single test runs.
Impact: upgrade-test fails on every tag; with the new gating, generate-changelog and update-website-docs are then permanently blocked.
Fix: replace each process substitution with temp files (... | sort > "$STATE_DIR/cur", then comm -23 "$STATE_DIR/cur" "$pre"), or switch hack/cozytest.sh to #!/bin/bash.
B2 — Asset download lacks --fail; a 4xx/5xx body is written into the image file.
File: .github/workflows/tags.yaml:291-295
Evidence: curl -sSL -H ... -o _out/assets/nocloud-amd64.raw.xz "$ASSET_URL" — -sSL does not fail on HTTP errors, so an API 4xx/5xx returns exit 0 and writes the error body into the output file.
Impact: a corrupted Talos image is consumed downstream and the job fails later with an unrelated-looking error instead of failing fast at the download.
Fix: add --fail --retry 3 --retry-delay 5.
B3 — Post-upgrade HelmRelease check passes vacuously.
File: hack/e2e-upgrade-cozystack.bats:645-682
Evidence: helm upgrade cozy-installer --wait blocks only on the installer chart (operator + the platform PackageSource), not on the platform HelmReleases, which Flux re-reconciles asynchronously from the new source. The loop computes new_not_ready = (HRs not Ready now) − (pre-upgrade not-ready baseline) and breaks the instant that set is empty. In the window after helm upgrade returns but before Flux marks anything not-ready, every HR still reads Ready=True from the old reconcile, so after the single sleep 10 (line 647) the loop prints "No HelmReleases newly not-ready after 0s" and passes — without any HR having re-applied the new release. pre-hr-count (line 585) is captured but never compared.
Impact: the test's central claim ("all HelmReleases reconcile after upgrade") is not actually verified; a broken upgrade can pass green.
Fix: require a positive rollout signal before the not-ready delta — assert the PackageSource digest advanced and was observed, or that HR lastAppliedRevision / chart version moved to the new version, before entering the loop.
B4 — First-of-major (vX.0.0) and missing-previous-stable hard-fail the job and block release docs.
File: hack/e2e-upgrade-cozystack.bats:36-39, 49-52
Evidence: find_previous_release returns 1 when minor == 0 or no stable previous-minor tag exists. hack/cozytest.sh injects set -e at the top of every @test body (line 91), so PREV_RELEASE=$(find_previous_release) in the first test aborts with exit 1 → the job fails.
Impact: the first release of a new major has no upgrade path to test yet would block its own generate-changelog / update-website-docs. This is an expected condition, not a regression.
Fix: detect these cases and skip gracefully (exit 0 with a clear "no previous release, nothing to upgrade" message) instead of failing.
B5 — Gating regression on already-published tags.
File: .github/workflows/tags.yaml (generate-changelog / update-website-docs needs + if)
Evidence: upgrade-test carries if: needs.prepare-release.outputs.skip != 'true', so on a re-pushed/already-published tag it is skipped → result == 'skipped'. generate-changelog now requires needs.upgrade-test.result == 'success', and 'skipped' != 'success', so the changelog — previously gated only on needs.prepare-release.result == 'success' — no longer runs on the skip path.
Impact: re-running an already-published release no longer regenerates the changelog/website docs.
Fix: gate on needs.upgrade-test.result != 'failure', or add an explicit needs.prepare-release.outputs.skip == 'true' OR-branch.
Non-blocking
- PostgreSQL seed can hit a standby.
hack/e2e-upgrade-cozystack.bats:305-309— withreplicas: 2,-l cnpg.io/cluster=postgres-${name}+.items[0]can select a standby; the local-socketINSERTthen fails read-only. Pin to the primary (,cnpg.io/instanceRole=primary) or write through the-rwservice. Same applies post-upgrade at745-750. - Lexicographic version pick.
bats:511—yq 'keys | sort_by(.) | .[-1]'is lexicographic; correct only while every key is a two-digit minor (v1.30-v1.35). A futurev1.9/v1.100would pick wrong. Sort numerically orsort -V. - Stale namespace whitelist.
bats:879— thecozy-system cozy-fluxcd cozy-linstor cozy-metallblist predates the expanded workloads; pod regressions incozy-kubevirt/cozy-kamaji/cozy-kafka-operator/ etc. are invisible. Iterate overkubectl get ns -o name | grep '^namespace/cozy-'. - Convention drift vs
docs/agents/e2e-testing.md. Baresleep 5/sleep 10precede severalkubectl waits (bats:297, 373, 444, 647, 707) without the mandateduntil kubectl get …; do sleep 2; doneexistence backstop or aTODO(e2e-replace-fixed-timeouts):justification. That doc landed after this branch was cut; reconcile on rebase. - No remediation guard. The post-upgrade check does not inspect
.status.history, so a parent HR that uninstalled+reinstalled during the upgrade but ended Ready is not caught (docs/agents/e2e-testing.md§6). - Post-upgrade version read lacks a fallback.
bats:688—actual=$(kubectl get cm cozystack-version …)has no:-0/|| true(unlike the pre-upgrade capture at602); an absent CM yields a cryptic "integer expression expected" at[ "$actual" -ge "$previous" ]. - Stale base. Head is far behind
mainand currently conflicts;mainnow also carries a full-suiterelease-e2e.yamlon tags. Rebase, then decide whether this upgrade job should share infra withrelease-e2e.yamlrather than provision a second Talos cluster per tag.
Reimplements the intent of #2401 (previous-minor-stable -> current upgrade test) over the post-Chainsaw-migration codebase. The old #2401 was a single 895-line BATS file, now obsolete; this builds a two-phase Chainsaw lane bracketing an external helm upgrade, reusing existing files throughout. Flow (make upgrade-cozystack, in the e2e sandbox): install-previous (bats, prev OCI chart) -> seed (chainsaw, skipDelete) -> helm upgrade to in-tree chart (bats) -> verify (chainsaw, assert-only). DRY over existing files: - 4 composite actions (.github/actions/e2e-{download-assets,prepare,collect, teardown}) shared by both the e2e and new upgrade-e2e jobs; the e2e job is refactored onto them (behavior-preserving). - hack/e2e-wait-hr-ready.sh: the all-HR-Ready gate extracted from e2e-install-cozystack.bats so install and upgrade share one legible gate. - hack/upgrade-prev-version.sh (+ unit test) resolves the baseline version. - seed suites apply the existing app manifests by relative path (no new CRs). Trigger: an upgrade-e2e job in pull-requests.yaml, parallel to e2e, gated on the release label or a maintainer upgrade-e2e label. Advisory / non-blocking. Coverage: postgres + mariadb canary rows, redis/VM volume survival, migration stamp advance, all-HRs-Ready + no-new-unbound-PV. The tenant Kubernetes survival suites (#2931 path) are env-gated OFF (UPGRADE_E2E_TENANT_K8S) as the flakiest surface, pending dev-stand validation. Co-Authored-By: Claude Opus 4.8 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
Reimplements the intent of #2401 (previous-minor-stable -> current upgrade test) over the post-Chainsaw-migration codebase. The old #2401 was a single 895-line BATS file, now obsolete; this builds a two-phase Chainsaw lane bracketing an external helm upgrade, reusing existing files throughout. Flow (make upgrade-cozystack, in the e2e sandbox): install-previous (bats, prev OCI chart) -> seed (chainsaw, skipDelete) -> helm upgrade to in-tree chart (bats) -> verify (chainsaw, assert-only). DRY over existing files: - 4 composite actions (.github/actions/e2e-{download-assets,prepare,collect, teardown}) shared by both the e2e and new upgrade-e2e jobs; the e2e job is refactored onto them (behavior-preserving). - hack/e2e-wait-hr-ready.sh: the all-HR-Ready gate extracted from e2e-install-cozystack.bats so install and upgrade share one legible gate. - hack/upgrade-prev-version.sh (+ unit test) resolves the baseline version. - seed suites apply the existing app manifests by relative path (no new CRs). Trigger: an upgrade-e2e job in pull-requests.yaml, parallel to e2e, gated on the release label or a maintainer upgrade-e2e label. Advisory / non-blocking. Coverage: postgres + mariadb canary rows, redis/VM volume survival, migration stamp advance, all-HRs-Ready + no-new-unbound-PV. The tenant Kubernetes survival suites (#2931 path) are env-gated OFF (UPGRADE_E2E_TENANT_K8S) as the flakiest surface, pending dev-stand validation. Co-Authored-By: Claude Opus 4.8 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
Reimplements the intent of #2401 (previous-minor-stable -> current upgrade test) over the post-Chainsaw-migration codebase. The old #2401 was a single 895-line BATS file, now obsolete; this builds a two-phase Chainsaw lane bracketing an external helm upgrade, reusing existing files throughout. Flow (make upgrade-cozystack, in the e2e sandbox): install-previous (bats, prev OCI chart) -> seed (chainsaw, skipDelete) -> helm upgrade to in-tree chart (bats) -> verify (chainsaw, assert-only). DRY over existing files: - 4 composite actions (.github/actions/e2e-{download-assets,prepare,collect, teardown}) shared by both the e2e and new upgrade-e2e jobs; the e2e job is refactored onto them (behavior-preserving). - hack/e2e-wait-hr-ready.sh: the all-HR-Ready gate extracted from e2e-install-cozystack.bats so install and upgrade share one legible gate. - hack/upgrade-prev-version.sh (+ unit test) resolves the baseline version. - seed suites apply the existing app manifests by relative path (no new CRs). Trigger: an upgrade-e2e job in pull-requests.yaml, parallel to e2e, gated on the release label or a maintainer upgrade-e2e label. Advisory / non-blocking. Coverage: postgres + mariadb canary rows, redis/VM volume survival, migration stamp advance, all-HRs-Ready + no-new-unbound-PV. The tenant Kubernetes survival suites (#2931 path) are env-gated OFF (UPGRADE_E2E_TENANT_K8S) as the flakiest surface, pending dev-stand validation. Co-Authored-By: Claude Opus 4.8 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
|
Superseded by #3276 |
Reimplements the intent of #2401 (previous-minor-stable -> current upgrade test) over the post-Chainsaw-migration codebase. The old #2401 was a single 895-line BATS file, now obsolete; this builds a two-phase Chainsaw lane bracketing an external helm upgrade, reusing existing files throughout. Flow (make upgrade-cozystack, in the e2e sandbox): install-previous (bats, prev OCI chart) -> seed (chainsaw, skipDelete) -> helm upgrade to in-tree chart (bats) -> verify (chainsaw, assert-only). DRY over existing files: - 4 composite actions (.github/actions/e2e-{download-assets,prepare,collect, teardown}) shared by both the e2e and new upgrade-e2e jobs; the e2e job is refactored onto them (behavior-preserving). - hack/e2e-wait-hr-ready.sh: the all-HR-Ready gate extracted from e2e-install-cozystack.bats so install and upgrade share one legible gate. - hack/upgrade-prev-version.sh (+ unit test) resolves the baseline version. - seed suites apply the existing app manifests by relative path (no new CRs). Trigger: an upgrade-e2e job in pull-requests.yaml, parallel to e2e, gated on the release label or a maintainer upgrade-e2e label. Advisory / non-blocking. Coverage: postgres + mariadb canary rows, redis/VM volume survival, migration stamp advance, all-HRs-Ready + no-new-unbound-PV. The tenant Kubernetes survival suites (#2931 path) are env-gated OFF (UPGRADE_E2E_TENANT_K8S) as the flakiest surface, pending dev-stand validation. Co-Authored-By: Claude Opus 4.8 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
Reimplements the intent of #2401 (previous-minor-stable -> current upgrade test) over the post-Chainsaw-migration codebase. The old #2401 was a single 895-line BATS file, now obsolete; this builds a two-phase Chainsaw lane bracketing an external helm upgrade, reusing existing files throughout. Flow (make upgrade-cozystack, in the e2e sandbox): install-previous (bats, prev OCI chart) -> seed (chainsaw, skipDelete) -> helm upgrade to in-tree chart (bats) -> verify (chainsaw, assert-only). DRY over existing files: - 4 composite actions (.github/actions/e2e-{download-assets,prepare,collect, teardown}) shared by both the e2e and new upgrade-e2e jobs; the e2e job is refactored onto them (behavior-preserving). - hack/e2e-wait-hr-ready.sh: the all-HR-Ready gate extracted from e2e-install-cozystack.bats so install and upgrade share one legible gate. - hack/upgrade-prev-version.sh (+ unit test) resolves the baseline version. - seed suites apply the existing app manifests by relative path (no new CRs). Trigger: an upgrade-e2e job in pull-requests.yaml, parallel to e2e, gated on the release label or a maintainer upgrade-e2e label. Advisory / non-blocking. Coverage: postgres + mariadb canary rows, redis/VM volume survival, migration stamp advance, all-HRs-Ready + no-new-unbound-PV. The tenant Kubernetes survival suites (#2931 path) are env-gated OFF (UPGRADE_E2E_TENANT_K8S) as the flakiest surface, pending dev-stand validation. Co-Authored-By: Claude Opus 4.8 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
Reimplements the intent of #2401 (previous-minor-stable -> current upgrade test) over the post-Chainsaw-migration codebase. The old #2401 was a single 895-line BATS file, now obsolete; this builds a two-phase Chainsaw lane bracketing an external helm upgrade, reusing existing files throughout. Flow (make upgrade-cozystack, in the e2e sandbox): install-previous (bats, prev OCI chart) -> seed (chainsaw, skipDelete) -> helm upgrade to in-tree chart (bats) -> verify (chainsaw, assert-only). DRY over existing files: - 4 composite actions (.github/actions/e2e-{download-assets,prepare,collect, teardown}) shared by both the e2e and new upgrade-e2e jobs; the e2e job is refactored onto them (behavior-preserving). - hack/e2e-wait-hr-ready.sh: the all-HR-Ready gate extracted from e2e-install-cozystack.bats so install and upgrade share one legible gate. - hack/upgrade-prev-version.sh (+ unit test) resolves the baseline version. - seed suites apply the existing app manifests by relative path (no new CRs). Trigger: an upgrade-e2e job in pull-requests.yaml, parallel to e2e, gated on the release label or a maintainer upgrade-e2e label. Advisory / non-blocking. Coverage: postgres + mariadb canary rows, redis/VM volume survival, migration stamp advance, all-HRs-Ready + no-new-unbound-PV. The tenant Kubernetes survival suites (#2931 path) are env-gated OFF (UPGRADE_E2E_TENANT_K8S) as the flakiest surface, pending dev-stand validation. Co-Authored-By: Claude Opus 4.8 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
2 participants
What this PR does
Adds automated upgrade testing that runs on every tag push as part of the release pipeline. Tests the most common upgrade path: previous latest minor release → current version.
New files
hack/e2e-upgrade-cozystack.bats— 15-step BATS test script covering install → workloads → upgrade → validatepackages/core/testing/Makefile—upgrade-cozystacktarget.github/workflows/tags.yaml—upgrade-testjob (runs in parallel with changelog generation)How it works
e2e-prepare-cluster.bats)git taggit show)helm upgrade(same mechanism users use)helm upgrade installer packages/core/installer)Ready=TrueCrashLoopBackOffpodsBoundDesign decisions
e2e-install-cozystack.batsoracle-vm-24cpu-96gb-x86-64runner with 150-minute timeoutRelease note
Summary by CodeRabbit