refactor(seaweedfs): split seaweedfs-system into seaweedfs-db + seaweedfs-system - #2601
Conversation
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 addresses a race condition during fresh tenant installations where seaweedfs-filer pods would crashloop due to the database not being ready. By decoupling the database infrastructure from the application components and utilizing Flux's dependency management and health check expressions, we ensure a reliable deployment order. This change requires Flux 2.8.x and includes a migration path for existing deployments. Highlights
New Features🧠 You can now enable Memory (public preview) 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 the 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 counterproductive. 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
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request splits the SeaweedFS system into two coordinated HelmReleases to eliminate a startup race condition. The database release (seaweedfs-db) is now provisioned first with CNPG readiness gating via healthCheckExprs, and the workload release (seaweedfs-system) depends on successful database deployment. A migration script handles existing deployments. ChangesSeaweedFS System Split
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. Comment |
There was a problem hiding this comment.
Code Review
This pull request splits the seaweedfs-system HelmRelease into seaweedfs-db and seaweedfs-system to ensure the database is ready before the application starts, resolving an EPERM crashloop issue. The changes introduce a new system chart, a migration script for existing clusters, and utilize FluxCD's healthCheckExprs for readiness gating. Feedback identifies a shell compatibility issue in the migration script, missing resource fields in the database values schema, and the presence of local absolute paths in the implementation plan.
| @@ -0,0 +1,32 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
The script uses set -o pipefail (on line 13), which is a bash-specific option and not supported by POSIX-compliant shells like dash (the default /bin/sh on many systems). To ensure the script executes correctly with this option, the shebang should be changed to #!/bin/bash.
| #!/bin/sh | |
| #!/bin/bash |
| ## @typedef {struct} DB - Database configuration. | ||
| ## @field {int} [replicas] - Number of database replicas. | ||
| ## @field {quantity} [size] - Persistent Volume size. | ||
| ## @field {string} [storageClass] - StorageClass used to store the data. |
There was a problem hiding this comment.
The DB typedef is missing the resources and resourcesPreset fields. These are being passed by the wrapper template in packages/extra/seaweedfs/templates/seaweedfs-db.yaml. Adding them here ensures they are included in the generated JSON schema and documentation. Note that the database.yaml template in this chart also needs to be updated to use these values instead of hardcoded ones.
## @typedef {struct} DB - Database configuration.
## @field {int} [replicas] - Number of database replicas.
## @field {quantity} [size] - Persistent Volume size.
## @field {string} [storageClass] - StorageClass used to store the data.
## @field {string} [resourcesPreset] - Resource preset.
## @field {Resources} [resources] - Resource configuration.| db: | ||
| replicas: 2 | ||
| size: 10Gi | ||
| storageClass: "" |
There was a problem hiding this comment.
Include default values for resourcesPreset and resources in the db block to align with the updated typedef and provide defaults for the schema.
db:
replicas: 2
size: 10Gi
storageClass: ""
resourcesPreset: "nanosmall"
resources: {}|
|
||
| - [ ] **Step 3: Verify directory structure** | ||
|
|
||
| Run: `ls /home/daniil/aenix/cozystack-split-seaweedfs/packages/system/seaweedfs-db/` |
There was a problem hiding this comment.
This document contains absolute paths specific to the author's local environment (e.g., /home/daniil/aenix/cozystack-split-seaweedfs/). These should be replaced with relative paths or placeholders to ensure the documentation is portable and clean. This applies to multiple locations throughout the file.
| Run: `ls /home/daniil/aenix/cozystack-split-seaweedfs/packages/system/seaweedfs-db/` | |
| Run: ls packages/system/seaweedfs-db/ |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/system/seaweedfs-db/Chart.yaml (1)
1-3: 💤 Low valueConsider adding a description field.
While not strictly required, adding a
descriptionfield to the Chart.yaml improves discoverability and follows Helm best practices. The sibling charts in this repository may provide examples.📝 Suggested addition
apiVersion: v2 name: cozy-seaweedfs-db +description: SeaweedFS Database (CNPG Cluster) for Cozystack version: 0.0.0 # Placeholder, the actual version will be automatically set during the build process🤖 Prompt for 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. In `@packages/system/seaweedfs-db/Chart.yaml` around lines 1 - 3, Add a descriptive "description" field to the Helm chart metadata in Chart.yaml for cozy-seaweedfs-db: update the Chart.yaml (the top-level apiVersion/name/version block) to include a concise human-readable description string explaining the chart purpose (e.g., what cozy-seaweedfs-db deploys and any notable behavior), matching style of sibling charts so discoverability and Helm best practices are followed.docs/superpowers/specs/2026-05-10-split-seaweedfs-system-design.md (1)
7-9: 💤 Low valueAdd language specifier to fenced code block.
The code block showing the error message should specify a language for proper syntax highlighting and to satisfy the markdown linter.
📝 Suggested fix
-``` +```text dial tcp <ClusterIP>:5432 (seaweedfs-db-rw): connect: operation not permitted</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/superpowers/specs/2026-05-10-split-seaweedfs-system-design.mdaround
lines 7 - 9, The fenced code block containing the error message in the document
should include a language specifier for proper highlighting and linting; update
the fenced block around the line with the text "dial tcp :5432
(seaweedfs-db-rw): connect: operation not permitted" (the existing
triple-backtick block) to use a language tag such as text (i.e., change ``` todocs/superpowers/plans/2026-05-10-split-seaweedfs-system.md (1)
169-175: 💤 Low valueAdd language specifier to fenced code block.
For consistency with the rest of the plan and to satisfy the markdown linter, add a language specifier to this code block.
📝 Suggested fix
-``` +```yaml --- apiVersion: postgresql.cnpg.io/v1 kind: Cluster🤖 Prompt for 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. In `@docs/superpowers/plans/2026-05-10-split-seaweedfs-system.md` around lines 169 - 175, The fenced code block containing the Kubernetes manifest (starting with "apiVersion: postgresql.cnpg.io/v1", "kind: Cluster", and "metadata: name: seaweedfs-db") needs a language specifier; update the opening fence from ``` to ```yaml so the block becomes a YAML code block to satisfy the markdown linter and maintain consistency with other plan examples.packages/system/seaweedfs-db/values.yaml (1)
5-7: 💤 Low valueUnused typedef: Resources is documented but not referenced.
The
Resourcestypedef definescpuandmemoryfields, but these are not used anywhere in thedbconfiguration below. This may be a copy-paste artifact from another values file.♻️ Suggested cleanup
If the Resources typedef is not needed:
## ## `@section` Database parameters ## -## `@typedef` {struct} Resources - Resource configuration. -## `@field` {quantity} [cpu] - Number of CPU cores allocated. -## `@field` {quantity} [memory] - Amount of memory allocated. - ## `@typedef` {struct} DB - Database configuration. ## `@field` {int} [replicas] - Number of database replicas. ## `@field` {quantity} [size] - Persistent Volume size. ## `@field` {string} [storageClass] - StorageClass used to store the data.However, if resources will be added to the db configuration in the future, keeping the typedef is fine.
🤖 Prompt for 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. In `@packages/system/seaweedfs-db/values.yaml` around lines 5 - 7, The typedef "Resources" (struct Resources with cpu and memory) is declared but not referenced by the db configuration; either remove this unused typedef block or wire it into the db values by adding a resources entry that consumes cpu and memory (e.g., ensure the db chart's values structure includes a resources object and any templates reference values.db.resources.cpu/memory or similar). Locate the "Resources" typedef and either delete it or add a values key (e.g., db.resources) and update relevant templates/deployment specs to read values.db.resources.cpu and values.db.resources.memory so the fields are actually used.packages/extra/seaweedfs/templates/seaweedfs.yaml (1)
111-113: ⚡ Quick winDocument the rationale for the 10× faster reconciliation interval.
The
intervalwas reduced from5mto30s, which increases reconciliation frequency by 10×. This could impact cluster load, especially with many tenants. Please clarify whether this aggressive interval is necessary for quick recovery after the DB becomes ready, or if a more moderate value (e.g.,1mor2m) would suffice.🤖 Prompt for 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. In `@packages/extra/seaweedfs/templates/seaweedfs.yaml` around lines 111 - 113, Add a short in-file comment above the interval setting that explains why the reconciliation interval for the SeaweedFS resource was reduced from 5m to 30s (or change the value) — reference the interval key and the dependsOn entry ({{ .Release.Name }}-db) and state whether the 30s is required for fast recovery after the DB becomes ready or if a moderate value like 1m/2m is preferred to reduce load; update the interval to the chosen moderate value if you decide not to keep 30s and ensure the rationale comment documents the trade-offs for cluster load and tenant scale.
🤖 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 `@docs/superpowers/specs/2026-05-10-split-seaweedfs-system-design.md`:
- Around line 68-82: This PR adds HelmRelease.spec.healthCheckExprs (see
HelmRelease.spec.healthCheckExprs in
packages/extra/seaweedfs/templates/seaweedfs-db.yaml) which requires
helm-controller v1.5.0+/Flux 2.8.x; because the repo is pinned to Flux 2.7.x
(helm-controller v1.4.3) you must not merge this change until the Flux upgrade
lands—either (a) remove or revert the healthCheckExprs usage and instead use a
supported wait strategy/timeout fallback, or (b) gate this PR on the Flux
upgrade by confirming the Flux 2.8.x upgrade PR exists and will merge first and
add a note in this PR linking that upgrade and/or add CI gating that blocks
merge until the upgrade is merged.
In `@packages/extra/seaweedfs/templates/seaweedfs-db.yaml`:
- Around line 27-31: Add a comment immediately above the healthCheckExprs block
documenting that this field requires Flux v2.8.x (helm-controller v1.5.0+) and
that on older Flux (e.g., v2.7.x) the HelmRelease will either fail to parse or
silently ignore healthCheckExprs, removing the readiness check; update the
comment to mention the minimum Flux and helm-controller versions and the
resulting behavior so maintainers know to upgrade Flux before relying on the
healthCheckExprs in this HelmRelease.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-05-10-split-seaweedfs-system.md`:
- Around line 169-175: The fenced code block containing the Kubernetes manifest
(starting with "apiVersion: postgresql.cnpg.io/v1", "kind: Cluster", and
"metadata: name: seaweedfs-db") needs a language specifier; update the opening
fence from ``` to ```yaml so the block becomes a YAML code block to satisfy the
markdown linter and maintain consistency with other plan examples.
In `@docs/superpowers/specs/2026-05-10-split-seaweedfs-system-design.md`:
- Around line 7-9: The fenced code block containing the error message in the
document should include a language specifier for proper highlighting and
linting; update the fenced block around the line with the text "dial tcp
<ClusterIP>:5432 (seaweedfs-db-rw): connect: operation not permitted" (the
existing triple-backtick block) to use a language tag such as text (i.e., change
``` to ```text) so the markdown linter and renderers treat it as plain text.
In `@packages/extra/seaweedfs/templates/seaweedfs.yaml`:
- Around line 111-113: Add a short in-file comment above the interval setting
that explains why the reconciliation interval for the SeaweedFS resource was
reduced from 5m to 30s (or change the value) — reference the interval key and
the dependsOn entry ({{ .Release.Name }}-db) and state whether the 30s is
required for fast recovery after the DB becomes ready or if a moderate value
like 1m/2m is preferred to reduce load; update the interval to the chosen
moderate value if you decide not to keep 30s and ensure the rationale comment
documents the trade-offs for cluster load and tenant scale.
In `@packages/system/seaweedfs-db/Chart.yaml`:
- Around line 1-3: Add a descriptive "description" field to the Helm chart
metadata in Chart.yaml for cozy-seaweedfs-db: update the Chart.yaml (the
top-level apiVersion/name/version block) to include a concise human-readable
description string explaining the chart purpose (e.g., what cozy-seaweedfs-db
deploys and any notable behavior), matching style of sibling charts so
discoverability and Helm best practices are followed.
In `@packages/system/seaweedfs-db/values.yaml`:
- Around line 5-7: The typedef "Resources" (struct Resources with cpu and
memory) is declared but not referenced by the db configuration; either remove
this unused typedef block or wire it into the db values by adding a resources
entry that consumes cpu and memory (e.g., ensure the db chart's values structure
includes a resources object and any templates reference
values.db.resources.cpu/memory or similar). Locate the "Resources" typedef and
either delete it or add a values key (e.g., db.resources) and update relevant
templates/deployment specs to read values.db.resources.cpu and
values.db.resources.memory so the fields are actually used.
🪄 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: d9aed729-becb-4727-a3d1-b0a9ea155b00
📒 Files selected for processing (13)
docs/superpowers/plans/2026-05-10-split-seaweedfs-system.mddocs/superpowers/specs/2026-05-10-split-seaweedfs-system-design.mdpackages/core/platform/images/migrations/migrations/39packages/core/platform/sources/seaweedfs-application.yamlpackages/extra/seaweedfs/templates/seaweedfs-db.yamlpackages/extra/seaweedfs/templates/seaweedfs.yamlpackages/system/seaweedfs-db/Chart.yamlpackages/system/seaweedfs-db/Makefilepackages/system/seaweedfs-db/README.mdpackages/system/seaweedfs-db/templates/database.yamlpackages/system/seaweedfs-db/values.schema.jsonpackages/system/seaweedfs-db/values.yamlpackages/system/seaweedfs/values.yaml
💤 Files with no reviewable changes (1)
- packages/system/seaweedfs/values.yaml
| ```yaml | ||
| spec: | ||
| waitStrategy: | ||
| name: poller # required for healthCheckExprs to be evaluated | ||
| healthCheckExprs: | ||
| - apiVersion: postgresql.cnpg.io/v1 | ||
| kind: Cluster | ||
| current: has(status.conditions) && status.conditions.exists(e, e.type == 'Ready' && e.status == 'True') | ||
| failed: has(status.conditions) && status.conditions.exists(e, e.type == 'Ready' && e.status == 'False') | ||
| ``` | ||
|
|
||
| `exists` requires at least one matching condition element, so a missing `status.conditions` evaluates to `false` and the wait keeps polling — important early in the lifecycle when the CR has been applied but CNPG hasn't yet stamped any conditions. | ||
|
|
||
| The expression matches every `postgresql.cnpg.io/v1 Cluster` resource managed by the release (only `seaweedfs-db` here). The HR's `wait` phase blocks until `current` returns `true`, which only happens once CNPG sets `Ready=True` on the Cluster. Helm's existing `timeout: 10m` is the outer ceiling — if CNPG never reaches Ready, the HR fails with a clear message instead of silently letting the dependent HR proceed. | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check current Flux version in the repository
echo "Searching for Flux version references..."
rg -n "flux.*version|fluxcd.*v2\." --type=yaml -C2 -g '!docs/**'Repository: cozystack/cozystack
Length of output: 523
🏁 Script executed:
# Search for Flux version pins more broadly
find . -type f \( -name "*.yaml" -o -name "*.yml" -o -name "*.md" -o -name "*.json" \) ! -path "*/.*" | head -50 | xargs grep -l "flux.*2\." 2>/dev/null | head -20Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Look for version information in common locations
echo "=== Checking for FluxCD version in various config files ==="
for file in fluxcd.yaml flux-release.yaml Chart.yaml kustomization.yaml values.yaml release.yaml; do
find . -name "$file" -type f ! -path "*/.*" -exec grep -H "version" {} \; 2>/dev/null
doneRepository: cozystack/cozystack
Length of output: 50375
🏁 Script executed:
# Search the docs directory for healthCheckExprs references
echo "=== Searching for healthCheckExprs feature documentation ==="
rg -n "healthCheckExprs" --type=yaml --type=markdownRepository: cozystack/cozystack
Length of output: 6096
🏁 Script executed:
# Check for Flux version in the main README or installation files
echo "=== Checking README and main docs ==="
rg -n "flux.*2\.[0-9]" --type=markdown -C 2 docs/ 2>/dev/null | head -40Repository: cozystack/cozystack
Length of output: 701
🏁 Script executed:
# Look at the actual seaweedfs-db template to see current implementation
head -50 ./packages/extra/seaweedfs/templates/seaweedfs-db.yamlRepository: cozystack/cozystack
Length of output: 1451
🏁 Script executed:
# Search for any version requirements or comments about healthCheckExprs
rg -i "healthcheckexprs|helm.*controller|v1.5.0" --type=yaml --type=markdown -B 2 -A 2Repository: cozystack/cozystack
Length of output: 50377
🏁 Script executed:
# Check Flux release notes or documentation in the repo
find . -type f -name "*.md" ! -path "*/.*" -exec rg -l "helm.controller|healthCheckExprs.*version|2\.8" {} \;Repository: cozystack/cozystack
Length of output: 407
🌐 Web query:
Flux 2.8.0 helm-controller v1.5.0 healthCheckExprs feature release
💡 Result:
Flux 2.8.0 (feature release) shipped with helm-controller v1.5.0 on 2026-02-24, and the helm-controller v1.5.0 release introduced the HelmRelease.spec.healthCheckExprs feature (custom health checks via CEL expressions) [1][2]. What changed (healthCheckExprs / CEL health checks): - helm-controller v1.5.0 added support for custom health checks on HelmRelease using CEL expressions via the.spec.healthCheckExprs field [2][3]. - The helm-controller API docs specify that.spec.healthCheckExprs evaluates CEL expressions for custom resources (with per-entry apiVersion/kind, and optional inProgress/failed and required current) and that evaluation order is inProgress → failed → current; the first expression that returns true determines the custom resource’s health status [4]. - The feature is tied to the Helm v4 “kstatus” based health checking flow: the GitHub issue notes it is only available for kstatus health checks implemented in Helm 4 (i.e., the waiter WaitStrategy), and the legacy WaitStrategy won’t be able to use it [5]. Release timing / versions: - Flux v2.8.0 release tag is dated 2026-02-24 and explicitly lists helm-controller v1.5.0 as part of the Flux 2.8.0 component versions [1]. - helm-controller v1.5.0 changelog shows release date 2026-02-20 and documents the defaults/behavior changes plus the addition of.spec.healthCheckExprs support [2].
Citations:
- 1: https://github.com/fluxcd/flux2/releases/tag/v2.8.0
- 2: https://github.com/fluxcd/helm-controller/blob/v1.5.1/CHANGELOG.md
- 3: https://fluxcd.io/blog/2026/02/flux-v2.8.0/
- 4: https://github.com/fluxcd/helm-controller/blob/main/docs/spec/v2/helmreleases.md
- 5: Add support for custom health checks via CEL expressions fluxcd/helm-controller#1382
🏁 Script executed:
# Check for any recent Flux version changes or upgrade PRs
rg -n "flux.*2\.8|helm.*controller.*v1\.5" --type=markdown --type=yaml -C 3 | head -60Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Look for any issue or PR references about Flux upgrades
find . -type f \( -name "*.md" -o -name "*.yaml" \) ! -path "*/.*" -exec grep -l "flux.*upgrade\|2\.8\.0\|upgrade.*flux" {} \; 2>/dev/nullRepository: cozystack/cozystack
Length of output: 232
🏁 Script executed:
# Check the git history for recent Flux-related changes
git log --oneline --all -30 2>/dev/null | grep -i "flux\|helm.controller"Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Check the v1.2.0 changelog for Flux upgrade information
grep -A 5 -B 5 "flux\|2\.8" docs/changelogs/v1.2.0.md | head -40Repository: cozystack/cozystack
Length of output: 3265
🏁 Script executed:
# Check if there are any recent version bumps in values
rg "version.*2\.[78]" packages/system/fluxcd/ packages/core/flux-aio/ -A 2 -B 2Repository: cozystack/cozystack
Length of output: 343
🏁 Script executed:
# Look at the fluxcd values to see what version is specified
cat packages/system/fluxcd/values.yaml | grep -A 10 -B 10 "version"Repository: cozystack/cozystack
Length of output: 522
Block merge until Flux is upgraded to 2.8.x.
The PR uses HelmRelease.spec.healthCheckExprs (in packages/extra/seaweedfs/templates/seaweedfs-db.yaml, lines 27–30) which requires helm-controller v1.5.0+, shipped with Flux 2.8.0. The repository is currently pinned to Flux 2.7.x with helm-controller v1.4.3, which does not support this feature. Verify that a Flux upgrade PR exists and will merge first, or gate this PR on the Flux upgrade completing.
🤖 Prompt for 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.
In `@docs/superpowers/specs/2026-05-10-split-seaweedfs-system-design.md` around
lines 68 - 82, This PR adds HelmRelease.spec.healthCheckExprs (see
HelmRelease.spec.healthCheckExprs in
packages/extra/seaweedfs/templates/seaweedfs-db.yaml) which requires
helm-controller v1.5.0+/Flux 2.8.x; because the repo is pinned to Flux 2.7.x
(helm-controller v1.4.3) you must not merge this change until the Flux upgrade
lands—either (a) remove or revert the healthCheckExprs usage and instead use a
supported wait strategy/timeout fallback, or (b) gate this PR on the Flux
upgrade by confirming the Flux 2.8.x upgrade PR exists and will merge first and
add a note in this PR linking that upgrade and/or add CI gating that blocks
merge until the upgrade is merged.
| healthCheckExprs: | ||
| - apiVersion: postgresql.cnpg.io/v1 | ||
| kind: Cluster | ||
| current: has(status.conditions) && status.conditions.exists(e, e.type == 'Ready' && e.status == 'True') | ||
| failed: has(status.conditions) && status.conditions.exists(e, e.type == 'Ready' && e.status == 'False') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check deployed Flux version for helm-controller
kubectl get deployment -n flux-system helm-controller -o jsonpath='{.spec.template.spec.containers[0].image}' 2>/dev/null || echo "Flux not found"Repository: cozystack/cozystack
Length of output: 78
🏁 Script executed:
# First, let's examine the seaweedfs-db.yaml file
cat -n packages/extra/seaweedfs/templates/seaweedfs-db.yamlRepository: cozystack/cozystack
Length of output: 1752
🏁 Script executed:
# Search for any existing documentation or validation about Flux version in the seaweedfs package
rg -i "flux|helm-controller|version|2\.8" packages/extra/seaweedfs/ -A 2 -B 2Repository: cozystack/cozystack
Length of output: 38057
🏁 Script executed:
# Check if there are any migration or bootstrap scripts that validate Flux version
find packages/extra/seaweedfs -type f \( -name "*.yaml" -o -name "*.sh" -o -name "*.md" \) -exec grep -l "flux\|Flux\|validation\|preflight" {} \;Repository: cozystack/cozystack
Length of output: 166
🏁 Script executed:
# Verify the file structure to ensure umbrella pattern compliance
tree -L 3 packages/extra/seaweedfs/ 2>/dev/null || find packages/extra/seaweedfs -type f -o -type d | head -20Repository: cozystack/cozystack
Length of output: 695
🌐 Web query:
Flux helm-controller healthCheckExprs version requirement 2.8.0
💡 Result:
Flux’s HelmRelease .spec.healthCheckExprs (CEL-based custom health checks) requires helm-controller v1.5.0+; Flux 2.8.0 uses helm-controller v1.5.0 and introduces this support, so it is available starting with Flux 2.8.0 and any helm-controller >=1.5.0.[1][2] What you asked (“Flux helm-controller healthCheckExprs version requirement 2.8.0”): if your Flux version is 2.8.0 (released with helm-controller v1.5.0), you can use .spec.healthCheckExprs on HelmRelease resources.[1][3] Notes/constraints from the official docs: - .spec.healthCheckExprs is evaluated only when Helm wait is enabled (i.e., .spec.disableWait is false) and when the poller wait strategy is used (.spec.waitStrategy.name is poller).[3] - It is implemented via CEL expressions with current (required) and optional inProgress/failed.[3] - The implementation is only available for the kstatus-based health checks in Helm 4 (i.e., the poller wait strategy); users using the legacy wait strategy can’t use the feature.[4] Examples and field semantics are documented here: the HelmRelease “Health check expressions” section.[3]
Citations:
- 1: https://github.com/fluxcd/flux2/releases/tag/v2.8.0
- 2: https://github.com/fluxcd/helm-controller/blob/v1.5.1/CHANGELOG.md
- 3: https://fluxcd.io/flux/components/helm/helmreleases/
- 4: Add support for custom health checks via CEL expressions fluxcd/helm-controller#1382
🌐 Web query:
Flux HelmRelease healthCheckExprs supported version helm-controller
💡 Result:
Flux HelmRelease supports custom health check expressions via the .spec.healthCheckExprs field (CEL expressions), and this feature is tied to the Helm 4 + waitStrategy: poller flow using kstatus readers. [1][2] Supported Helm-controller version (where this feature landed): fluxcd/helm-controller v1.5.0 (released 2026-02-20), which includes “Add support for custom health checks via CEL expressions” and notes that “custom health checks via CEL expressions” are supported via .spec.healthCheckExprs. [2] Implementation/usage notes (to ensure the expressions actually evaluate): - .spec.healthCheckExprs is evaluated only when Helm action wait is enabled (.spec.disableWait is false) and when the poller wait strategy is used (.spec.waitStrategy.name is poller). [1] - The controller evaluates inProgress first, then failed, then current; the first expression that evaluates to true determines health status. [1]
Citations:
- 1: https://fluxcd.io/flux/components/helm/helmreleases/
- 2: https://github.com/fluxcd/helm-controller/blob/v1.5.0/CHANGELOG.md
Document the minimum Flux version requirement for healthCheckExprs.
The healthCheckExprs field on line 27 requires Flux 2.8.x (helm-controller v1.5.0+). If Flux is still on 2.7.x, the HelmRelease will either fail to parse or silently ignore the healthCheckExprs field, defeating the purpose of this readiness check.
Add a comment documenting this dependency:
Proposed documentation addition
+ # REQUIRES: Flux 2.8.x+ (helm-controller v1.5.0+) for healthCheckExprs support
# `poller` waitStrategy is required for healthCheckExprs to be evaluated.
# Without it, the HR flips Ready as soon as helm install applies the Cluster CR,
# before CNPG has bootstrapped postgres.
waitStrategy:🤖 Prompt for 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.
In `@packages/extra/seaweedfs/templates/seaweedfs-db.yaml` around lines 27 - 31,
Add a comment immediately above the healthCheckExprs block documenting that this
field requires Flux v2.8.x (helm-controller v1.5.0+) and that on older Flux
(e.g., v2.7.x) the HelmRelease will either fail to parse or silently ignore
healthCheckExprs, removing the readiness check; update the comment to mention
the minimum Flux and helm-controller versions and the resulting behavior so
maintainers know to upgrade Flux before relying on the healthCheckExprs in this
HelmRelease.
…collision Main has a conditional dependsOn:[ingress-nginx-system] in the application HR (added after #2601 branched). PR #2601 added an unconditional dependsOn:[seaweedfs-db]. The merge produced two `dependsOn:` keys in the same HelmRelease spec, which the Helm post-render rejected: Helm install failed for release tenant-root/seaweedfs ...: error while running post render on files: map[string]interface {}(nil): yaml: unmarshal errors Collapse into one list: the db dependency is unconditional; the ingress dependency stays inside the existing `if eq $ingress .Release.Namespace` guard so sub-tenants that inherit ingress from a parent namespace don't deadlock on a non-existent local HR. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
28b02d5 to
2e50619
Compare
…collision Main has a conditional dependsOn:[ingress-nginx-system] in the application HR (added after #2601 branched). PR #2601 added an unconditional dependsOn:[seaweedfs-db]. The merge produced two `dependsOn:` keys in the same HelmRelease spec, which the Helm post-render rejected: Helm install failed for release tenant-root/seaweedfs ...: error while running post render on files: map[string]interface {}(nil): yaml: unmarshal errors Collapse into one list: the db dependency is unconditional; the ingress dependency stays inside the existing `if eq $ingress .Release.Namespace` guard so sub-tenants that inherit ingress from a parent namespace don't deadlock on a non-existent local HR. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
…collision Main has a conditional dependsOn:[ingress-nginx-system] in the application HR (added after #2601 branched). PR #2601 added an unconditional dependsOn:[seaweedfs-db]. The merge produced two `dependsOn:` keys in the same HelmRelease spec, which the Helm post-render rejected: Helm install failed for release tenant-root/seaweedfs ...: error while running post render on files: map[string]interface {}(nil): yaml: unmarshal errors Collapse into one list: the db dependency is unconditional; the ingress dependency stays inside the existing `if eq $ingress .Release.Namespace` guard so sub-tenants that inherit ingress from a parent namespace don't deadlock on a non-existent local HR. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
2e50619 to
a7a0611
Compare
There was a problem hiding this comment.
Code Review
This pull request refactors the SeaweedFS deployment by extracting the database component into a dedicated seaweedfs-db package and introducing a serial installation chain. It includes a migration script to transition existing clusters to the new Helm release and increases installation timeouts to 15 minutes to account for the extended bootstrap time. Review feedback highlights a portability issue in the migration script's shebang when using pipefail, an inconsistency regarding the upgrade.force flag in the SeaweedFS template, and the need to make resource limits configurable in the new database package.
| @@ -0,0 +1,32 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
The script uses set -o pipefail (line 13), which is a non-POSIX feature not supported by the standard /bin/sh (often dash on Debian/Ubuntu systems). As noted in the E2E tests in this repository, pipefail is unavailable in the default shell. To ensure the script handles pipe failures correctly and remains portable, please change the shebang to #!/bin/bash.
| #!/bin/sh | |
| #!/bin/bash |
There was a problem hiding this comment.
Holding off on this one. The script runs in the migrations container which is alpine:3.22 (see packages/core/platform/images/migrations/Dockerfile:1), where /bin/sh is BusyBox ash. BusyBox ash has supported pipefail since 1.30 (2018) and Alpine 3.22 ships 1.36+, so the dash-on-Debian concern doesn't apply here.
Repo precedent is also consistent: the orchestrator run-migrations.sh and 22 of the existing migrations (19 through 41) all use the same #!/bin/sh + set -euo pipefail pattern. Switching #42 alone to #!/bin/bash would introduce inconsistency without changing behavior.
| force: true | ||
| remediation: | ||
| retries: -1 |
There was a problem hiding this comment.
The pull request description states that upgrade.force: true was dropped to align with a platform-wide cleanup and avoid conflicts with Flux's Server-Side Apply (SSA) mode. However, it is being explicitly added here. If this component is moving towards SSA or if you want to maintain consistency with the "platform-wide cleanup" mentioned, this line should be removed.
remediation:
retries: -1There was a problem hiding this comment.
This line is pre-existing on main and untouched by this PR — git show main:packages/extra/seaweedfs/templates/seaweedfs.yaml has the same upgrade: { force: true } at the same location. The PR diff doesn't add it.
The PR body's "The new HR's upgrade.force: true was dropped…" refers to the new seaweedfs-db HR, not the pre-existing seaweedfs-system HR. The new HR at packages/extra/seaweedfs/templates/seaweedfs-db.yaml correctly has only remediation.retries: -1 under upgrade:, no force. The platform-wide cleanup of force: true from seaweedfs-system is out of scope here and worth its own PR.
| ## @typedef {struct} DB - Database configuration. | ||
| ## @field {int} [replicas] - Number of database replicas. | ||
| ## @field {quantity} [size] - Persistent Volume size. | ||
| ## @field {string} [storageClass] - StorageClass used to store the data. | ||
|
|
||
| ## @param {DB} db - Database configuration. | ||
| db: | ||
| replicas: 2 | ||
| size: 10Gi | ||
| storageClass: "" |
There was a problem hiding this comment.
The Resources typedef (defined on lines 5-7) is currently unused in the DB struct definition. Additionally, the database.yaml template in this package has hardcoded resource limits. To make the database resources configurable (as attempted by the wrapper chart in packages/extra/seaweedfs/templates/seaweedfs-db.yaml), please add the resources field to the DB typedef and the db block. Note that you should also update packages/system/seaweedfs-db/templates/database.yaml to use .Values.db.resources.
## @typedef {struct} DB - Database configuration.
## @field {int} [replicas] - Number of database replicas.
## @field {quantity} [size] - Persistent Volume size.
## @field {Resources} [resources] - Resource requests and limits.
## @field {string} [storageClass] - StorageClass used to store the data.
## @param {DB} db - Database configuration.
db:
replicas: 2
size: 10Gi
storageClass: ""
resources:
limits:
cpu: "1"
memory: 2048Mi
requests:
cpu: 100m
memory: 512MiThere was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/e2e-install-cozystack.bats (1)
21-40: 💤 Low valueConsider trap-based cleanup for temporary files.
The current approach leaves temporary files in
/tmpif the test fails before reaching line 40. While acceptable in a test environment, using a trap would ensure cleanup on both success and failure paths.♻️ Optional improvement using trap
local kubeovn_yaml linstor_yaml images_list kubeovn_yaml=$(mktemp) linstor_yaml=$(mktemp) images_list=$(mktemp) + trap 'rm -f "$kubeovn_yaml" "$linstor_yaml" "$images_list"' EXIT helm template packages/system/kubeovn > "$kubeovn_yaml" helm template packages/system/linstor > "$linstor_yaml" yq -N ' (..|select(has("containers"))|.containers[]|.image), (..|select(has("initContainers"))|.initContainers[]|.image) ' "$kubeovn_yaml" "$linstor_yaml" > "$images_list" hack/e2e-prepull-images.sh < "$images_list" - rm -f "$kubeovn_yaml" "$linstor_yaml" "$images_list"🤖 Prompt for 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. In `@hack/e2e-install-cozystack.bats` around lines 21 - 40, Add an EXIT trap to ensure temporary files are removed even if the script fails: after creating the temp filenames (kubeovn_yaml, linstor_yaml, images_list) register a trap that deletes those files on EXIT (and unset or no-op if variables are empty), then you can keep or remove the final rm -f "$kubeovn_yaml" "$linstor_yaml" "$images_list" (it will be redundant but safe); ensure the trap references the same variable names (kubeovn_yaml, linstor_yaml, images_list) and runs before any commands that may fail (i.e., immediately after mktemp assignments).
🤖 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.
Nitpick comments:
In `@hack/e2e-install-cozystack.bats`:
- Around line 21-40: Add an EXIT trap to ensure temporary files are removed even
if the script fails: after creating the temp filenames (kubeovn_yaml,
linstor_yaml, images_list) register a trap that deletes those files on EXIT (and
unset or no-op if variables are empty), then you can keep or remove the final rm
-f "$kubeovn_yaml" "$linstor_yaml" "$images_list" (it will be redundant but
safe); ensure the trap references the same variable names (kubeovn_yaml,
linstor_yaml, images_list) and runs before any commands that may fail (i.e.,
immediately after mktemp assignments).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ba14897-c627-4ebd-acb3-c4ef1619713f
📒 Files selected for processing (1)
hack/e2e-install-cozystack.bats
Empty commit to fire a synchronize event against the new base (split-seaweedfs-db) after rebasing onto PR #2601; the base-change edited event does not trigger the Pull Request workflow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
…edfs-system Splits the seaweedfs-system HelmRelease in two so the CNPG Cluster/seaweedfs-db lives in its own HR (seaweedfs-db), and the application HR (seaweedfs-system) dependsOn it. The new HR uses Flux v2 HelmRelease.spec.healthCheckExprs with a CEL expression on Cluster.status.conditions[type=Ready] plus waitStrategy.name: poller, so its Ready=True only flips after the postgres primary is actually serving connections — not just after helm install applied the Cluster CR. This eliminates the seaweedfs-filer CrashLoopBackOff race on a fresh tenant install. With Cilium kubeProxyReplacement: true, socket-LB returns EPERM from connect(2) to ClusterIPs with no Ready endpoints. Pre-split, the filer StatefulSet scheduled concurrently with the CNPG bootstrap (~55–70s of unavailable postgres), each connect() failed EPERM, kubelet exponential restart backoff pushed past the e2e bats 'kubectl wait hr/seaweedfs-system --timeout=2m' window, and the 'Configure Tenant and wait for applications' test failed. healthCheckExprs uses Flux's three-predicate form: route ClusterIsNotReady (the CNPG bootstrap condition) to `inProgress` so the HR keeps polling, keep `failed` for other Ready=False reasons, and rely on the HR `timeout: 10m` as the real backstop for a genuinely stuck cluster. A plain Ready=False predicate would flag the HR failed within seconds of creating the Cluster CR. Migration 42 adopts existing Cluster/seaweedfs-db resources into the new release on upgrade by rewriting meta.helm.sh/release-name and stamping helm.sh/resource-policy: keep so the seaweedfs-system upgrade (which no longer renders the Cluster) does not delete it during the transition. The application HR's dependsOn collapses the db dependency (unconditional) with the existing ingress dependency (guarded by `if eq $ingress .Release.Namespace` so sub-tenants that inherit ingress from a parent namespace don't deadlock on a non-existent local HR) into one list. Requires Flux v2.8.x (helm-controller v1.5.0+) for healthCheckExprs. Provided by the parent PR #2602 in this chain. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
PR #2619 split seaweedfs into a serialized chain (seaweedfs-db -> seaweedfs-system -> seaweedfs) that needs ~5 min of wall-clock to install: CNPG bootstrap, master quorum, then the wrapper. With the prior 5m wait budget on the parent tenant HR, helm-controller would time out before the children reach Ready, rollback would delete the child HRs, and the upgrade would retry in a loop. The e2e bats step that patches the Tenant CR and waits 60s for child HRs to exist landed in the rollback gap and timed out. Two complementary changes: - tenant-root is created statically by cozystack-basics, so its HelmRelease spec.timeout is bumped 5m -> 15m directly. - Sub-tenants (Tenant CRs) get their HR built by cozystack-api from the tenant ApplicationDefinition. Add the release.cozystack.io/helm-install-timeout=15m annotation that kubernetes-rd already uses to opt the kind into the longer budget. Keeps both values in lockstep and documents the coupling inline. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> (cherry picked from commit cc9da3a)
values.yaml declared an unused Resources typedef while templates/database.yaml hardcoded cpu/memory limits and requests. The wrapper chart at packages/extra/seaweedfs/templates/seaweedfs-db.yaml already passes db.resources through cozy-lib.resources.defaultingSanitize into the sub-chart values, so the user-facing db.resources and db.resourcesPreset on the outer chart silently dropped on the way in. Drop the unused typedef, accept .Values.db.resources as a free-form object (the wrapper already sanitizes it into the K8s nested limits/requests shape that CNPG Cluster expects), and wire it through to the Cluster manifest. Defaults are preserved verbatim so helm template output is byte-identical for a no-override install. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
7aac5ca to
401993a
Compare
c6c67d2 to
7157158
Compare
Empty commit to fire a synchronize event against the new base (split-seaweedfs-db) after rebasing onto PR #2601; the base-change edited event does not trigger the Pull Request workflow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The flux-bump-v048 base branch removed upgrade.force from every other HelmRelease in 7f94182 because helm-controller v0.48 refuses to combine Helm's --force replace with server-side apply's --force-conflicts ("cannot use force conflicts and force replace together"). This refactor re-introduced the field on the seaweedfs-system HR by accident; CI E2E fails with the same error in a loop until the seaweedfs subtree times out the tenant install. Drop the line so the upgrade path matches every other HR on the base branch. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Empty commit to fire a synchronize event against the new base (split-seaweedfs-db) after rebasing onto PR #2601; the base-change edited event does not trigger the Pull Request workflow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
…(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 --> [](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 -->
Empty commit to fire a synchronize event against the new base (split-seaweedfs-db) after rebasing onto PR #2601; the base-change edited event does not trigger the Pull Request workflow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.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 -->
…st the default Migration 43 performs the v1.5.0 db-split hand-over: it re-owns Cluster/seaweedfs-db from <name>-system to the new <name>-db release and stamps helm.sh/resource-policy: keep, so the <name>-system upgrade — whose post-split chart no longer renders the Cluster — does not prune it as a removed resource. It compared the owning release name against the literal "seaweedfs-system". SeaweedFS is a user-creatable kind, so an instance named `foo` is owned by `foo-system` and was silently skipped: no re-own, no keep. The <name>-system upgrade then deleted the Cluster, CNPG took its PVC with it, and the filer metadata — the index for every object in that tenant's S3 — was destroyed. Data loss, not an outage: the volume PVCs still hold the bytes, nothing can find them. Verified live on the upgrade stand, which is a natural control group. All five tenants sit in the SAME failed-upgrade loop (<name>-system last-deployed is rev 1, a pre-split revision whose manifest still contains the Cluster, so every retry recomputes the same deletion): tenant-root/dsplit/l instance `seaweedfs` owner=seaweedfs-db keep Cluster alive tenant-named instance `foo` no Cluster at all Cluster GONE The only difference is that migration 43 fired for the default-named instances and skipped `foo`. That also answers whether `keep` is sufficient: three tenants survive the identical prune loop with it. Correction to the reported mechanism: this is the 1.4->1.5 db split (PR #2601, v1.5.0), not a 1.5->1.6 change — v1.5.3 and main are identical on every db-split file. And the split does protect the object; it just only protected the instances someone happened to name `seaweedfs`. - lib/seaweedfs-db-adopt.sh: shared hand-over matching the `-system` SUFFIX, so `foo-system` -> `foo-db` exactly as `seaweedfs-system` -> `seaweedfs-db`. Idempotent, and refuses a release named literally `-system` rather than annotating an owner of `-db` that no release would claim. - Migration 43 sources it: covers clusters upgrading from before 43 (1.4.x). - Migration 53 (new) re-runs it: migrations never re-run, so any cluster already at >= 44 ran the hardcoded version and is still exposed. As a pre-upgrade hook it lands before <name>-system re-renders, closing the window on THIS upgrade. This is the one that matters for 1.5.x -> 1.6. - targetVersion 53 -> 54. Tests fail on unfixed code: the non-default-name case produces no ANNOTATE at all against the hardcoded 43, while the default-name case passes — which is precisely the bug's shape. They drive the real migration scripts against a fake kubectl, following hack/migration-50-etcd-adopt.bats. LIMIT, stated plainly: a Cluster that is already deleted cannot be recovered by either migration — there is nothing left to re-annotate and the PVC went with it. Such a tenant needs seaweedfs-db restored from a backup. Runbook Step 0 adds the ownership audit that distinguishes at-risk from already-lost, and records that <name>-db can report Ready while its Cluster is gone (it rendered fine; a later <name>-system prune removed it and Flux has not re-checked) — so the audit trusts `kubectl get cluster`, not the HelmRelease status. Review round 2 — two ways this still lost the database: - OWNERSHIP IS NOT SAFETY. The helper skipped any Cluster already owned by <name>-db, treating ownership as proof the hand-over was done. It is not: where the hand-over was skipped, <name>-system prunes the Cluster and <name>-db RECREATES it under its own ownership with NO keep, while <name>-system's prune baseline still lists it — so the next reconcile deletes it again. That is the observed delete/recreate loop, and the skip walked straight past it. Verified on the stand: tenant-l and tenant-root are seaweedfs-db-owned AND their <name>-system deployed revision (rev 1) still contains the Cluster, so only keep saves them; tenant-fresh is seaweedfs-db-owned without keep but was installed after the split, so its baseline never had one. Telling those apart needs the release's deployed manifest, which this script cannot read cheaply or reliably. The costs are asymmetric — a needless keep leaves an orphan on delete (now reclaimed by the cleanup hook), a missing one loses the database — so keep is stamped on every Cluster owned by either side of the split. - FAIL OPEN, THEN STAMP. `set -euo pipefail` does not abort on a failing command substitution in a `for` word-list: the loop ran zero times, the script continued, and stamp_cozystack_version ran anyway. Migrations never re-run, so one throttle, RBAC hiccup, or not-yet-established CNPG apiservice behind the pre-upgrade hook permanently left every at-risk tenant exposed with no later migration to catch them. Reproduced: against the old helper a failing fleet scan still logged STAMP 54. Now every kubectl failure is fatal EXCEPT the two that genuinely mean "nothing to do" — the CNPG resource type not being served (the fail-open that IS load-bearing: a cluster without CNPG must still upgrade) and a Cluster vanishing between scan and read. An unreadable annotation is no longer indistinguishable from an absent one, and the unowned case now warns instead of passing silently. - ORPHAN ON DELETE. `keep` is permanent (removal deferred to the 1.7 batch migrations) and also survives the <name>-db release's own uninstall, so `kubectl delete seaweedfs <name>` left the CNPG Cluster, its Postgres pods and its PVCs behind — the cleanup hook selects app.kubernetes.io/instance=<name>-system, which the db PVCs (cnpg.io/cluster=seaweedfs-db) do not carry. Migration 53 widened that from pre-1.5 default-named tenants to every instance name, so the hook now deletes the Cluster explicitly; its PVCs carry ownerReferences to it and follow. - Tests assert the stamped VERSION (44 / 54), not a bare "STAMP": a wrong number would loop run-migrations.sh forever and the old assertion could not tell. Review round 3 — the reclaim introduced a data-loss path of its own: - DELETING ONE APP DELETED ANOTHER APP'S DATABASE. The db chart hardcodes the Cluster name `seaweedfs-db` for every instance, and the reclaim was scoped by that name in the namespace rather than by what the release owns. templates/seaweedfs-db.yaml gates the <name>-db HelmRelease on `topology != "Client"`; the cleanup hook had no such gate. Verified by render: `helm template s3-remote . --set topology=Client` emits ZERO s3-remote-db HelmReleases yet still emitted the Role granting delete on clusters/seaweedfs-db AND `kubectl delete cluster.postgresql.cnpg.io -n tenant-root seaweedfs-db`. So in a namespace running a server instance plus a Client, deleting the CLIENT destroyed the SERVER's filer metadata — CNPG takes the PVC via ownerReferences — for an app the operator never touched. Reachable the same way via a second Simple instance, whose <name>-db loses Helm's ownership check but which still installs and still carries the hook. main's hook was instance-scoped; this was new, and mine. Now gated exactly as seaweedfs-db.yaml is, and the Job re-checks meta.helm.sh/release-name == <release>-db at run time before deleting. The gate stops the permission being granted where it can never be legitimate; the runtime check stops the delete where it can. - `|| echo` turned any API/authz/admission failure into exit zero, so backoffLimit never retried and hook-delete-policy removed the only cleanup Job while the keep-protected Cluster stayed orphaned. A failed reclaim now fails the Job. - TEST SHELL. The migrations run under /bin/sh = busybox ash (the image is FROM alpine, run-migrations.sh is #!/bin/sh), but the tests drove them with bash — so the fail-closed guarantees were asserted in a shell that never runs them, which matters more than usual given the bug they cover was a fail-open. The scripts are now invoked via `sh` and the fake kubectl is POSIX sh. Verified directly against the production base image (busybox 1.37.0 / alpine:3.24): `set -o pipefail` is supported, every script passes `ash -n`, and the failing-fleet-scan path exits non-zero without stamping under real ash. The recipe is in the bats header. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
What this PR does
Splits the
seaweedfs-systemHelmRelease into two: the CNPGCluster/seaweedfs-dbmoves to its own HR (seaweedfs-db), and the application HR (seaweedfs-system)dependsOnit. The new HR uses Flux v2HelmRelease.spec.healthCheckExprswith a CEL expression onCluster.status.conditions[type=Ready]pluswaitStrategy.name: poller, so itsReady=Trueonly flips after the postgres primary is actually serving connections — not just afterhelm installapplied theClusterCR.This eliminates the
seaweedfs-filerCrashLoopBackOff race on a fresh tenant install. With CiliumkubeProxyReplacement: true, socket-LB returnsEPERMfromconnect(2)to ClusterIPs with no Ready endpoints. Pre-split, the filer StatefulSet scheduled concurrently with the CNPG bootstrap (~55–70s of unavailable postgres), eachconnect()failedEPERM, kubelet exponential restart backoff pushed past the e2e batskubectl wait hr/seaweedfs-system --timeout=2mwindow, and the "Configure Tenant and wait for applications" test failed.Stacked on #2602
This PR depends on #2602 (Flux v2.8.0 upgrade) —
healthCheckExprsis a helm-controller v1.5.0 feature. GitHub base set toflux-bump-v048so the diff cleanly shows only this PR's changes; will auto-rebase ontomainonce #2602 merges.Commits
refactor(seaweedfs)— the split itself. Squash of the three iterative commits from the consolidated branch (ci: consolidate CI fixes (#2602 flux 2.8 + #2601 seaweedfs-db + #2612 kubevirt TPM + #2615 e2e trace) #2619): the initial split, adependsOnmerge-collision fix (collapses unconditionalseaweedfs-dbdependency with the namespace-guarded ingress dependency into one list), and thehealthCheckExprsthree-predicate form (route CNPG'sClusterIsNotReadybootstrap condition toinProgress, reservefailedfor otherReady=Falsereasons, lean on the HRtimeout: 10mas the real backstop).fix(tenant)— bump parent HRspec.timeoutfrom 5m → 15m so the serializedseaweedfs-db → seaweedfs-system → seaweedfschain has enough wall-clock budget. Two complementary changes: the statictenant-rootHR (created bycozystack-basics) gets itsspec.timeoutbumped directly; sub-tenants (Tenant CRs whose HR is built bycozystack-apifrom thetenantApplicationDefinition) get therelease.cozystack.io/helm-install-timeout=15mannotation thatkubernetes-rdalready uses to opt into the longer budget.test(e2e)— consolidate two redundant child-HR waits inhack/e2e-install-cozystack.batsinto one budgeted assert. The secondkubectl wait(hr/monitoring hr/seaweedfs-system --timeout=2m) was a residue of aflux-reconcile-forceworkaround removed in feat(operator): expose HelmRelease generation knobs (interval, retry-interval, timeouts, max-history) #2509;monitoring's Ready is already implied byhr/tenant-rootReady andseaweedfs-system's byhr/seaweedfsReady, so the line was tautologically true by the time it ran.Migration
packages/core/platform/images/migrations/migrations/42adopts existingCluster/seaweedfs-dbresources into the new release on upgrade — rewritesmeta.helm.sh/release-name: seaweedfs-system → seaweedfs-dband stampshelm.sh/resource-policy: keepso theseaweedfs-systemupgrade (which no longer renders the Cluster) does not delete it during the transition. Renumbered from the consolidated branch's slot 40 to fit fresh main (last migration is 41).targetVersionbumped 42 → 43.The new HR's
upgrade.force: truewas dropped to match #2602's platform-wide cleanup (Helm v4 in helm-controller v1.5.0 rejects combining--force-replacewith SSA's--force-conflicts).Verification
helm templaterendersseaweedfs-dbHR cleanly withhealthCheckExprs,waitStrategy: poller, andupgrade: { remediation: { retries: -1 } }(noforce: true).sh -n.Release note
Summary by CodeRabbit
New Features
Chores