Skip to content

Fix create delete infinite loop#373

Merged
nolancon merged 6 commits into
mainfrom
fix-create-delete-infinite-loop
May 14, 2026
Merged

Fix create delete infinite loop#373
nolancon merged 6 commits into
mainfrom
fix-create-delete-infinite-loop

Conversation

@nolancon
Copy link
Copy Markdown
Collaborator

@nolancon nolancon commented May 13, 2026

Description of your changes

This pull request addresses an edge case in the S3 bucket controller where disabled buckets with no backends could cause an infinite etcd write loop. The main change ensures that such buckets are treated as "existing" to prevent unnecessary create operations, and a dedicated test is added to verify this behavior.

Bugfix to prevent etcd write loops for disabled buckets:

  • Updated Observe logic in observe.go to return ResourceExists: true for disabled buckets with no backends, preventing repeated and unnecessary create attempts that could fill up etcd.

Test coverage improvements:

  • Added TestObserveDisabledBucketNoBackendsDoesNotTriggerCreate in observe_test.go to prove that the fix prevents the etcd write loop by ensuring ResourceExists: true is returned in this scenario.
  • Added necessary imports for runtime and fake client to support new tests

I have:

  • Run make ready-for-review to ensure this PR is ready for review.
  • Run make ceph-chainsaw to validate these changes against Ceph. This step is not always necessary. However, for changes related to S3 calls it is sensible to validate against an actual Ceph cluster. Localstack is used in our CI Chainsaw suite for convenience and there can be disparity in S3 behaviours between it and Ceph. See docs/TESTING.md for information on how to run tests against a Ceph cluster.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Existing CI

@nolancon nolancon force-pushed the fix-create-delete-infinite-loop branch 2 times, most recently from 1b709ef to 971d514 Compare May 14, 2026 10:59
Mojachieee and others added 4 commits May 14, 2026 14:20
Observe was returning ResourceExists:false for disabled buckets with
empty atProvider.backends, causing the Crossplane reconciler to invoke
Create (and write three etcd annotations) on every cycle. Return
ResourceExists:true,ResourceUpToDate:true instead, and drive the
condition to Unavailable on first observe so subsequent reconciles
are no-ops.
@nolancon nolancon force-pushed the fix-create-delete-infinite-loop branch from 50a55f3 to fe3bc03 Compare May 14, 2026 14:22
@nolancon nolancon requested a review from Copilot May 14, 2026 14:25
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request fixes an edge case in the Bucket controller’s Observe path where a disabled Bucket CR with no status backends could be repeatedly treated as “needs create,” causing Crossplane to continuously write the external-create-pending annotation and requeue (an etcd write loop). The change treats this specific disabled/no-backends/non-deleting scenario as “externally existing and up-to-date” to stop create attempts, and adds a focused regression test.

Changes:

  • Update Observe to return ResourceExists: true (and ResourceUpToDate: true) for disabled buckets that have no backends and are not being deleted.
  • Add a regression test covering the disabled/no-backends behavior and a control case for non-disabled buckets.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
internal/controller/bucket/observe.go Adds an early-return edge case so disabled buckets with empty status backends don’t trigger Crossplane create-pending loops.
internal/controller/bucket/observe_test.go Adds a dedicated test ensuring Observe reports ResourceExists:true for the disabled/no-backends case and preserves existing behavior for enabled buckets.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/controller/bucket/observe.go Outdated
Comment thread internal/controller/bucket/observe.go Outdated
nolancon and others added 2 commits May 14, 2026 15:36
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@nolancon nolancon marked this pull request as ready for review May 14, 2026 14:37
@nolancon nolancon requested a review from Shunpoco May 14, 2026 14:37
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 28.95%. Comparing base (11cb232) to head (752729d).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #373      +/-   ##
==========================================
+ Coverage   28.58%   28.95%   +0.37%     
==========================================
  Files          60       60              
  Lines        5577     5598      +21     
==========================================
+ Hits         1594     1621      +27     
+ Misses       3899     3894       -5     
+ Partials       84       83       -1     
Flag Coverage Δ
unittests 28.95% <100.00%> (+0.37%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Mojachieee Mojachieee self-requested a review May 14, 2026 15:02
@nolancon nolancon merged commit 5d99aa4 into main May 14, 2026
6 of 10 checks passed
@nolancon nolancon deleted the fix-create-delete-infinite-loop branch May 14, 2026 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants