fix(harbor): drive bucket-secret.yaml from values, gate HR on BucketInfo - #2528
Conversation
Two bugs caused the harbor E2E to fail (CI #25049445125): 1. cozy-harbor's bucket-secret.yaml called `index $existingSecret.data "BucketInfo"` unconditionally. During first-time install the COSI BucketAccess controller may create the credentials Secret as a placeholder before populating it, so `.data` is nil and the chart render crashes with `index of untyped nil`. 2. The downstream `<release>-system` HelmRelease starts reconciling immediately, in parallel with bucket provisioning, hitting bug #1 on every retry within the test's 5-minute window. The previous shape relied on `lookup` to read the Secret at render time. helm-controller's upgrade trigger is digest-based over composed values + chart artifact, so a `lookup` returning new data on a later reconcile is not enough on its own to force an upgrade — the rendered output may diverge but the digest does not. Switch to a values-driven shape: - `bucket-secret.yaml` now reads `.Values.bucket.bucketInfo` (a JSON string) and uses `dig` for safe access; if the value is empty or the expected nested fields are missing, no `*-registry-s3` Secret is rendered. - The `<release>-system` HR sources `BucketInfo` via `valuesFrom` with `targetPath: bucket.bucketInfo`. With the default `optional: false`, helm-controller will refuse to compose values until the key exists, which both gates initial reconciliation on the BucketAccess Secret being populated and forces a config-digest change (and thus a helm upgrade) when its contents change. - `bucket.secretName` is removed from the system chart's values and from the apps chart's `values:` block; the only consumer (the `lookup` call) is gone. Verified four scenarios via `helm template` against the system chart: unset bucketInfo, empty string, empty JSON object `{}`, and a fully-populated BucketInfo — only the last renders the registry-s3 Secret; the others render nothing without erroring. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
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 improves the reliability and reconciliation logic for Harbor's S3 bucket configuration. By shifting from a lookup-based mechanism to a values-driven approach, the system now correctly gates the deployment on the availability of bucket credentials and ensures that configuration changes trigger appropriate Helm upgrades. 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. 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
|
|
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 (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughHelmRelease value injection now pulls bucket configuration from a Secret ( Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as Helm Controller
participant HelmRelease as HelmRelease (manifest)
participant BucketSecret as Secret ({{Release}}-registry-bucket)
participant HelmChart as Harbor Helm Chart
participant K8s as Kubernetes API
Controller->>HelmRelease: read HelmRelease (includes valuesFrom -> Secret key BucketInfo)
Controller->>BucketSecret: fetch Secret `{{Release}}-registry-bucket`
BucketSecret-->>Controller: return BucketInfo (merged into release values)
Controller->>HelmChart: render chart with merged values
HelmChart->>K8s: create/update `*-registry-s3` Secret (if spec.secretS3 & spec.bucketName present)
K8s-->>HelmChart: Secret created/updated → Helm release reconciled
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
There was a problem hiding this comment.
Code Review
This pull request updates the Harbor package to source bucket configuration via the COSI BucketAccess controller, replacing the direct secret name reference with a dynamically sourced bucketInfo value. The templates have been refactored to parse this information and use safer field access methods. A review comment suggests further refining the conditional logic in bucket-secret.yaml to ensure the Secret manifest is only generated when all required bucket information is present, and recommends using the dig function consistently for all field extractions.
| {{- $secretS3 := dig "spec" "secretS3" dict $bucketInfo }} | ||
| {{- if $secretS3 }} | ||
| {{- $accessKeyID := index $secretS3 "accessKeyID" }} | ||
| {{- $accessSecretKey := index $secretS3 "accessSecretKey" }} | ||
| {{- $endpoint := index $secretS3 "endpoint" }} | ||
| {{- $bucketName := dig "spec" "bucketName" "" $bucketInfo }} |
There was a problem hiding this comment.
To fully align with the PR description's goal of rendering nothing for partial values, consider moving the $bucketName extraction up and including it in the conditional check. This ensures the Secret is only generated when both credentials and the bucket name are available. Additionally, using dig for the individual fields provides safer defaults and more consistent logic compared to index.
{{- $secretS3 := dig "spec" "secretS3" dict $bucketInfo }}
{{- $bucketName := dig "spec" "bucketName" "" $bucketInfo }}
{{- if and $secretS3 $bucketName }}
{{- $accessKeyID := dig "accessKeyID" "" $secretS3 }}
{{- $accessSecretKey := dig "accessSecretKey" "" $secretS3 }}
{{- $endpoint := dig "endpoint" "" $secretS3 }}There was a problem hiding this comment.
Tightened the guard in 5fb337d: the template now wraps rendering in {{- if and .accessKeyID .accessSecretKey .endpoint $.Values.spec.bucketName }} inside the existing with .Values.spec/with .secretS3 blocks, so the registry-s3 Secret is only emitted once every required field is populated. Partial BucketInfo now falls into the no-render path alongside missing/empty input. helm-unittest coverage added in 4ecb8fb pins all four scenarios.
There was a problem hiding this comment.
To confirm explicitly: the if and .accessKeyID .accessSecretKey .endpoint $.Values.spec.bucketName guard requires every required field non-empty before the Secret block is emitted — functionally identical to the proposed if and $accessKeyID $accessSecretKey $endpoint $bucketName form. Coverage in tests/bucket_secret_test.yaml (4ecb8fb) pins partial-secretS3 and missing-bucketName as no-render cases alongside spec-absent / spec-empty / secretS3-empty (7/7 green). Happy to refactor the body to explicit dig reads as a follow-up if reviewers prefer that style.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/system/harbor/templates/bucket-secret.yaml`:
- Around line 3-8: The template currently only checks $secretS3 existence and
then unconditionally indexes accessKeyID/accessSecretKey/endpoint/bucketName
which can emit empty values; update the conditional so the Secret is rendered
only when all required BucketInfo fields are present (accessKeyID,
accessSecretKey, endpoint, bucketName). Concretely: evaluate each key from
$secretS3 using index/dig into variables ($accessKeyID, $accessSecretKey,
$endpoint, $bucketName) and change the outer if to require all four to be
non-empty (e.g., combine checks with and or a single conditional that tests each
variable) so the Secret block is skipped unless every required field exists.
🪄 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: bdad2c96-61d3-427d-80d3-4f5fd3258e5e
📒 Files selected for processing (3)
packages/apps/harbor/templates/harbor.yamlpackages/system/harbor/templates/bucket-secret.yamlpackages/system/harbor/values.yaml
The previous fix (a21c18f) sourced BucketInfo via Flux valuesFrom with `targetPath: bucket.bucketInfo`. Flux runs values with `targetPath` through Helm's `strvals.ParseInto`, which splits the value on commas as list separators. The COSI BucketInfo JSON is comma-rich, so values resolution bailed: could not resolve Secret chart values reference 'tenant-X/harbor-X-registry-bucket' with key 'BucketInfo': key "\"spec\":{\"bucketName\":\"bucket-1785...\"" has no value (cannot end with ,) Drop `targetPath`. With only `valuesKey: BucketInfo`, helm-controller unmarshals the value as YAML and merges at the chart's values root, so JSON commas stay nested instead of being split. The system chart's bucket-secret.yaml now reads `.Values.spec.bucketName` / `.Values.spec.secretS3.{accessKeyID,accessSecretKey,endpoint}`, guarded by nested `with` blocks so the registry-s3 Secret renders only when secretS3 has been populated. Gating semantics from the previous fix are preserved: with the default `optional: false`, helm-controller still refuses to compose values until the COSI BucketAccess controller writes `BucketInfo` into the Secret, and the value's content still drives the HR config-digest. Verified via `helm template` against the system chart with three scenarios: missing `spec` and `spec` without `secretS3` render nothing; full BucketInfo renders all five S3 env keys correctly. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/system/harbor/templates/bucket-secret.yaml (1)
2-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRender
registry-s3only when all required BucketInfo fields are present.The current
with .secretS3gate still allows partially populated data, so this template can emit a Secret with empty S3 credentials or bucket name.Proposed fix
{{- with .Values.spec }} {{- with .secretS3 }} +{{- $accessKeyID := index . "accessKeyID" }} +{{- $accessSecretKey := index . "accessSecretKey" }} +{{- $endpoint := index . "endpoint" }} +{{- $bucketName := $.Values.spec.bucketName }} +{{- if and $accessKeyID $accessSecretKey $endpoint $bucketName }} --- apiVersion: v1 kind: Secret metadata: name: {{ $.Values.harbor.fullnameOverride }}-registry-s3 type: Opaque stringData: - REGISTRY_STORAGE_S3_ACCESSKEY: {{ index . "accessKeyID" | quote }} - REGISTRY_STORAGE_S3_SECRETKEY: {{ index . "accessSecretKey" | quote }} - REGISTRY_STORAGE_S3_REGIONENDPOINT: {{ index . "endpoint" | quote }} - REGISTRY_STORAGE_S3_BUCKET: {{ $.Values.spec.bucketName | quote }} + REGISTRY_STORAGE_S3_ACCESSKEY: {{ $accessKeyID | quote }} + REGISTRY_STORAGE_S3_SECRETKEY: {{ $accessSecretKey | quote }} + REGISTRY_STORAGE_S3_REGIONENDPOINT: {{ $endpoint | quote }} + REGISTRY_STORAGE_S3_BUCKET: {{ $bucketName | quote }} REGISTRY_STORAGE_S3_REGION: "us-east-1" +{{- end }} {{- end }} {{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/harbor/templates/bucket-secret.yaml` around lines 2 - 14, The template may render the registry-s3 Secret with missing fields because it only guards with .secretS3; update the Helm conditional to require all required BucketInfo fields before emitting the Secret (check accessKeyID, accessSecretKey, endpoint from .secretS3 and bucketName from $.Values.spec). Replace the current "{{- with .secretS3 }}" guard around the Secret with an explicit if that uses and(...) or separate checks (e.g., if and (index . "accessKeyID") (index . "accessSecretKey") (index . "endpoint") ($.Values.spec.bucketName)) so the Secret (metadata name {{ $.Values.harbor.fullnameOverride }}-registry-s3 and keys REGISTRY_STORAGE_S3_*) is only created when every field is non-empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/system/harbor/templates/bucket-secret.yaml`:
- Around line 2-14: The template may render the registry-s3 Secret with missing
fields because it only guards with .secretS3; update the Helm conditional to
require all required BucketInfo fields before emitting the Secret (check
accessKeyID, accessSecretKey, endpoint from .secretS3 and bucketName from
$.Values.spec). Replace the current "{{- with .secretS3 }}" guard around the
Secret with an explicit if that uses and(...) or separate checks (e.g., if and
(index . "accessKeyID") (index . "accessSecretKey") (index . "endpoint")
($.Values.spec.bucketName)) so the Secret (metadata name {{
$.Values.harbor.fullnameOverride }}-registry-s3 and keys REGISTRY_STORAGE_S3_*)
is only created when every field is non-empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65463b00-3757-4f96-98aa-881024b90893
📒 Files selected for processing (3)
packages/apps/harbor/templates/harbor.yamlpackages/system/harbor/templates/bucket-secret.yamlpackages/system/harbor/values.yaml
💤 Files with no reviewable changes (1)
- packages/system/harbor/values.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/apps/harbor/templates/harbor.yaml
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — the valuesFrom/no-targetPath reshape is the right primitive (gates HR on the BucketAccess Secret, dodges strvals comma-splitting, and routes the data through the digest path). But the chart has no tests/ directory at all, and this PR substantively reshapes the conditional rendering of bucket-secret.yaml across at least four input scenarios — none of which are pinned by helm-unittest assertions. CodeRabbit and Gemini also flagged a fifth scenario (partial BucketInfo) that the current guard doesn't cover.
Business context: Replace the lookup-against-Secret pattern in harbor/templates/bucket-secret.yaml with a values-driven path so the chart no longer crashes on index of untyped nil when the COSI BucketAccess Secret hasn't been written yet, and so credential changes actually trigger a helm upgrade.
Blockers
B1: chart has no helm-unittest coverage and this PR pivots conditional rendering
File: packages/system/harbor/templates/bucket-secret.yaml (no packages/system/harbor/tests/ exists, no packages/apps/harbor/tests/ either)
Issue: The PR description lists four manually-helm template'd cases (unset, empty string, empty JSON {}, fully populated) but none are committed as helm-unittest assertions. Cozystack's convention is that every chart ships test coverage (hack/helm-unit-tests.sh auto-discovers tests/); harbor is one of the holdouts, and the most active path of this PR — conditional Secret rendering driven by .Values.spec.secretS3 — is exactly the kind of thing tests/bucket-secret_test.yaml would pin. Without it, the next change to this template can silently regress any of the four scenarios.
Evidence: ls packages/system/harbor/tests/ → No such file or directory. find packages/{system,apps}/harbor -name '*_test.yaml' → empty. The CI make unit-tests step (in place since #1643 / commit 1c9ae2b) runs against any package with a tests/ dir; harbor is currently un-asserted.
Fix: Add packages/system/harbor/tests/bucket-secret_test.yaml with four test cases mirroring the PR description's manual test plan: (a) .Values.spec unset → no Secret rendered, (b) .Values.spec.secretS3 empty object → no Secret, (c) all required fields present → Secret with the four expected stringData entries, (d) optionally a partial-population case once B2 is addressed (asserts no Secret rendered).
B2: partial-population renders Secret with <no value> strings
File: packages/system/harbor/templates/bucket-secret.yaml:3-8
Issue: The current guard {{- with .secretS3 }} only checks that the secretS3 object is truthy — not that accessKeyID, accessSecretKey, endpoint, and $.Values.spec.bucketName are all populated. If a future BucketAccess controller bug or schema drift writes a partial BucketInfo, the rendered Secret carries "<no value>" for the missing keys and harbor registry silently fails S3 auth.
Evidence: Both CodeRabbit (Major) and Gemini (Low) flagged this on the same template lines. In practice COSI writes BucketInfo atomically so partial state is unlikely, but the PR description's stated intent is "renders nothing rather than crashing" — partial-populate is the gap between that intent and the current guard.
Fix: Tighten the guard to {{- if and .accessKeyID .accessSecretKey .endpoint $.Values.spec.bucketName }} (or extract a $ready helper), so partial input falls into the no-render path alongside unset/empty.
The COSI BucketAccess controller is expected to populate `BucketInfo` atomically, but the prior guard (`with .Values.spec` / `with .secretS3`) only short-circuits on missing or empty inputs. A partially populated `secretS3` would still emit the registry-s3 Secret with empty `accessSecretKey`/`endpoint` values, and a missing `bucketName` was not guarded at all — silently breaking Harbor's S3 storage backend. Tighten the inner guard to require all four required fields (accessKeyID, accessSecretKey, endpoint, bucketName) before the Secret is emitted, so partial input falls into the no-render path alongside unset/empty. The body now uses dot-access on `.secretS3` for consistency with the new gate. Behaviour for fully populated input is unchanged. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
`hack/helm-unit-tests.sh` auto-discovers any package whose Makefile has a `test` target. `packages/system/harbor` had none, so the conditional rendering of `bucket-secret.yaml` (the path this PR actively reshapes) was un-asserted in CI and the bucket-secret guard would not be pinned against future refactors. Add the canonical `test: helm unittest .` target, matching every other `packages/system/*` package that ships a `tests/` directory (linstor-scheduler, linstor-gui, cozystack-scheduler, nfs-driver). Add `tests/bucket_secret_test.yaml` with seven cases covering: spec absent, spec empty, secretS3 absent, secretS3 empty, secretS3 partial (only one inner field), bucketName missing, and fully populated. The first six assert no Secret is rendered; the last asserts the four expected `stringData` entries plus the static region. 7/7 pass locally (`bash hack/helm-unit-tests.sh`). Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
Aleksei Sviridkin (@lexfrei) both blockers addressed: B2 (5fb337d): tightened the guard to B1 (4ecb8fb): added |
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM. Both blockers from the prior review are addressed:
- B1 (helm-unittest coverage):
4ecb8fb0addspackages/system/harbor/tests/bucket_secret_test.yaml+test:target inpackages/system/harbor/Makefile. Covers seven scenarios — spec absent, spec empty, secretS3 absent, secretS3 empty, secretS3 partial, bucketName missing, fully populated — and pins the rendered<release>-registry-s3Secret's stringData fields. 7/7 pass locally (helm unittest .frompackages/system/harbor). - B2 (partial-population guard):
5fb337detightens the conditional to{{- if and .accessKeyID .accessSecretKey .endpoint $.Values.spec.bucketName }}inside the existingwithblocks. Partial BucketInfo now falls into the no-render path alongside missing/empty input. The dedicated test case ("no Secret rendered when secretS3 is partially populated") pins this against future regressions.
The final shape of the valuesFrom integration is also right: 2555a9ac drops targetPath so Flux unmarshals the COSI-managed BucketInfo JSON at the values root instead of running it through Helm's strvals parser (which would split JSON on commas). The inline comment in packages/apps/harbor/templates/harbor.yaml documents the reasoning. Combined with default optional: false, this gates the <release>-system HelmRelease on the BucketAccess Secret being populated and routes credential changes through the digest path so a helm upgrade actually fires when they arrive.
E2E Tests: SUCCESS on this HEAD — the last unchecked item in the test plan ("re-run harbor E2E on a fresh stand") is effectively verified by CI.
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>
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> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
Successfully created backport PR for |
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> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
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>
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>
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>
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>
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>
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>
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>
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>
…ay.bats tenant teardown (#2558) ## 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](https://github.com/cozystack/cozystack/actions/runs/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`: - ✅ **#2508** — installer namespace bootstrap (Helm namespace-ownership conflict on cold install) — merged - ✅ **#2509** — operator HelmRelease config knobs (`seaweedfs-system` 2-min wait race within Flux's 5-min reconcile windows) — merged - ✅ **#2528** — harbor bucket-secret + BucketInfo gating (harbor `ValuesError` on first install) — merged - ✅ **#2529** — objectstorage-controller BucketAccess conflict retry — merged Companion PRs in the #2619 split (independent of this PR, ordering-wise): - **#2602** — Flux v2.8.0 + chart fixes - **#2601** — seaweedfs-system split 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 ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2558?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 -->
Summary
cozy-harbor/templates/bucket-secret.yamlno longer relies onlookupagainst the BucketAccess credentials Secret. It is now driven by.Values.bucket.bucketInfo(a JSON string) withdig-based safe accessors, so a missing/empty/partial value renders nothing rather than crashing withindex of untyped nil(the failure mode that broke harbor in CI #25049445125).<release>-systemHelmRelease now sourcesBucketInfoviavaluesFrom(valuesKey: BucketInfo,targetPath: bucket.bucketInfo). With the defaultoptional: false, helm-controller refuses to compose values until the COSI BucketAccess controller has populated the Secret — both gating initial reconciliation and forcing a config-digest change (and thus a helm upgrade) when the credentials arrive. Flux HRdependsOncannot reference COSI resources directly, sovaluesFromis the correct primitive here.bucket.secretNamewas the only consumer of the oldlookupand is removed from both the system chart'svalues.yamldefault and the apps chart'svalues:block.Why this shape
helm-controller's upgrade trigger is digest-based over composed values + chart artifact; a
lookupreturning new data on a later reconcile is not enough to force an upgrade on its own. RoutingBucketInfothroughvaluesFromputs the bucket data on the digest path, so the chart re-applies the moment credentials become available.Test plan
helm templateofpackages/system/harborwithbucket.bucketInfounset, empty string, empty JSON object{}, and fully populated — only the populated case renders the*-registry-s3Secret; the others render nothing without erroring.helm templateofpackages/apps/harborproduces the expected<release>-systemHR with the newvaluesFromentry and the cleaned-upvalues:block.Summary by CodeRabbit