Skip to content

ci(e2e): drop 3x retry on Run E2E + Install Cozystack, wait out gateway.bats tenant teardown - #2558

Merged
Aleksei Sviridkin (lexfrei) merged 6 commits into
mainfrom
daniil/split-drop-retry
Jun 9, 2026
Merged

ci(e2e): drop 3x retry on Run E2E + Install Cozystack, wait out gateway.bats tenant teardown#2558
Aleksei Sviridkin (lexfrei) merged 6 commits into
mainfrom
daniil/split-drop-retry

Conversation

@myasnikovdaniil

@myasnikovdaniil myasnikovdaniil commented May 2, 2026

Copy link
Copy Markdown
Contributor

What this PR does

Drops the 3× retry loop on Run E2E tests and Install Cozystack into sandbox. Prepare environment keeps its 3× retry — that step is pure infrastructure (Talos image download, sandbox VM boot, network) where transient runner hiccups warrant a retry.

On failure, the test step now captures kubectl get hr -A -o wide and kubectl get events -A under a collapsible group so triage starts with the actual broken-state snapshot.

Note

An earlier revision of this PR also doubled every bats timeout. That commit was dropped in a rebase and is intentionally not restored: the timeout class that actually matters (per-app HR-Ready waits) has since been standardized at 5m on main (7b9f286), making a blanket 2× redundant.

Fixes gateway.bats teardown leakage. The nested-tenant tests deleted tenants fire-and-forget, parent and child back-to-back. The leftover uninstalls (each blocked on a cleanup Job, parents wedged on still-terminating child namespaces) plus one mid-install child HR occupied exactly 5 workers on the --concurrent=5 tenants helm-controller shard, starving whichever app test ran next — observed as the harbor HR sitting unreconciled for its whole 5m HR-Ready budget in run 27020081550, surfaced by this PR's own retry removal + diagnostics dump. Teardown now deletes child→parent with hard wait hr --for=delete between, so a wedged tenant uninstall fails gateway.bats itself, not an innocent neighbor.

Why

Audit of 30 successful PR runs found that across 5 sampled failure attempts, 25/25 retries on Run E2E tests failed — the retry loop never recovered a flake, only stretched deterministic failures and tripled diagnostic wall-time. Same data shape on Install Cozystack.

Beyond wasted CI time, the retry was hiding ~10 deterministic bugs (Helm namespace-ownership conflict, seaweedfs HR timeout, harbor BucketInfo wiring, vminstance disk race, etc.). Each failure looked like a "flake" because the retry sometimes coincided with whatever transient state had cleared — the retry never fixed the bug, just delayed surfacing.

Dependencies

The deterministic bugs the retry was masking are now fixed on main:

Companion PRs in the #2619 split (independent of this PR, ordering-wise):

This PR does NOT depend on #2602/#2601 — it now touches only the workflow file and gateway.bats teardown, both on top of fresh main.

Surfaced from #2500.

Release note

NONE

Summary by CodeRabbit

  • Chores
    • CI prepare-environment step now reports plain attempt counts with clear success/failure messages.
    • Install and per-app test steps no longer retry; each runs once and fails immediately on error. Failed apps log diagnostics and job proceeds to remaining apps while overall job fails.
  • Tests
    • End-to-end tests and install/prepare flows use longer, more tolerant timeouts and added existence polling to reduce flakiness and improve diagnostics.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 2, 2026

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Refactors the PR E2E GitHub Actions job to simplify retries and diagnostics, and increases timeouts/polling across many e2e test scripts to reduce flakiness and tolerate slower provisioning.

Changes

E2E Workflow Error Handling

Layer / File(s) Summary
Prepare Environment Retry Logic
.github/workflows/pull-requests.yaml
Step retains a 3-attempt retry loop but changes messaging: prints plain "Attempt X failed, exiting..." on final failure and "Prepare environment completed after N attempts" on success.
Install Cozystack Simplification
.github/workflows/pull-requests.yaml
Removed the retry loop; runs make ... install-cozystack once. On failure it immediately fails after printing grouped Kubernetes diagnostics (HelmRelease status, non-ready HelmReleases describe, recent cluster events).
E2E Test Execution with Diagnostics
.github/workflows/pull-requests.yaml
Per-app retries removed: each test-apps-$app runs once; on failure logs a "no retry — see diagnostics below" message, records the failed app, prints per-app HelmRelease and event diagnostics, continues to next app while aggregating failures to fail the job.

