fix(objectstorage-controller): retry on Bucket conflict during BucketAccess reconcile by myasnikovdaniil · Pull Request #2529 · cozystack/cozystack

myasnikovdaniil · 2026-04-29T06:40:38Z

What this PR does

Carries an upstream-bound patch against COSI v0.2.2's BucketAccess sidecar reconciler so it survives the optimistic-concurrency race with the central Bucket reconciler running in the same process.

In sidecar/pkg/bucketaccess/bucketaccess_controller.go, the Add path does:

bucket, _ := buckets().Get(...)
// ...
if controllerutil.AddFinalizer(bucket, BABucketFinalizer) {
    buckets().Update(ctx, bucket, ...)   // <-- 409 here
}

Between Get and Update the Bucket reconciler in the same binary can mutate the object and bump resourceVersion, producing:

Operation cannot be fulfilled on buckets.objectstorage.k8s.io "bucket-...":
the object has been modified; please apply your changes to the latest
version and try again

surfaced to users as a FailedGrantAccess event on the BucketAccess.

This patch wraps the Bucket finalizer add in retry.RetryOnConflict(retry.DefaultRetry, ...) and re-Gets the Bucket inside the closure so each retry mutates the freshest version.

Patch is held as 91-bucketaccess-conflict-retry.diff and consumed at image build time (re-introduces git to the source stage and the COPY patches /patches + git apply /patches/*.diff steps that existed before c29d501b dropped them when 89/90 were upstreamed). The convention is identical: drop the local patch the moment the upstream PR merges and ships in a tagged release.

Release note

[objectstorage-controller] Carry a patch that retries on Bucket update conflict during BucketAccess reconcile, eliminating the "object has been modified" FailedGrantAccess event seen in CI.

Test plan

make image for system/objectstorage-controller and verify values.yaml picks up the new digest.
Re-run the cozystack-e2e suite that emitted the FailedGrantAccess: ... object has been modified event on tenant-test/BucketAccess/bucket-test-admin and confirm it no longer fires.
Verify kubectl describe bucketaccess reports AccessGranted=true on first reconcile, no requeue from the conflict path.

Summary by CodeRabbit

Bug Fixes
- Enhanced bucket access update reliability with improved conflict handling during concurrent operations. The system now automatically retries operations and refetches data to prevent update conflicts.
Chores
- Improved Docker build process with automatic patch application support for source customization.

… BucketAccess reconcile Upstream COSI v0.2.2's BucketAccess reconciler does a Get->mutate->Update on the parent Bucket and surfaces "Operation cannot be fulfilled ... the object has been modified" as a FailedGrantAccess event when it races against the Bucket reconciler in the same controller process. Wrap the mutation in retry.RetryOnConflict so the reconcile loop refreshes and retries instead of leaking the conflict to users. Carried as 91-bucketaccess-conflict-retry.diff until upstreamed (cf. 89-reconciliation.diff and 90-bucket-name.diff, both dropped in c29d501 once merged upstream). Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>

gemini-code-assist · 2026-04-29T06:40:45Z

Summary of Changes

Hello, I'm Gemini Code Assist¹! 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 in the BucketAccess reconciler where concurrent updates to the Bucket object result in 409 conflict errors. By wrapping the finalizer update in a retry mechanism, the controller can now successfully reconcile even when the object version is modified by the central Bucket reconciler during the process.

Highlights

Conflict Resolution: Implemented retry logic using retry.RetryOnConflict to handle optimistic-concurrency race conditions during BucketAccess reconciliation.
Build Process Update: Re-introduced git and patch application steps in the Dockerfile to support carrying the upstream-bound patch.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Ignored Files

Ignored by pattern: **/*.diff (1)
- packages/system/objectstorage-controller/images/objectstorage/patches/91-bucketaccess-conflict-retry.diff

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	Gemini (@gemini-code-assist)	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

coderabbitai · 2026-04-29T06:40:51Z

📝 Walkthrough

Walkthrough

The Dockerfile is modified to install git and apply local patch files to the container-object-storage-interface source during the build process. A new patch file is introduced that modifies the BucketAccess listener's Add method to handle update conflicts using retry logic with re-fetching and conditional finalizer addition.

Changes

Cohort / File(s)	Summary
Build Configuration & Patch Infrastructure `packages/system/objectstorage-controller/images/objectstorage/Dockerfile`, `packages/system/objectstorage-controller/images/objectstorage/patches/91-bucketaccess-conflict-retry.diff`	Dockerfile updated to install git and apply local patches. New patch file implements conflict retry mechanism in BucketAccess listener's `Add` method with re-fetching logic and conditional finalizer addition.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A patch hops in with retry grace,
Conflicts resolved at steady pace,
Git installs, patches apply,
BucketAccess won't let updates die! 🔄✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(objectstorage-controller): retry on Bucket conflict during BucketAccess reconcile' clearly and specifically describes the main change: adding retry logic to handle Bucket update conflicts in the BucketAccess reconciler.

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

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch daniil/bucketaccess-object-modified-retry

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

gemini-code-assist

Code Review

This pull request updates the objectstorage Dockerfile to install git and apply local patches to the upstream source. However, the required patch files are missing from the changes, which will cause the build to fail. Additionally, the PR needs to be updated to follow the repository's style guide regarding Conventional Commits and commit sign-offs.

IvanHunters

The retry logic is correct — retry.RetryOnConflict with re-Get inside the closure is the right Kubernetes idiom for this kind of conflict. The patch itself applies cleanly against the upstream v0.2.2 tag.

However, the fix is not actually deployed: values.yaml still references the image that was built before this Dockerfile change was introduced, so the patch never makes it into the running binary. The make image step needs to be run and the resulting digest committed.

Required:

make -C packages/system/objectstorage-controller image

Then commit the updated values.yaml with the new image digest.

One more thing: Gemini's comment saying "patch file missing" is incorrect — 91-bucketaccess-conflict-retry.diff is present in the PR.

myasnikovdaniil · 2026-04-30T05:04:56Z

IvanHunters we build artifacts on release/test time, i dont need to build image myself and commit new image tag

IvanHunters

LGTM

Aleksei Sviridkin (lexfrei)

LGTM as a second opinion. The retry shape is correct: retry.RetryOnConflict(retry.DefaultRetry, …) with the re-Get inside the closure is the canonical Kubernetes idiom for finalizer adds racing against another controller, and it preserves the original "skip update if AddFinalizer reports no-op" semantics on every retry. retry.DefaultRetry bounds total wait at ~1s with exponential backoff — won't pile up if conflicts persist. Patch applies cleanly against the upstream v0.2.2 tag.

The Dockerfile change re-introduces precisely the convention removed in 50d6c29 / c29d501 (March 2026), when upstream patches 89 and 90 were dropped after merging — same pattern (alpine git + COPY patches /patches + RUN git apply /patches/*.diff). The expectation that this patch goes away the moment upstream ships it in a tagged release is consistent with what the repo did with 89/90.

Build/E2E green: make build in CI rebuilds the controller image with the patch applied and the resulting digest lands in values.yaml via pr.patch, so the E2E sandbox actually exercises the patched binary even though the in-repo values.yaml digest is unchanged here. The release path will re-bake the digest on the next tag (tags.yaml runs make build and commits the artifacts), which is also consistent with how the controller image is normally rolled.

Two non-blocking nits worth folding in either here or as a follow-up:

Add the upstream PR/issue reference to the patch header for traceability — kubernetes-sigs/container-object-storage-interface#302 is the matching upstream PR; #166 tracks the broader "object has been modified" class. A line like Upstream-PR: kubernetes-sigs/container-object-storage-interface#302 in the patch's comment block would make the "drop when upstream merges" gate explicit and grep-able from a future maintainer's perspective.
The PR description says "the Bucket reconciler in the same controller process" — controller and sidecar are actually separate binaries (Dockerfile builds image-controller and image-sidecar from the same source as distinct targets), so the race is between processes/containers, not goroutines in one process. Doesn't change the fix or its correctness — RetryOnConflict covers both — but the wording in the PR body might confuse a future reader debugging a different race.

Audit of 30 successful PR runs found that across 5 sampled failure attempts, **25/25 retries** on `Run E2E tests` failed — the retry loop never recovered a flake, only stretched deterministic failures and tripled diagnostic wall-time. Same data shape on `Install Cozystack`. Drop both. `Prepare environment` keeps its 3x retry — that step is pure infrastructure (Talos image download, sandbox VM boot, network) where transient runner hiccups warrant a retry. On failure, the test step now captures `kubectl get hr -A -o wide` and `kubectl get events -A` under a collapsible group so triage starts with the actual broken-state snapshot. Depends on: - #2508 — installer namespace bootstrap (Helm namespace-ownership conflict) - #2509 — operator HelmRelease config knobs (`seaweedfs-system` 2-min wait race) - #2528 — harbor bucket-secret + BucketInfo gating (harbor ValuesError) - #2529 — objectstorage-controller BucketAccess conflict retry - the daniil/split-vminstance PR (vminstance disk race + VM IP/ready timeouts) - the daniil/split-event-driven PR (existence backstops surfacing real errors) Until those land, dropping the retry will fail CI for unrelated PRs that hit the seaweedfs / harbor / installer / vminstance races. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-By: Claude <noreply@anthropic.com>

Audit of 30 successful PR runs found that across 5 sampled failure attempts, **25/25 retries** on `Run E2E tests` failed — the retry loop never recovered a flake, only stretched deterministic failures and tripled diagnostic wall-time. Same data shape on `Install Cozystack`. Drop both. `Prepare environment` keeps its 3x retry — that step is pure infrastructure (Talos image download, sandbox VM boot, network) where transient runner hiccups warrant a retry. On failure, the test step now captures `kubectl get hr -A -o wide` and `kubectl get events -A` under a collapsible group so triage starts with the actual broken-state snapshot. Depends on: - #2508 — installer namespace bootstrap (Helm namespace-ownership conflict) - #2509 — operator HelmRelease config knobs (`seaweedfs-system` 2-min wait race) - #2528 — harbor bucket-secret + BucketInfo gating (harbor ValuesError) - #2529 — objectstorage-controller BucketAccess conflict retry - the daniil/split-vminstance PR (vminstance disk race + VM IP/ready timeouts) - the daniil/split-event-driven PR (existence backstops surfacing real errors) Until those land, dropping the retry will fail CI for unrelated PRs that hit the seaweedfs / harbor / installer / vminstance races. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>

Audit of 30 successful PR runs found that across 5 sampled failure attempts, **25/25 retries** on `Run E2E tests` failed — the retry loop never recovered a flake, only stretched deterministic failures and tripled diagnostic wall-time. Same data shape on `Install Cozystack`. Drop both. `Prepare environment` keeps its 3x retry — that step is pure infrastructure (Talos image download, sandbox VM boot, network) where transient runner hiccups warrant a retry. On failure, the test step now captures `kubectl get hr -A -o wide` and `kubectl get events -A` under a collapsible group so triage starts with the actual broken-state snapshot. Depends on: - #2508 — installer namespace bootstrap (Helm namespace-ownership conflict) - #2509 — operator HelmRelease config knobs (`seaweedfs-system` 2-min wait race) - #2528 — harbor bucket-secret + BucketInfo gating (harbor ValuesError) - #2529 — objectstorage-controller BucketAccess conflict retry - the daniil/split-vminstance PR (vminstance disk race + VM IP/ready timeouts) - the daniil/split-event-driven PR (existence backstops surfacing real errors) Until those land, dropping the retry will fail CI for unrelated PRs that hit the seaweedfs / harbor / installer / vminstance races. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-By: Claude <noreply@anthropic.com>

…ay.bats tenant teardown (#2558) ## What this PR does Drops the 3× retry loop on `Run E2E tests` and `Install Cozystack into sandbox`. `Prepare environment` keeps its 3× retry — that step is pure infrastructure (Talos image download, sandbox VM boot, network) where transient runner hiccups warrant a retry. On failure, the test step now captures `kubectl get hr -A -o wide` and `kubectl get events -A` under a collapsible group so triage starts with the actual broken-state snapshot. > [!NOTE] > An earlier revision of this PR also doubled every bats timeout. That commit was dropped in a rebase and is intentionally **not restored**: the timeout class that actually matters (per-app HR-Ready waits) has since been standardized at 5m on `main` (7b9f286), making a blanket 2× redundant. **Fixes gateway.bats teardown leakage.** The nested-tenant tests deleted tenants fire-and-forget, parent and child back-to-back. The leftover uninstalls (each blocked on a cleanup Job, parents wedged on still-terminating child namespaces) plus one mid-install child HR occupied exactly 5 workers on the `--concurrent=5` tenants helm-controller shard, starving whichever app test ran next — observed as the harbor HR sitting unreconciled for its whole 5m HR-Ready budget in [run 27020081550](https://github.com/cozystack/cozystack/actions/runs/27020081550), surfaced by this PR's own retry removal + diagnostics dump. Teardown now deletes child→parent with hard `wait hr --for=delete` between, so a wedged tenant uninstall fails gateway.bats itself, not an innocent neighbor. ## Why Audit of 30 successful PR runs found that across 5 sampled failure attempts, **25/25 retries** on `Run E2E tests` failed — the retry loop never recovered a flake, only stretched deterministic failures and tripled diagnostic wall-time. Same data shape on `Install Cozystack`. Beyond wasted CI time, the retry was hiding ~10 deterministic bugs (Helm namespace-ownership conflict, seaweedfs HR timeout, harbor BucketInfo wiring, vminstance disk race, etc.). Each failure looked like a "flake" because the retry sometimes coincided with whatever transient state had cleared — the retry never fixed the bug, just delayed surfacing. ## Dependencies The deterministic bugs the retry was masking are now fixed on `main`: - ✅ **#2508** — installer namespace bootstrap (Helm namespace-ownership conflict on cold install) — merged - ✅ **#2509** — operator HelmRelease config knobs (`seaweedfs-system` 2-min wait race within Flux's 5-min reconcile windows) — merged - ✅ **#2528** — harbor bucket-secret + BucketInfo gating (harbor `ValuesError` on first install) — merged - ✅ **#2529** — objectstorage-controller BucketAccess conflict retry — merged Companion PRs in the #2619 split (independent of this PR, ordering-wise): - **#2602** — Flux v2.8.0 + chart fixes - **#2601** — seaweedfs-system split This PR does NOT depend on #2602/#2601 — it now touches only the workflow file and gateway.bats teardown, both on top of fresh `main`. Surfaced from #2500. ### Release note ```release-note NONE ```  ## Summary by CodeRabbit * **Chores** * CI prepare-environment step now reports plain attempt counts with clear success/failure messages. * Install and per-app test steps no longer retry; each runs once and fails immediately on error. Failed apps log diagnostics and job proceeds to remaining apps while overall job fails. * **Tests** * End-to-end tests and install/prepare flows use longer, more tolerant timeouts and added existence polling to reduce flakiness and improve diagnostics.  [![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2558?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)

myasnikovdaniil requested review from IvanHunters, Andrey Kolkov (androndo), Andrei Kvapil (kvaps), Aleksei Sviridkin (lexfrei), Timofei Larkin (lllamnyp) and Kirill Ilin (sircthulhu) as code owners April 29, 2026 06:40

github-actions Bot added the area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review label Apr 29, 2026

dosubot Bot added the size/M This PR changes 30-99 lines, ignoring generated files label Apr 29, 2026

dosubot Bot added area/storage Issues or PRs related to storage (linstor, seaweedfs, bucket, velero, harbor) kind/bug Categorizes issue or PR as related to a bug upstream-issue Requires resolving an issue in an upstream project labels Apr 29, 2026

gemini-code-assist Bot reviewed Apr 29, 2026

View reviewed changes

Comment thread packages/system/objectstorage-controller/images/objectstorage/Dockerfile

Comment thread packages/system/objectstorage-controller/images/objectstorage/Dockerfile

IvanHunters requested changes Apr 29, 2026

View reviewed changes

myasnikovdaniil requested a review from IvanHunters April 30, 2026 09:59

IvanHunters approved these changes Apr 30, 2026

View reviewed changes

dosubot Bot added the lgtm This PR has been approved by a maintainer label Apr 30, 2026

myasnikovdaniil changed the title ~~[objectstorage-controller] retry on Bucket conflict during BucketAccess reconcile~~ fix(objectstorage-controller): retry on Bucket conflict during BucketAccess reconcile Apr 30, 2026

This was referenced Apr 30, 2026

feat(e2e): consolidated E2E optimization changes (cozyreport + helmrelease-interval) #2500

Closed

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

Merged

Aleksei Sviridkin (lexfrei) approved these changes May 4, 2026

View reviewed changes

myasnikovdaniil merged commit 6ad5bf0 into main May 4, 2026
23 checks passed

myasnikovdaniil deleted the daniil/bucketaccess-object-modified-retry branch May 4, 2026 13:50

myasnikovdaniil mentioned this pull request May 13, 2026

Move E2E tests out of PR pipeline to release/nightly #2397

Closed

Navigation Menu

Uh oh!

fix(objectstorage-controller): retry on Bucket conflict during BucketAccess reconcile - #2529

fix(objectstorage-controller): retry on Bucket conflict during BucketAccess reconcile#2529
myasnikovdaniil merged 1 commit into
mainfrom
daniil/bucketaccess-object-modified-retry

myasnikovdaniil commented Apr 29, 2026 •

edited

Loading

Uh oh!

gemini-code-assist Bot commented Apr 29, 2026

Uh oh!

coderabbitai Bot commented Apr 29, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Uh oh!

Uh oh!

IvanHunters left a comment

Uh oh!

myasnikovdaniil commented Apr 30, 2026

Uh oh!

IvanHunters left a comment

Uh oh!

Aleksei Sviridkin (lexfrei) left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

myasnikovdaniil commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Release note

Test plan

Summary by CodeRabbit

Uh oh!

gemini-code-assist Bot commented Apr 29, 2026

Summary of Changes

Highlights

Footnotes

Uh oh!

coderabbitai Bot commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

IvanHunters left a comment

Choose a reason for hiding this comment

Uh oh!

myasnikovdaniil commented Apr 30, 2026

Uh oh!

IvanHunters left a comment

Choose a reason for hiding this comment

Uh oh!

Aleksei Sviridkin (lexfrei) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

myasnikovdaniil commented Apr 29, 2026 •

edited

Loading

coderabbitai Bot commented Apr 29, 2026 •

edited

Loading