Skip to content

apiextension/definition: don't attempt to start composite controllers multiple times#5437

Merged
negz merged 2 commits intocrossplane:masterfrom
sttts:sttts-apiextension-definition-xr-multiple-controllers
Mar 1, 2024
Merged

apiextension/definition: don't attempt to start composite controllers multiple times#5437
negz merged 2 commits intocrossplane:masterfrom
sttts:sttts-apiextension-definition-xr-multiple-controllers

Conversation

@sttts
Copy link
Copy Markdown
Contributor

@sttts sttts commented Feb 29, 2024

Description of your changes

The old code depended on controller engine's idem-potency of Run(...). This PR makes this logic explicit in the XRD reconciler, skipping everything around controller start too.

Fixes parts of #5400, more is coming in #5422.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added or updated unit tests.
  • [ ] Added or updated e2e tests.
  • [ ] Linked a PR or a docs tracking issue to document this change.
  • [ ] Added backport release-x.y labels to auto-backport this PR.

Need help with this checklist? See the cheat sheet.

…ontrollers

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>
…rceInformers everywhere

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>
@sttts sttts requested review from a team and negz as code owners February 29, 2024 14:55
@sttts sttts requested a review from turkenh February 29, 2024 14:55
@negz
Copy link
Copy Markdown
Member

negz commented Feb 29, 2024

@sttts Can you provide more detail on the motivation please. Isn't Start still idempotent?

Edit: Also, I'm still waking up so maybe missing something obvious but it doesn't look like this PR currently touches any starting code. Only stopping code. 🤔

@sttts
Copy link
Copy Markdown
Contributor Author

sttts commented Mar 1, 2024

In realtime composition mode we are registering the XR cache in order to manage composite informers referenced by these XRs. That part is not idem-potent. Previsouly every run through the definition reconciler without starting a new controller was replacing these XR caches with unstarted ones.

Edit: Also, I'm still waking up so maybe missing something obvious but it doesn't look like this PR currently touches any starting code. Only stopping code. 🤔

There is an early return added in case the controller is running.

@github-actions
Copy link
Copy Markdown

Backport failed for release-1.15, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-1.15
git worktree add -d .worktree/backport-5437-to-release-1.15 origin/release-1.15
cd .worktree/backport-5437-to-release-1.15
git switch --create backport-5437-to-release-1.15
git cherry-pick -x 947c76f0f8fee1651ed73265b17927ceb992649c 1c949ed056ef03652a273c1cdf5316446688015c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants