Repository navigation
fix(objectstorage-controller): converge BucketClaim readiness and speed up COSI provisioner failover - #3034
Conversation
…et is ready The dynamic-provisioning path re-reads the live Bucket after create and copies its readiness onto the BucketClaim, but only during a reconcile. Nothing re-drives that reconcile afterwards: the controller does not watch Buckets, and the generic controller drops no-op resync deltas (reflect.DeepEqual), so once the BucketClaim stops changing the listener is never re-invoked. The Bucket's false->true transition, performed asynchronously by the sidecar on a different object, is therefore reflected onto the claim only if the Bucket happened to be ready during that single reconcile -- a race the sidecar usually loses, leaving bucketReady frozen at false and BucketAccess never granted. Return an error while the backend Bucket is not ready so the work queue requeues the claim with exponential backoff; each retry re-reads the live Bucket until its readiness has been propagated, converging deterministically instead of depending on a resync that never fires. Add a unit test pinning the contract (requeue while not ready, propagate true once ready) and adjust the existing status-update conflict-retry test, which pinned the old never-requeue behavior. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
The controller and the per-driver sidecar both run single-replica, so leader election provides no high availability -- it only governs how quickly a freshly recreated pod takes over. With the upstream 150s lease duration a recreated provisioner pod stalls reconciliation for up to 150s while it waits out the stale lease still held by the previous, now-deleted pod. Lower the lease timings to the standard Kubernetes values (15s/10s/2s) so a recreated pod resumes reconciliation in roughly 15s. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
…ptly
The bucket suite waited on '{.status.bucketReady}' without an expected value;
a bare jsonpath match is satisfied by the literal "false", so the wait passed
against an unready claim. Both suites also allowed 300s/600s, long enough to
tolerate the propagation race rather than catch it, so they stayed green
whether or not the BucketClaim ever converged.
Assert bucketReady=true on a 120s bound. Convergence is now controller-driven
(the controller requeues the claim until the backend Bucket's readiness
propagates) and completes within tens of seconds; a claim frozen by the
propagation race never converges and now fails the wait fast instead of being
masked.
Assisted-By: Claude <[email protected]>
Signed-off-by: Aleksei Sviridkin <[email protected]>
The objectstorage-controller package ships the COSI control plane; its commits use the objectstorage-controller scope, which had no area mapping and fell back to area/uncategorized. Map it to area/storage alongside the other COSI/storage scopes. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Aleksei Sviridkin (lexfrei)
added
the
kind/backport
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR updates object storage readiness handling so ChangesBucketClaim readiness, controller timing, and validation updates
Sequence Diagram(s)sequenceDiagram
participant BucketClaimListener
participant Kubernetes API
participant Sidecar
BucketClaimListener->>Kubernetes API: create backend Bucket
BucketClaimListener->>Kubernetes API: Get live Bucket
Kubernetes API-->>BucketClaimListener: Bucket.Status.BucketReady
alt Bucket not ready
BucketClaimListener->>Kubernetes API: set BucketClaim.Status.BucketReady=false
BucketClaimListener-->>Kubernetes API: return warning/error for requeue
else Bucket ready
BucketClaimListener->>Kubernetes API: set BucketClaim.Status.BucketReady=true
BucketClaimListener-->>Kubernetes API: return success
end
Sidecar->>Kubernetes API: update Bucket.Status.BucketReady=true
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
github-actions
Bot
added
size/L
Aleksei Sviridkin (lexfrei)
added
area/storage
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 critical reliability and performance issues within the COSI control plane. By ensuring deterministic convergence of BucketClaim readiness and optimizing leader election parameters, the changes improve both the stability of bucket provisioning and the recovery speed of the provisioner during pod recreation. These improvements are validated by updated end-to-end tests that now provide faster feedback and better diagnostic context. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the E2E tests in bucket.bats and harbor.bats to explicitly assert that BucketClaim status bucketReady converges to true with a tighter timeout of 120 seconds. It also adds diagnostic output upon failure to help surface propagation-race regressions quickly. The reviewer suggested also printing the status of the objectstorage-controller pods in bucket.bats for consistency with harbor.bats to aid in diagnosing controller-side issues.
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.
| kubectl -n tenant-test wait bucketclaims.objectstorage.k8s.io bucket-${name} --timeout=120s --for=jsonpath='{.status.bucketReady}'=true || { | ||
| echo "=== BucketClaim did not converge to bucketReady=true ===" | ||
| kubectl -n tenant-test get bucketclaims.objectstorage.k8s.io bucket-${name} -o yaml 2>&1 || true | ||
| echo "=== backend Buckets (cluster-scoped) ===" | ||
| kubectl get buckets.objectstorage.k8s.io -o wide 2>&1 || true | ||
| false | ||
| } |
There was a problem hiding this comment.
For consistency with the diagnostic output in harbor.bats, it would be highly beneficial to also print the status of the objectstorage-controller pods here when the BucketClaim fails to converge. This helps in diagnosing controller-side issues directly from the bucket.bats test logs.
kubectl -n tenant-test wait bucketclaims.objectstorage.k8s.io bucket-${name} --timeout=120s --for=jsonpath='{.status.bucketReady}'=true || {
echo "=== BucketClaim did not converge to bucketReady=true ==="
kubectl -n tenant-test get bucketclaims.objectstorage.k8s.io bucket-${name} -o yaml 2>&1 || true
echo "=== backend Buckets (cluster-scoped) ==="
kubectl get buckets.objectstorage.k8s.io -o wide 2>&1 || true
echo "=== objectstorage-controller ==="
kubectl -n cozy-objectstorage-controller get pods 2>&1 || true
false
}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/system/objectstorage-controller/images/objectstorage/patches/92-bucketclaim-propagate-ready.diff`:
- Around line 119-121: The test around newListener(client).Add only verifies
that an error is returned, which can pass for unrelated failures. Update this
assertion to check that Add returns the specific readiness-requeue error path by
matching a stable substring such as not ready yet, so the subtest stays tied to
the intended BucketClaim readiness behavior.
🪄 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: 40046052-74aa-4dbc-998a-b99e6f3cf56d
📒 Files selected for processing (5)
.github/workflows/pr-labeler.yamlhack/e2e-apps/bucket.batshack/e2e-apps/harbor.batspackages/system/objectstorage-controller/images/objectstorage/patches/92-bucketclaim-propagate-ready.diffpackages/system/objectstorage-controller/images/objectstorage/patches/93-shorten-leaderelection-lease.diff
Maxim Kitsunoff (kitsunoff)
left a comment
Maxim Kitsunoff (kitsunoff)
left a comment
There was a problem hiding this comment.
Solid, well-scoped fix. The requeue-on-not-ready mechanism is the correct way to drive convergence here — verified against the upstream generic controller, which drops no-op resync deltas via reflect.DeepEqual, so periodic resync alone could never re-drive the claim. Patches apply cleanly, build and unit tests are green, and the leader-election retiming to the standard 15/10/2 values is sound for single-replica components. Two non-blocking notes below; neither needs to gate the merge.
| false | ||
| } | ||
| timeout 60 sh -ec "until kubectl -n tenant-test get bucketaccesses.objectstorage.k8s.io bucket-${name}-admin >/dev/null 2>&1; do sleep 2; done" | ||
| kubectl -n tenant-test wait bucketaccesses.objectstorage.k8s.io bucket-${name}-admin --timeout=300s --for=jsonpath='{.status.accessGranted}' |
There was a problem hiding this comment.
Observation (non-blocking): These accessGranted waits (lines 37 and 39) still use the bare --for=jsonpath='{.status.accessGranted}' form — the exact pattern you just documented and fixed for bucketReady a few lines above, where a bare match also accepts the literal false. harbor.bats:64 already asserts =true for accessGranted. Since you're tightening this contract in the same file, consider aligning these two waits to =true for consistency. Strictly outside this PR's scope, so non-blocking.
| + // this point. So while the Bucket is not ready, return an error to force a | ||
| + // rate-limited requeue; each retry re-reads the live Bucket above until its | ||
| + // readiness has been propagated onto the claim. | ||
| + if !statusBucketReady { |
There was a problem hiding this comment.
Observation (non-blocking): When the backend Bucket never becomes ready (driver failure/misconfig), this path returns an error on every reconcile indefinitely. That's correct convergence behavior — the rate limiter caps backoff at 30s, so it's not a busy-loop — but the only signal is a klog.V(3) line, which is suppressed at default verbosity, and no Warning event or terminal condition is surfaced on the claim. A permanently stuck claim is therefore effectively silent. The related driver-side bug is already tracked in #2429; flagging only the observability gap in case a surfaced event/condition is worth a follow-up.
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM
The controller-side requeue, the test pin, and the e2e tightening line up with upstream COSI v0.2.2 semantics (verified reflect.DeepEqual drop at internal/runtime/controller.go:352-354), and both upgrade and fresh-install impact are clean.
Caveats
-
Per-requeue API cost is bounded but non-trivial at startup. While
bucketReady=false, each retry runsprovisionBucketClaimOperationend-to-end:Get(BucketClaim)+UpdateStatus(BucketClaim)+Get(Bucket)+Get(BucketClaim)+Update(BucketClaim)(controller/pkg/bucketclaim/bucketclaim.go:257-279always callsUpdateregardless of whetherAddFinalizermutated the object — pre-existing upstream pattern, not introduced here, but more visible now). With the exponential-failure rate limiter capped at 30s (internal/runtime/controller.go:113), a freshly restarted controller with N stuck claims pays ~5N API calls in the first ~30s burst, then settles to ~N·5/30s steady-state until convergence. Fine at typical cluster scale, worth knowing if a customer ever hits a high-N stuck-claim backlog. -
bucket.bats:37,39still uses bare--for=jsonpath='{.status.accessGranted}'whileharbor.bats:64was updated to=true.BucketAccess.status.accessGrantedis only ever set totruein upstream (sidecar/pkg/bucketaccess/bucketaccess_controller.go:295is the only writer; the field is otherwise unset), so the bare form works in practice. The comment atbucket.bats:26-28flags exactly the "bare jsonpath matches false too" trap for thebucketReadycase, then leaves it foraccessGranted. Cosmetic, but the contract is fragile against an upstream change that ever writesfalse.
… pods on bucket failure The two bucketaccesses waits used the bare jsonpath form, which also matches the literal "false"; require =true so a never-granted access fails the wait instead of passing against an ungranted object. Also print the objectstorage-controller pod status in the bucketReady failure block so controller-side issues are visible from the test log. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
…ackend Bucket readiness While the backend Bucket is not ready the controller returns an error to force a requeue, but that error is only logged at klog.V(3) and is suppressed at default verbosity, so an operator cannot see why the claim is stuck. Record a WaitingForBucket Warning event on the BucketClaim on each requeue (event aggregation folds the repetition) and still return the error so the requeue happens as before. Pin the requeue contract in the test by asserting the error carries the stable "not ready yet" substring, not merely that it is non-nil. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
5991808
|
Git push to origin failed for release-1.5 with exitcode 1 |
Conditional PR builds (the per-package matrix in pull-requests.yaml) only rebuild packages whose dir changed. Every other package falls back to the image refs committed in the tree — the last release's digests (currently v1.5.0), which go stale as main advances. So e2e and the installer's packages artifact ran current-main configs against last-release images for everything outside the PR's build matrix, and a PR installed via the cozy-installer chart was an incoherent mix. This is the systemic gap in issue #3143 (its concrete bite: the merged objectstorage-controller requeue fix #3034 never ran in e2e, flaking the harbor BucketClaim check). In finalize, pull the cozystack-packages:main OCI artifact that build-main already publishes (the whole current-main packages tree, refs digest-pinned to the images build-main pushed) and walk every ref-bearing file in it (values.yaml + images/*.tag), overlaying the repo's copy. This is driven by the artifact, not the build matrix, so it covers the ENTIRE first-party tree — apps/system/core AND extra/, library/ — and a merged fix to any first-party image (e.g. the objectstorage-sidecar ref under extra/seaweedfs) takes effect. For a file build-main did not rebuild the artifact copy is byte-identical and is skipped. Skipped explicitly: units the PR itself rebuilt (their pr-<N>-<sha> refs win), packages/core/talos and packages/core/installer (owned by their dedicated build-talos / finalize-installer jobs), and vendored charts/ subtrees. The copy is self-validating: a file is overlaid only when every differing line is image-reference-bearing (a full @sha256: ref, or a split image/repository/registry/tag/digest key, or a --…-image= arg); otherwise it keeps its committed ref. A missing artifact is a no-op (prior behaviour) and the step is best-effort so it never blocks a PR. No build-main change is needed: cozystack-packages:main is already published by `make build`. Runs before the installer build so the flux-pushed packages artifact and the pr.patch e2e applies both reflect the overlay. Result: a PR is tested and installed as current-main + its own changes, coherent across config and image, instead of current-main configs running on last-release images. Covered by hack/overlay-main-images_test.bats (run by `make unit-tests` in the checks job): single-line and split-form ref overlay, extra/* coverage, skip-rebuilt, talos/installer skip, vendored-charts prune, drift self-validation, and missing-artifact no-op. Assisted-By: Claude <[email protected]> Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
Conditional PR builds (the per-package matrix in pull-requests.yaml) only rebuild packages whose dir changed. Every other package falls back to the image refs committed in the tree — the last release's digests (currently v1.5.0), which go stale as main advances. So e2e and the installer's packages artifact ran current-main configs against last-release images for everything outside the PR's build matrix, and a PR installed via the cozy-installer chart was an incoherent mix. This is the systemic gap in issue #3143 (its concrete bite: the merged objectstorage-controller requeue fix #3034 never ran in e2e, flaking the harbor BucketClaim check). In finalize, pull the cozystack-packages:main OCI artifact that build-main already publishes (the whole current-main packages tree, refs digest-pinned to the images build-main pushed) and walk every ref-bearing file in it (values.yaml + images/*.tag), overlaying the repo's copy. This is driven by the artifact, not the build matrix, so it covers the ENTIRE first-party tree — apps/system/core AND extra/, library/ — and a merged fix to any first-party image (e.g. the objectstorage-sidecar ref under extra/seaweedfs) takes effect. For a file build-main did not rebuild the artifact copy is byte-identical and is skipped. Skipped explicitly: units the PR itself rebuilt (their pr-<N>-<sha> refs win), packages/core/talos and packages/core/installer (owned by their dedicated build-talos / finalize-installer jobs), and vendored charts/ subtrees. The copy is self-validating: a file is overlaid only when every differing line is image-reference-bearing (a full @sha256: ref, or a split image/repository/registry/tag/digest key, or a --…-image= arg); otherwise it keeps its committed ref. A missing artifact is a no-op (prior behaviour) and the step is best-effort so it never blocks a PR. No build-main change is needed: cozystack-packages:main is already published by `make build`. Runs before the installer build so the flux-pushed packages artifact and the pr.patch e2e applies both reflect the overlay. Result: a PR is tested and installed as current-main + its own changes, coherent across config and image, instead of current-main configs running on last-release images. Covered by hack/overlay-main-images_test.bats (run by `make unit-tests` in the checks job): single-line and split-form ref overlay, extra/* coverage, skip-rebuilt, talos/installer skip, vendored-charts prune, drift self-validation, and missing-artifact no-op. Assisted-By: Claude <[email protected]> Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
…Claim readiness and speed up COSI provisioner failover (#3532) ## What this PR does Hand-backport of #3034 to `release-1.5`. The `backport` label was applied at merge time but no backport PR was ever opened, so the change never reached this branch — found by auditing the branch with `cmd/backport-audit`, which reported it as `MISSING`. Every candidate that went missing this way merged before #3155 landed, when `conflict_resolution` was passed as a top-level input instead of nested under `experimental`. The action ignored it and fell back to `fail`, which on a conflicting cherry-pick opens no PR and reports no failure, so these were dropped with no draft to find and no red check to notice. The six original commits are cherry-picked with `-x`, unchanged and in order. No adaptation was needed: the cherry-pick was clean, and the net diff is identical to the change on `main`. ### Verification The two `.diff` files are image patches applied at build time against upstream `container-object-storage-interface`, so what matters is that this branch pins the same upstream source. It does — both `release-1.5` and `main` set `ARG COMMIT_REF=v0.2.2` in `images/objectstorage/Dockerfile`, and the resulting patch set on this branch (`91`, `92`, `93`) is byte-identical to `main`'s. Patch `91` was already present here unchanged, so `92` applies over the same base it does on `main`. The branch differs from `main` only in its builder pins (`golang:1.24` versus `1.26`, unpinned base digests), none of which affect patch application. ### Screenshots Not applicable — no UI change. ### Release note ```release-note fix(objectstorage-controller): converge BucketClaim readiness to the backend Bucket and speed up single-replica COSI provisioner failover ```
4 participants
What this PR does
Fixes two failure modes in the COSI control plane built from upstream container-object-storage-interface v0.2.2, both carried as image patches.
BucketClaim never converges to ready (#3032). The controller does not watch Bucket objects, and the generic controller drops no-op resync deltas, so a BucketClaim whose backend Bucket flips to ready after the first reconcile stayed
bucketReady=falseindefinitely — BucketAccess was never granted and the consuming app (bucket, harbor) got no credentials. The existing re-read of the live Bucket only ran during a reconcile that nothing re-triggered. The controller now returns an error while the Bucket is not ready, forcing a rate-limited requeue that re-reads the Bucket until its readiness propagates, so the claim converges deterministically instead of racing the async sidecar.Slow provisioner failover on pod recreate (part of #2429). The controller and the per-driver sidecar run single-replica, so leader election adds no availability — only failover latency. The upstream 150s lease made a recreated provisioner pod stall reconciliation for up to ~150s. The lease is lowered to the standard Kubernetes values (15s/10s/2s), so a recreated pod resumes in ~15s. The stale unix-socket CrashLoop in #2429 was already resolved by the seaweedfs-cosi-driver v0.3.1 self-heal; the silent "Bucket ready with no backend bucket" finding on that issue is a separate driver bug and is left tracked there.
Coverage: a unit test in the patched controller pins the requeue/propagation contract (fails without the requeue), and the bucket/harbor e2e suites now assert
bucketReady=trueon a tight bound instead of tolerating the lag, with scoped diagnostics on the failure path.Closes #3032
Refs #2429
Screenshots
N/A — no UI changes.
Release note
Summary by CodeRabbit
Bug Fixes
bucketReadystatus and requeue when readiness isn’t yet observed.Chores