fix(seaweedfs): restore -lock BucketClass, s3 service name, and drop volumeSizeLimitMB - #2943
fix(seaweedfs): restore -lock BucketClass, s3 service name, and drop volumeSizeLimitMB#2943myasnikovdaniil wants to merge 3 commits into
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 resolves several regressions introduced by the recent SeaweedFS chart upgrade. By restoring essential access control objects, correcting service naming conventions, and optimizing volume size limits, the changes restore the expected multi-tenant behavior and network connectivity for the storage system. 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. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize 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
|
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR extends the SeaweedFS COSI Helm template with a lock-enabled ChangesSeaweedFS Helm chart and bucket E2E test updates
CAPI providers core startup probe
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request reduces the SeaweedFS volumeSizeLimitMB from 30000 to 1000 to allow more parallel collections on a 10 GiB E2E sandbox PVC. The review feedback correctly identifies an incorrect mathematical operator in the newly added comment and flags the missing release-note block in the PR description, which is required by the repository style guide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| seaweedfs: | ||
| master: | ||
| volumeSizeLimitMB: 30000 | ||
| # 30 GB per volume × 10 GiB PVC = 0 → rounded to 1 maxVolume per |
There was a problem hiding this comment.
| # blocks on `assign volume: No writable volumes` until the test | ||
| # times out. Smaller volumes = more parallel collections on a | ||
| # 10 GiB E2E sandbox PVC. The upstream chart default is 1000. | ||
| volumeSizeLimitMB: 1000 |
There was a problem hiding this comment.
The PR body is missing the required release-note block. According to the Repository Style Guide, every PR must contain a release note block in its description:
```release-note
type(scope): human-readable changelog entry
Please update the PR description to include this block.
<details>
<summary>References</summary>
1. PR body must contain a release note block: ```release-note\ntype(scope): human-readable changelog entry\n``` <sup>([link](https://github.com/cozystack/cozystack/blob/main/.gemini/styleguide.md))</sup>
</details>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@packages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-bucket-class.yaml`:
- Around line 14-42: The modifications to the BucketClass and BucketAccessClass
resources in the vendored SeaweedFS chart templates under
packages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-bucket-class.yaml
are being made directly to upstream-vendored content, which will be overwritten
on the next make update. Instead, apply these changes upstream to the SeaweedFS
chart repository first, then re-vendor the updated chart into this project by
running make update in the packages/system/seaweedfs directory to ensure the
fixes remain durable and follow the umbrella pattern for vendored charts.
In `@packages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-service.yaml`:
- Line 5: The S3 service name at line 5 uses the template function `{{ template
"seaweedfs.name" . }}-s3` which creates a mismatch with the COSI deployment that
expects `{{ include "seaweedfs.componentName" (list . "s3") }}`. This
inconsistency causes DNS resolution failures when COSI tries to connect to the
S3 service when `cosi.endpoint` is unset and `s3.ingress.enabled` is false.
Replace the service name definition in the s3-service.yaml template to use `{{
include "seaweedfs.componentName" (list . "s3") }}` instead, matching the naming
pattern that COSI deployment already targets, ensuring consistent DNS resolution
across both services.
🪄 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: e57eb6de-fb7c-4520-97b2-32d2d98bdcac
📒 Files selected for processing (3)
packages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-bucket-class.yamlpackages/system/seaweedfs/charts/seaweedfs/templates/s3/s3-service.yamlpackages/system/seaweedfs/values.yaml
| kind: BucketClass | ||
| apiVersion: objectstorage.k8s.io/v1alpha1 | ||
| metadata: | ||
| name: {{ .Values.cosi.bucketClassName }}-lock | ||
| driverName: {{ .Values.cosi.driverName }} | ||
| deletionPolicy: Retain | ||
| parameters: | ||
| objectLockEnabled: "true" | ||
| objectLockRetentionMode: "COMPLIANCE" | ||
| objectLockRetentionDays: "365" | ||
| --- | ||
| kind: BucketAccessClass | ||
| apiVersion: objectstorage.k8s.io/v1alpha1 | ||
| metadata: | ||
| name: {{ .Values.cosi.bucketClassName }} | ||
| driverName: {{ .Values.cosi.driverName }} | ||
| authenticationType: KEY | ||
| parameters: | ||
| accessPolicy: readwrite | ||
| --- | ||
| kind: BucketAccessClass | ||
| apiVersion: objectstorage.k8s.io/v1alpha1 | ||
| metadata: | ||
| name: {{ .Values.cosi.bucketClassName }}-readonly | ||
| driverName: {{ .Values.cosi.driverName }} | ||
| authenticationType: KEY | ||
| parameters: | ||
| accessPolicy: readonly | ||
| {{- end }} |
There was a problem hiding this comment.
Avoid patching vendored SeaweedFS chart templates directly.
Lines 14-42 modify upstream-vendored chart content under packages/system/seaweedfs/charts/seaweedfs/templates/**. These changes are likely to be overwritten on the next make update, so the fix should be made upstream and then re-vendored to keep this regression fix durable.
As per coding guidelines, packages/system/** should follow the umbrella pattern with vendored upstream charts in charts/, and based on learnings, SeaweedFS template fixes in packages/system/seaweedfs/charts/seaweedfs/ must be applied upstream then pulled in via packages/system/seaweedfs/Makefile (make update).
🤖 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/charts/seaweedfs/templates/cosi/cosi-bucket-class.yaml`
around lines 14 - 42, The modifications to the BucketClass and BucketAccessClass
resources in the vendored SeaweedFS chart templates under
packages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-bucket-class.yaml
are being made directly to upstream-vendored content, which will be overwritten
on the next make update. Instead, apply these changes upstream to the SeaweedFS
chart repository first, then re-vendor the updated chart into this project by
running make update in the packages/system/seaweedfs directory to ensure the
fixes remain durable and follow the umbrella pattern for vendored charts.
Sources: Coding guidelines, Learnings
| kind: Service | ||
| metadata: | ||
| name: {{ include "seaweedfs.componentName" (list . "s3") }} | ||
| name: {{ template "seaweedfs.name" . }}-s3 |
There was a problem hiding this comment.
S3 Service name now conflicts with the COSI fallback endpoint contract.
Line 5 uses {{ template "seaweedfs.name" . }}-s3, but COSI’s non-ingress fallback still targets {{ include "seaweedfs.componentName" (list . "s3") }} (packages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-deployment.yaml, Lines 70-85 in provided context). In deployments with cosi.endpoint unset and s3.ingress.enabled: false, this creates a DNS mismatch (<name>-s3 vs <fullname>-s3) and breaks COSI-to-S3 connectivity.
🤖 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/charts/seaweedfs/templates/s3/s3-service.yaml` at
line 5, The S3 service name at line 5 uses the template function `{{ template
"seaweedfs.name" . }}-s3` which creates a mismatch with the COSI deployment that
expects `{{ include "seaweedfs.componentName" (list . "s3") }}`. This
inconsistency causes DNS resolution failures when COSI tries to connect to the
S3 service when `cosi.endpoint` is unset and `s3.ingress.enabled` is false.
Replace the service name definition in the s3-service.yaml template to use `{{
include "seaweedfs.componentName" (list . "s3") }}` instead, matching the naming
pattern that COSI deployment already targets, ensuring consistent DNS resolution
across both services.
…lass The seaweedfs 4.31 upstream bump (#2834) rewrote packages/system/seaweedfs/charts/seaweedfs/templates/cosi/cosi-bucket-class.yaml from scratch and silently dropped the supplementary COSI resources that cozystack ships on top of the upstream chart: - BucketClass {name}-lock (object lock, COMPLIANCE mode, 365 days retention) — used by apps that store immutable backups - BucketAccessClass {name} parameters.accessPolicy: readwrite — the explicit grant for the default access class - BucketAccessClass {name}-readonly (accessPolicy: readonly) — used by every Bucket app that declares readonly users apps/bucket still renders BucketAccess resources that reference {name}-readonly, but the class no longer exists, so the COSI controller never picks the BucketAccess up: no finalizer is added, status.accessGranted stays empty, and the bucket E2E test waits 300s and times out on the viewer access. The admin (readwrite) access class still works because it falls back to the default BAC. Restore the three resources so the system chart again matches the shape that packages/extra/seaweedfs/templates/client/cosi-bucket-class.yaml already ships for the client topology (where this never regressed). Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
The seaweedfs 4.31 upstream bump (#2834) switched the s3 Service name from "$chartname-s3" (built with the seaweedfs.name helper, evaluates to "seaweedfs-s3") to the seaweedfs.componentName helper, which is release-name based and now evaluates to "$release-s3" — "seaweedfs- system-s3" for the tenant-root deployment. The bats E2E asset hack/e2e-apps/bucket.bats hard-codes "service/seaweedfs-s3" for the port-forward that sets up its S3 client; backup-controller and other in-cluster consumers also referenced the legacy name. After the bump, the legacy Service is gone and the port-forward immediately exits, the test then times out on the 30-second nc poll for localhost:8333. Restore the legacy Service name. Internal references inside the chart (s3-deployment -filer=, etc) keep using componentName for the filer endpoint, so this change only re-exposes the public-facing name without touching the internal topology. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
Cozystack overrides the upstream seaweedfs volumeSizeLimitMB from 1000 to 30000 (30 GB), but the e2e sandbox provisions only a 10 GiB PVC per volume server. With maxVolumes=0 (auto), seaweedfs computes maxVolumes as int(10240 / 30000) → 0, then rounds up to 1, so each volume server can host exactly 1 logical volume. With defaultReplication=001 (one replica on a different node within the same rack) two volume servers share that single volume, giving the cluster a hard limit of one writable collection at a time. Since #2716 added a default bucket-cozy-backups Bucket to every tenant-root, that bucket now grabs the only slot on install. Any second collection — the bucket E2E (bucket-test in tenant-test), a tenant app's own bucket, etc. — sits in 'assign volume: No writable volumes' forever, and mc cp hangs until the test times out. Master logs reflect this clearly: failed to find writable volumes for collection:bucket-... replication:001 ttl: error: No writable volumes and no free volumes left topo failed to pick 1 from 0 node candidates volume_growth.go:135 create 6 volume, created 0: Not enough data nodes found! Lower volumeSizeLimitMB back to the upstream chart default of 1000, which makes a 10 GiB PVC fit ~10 logical volumes per server and unblocks all per-collection bucket workloads on small sandboxes. Production clusters still get plenty of headroom — volume size is a soft cap, not a preallocation. Signed-off-by: Ivan Okhotnikov <ivan.okhotnikov@aenix.io>
0719a30 to
e69d268
Compare
## What Consolidated landing branch for a set of interdependent platform/CI stabilization fixes. It began as the source PRs in the table below (in dependency order) and has since grown with review-driven fixes and a few production-behavior changes — the full current inventory is under **Scope** below. Landing them as one unit lets CI run against the real combined state instead of a fragile bottom-up merge train. ## Source PRs (dependency order) | # | Commit | Source PR | Addresses | |---|--------|-----------|-----------| | 1 | fix(capi): startupProbe on capi-controller-manager | #2946 | capi crashloop during cert provisioning | | 2 | test(e2e): bucket.bats port-forward + S3 client reliability | #2944 | flaky bucket test | | 3 | fix(seaweedfs): restore -lock BucketClass, s3 svc name, drop volumeSizeLimitMB | #2943 | missing bucket/access classes | | 4 | test(e2e): in-cluster Cilium endpoint-leak healer (install + apps) | #2874 | cilium "IP already in use" leak | | 5 | refactor(build): mode=max registry cache + main-only warmer | #2938 | build cache | | 6 | ci(build): isolate each PR build on its own ephemeral runner VM | #2939 | shared-buildkit contention | | 7 | fix(e2e): LINSTOR post-install waits on a single 15m deadline | #2928 | LINSTOR provisioning timeouts | | 8 | test(metallb): assert digest-pinned image form, not version literal | #2873 | brittle metallb assertion | ## Why consolidated These form a dependency DAG (verified from CI logs): e.g. 2943 needs 2946, 2938 needs 2874, 2928 needs 2938/2939, 2873 needs 2928. Tested bottom-up, each lower PR runs with none of the fixes above it and can't go green alone. This branch carries all of them, so CI runs against the real combined state. ## Verification status CI is green on the latest head (`5653c30`): full-suite E2E passes end-to-end and is reproducible (2 of 3 runs on this SHA green). The one red run was a LINSTOR tie-breaker / DRBD-metadata infra flake on the sandbox, unrelated to the diff — a different environmental subsystem failed each run (details in the comments below). The earlier 3-hour crust-gather snapshot hang is fixed in `5653c30`. ## Scope beyond the original 8 The branch has grown past the 8 source PRs above with review-driven fixes and a few production-behavior changes. For reviewer transparency, the full set: **Review fixes (@lexfrei review):** - **B1 (blocker)** — `8f41910` converts the SeaweedFS `-lock`/`-readonly` BucketClasses and the s3 service-name override into `patches/`, wired into `make update`, so a re-vendor no longer drops them. - **FU1** — `cc99230` grants the cilium leak-healer `delete` on pods (both delete remedies were RBAC-forbidden). - **FU3** — `014de3f` adds a unit test for the HelmRelease update conflict-retry path. - **FU4** — `6deddc6` makes the leak-healer refuse a disconnect for any non-terminal owner phase, not just `Running`. - **FU7** — `f3914d2` sets `persist-credentials: false` on the build-cache checkout (SHA-pinning is handled repo-wide in #2849). **Production-behavior changes (not in the table above):** - VPA `updateMode: Auto → Initial` for etcd (`6e9ff90`) and monitoring (`0997105`), plus `vmselect`/`vmstorage` `minAllowed` floors — stops install-time eviction churn. - cozystack-api: `retry.RetryOnConflict` on the Application→HelmRelease update path (`728f676`). **E2E robustness follow-ups:** per-test crust-gather snapshots (`50e5f94`), EtcdBackupSchedule wait (`763b85e`), tenant-node single-deadline wait (`9e43d45`), harbor BucketClaim 10m budget (`d7f244f`), tenant API via LoadBalancer (`9beda4e`), crust-gather pin (`95f42aa`). **Not addressed (by decision):** FU2 — the s3 Service keeps `seaweedfs.name`-based naming via patch (rendered output unchanged; cozystack renders `fullname == seaweedfs`, so it matches the `componentName`/`fullname` siblings in practice). FU6 — fork PR build push is handled by the existing mirror-to-same-repo-branch workflow. ## Relates to #2946 #2944 #2943 #2874 #2938 #2939 #2928 #2873 — if this lands, those can be closed; otherwise they remain the granular per-PR review path. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added CI build cache warming for main branch builds. * Added a best-effort in-cluster Cilium endpoint self-heal watchdog for e2e installs. * Extended SeaweedFS COSI with object-lock support plus separate readonly access. * **Improvements** * Reduced e2e install flakiness with a shared readiness deadline, safer waits, and improved cleanup/diagnostics capture. * Prevented install-time churn by switching VPA update mode to **Initial**. * Added container startup probes and updated SeaweedFS volume sizing. * **Tests** * Improved e2e robustness (etcd backup schedule waiting, digest-pinned image checks, and corrected S3 port-forwarding). <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…volumeSizeLimitMB (cozystack#2943) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
superseded by #2948 |
What this PR does
Three seaweedfs regressions introduced by the 4.31 chart bump (#2834), extracted from #2919 for faster review.
volumeSizeLimitMB— prevents multiple collections from being created in the same volume; removing it restores normal multi-tenant bucket behaviour.Cherry-picks of commits authored by IvanHunters from #2919.
Test plan
BucketClassobjects present after installSummary by CodeRabbit
New Features
Chores
Tests