E2E tests and helper scripts — increased wait/delete/poll timeouts

Layer / File(s) Summary
Global e2e timeout increases (many apps)
hack/e2e-apps/*, hack/e2e-install-cozystack.bats
Widespread increases to kubectl/helm/port-forward/curl timeouts and polling windows across app-specific tests (bucket, clickhouse, etcd, external-dns, foundationdb, harbor, kafka, mariadb, mongodb, openbao, postgres, qdrant, redis, vminstance, run-kubernetes.sh, openapi tests). Several tests add explicit "wait for existence" polling before kubectl wait and adjust cleanup delete timeouts.
Install flow pre-pull and Helm timeout
hack/e2e-install-cozystack.bats
Pre-pull now renders charts to temp files and extracts images with yq; installer helm upgrade timeout increased (2m→8m); operator/HR readiness waits use larger per-HR timeouts and extended polling.
Run-kubernetes script time budgets
hack/e2e-apps/run-kubernetes.sh
Multiple tenant/cluster provisioning, port-forward, node join, backend/NFS, and post-test readiness waits increased (delete-waits, pod/Deployment/LoadBalancer/NFS waits all lengthened).

Sequence Diagram(s)

sequenceDiagram
  participant ComponentA
  participant ComponentB
  ComponentA->>ComponentB: observable interaction
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~15 minutes

Possibly related issues

Possibly related PRs

  • cozystack/cozystack#2615 — Similar changes to e2e-install-cozystack.bats pre-pull logic using rendered Helm templates and yq.

Suggested reviewers

  • lexfrei
  • kvaps
  • lllamnyp
  • androndo
  • sircthulhu

Poem

🐰 I hopped through YAML, trims in sight,
prepare tries calmer, logging light,
install runs once, then shows the traces,
tests wait longer in patient spaces,
a rabbit cheers: clearer runs tonight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main changes: removing retry logic from E2E and Install Cozystack steps, and addressing gateway.bats tenant teardown.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch daniil/split-drop-retry

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review size/M This PR changes 30-99 lines, ignoring generated files labels May 2, 2026
@myasnikovdaniil
myasnikovdaniil marked this pull request as ready for review May 6, 2026 07:13
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Note

Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported.

@dosubot dosubot Bot added area/ci Issues or PRs related to CI workflows, GitHub Actions, automation area/testing Issues or PRs related to testing (e2e, bats, unit tests) do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress labels May 6, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/pull-requests.yaml:
- Line 235: The attempt counter is off because it's initialized to 0 but only
incremented on failures, so first-try success prints "0 attempts" and retry
counts are off; fix by initializing attempt to 1 (so the first run counts as
attempt 1) and keep the existing increment-in-failure logic, ensuring the echo
"Prepare environment completed after $attempt attempts" reflects the actual
number of runs; update the variable initialization for attempt (and verify any
incrementing logic around the retry/failure branch that currently updates
attempt).
🪄 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: 564453a0-d0bf-46d0-bd7e-4d152395185c

📥 Commits

Reviewing files that changed from the base of the PR and between 5786afe and f61250f.

📒 Files selected for processing (1)
  • .github/workflows/pull-requests.yaml

Comment thread .github/workflows/pull-requests.yaml Outdated
@myasnikovdaniil
myasnikovdaniil force-pushed the daniil/split-drop-retry branch from f61250f to 60bddbb Compare May 7, 2026 06:28
@myasnikovdaniil myasnikovdaniil removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review labels May 7, 2026
@github-actions github-actions Bot added the area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review label May 7, 2026
@myasnikovdaniil
myasnikovdaniil force-pushed the daniil/split-drop-retry branch from a13b05b to 2895d94 Compare May 8, 2026 05:44
myasnikovdaniil added a commit that referenced this pull request May 14, 2026
Flux helm-controller v1.5.0 (shipped in Flux v2.8) replaced the legacy
Helm readiness logic with kstatus-based health checking, which polls
Deployment/StatefulSet/Service status and waits for kstatus to assess
rollout-ready. This typically adds 10-30s of latency between resource
apply and HR condition Ready=True compared to v2.7.

Our per-app bats tests had `kubectl wait hr/... --for=condition=ready
--timeout=` budgets of 20s, 30s, 60s, 100s — sized for v2.7's faster
readiness flip and previously masked by the 3x retry on Run E2E tests
(dropped in #2558). Under v2.8 with no retry, every app that runs after
the first few hit its HR wait timeout exactly, surfacing as 13 of 16
test failures on PR #2619.

Standardise on 5m for HR Ready across all app tests. Tight enough to
catch genuine bring-up failures within the per-app job budget, loose
enough to absorb kstatus dispatch jitter on a management cluster that
is concurrently reconciling multiple tenant-test HRs.

Post-HR-Ready checks for app-specific resources (StatefulSet/Service/
PVC/operator CRs) remain — for CR-based apps kstatus only observes
that the chart applied, not that the operator finished. Audit of
which of those are actually redundant under v2.8 kstatus is queued
as a follow-up.

Also bumps the 10s "kamajicontrolplane appears" wait in
run-kubernetes.sh to 2m, and the kubernetes-addon HR Ready loop from
1m to 5m for the same reason.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
myasnikovdaniil added a commit that referenced this pull request May 19, 2026
Flux helm-controller v1.5.0 (shipped in Flux v2.8) replaced the legacy
Helm readiness logic with kstatus-based health checking, which polls
Deployment/StatefulSet/Service status and waits for kstatus to assess
rollout-ready. This typically adds 10-30s of latency between resource
apply and HR condition Ready=True compared to v2.7.

Our per-app bats tests had `kubectl wait hr/... --for=condition=ready
--timeout=` budgets of 20s, 30s, 60s, 100s — sized for v2.7's faster
readiness flip and previously masked by the 3x retry on Run E2E tests
(dropped in #2558). Under v2.8 with no retry, every app that runs after
the first few hit its HR wait timeout exactly, surfacing as 13 of 16
test failures on PR #2619.

Standardise on 5m for HR Ready across all app tests. Tight enough to
catch genuine bring-up failures within the per-app job budget, loose
enough to absorb kstatus dispatch jitter on a management cluster that
is concurrently reconciling multiple tenant-test HRs.

Post-HR-Ready checks for app-specific resources (StatefulSet/Service/
PVC/operator CRs) remain — for CR-based apps kstatus only observes
that the chart applied, not that the operator finished. Audit of
which of those are actually redundant under v2.8 kstatus is queued
as a follow-up.

Also bumps the 10s "kamajicontrolplane appears" wait in
run-kubernetes.sh to 2m, and the kubernetes-addon HR Ready loop from
1m to 5m for the same reason.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
myasnikovdaniil added a commit that referenced this pull request May 25, 2026
Flux helm-controller v1.5.0 (shipped in Flux v2.8) replaced the legacy
Helm readiness logic with kstatus-based health checking, which polls
Deployment/StatefulSet/Service status and waits for kstatus to assess
rollout-ready. This typically adds 10-30s of latency between resource
apply and HR condition Ready=True compared to v2.7.

Our per-app bats tests had `kubectl wait hr/... --for=condition=ready
--timeout=` budgets of 20s, 30s, 60s, 100s — sized for v2.7's faster
readiness flip and previously masked by the 3x retry on Run E2E tests
(dropped in #2558). Under v2.8 with no retry, every app that runs after
the first few hit its HR wait timeout exactly, surfacing as 13 of 16
test failures on PR #2619.

Standardise on 5m for HR Ready across all app tests. Tight enough to
catch genuine bring-up failures within the per-app job budget, loose
enough to absorb kstatus dispatch jitter on a management cluster that
is concurrently reconciling multiple tenant-test HRs.

Post-HR-Ready checks for app-specific resources (StatefulSet/Service/
PVC/operator CRs) remain — for CR-based apps kstatus only observes
that the chart applied, not that the operator finished. Audit of
which of those are actually redundant under v2.8 kstatus is queued
as a follow-up.

Also bumps the 10s "kamajicontrolplane appears" wait in
run-kubernetes.sh to 2m, and the kubernetes-addon HR Ready loop from
1m to 5m for the same reason.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
(cherry picked from commit 5ef02ee)
@myasnikovdaniil
myasnikovdaniil force-pushed the daniil/split-drop-retry branch from 2895d94 to f3601ba Compare May 25, 2026 06:51
@github-actions github-actions Bot added size/XL This PR changes 500-999 lines, ignoring generated files and removed size/M This PR changes 30-99 lines, ignoring generated files labels May 25, 2026
@myasnikovdaniil myasnikovdaniil changed the title ci(pull-requests): drop 3x retry on Run E2E + Install Cozystack ci+test: drop 3× retry on E2E + Install Cozystack, double bats timeouts May 25, 2026
@myasnikovdaniil
myasnikovdaniil force-pushed the daniil/split-drop-retry branch from c5e28d0 to 1bc7137 Compare May 29, 2026 04:30
myasnikovdaniil added a commit that referenced this pull request Jun 2, 2026
…(folds #2612) (#2602)

## What this PR does

Upgrades Flux v2.7.3 → v2.8.0 across both the vendored tenant chart and
the embedded management-cluster manifests, and folds in the chart fixes
that become hard errors under the new strict helm-controller v1.5.

Flux v2.8's helm-controller v1.5.0 ships:
- **Server-Side Apply with `--force-conflicts`** — strict CRD schema
enforcement; misplaced fields (silently dropped on v2.7) now fail the
apply.
- **kstatus-based health checking** by default — parent HR waits for
every applied resource (including child HRs) to be Ready before flipping
its own Ready, surfacing latent ordering deadlocks.
- **Helm v4 embedded** — `upgrade.force: true` is deprecated and now
conflicts with SSA.
- **`HelmRelease.spec.healthCheckExprs`** — prerequisite for proper
readiness gating (used in PR #2601 split).

Folds in PR #2612 (kubevirt-instancetypes null TPM fix) since the same
Flux upgrade triggers it.

## Commits

**Flux upgrade itself:**
- `feat(fluxcd)`: bump `flux-operator` / `flux-instance` vendored charts
to v0.48.0; web UI opt-in.
- `feat(flux)`: regenerate embedded management-cluster manifests via
`make update` in `packages/core/flux-aio` (timoni bundle build).

**Chart fixes for strict SSA — fields the chart sent that v2.7 silently
dropped, v2.8 rejects:**
- `fix(kubevirt-instancetypes)`: drop persistent strip that produced
null `preferredTPM` (folds #2612).
- `fix(foundationdb)`: relocate `faultDomain`, `imageType`, `labels`,
`minimumUptimeSecondsForBounce` from inside `automationOptions` to
direct children of `spec`.
- `fix(kafka)`: place `enableServiceLinks` under `template.pod`, not a
phantom `template.spec`.
- `fix(vm-instance)`: emit `disk: {}` (not `disk:`/null) when no bus is
set.
- `fix(platform)`: drop deprecated `upgrade.force: true` from
HelmReleases; fix `kafka` WorkloadMonitor `replicas` paths.

**Ordering / deadlock fixes under v2.8 kstatus:**
- `fix(vpa)`: break circular wait between parent install and nested
`vpa-for-vpa` HR.
- `fix(kubernetes)`: drop lookup-guarded parent-HR `dependsOn` on tenant
addon child HRs (parent waits on child via kstatus, child waited on
parent — deadlock).

**E2E waits for v2.8 kstatus timing:**
- `test(e2e)`: bump app HR-Ready waits to 5m (was 20s–100s under v2.7's
faster dispatch).
- `test(e2e)`: wait for parent HR Ready before downstream asserts in
`run-kubernetes.sh` and `vminstance.bats`.

## Scope discipline

This PR is part of the split of #2619 (the consolidated CI fixes branch)
into review-friendly pieces. Companion PRs:
- **PR #2601** (seaweedfs split) — folded into this PR (commits
`29c6afc8`, `0e8b46d7`, `7157158c`, `dccdeb52`, `f880b324`): the
seaweedfs-system → seaweedfs-db + seaweedfs-system split, its adoption
migration 43 (targetVersion 44), and the configurable db resources all
land here, because the strict-SSA `upgrade.force` removal and the
kstatus parent-HR timeout bump only make sense together with the split.
#2601 is superseded.
- **PR #2558** (drop 3× retry on `Run E2E` + `Install Cozystack`) —
independent, lands separately.
- Several smaller standalone fixes lifted out of #2619 (startup probes,
cert-manager `dependsOn`, prepull machinery, CSI HR timeout, NFS/OIDC
test improvements) — opened as separate PRs.

## Verification

- `helm template` renders cleanly for both `fluxcd` and
`fluxcd-operator` packages with `web.enabled=false` (default) and
`web.enabled=true`.
- Embedded `cmd/cozystack-operator` binary contains the v1.5.0 / v1.8.0
/ v2.1.0 controller image strings.
- No references to the v0.39-removed `--disable-wait-interruption` flag
anywhere in `packages/` or `internal/`.

### Release note

```release-note
Flux upgraded to v2.8.0 (helm-controller v1.5 — Helm v4 Server-Side Apply with --force-conflicts, kstatus health checking).

When upgrading existing clusters:
- Kubernetes 1.33+ is now required for the platform (management) cluster, and for any tenant cluster that enables the optional (default-off) Flux addon — that addon ships the bumped Flux too.
- HelmReleases no longer set `upgrade.force: true`. Helm v4 SSA resolves field-ownership conflicts automatically (`--force-conflicts`), but that is not the old client-side replace: immutable-field changes (e.g. StatefulSet volumeClaimTemplates/serviceName) no longer self-heal and require manual recreation — delete the object (e.g. `kubectl delete sts <name> --cascade=orphan`) and let Flux recreate it.
- KubeVirt: persistent TPM/EFI is re-enabled for the Windows 11/2k22/2k25 preferences (KubeVirt 1.8 VMPersistentState); each affected VM provisions an extra RWO backend-storage PVC from the default StorageClass.
- KubeVirt: the EOL centos.7*/centos.stream8* preferences are retained as deprecated, hidden aliases (`tags: hidden`, `instancetype.kubevirt.io/deprecated: "true"`) — existing VMInstances on these profiles keep rendering and need no action on upgrade, but the profiles are no longer offered for new VMs; repoint to centos.stream9/10 when convenient. The gn1.* GPU instancetypes are likewise retained.
- FoundationDB: imageType now reaches the operator (silently dropped pre-SSA); it is pinned to `split` to match the value existing clusters effectively ran, so upgrades stay non-disruptive. Set `imageType: unified` to migrate deliberately.
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Flux Status web UI: optional web server, config secret, service port,
HTTPRoute/Ingress, network policy, and RBAC roles.
  * New SeaweedFS DB Helm chart and optional managed DB release.

