✨ feat: scope controller name validation to manager instance #3241
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jingyih The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @jingyih! |
|
Hi @jingyih. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
jingyih
changed the title
| } | ||
|
|
||
| if cm.usedControllerNames.Has(name) { | ||
| return fmt.Errorf("controller with name %s already exists in this manager. Controller names must be unique within the same controller manager to avoid multiple controllers reporting the same metric. This validation can be disabled via the SkipNameValidation option", name) |
There was a problem hiding this comment.
When you have multiple managers, how do the metrics look? Is there a specific metric label that identifies the manager?
There was a problem hiding this comment.
No, there is nothing. The metrics are global and not scoped to the manager, which is why the check is as it is
There was a problem hiding this comment.
Thank you for the feedback. You've highlighted an important issue regarding metric collisions. I can see that the metrics are global and not scoped to individual manager instances, which is why the original check existed.
The main goal of this PR is to improve the developer experience for the common case of running a single manager per process. The current global validation sometimes require developers to use SkipNameValidation when the test recreating managers, which can mask naming conflicts until runtime. By scoping validation to the manager, these errors can be caught early during testing.
I am aware that this change introduces challenges for the less common scenario of running multiple managers in a single process. In order to improve dev experience for single controller manger use case, this PR requires dev to manually avoid naming conflict when working with multiple controller managers. I see two possible solutions to address this:
- Use separate Prometheus registries for each manager
- Add a manager-specific metric label
I'm open to suggestions and looking forward to your thoughts.
There was a problem hiding this comment.
I would recommend to disable the name validation in tests
|
/hold See: #3241 (comment) |
k8s-ci-robot
added
the
do-not-merge/hold
This PR refactors controller name validation to be scoped to each controller manager instance, rather than being enforced globally across the entire process.
Global uniqueness could cause conflicts in test suites where multiple managers are created and destroyed. Since the global list of names is not reset, tests can fail unexpectedly when they try to reuse a controller name in a new manager instance. Related issues:
The new implementation aligns controller name validation with the lifecycle of the manager, which IMO is the correct architectural approach.