Skip to content

fix(rbg-controller): watch owned RoleBasedGroupScalingAdapter resources#241

Merged
Syspretor merged 2 commits intosgl-project:mainfrom
sebest:fix/rbg-owns-rbgsa
Mar 27, 2026
Merged

fix(rbg-controller): watch owned RoleBasedGroupScalingAdapter resources#241
Syspretor merged 2 commits intosgl-project:mainfrom
sebest:fix/rbg-owns-rbgsa

Conversation

@sebest
Copy link
Copy Markdown
Contributor

@sebest sebest commented Mar 26, 2026

Summary

  • Add .Owns(&RoleBasedGroupScalingAdapter{}, builder.MatchEveryOwner) to the RBG controller's SetupWithManager
  • Without this, if an RBGSA is manually deleted, the RBG controller won't notice until its next unrelated reconcile
  • Uses MatchEveryOwner because RBGSA owner references don't set Controller: true, so the default .Owns() would silently never trigger

Test plan

  • Extended "Create scaling adapter when not exists" test to verify owner reference correctness (name + UID)
  • All existing tests pass

Add .Owns(&RoleBasedGroupScalingAdapter{}) to SetupWithManager so the
RBG controller is notified when an RBGSA is deleted or modified.
Without this, the controller only discovers RBGSA changes on its next
unrelated reconcile, leaving stale or missing adapters undetected.
@gemini-code-assist
Copy link
Copy Markdown

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@sebest sebest marked this pull request as ready for review March 26, 2026 04:08
@gemini-code-assist
Copy link
Copy Markdown

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

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 PR updates the RoleBasedGroup (RBG) controller wiring so that it will reconcile when its owned RoleBasedGroupScalingAdapter (RBGSA) resources change (notably when an adapter is manually deleted), and extends the unit test to validate the created adapter’s owner reference.

Changes:

  • Add an .Owns(&RoleBasedGroupScalingAdapter{}, builder.MatchEveryOwner) watch to the RBG controller.
  • Extend ReconcileScalingAdapter unit test assertions to check the adapter has an OwnerReference pointing back to the RBG.

Reviewed changes

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

File Description
internal/controller/workloads/rolebasedgroup_controller.go Adds an Owns() watch for RoleBasedGroupScalingAdapter using MatchEveryOwner.
internal/controller/workloads/rolebasedgroup_controller_test.go Enhances scaling-adapter reconcile test to assert OwnerReference fields exist/match.

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

Comment thread internal/controller/workloads/rolebasedgroup_controller_test.go
Comment thread internal/controller/workloads/rolebasedgroup_controller.go Outdated
@cheyang
Copy link
Copy Markdown
Collaborator

cheyang commented Mar 26, 2026

I left a few comments on the main risks below.

Comment thread internal/controller/workloads/rolebasedgroup_controller.go Outdated
…ry reconciles

Add RBGScalingAdapterPredicate alongside MatchEveryOwner so that
RBGSA status-only updates (bind/scale) no longer trigger RBG
reconciliation. MatchEveryOwner is retained because RBGSA owner
references do not set Controller: true.

Also improve test coverage: add TypeMeta to test RBG and validate
APIVersion/Kind in owner reference assertions.
@Syspretor Syspretor merged commit f27e10d into sgl-project:main Mar 27, 2026
10 of 11 checks passed
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