* **Improvements**
* CRD/schema enhancements: new provider kinds, validations, variant
option, and external checksum refs.
* Raised Kubernetes prerequisite to 1.30+; extended e2e timeouts for
reliability.

* **Chores**
* Bumped Flux Operator and Flux versions; documentation links updated to
fluxoperator.dev.
* **Bug Fixes**
  * Removed aggressive HelmRelease force-upgrade/install flags.

<!-- review_stack_entry_start -->

[![Review Change
Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2602?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)

<!-- review_stack_entry_end -->
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Base automatically changed from flux-bump-v048 to main June 2, 2026 09:08
@myasnikovdaniil
myasnikovdaniil force-pushed the daniil/split-drop-retry branch from 1bc7137 to bb69232 Compare June 5, 2026 08:55
Audit of 30 successful PR runs found that across 5 sampled failure
attempts, **25/25 retries** on `Run E2E tests` failed — the retry
loop never recovered a flake, only stretched deterministic failures
and tripled diagnostic wall-time. Same data shape on `Install
Cozystack`. Drop both.

`Prepare environment` keeps its 3x retry — that step is pure
infrastructure (Talos image download, sandbox VM boot, network)
where transient runner hiccups warrant a retry.

On failure, the test step now captures `kubectl get hr -A -o wide`
and `kubectl get events -A` under a collapsible group so triage
starts with the actual broken-state snapshot.

Depends on:
- #2508 — installer namespace bootstrap (Helm namespace-ownership conflict)
- #2509 — operator HelmRelease config knobs (`seaweedfs-system` 2-min wait race)
- #2528 — harbor bucket-secret + BucketInfo gating (harbor ValuesError)
- #2529 — objectstorage-controller BucketAccess conflict retry
- the daniil/split-vminstance PR (vminstance disk race + VM IP/ready timeouts)
- the daniil/split-event-driven PR (existence backstops surfacing real errors)

Until those land, dropping the retry will fail CI for unrelated PRs
that hit the seaweedfs / harbor / installer / vminstance races.

Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Assisted-By: Claude <noreply@anthropic.com>
Address review feedback from coderabbitai on
.github/workflows/pull-requests.yaml:235: $attempt is incremented only on
failure, so first-try success printed "completed after 0 attempts". Use
$((attempt + 1)) so the message reflects the total number of runs.

Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Mirror the diagnostics group already on `Run E2E tests` to the
`Install Cozystack into sandbox` step. Without this, install-step
failures (e.g. an HR not reaching Ready inside the bats `kubectl wait`
window) surface only as "error: timed out waiting for the condition
on helmreleases/<name>" with no cluster snapshot — re-running is the
only way to triage, which is the exact pattern the parent commit
removed.

On failure, dump three groups:

- `kubectl get hr -A -o wide` (last 100 lines): which HRs exist and
  their Ready/Status columns.
- `kubectl describe hr` for each not-Ready HR (tail -50): the
  Conditions block surfaces helm install/upgrade errors, missing
  CRDs, and chart-render failures — the actual cause.
- `kubectl get events -A --sort-by=.lastTimestamp` (last 50): cluster
  events that often pinpoint the failing pod/PVC/job.

Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
@myasnikovdaniil
myasnikovdaniil force-pushed the daniil/split-drop-retry branch from bb69232 to d8d4c71 Compare June 5, 2026 14:14
The last three gateway tests create nested tenants (gwprop,
gwparent/gwchild, rparent/rchild) and deleted them fire-and-forget,
with parent and child deletes back-to-back. The file exits in ~25s
while the uninstalls run on for minutes: each tenant uninstall blocks
on a cleanup Job in cozy-system, and a parent deleted alongside its
child wedges on the child's still-terminating namespace.

On the tenants helm-controller shard (--concurrent=5) the leftovers
from one gateway.bats run — three stuck parent uninstalls, one stuck
child uninstall, and the rchild HR still mid-install — occupied
exactly 5 workers, so the next test in line (harbor) sat unreconciled
for its whole 5m HR-Ready budget and failed without ever erroring.

Sequence the teardown instead: delete child, wait for its HR to go
away, then delete parent and wait again. The waits are hard failures
on purpose — if a tenant uninstall genuinely wedges, gateway.bats is
the test that should go red, not whichever app test runs next.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
@myasnikovdaniil myasnikovdaniil changed the title ci+test: drop 3× retry on E2E + Install Cozystack, double bats timeouts ci(e2e): drop 3x retry on Run E2E + Install Cozystack, wait out gateway.bats tenant teardown Jun 6, 2026
…eport

Run 27057500380 failed on harbor's BucketClaim never reaching
bucketReady=true, and neither artifact could explain why: the per-app
diagnostics dump has no bucket objects, and cozyreport collects
neither COSI resources nor objectstorage-controller / seaweedfs
provisioner logs — by the time the report is taken, the bucket state
has moved on.

Add a COSI module to cozyreport.sh (bucketclaims/bucketaccesses/
buckets/classes as custom-column tables plus full YAML, controller and
per-namespace provisioner logs) and a bucket-readiness snapshot to the
per-app failure dump in the PR workflow. The COSI CRDs ship no printer
columns, so plain kubectl get would show only NAME/AGE — readiness
fields are pulled explicitly.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
@github-actions github-actions Bot added size/L This PR changes 100-499 lines, ignoring generated files and removed size/M This PR changes 30-99 lines, ignoring generated files labels Jun 7, 2026
…licies type-check

cozystack-basics ships cluster-wide ValidatingAdmissionPolicies matching
tenants.apps.cozystack.io and the Gateway API types. KCM's VAP status
type-checker panics on a nil schema when it type-checks a policy against a
matched type that isn't resolvable yet; during a cold install this crashes
KCM, which stalls cert/token issuance, so cozystack-api never starts and the
tenants schema never resolves — a bootstrap deadlock.

Add dependsOn edges so the package installs only after cozystack-engine
(cozystack-api) and gateway-api-crds are Ready. Drift detection is off on
operator-generated HelmReleases, so a template capability gate would drop the
policy at first install and never re-add it; ordering is the reliable fix.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
(cherry picked from commit 03f1e7a)
@myasnikovdaniil myasnikovdaniil removed the area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review label Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — the platform ordering fix is correct, introduces no dependency cycle, is covered by a helm-unittest regression guard, and the full E2E suite is green. Comments below are non-blocking.

What I verified

  • cozystack-basics dependsOn ordering (the platform fix): cozystack-basics ships cluster-wide ValidatingAdmissionPolicies matching tenants.apps.cozystack.io (served by cozystack-api in cozystack-engine) and the Gateway API types (gateways/httproutes/tlsroutes from gateway-api-crds). Ordering the package after those APIs is the right fix. Verified there is no cycle: nothing dependsOn cozystack-basics (it is a dependency-graph leaf), cozystack-engine depends only on networking/cert-manager, and gateway-api-crds has an empty dependsOn. Rejecting a template capability-gate is justified — drift detection is off on operator-generated HelmReleases, so a gate would render the policy out on first install and never restore it. The full platform helm-unittest suite passes locally (27/27).
  • Retry removal + diagnostics: the shell logic is sound; the attempt-counter off-by-one is already fixed ($((attempt + 1))), and $4 is the correct READY column in kubectl get hr -A.
  • gateway.bats teardown: the test runner executes each test under set -e, so a wait --for=delete timeout on a genuinely wedged uninstall fails gateway.bats itself — which is the stated goal. The HR names are correct and the green E2E run confirms the happy path.
  • cozyreport COSI capture: resource scoping is correct (cluster-scoped buckets/bucketclasses/bucketaccessclasses without -A, namespaced bucketclaims/bucketaccesses with -A), all best-effort.

Non-blocking follow-ups

  1. The five new wait --for=delete calls in hack/e2e-apps/gateway.bats (lines 493, 551, 553, 681, 683) are unguarded, whereas every other --for=delete under hack/e2e-apps/ is guarded — etcd.bats with --ignore-not-found, and harbor.bats/kafka.bats/run-kubernetes.sh with 2>/dev/null || true. A bare kubectl wait --for=delete on a resource that is already absent at invocation returns error: no matching resources found (exit 1), which fails the test under set -e. The happy path is safe (the tenant is created earlier in the same test, so the HR exists), but adding --ignore-not-found — as etcd.bats already does — removes the residual race without weakening the intended "timeout fails the test" behavior.
  2. This PR also carries a platform install-ordering change (cozystack-basics) that affects every cozystack install, not only CI, yet it is labelled area/ci/area/testing only and the release note is NONE. Consider adding area/platform for discoverability, and double-checking whether a fresh-install KCM-deadlock fix warrants a release note (keeping NONE is fine if the broken ordering never shipped in a release).
  3. Minor: in the install-failure diagnostics, the docker exec … | while read pipeline is unguarded (only the inner describe has || true). Under set -eo pipefail a transient docker exec failure there would skip the trailing "recent events" group before the explicit exit 1. The step still fails correctly; only diagnostic completeness is at risk.

# uninstall carries over into the next .bats file, starving the
# helm-controller worker pool (--concurrent=5 on the tenants shard).
kubectl -n tenant-test delete tenant gwprop --ignore-not-found
kubectl -n tenant-test wait hr tenant-gwprop --for=delete --timeout=300s

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Every other --for=delete under hack/e2e-apps/ is guarded against the not-found-at-invocation case (etcd.bats uses --ignore-not-found, harbor.bats/kafka.bats/run-kubernetes.sh use 2>/dev/null || true). A bare kubectl wait --for=delete on a resource that is already absent when the command starts returns error: no matching resources found (exit 1), which fails the test under set -e. Suggest matching etcd.bats and appending --ignore-not-found here (and on the other four new waits): it keeps the intended "timeout on a wedged uninstall fails the test" behavior while removing the residual race. Non-blocking.

@lexfrei
Aleksei Sviridkin (lexfrei) merged commit 479929e into main Jun 9, 2026
13 checks passed
@lexfrei
Aleksei Sviridkin (lexfrei) deleted the daniil/split-drop-retry branch June 9, 2026 13:21
@lexfrei
Aleksei Sviridkin (lexfrei) restored the daniil/split-drop-retry branch June 9, 2026 14:20
@lexfrei
Aleksei Sviridkin (lexfrei) deleted the daniil/split-drop-retry branch June 9, 2026 14:21
IvanHunters pushed a commit that referenced this pull request Jun 15, 2026
A churn-heavy platform install intermittently wedges a pod in
ContainerCreating because the cilium-agent rejects its sandbox with
'[PUT /endpoint/{id}][400] putEndpointIdInvalid "IP ipv4:<X> is already
in use"', repeated until the agent restarts or the install times out.
The disputed IP has no live owner: IPAM and the ipcache released it, but
the agent's in-memory endpointManager still holds a stale Endpoint from a
previously-deleted pod whose CNI DEL was skipped or raced (so
removeReferencesLocked never ran). Neither the operator's CiliumEndpoint
GC nor the agent's veth-based endpoint GC can reap it. This surfaced as
~6 of the post-#2558 e2e failures, each on a different victim pod
(velero node-agent, fluent-bit, fdb-operator, cdi-operator, capi
bootstrap), failing PRs unrelated to the change under test. Tracked
upstream at cilium/cilium#38313 (closed stale); no released or
pre-release Cilium fixes it and no flag disables it.

hack/e2e-cilium-endpoint-leak-healer.sh runs as a file-scoped background
watchdog (setup_file/teardown_file). It detects a wedged pod, confirms
the IP is a true orphan -- an endpoint in the owning node's registry
holds it, that endpoint backs no live Running pod, and a different pod is
wedged requesting the same IP -- then evicts only that endpoint via
'cilium-dbg endpoint disconnect ipv4:<X>' (DELETE /endpoint/{id} ->
unexpose -> removeReferencesLocked). Blast radius is one dead endpoint,
not an agent restart. It is read-only until an orphan is positively
confirmed and refuses (loudly) to touch an endpoint backing a live pod,
so a genuine duplicate-IP bug stays visible rather than masked.

This is an interim CI mitigation, not a product fix; remove the script
and the bats hooks once a fixed Cilium ships.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
myasnikovdaniil added a commit that referenced this pull request Jun 17, 2026
A churn-heavy platform install intermittently wedges a pod in
ContainerCreating because the cilium-agent rejects its sandbox with
'[PUT /endpoint/{id}][400] putEndpointIdInvalid "IP ipv4:<X> is already
in use"', repeated until the agent restarts or the install times out.
The disputed IP has no live owner: IPAM and the ipcache released it, but
the agent's in-memory endpointManager still holds a stale Endpoint from a
previously-deleted pod whose CNI DEL was skipped or raced (so
removeReferencesLocked never ran). Neither the operator's CiliumEndpoint
GC nor the agent's veth-based endpoint GC can reap it. This surfaced as
~6 of the post-#2558 e2e failures, each on a different victim pod
(velero node-agent, fluent-bit, fdb-operator, cdi-operator, capi
bootstrap), failing PRs unrelated to the change under test. Tracked
upstream at cilium/cilium#38313 (closed stale); no released or
pre-release Cilium fixes it and no flag disables it.

hack/e2e-cilium-endpoint-leak-healer.sh runs as a file-scoped background
watchdog (setup_file/teardown_file). It detects a wedged pod, confirms
the IP is a true orphan -- an endpoint in the owning node's registry
holds it, that endpoint backs no live Running pod, and a different pod is
wedged requesting the same IP -- then evicts only that endpoint via
'cilium-dbg endpoint disconnect ipv4:<X>' (DELETE /endpoint/{id} ->
unexpose -> removeReferencesLocked). Blast radius is one dead endpoint,
not an agent restart. It is read-only until an orphan is positively
confirmed and refuses (loudly) to touch an endpoint backing a live pod,
so a genuine duplicate-IP bug stays visible rather than masked.

This is an interim CI mitigation, not a product fix; remove the script
and the bats hooks once a fixed Cilium ships.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ci Issues or PRs related to CI workflows, GitHub Actions, automation area/testing Issues or PRs related to testing (e2e, bats, unit tests) size/L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants