Encapsulate KCM controllers with their metadata #120371

atiratree · 2023-09-01T19:30:49Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

this is a followup to following PR and a discussion: #115813 (comment)

These metadata can be used to handle controllers in a generic way.
This enables showing feature gated controllers in kube-controller-manager's help.
It is possible to obtain a controllerName in the InitFunc so it can be passed down to and used by the controller.
include ServiceAccountTokenController in the NewRegistrableControllers to make it more generic
move start controller pre- and post- checks/actions out of StartControllers into StartController function

metadata about a controller:
- name
- requiredFeatureGates
- aliases
- isDisabledByDefault
- isCloudProviderController
- requiresSpecialHandling

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

similar thing can be done for cloud-controller-manager, but it is better to open a new separate PR for that

Does this PR introduce a user-facing change?

kube-controller-manager's help will include controllers behind a feature gate in `--controllers` flag

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

cmd/kube-controller-manager/app/controllermanager.go

atiratree · 2023-09-01T19:48:51Z

cmd/kube-controller-manager/app/controllermanager.go

-type serviceAccountTokenControllerStarter struct {
- rootClientBuilder clientbuilder.ControllerClientBuilder
+// RegistrableController map so that it can always run first.
+func newServiceAccountTokenRegistrableController(rootClientBuilder clientbuilder.ControllerClientBuilder) *registrableController {


can we move this controller to a new file?

atiratree · 2023-09-01T19:56:27Z

cc people from the last PR: @liggitt @nckturner @aojea @cici37 @alexanderConstantinescu @msau42 @jprzychodzen @soltysh @andrewsykim @cheftako @thockin @sftim

thockin

This was just a skim, but I personally like the direction. I'd emphasize the encapsulation by segregating this code behind a clean API (even if in the same pkg, but a different file) and making the code that calls it as simple and streamlined as possible.

cmd/kube-controller-manager/app/controllermanager.go

alexzielenski · 2023-09-05T20:04:20Z

/triage accepted

atiratree · 2023-09-18T21:41:46Z

as a result of the discussions, I have made the following changes:

renamed RegistrableController to ControllerDescriptor
removed ControllerDescriptor interface in favor of a struct
moved start controller pre- and post- checks/actions out of StartControllers into StartController function

aojea · 2023-09-18T21:42:21Z

cmd/kube-controller-manager/app/validatingadmissionpolicystatus.go

+ return &ControllerDescriptor{
+ name: names.ValidatingAdmissionPolicyStatusController,
+ initFunc: startValidatingAdmissionPolicyStatusController,
+ requiredFeatureGates: []featuregate.Feature{


I don´t have the feeling this is a good idea, because right now it looks pretty neat and clean, but as soon as the code evolves and new features are added this is just yet another place to update the feature gates ... or what happen if the controller has only one part of the logic with a feature gate? we need to run the controller and we still need to add a feature gate inside the controller logic

some food for thoughts

It depends on what tradeoff we want to make. If we want to have all the controller metadata and logic sprinkled everywhere or together. We already have feature gate logic sprinkled everywhere, so IMO this would not change that much. That is, one has to search for all occurrences of a feature gate to get the full picture.

or what happen if the controller has only one part of the logic with a feature gate? we need to run the controller and we still need to add a feature gate inside the controller logic

I think we would solve this the same way we do today. We can put a feature gate call inside the init function or inside the controller itself.

For the usual cases, we could reason about controllers without running them. That is, we could for example show which controllers are gated by which feature gates in kube-controller-manager's help

the logic is always going to be sprinkled everywhere, the feature gated code can be anything, a function or just part of a function ...

For the usual cases, we could reason about controllers without running them. That is, we could for example show which controllers are gated by which feature gates in kube-controller-manager's help

I think that the intent is good, but I'm afraid that the reality is different and it soon will get out of sync and we'll have an additional place interacting with the code that is not obvious for everybody

I know it only helps a little, but I have added a unit test to make sure people do not mix the old and the new approach.

atiratree · 2023-09-20T17:39:31Z

/test pull-kubernetes-conformance-kind-ga-only-parallel

atiratree · 2023-10-10T13:10:42Z

#119208 shows a use case for this refactoring and #119208 (comment) discusses how controller name should be consumed.

atiratree · 2023-10-10T13:11:08Z

@thockin could you please review again?

atiratree · 2023-10-16T18:35:16Z

rebased

thockin

really minor questions

cmd/kube-controller-manager/app/controllermanager.go

- These metadata can be used to handle controllers in a generic way. - This enables showing feature gated controllers in kube-controller-manager's help. - It is possible to obtain a controllerName in the InitFunc so it can be passed down to and used by the controller. metadata about a controller: - name - requiredFeatureGates - isDisabledByDefault - isCloudProviderController

… to make it more generic - pass a map of controllerDescriptors instead of a function

…ollers into StartController function the function is reused by ServiceAccountTokenController

- controller descriptors should not be feature gated - aliases should not be defined for new controllers and have only a canonical name

thockin · 2023-10-27T18:04:45Z

Thanks!

/lgtm
/approve

k8s-ci-robot · 2023-10-27T18:04:52Z

LGTM label has been added.

Git tree hash: 2652230cbcb0ffbf6673bdab4f6cb7022ae35a07

k8s-ci-robot · 2023-10-27T18:05:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atiratree, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/kube-controller-manager/OWNERS~~ [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

atiratree · 2023-10-30T13:45:54Z

Thanks for the review @thockin. Unfortunately I missed controllers enabled by default in one test case. And just ran into it during a review #119208 (comment).

I have opened a small PR for this: #121611

k8s-ci-robot requested review from mwielgus and saad-ali September 1, 2023 19:30

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 1, 2023

atiratree mentioned this pull request Sep 1, 2023

Introduce controller aliases for KCM and CCM #115813

Merged

atiratree commented Sep 1, 2023

View reviewed changes

thockin reviewed Sep 1, 2023

View reviewed changes

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 5, 2023

atiratree force-pushed the encapsulate-kcm-controllers branch from c40db78 to eca4ea1 Compare September 18, 2023 21:37

aojea reviewed Sep 18, 2023

View reviewed changes

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 19, 2023

atiratree mentioned this pull request Oct 10, 2023

Decouple TaintManager from NodeLifeCycleController (KEP-3902) #119208

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2023

atiratree force-pushed the encapsulate-kcm-controllers branch 2 times, most recently from 280db77 to c3b3fb6 Compare October 16, 2023 18:35

atiratree force-pushed the encapsulate-kcm-controllers branch from c3b3fb6 to c91e15f Compare October 20, 2023 17:35

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2023

thockin self-assigned this Oct 25, 2023

thockin reviewed Oct 26, 2023

View reviewed changes

cmd/kube-controller-manager/app/controllermanager.go Outdated Show resolved Hide resolved

cmd/kube-controller-manager/app/controllermanager.go Show resolved Hide resolved

atiratree added 5 commits October 27, 2023 13:28

include ServiceAccountTokenController in the NewControllerDescriptors…

a85779b

… to make it more generic - pass a map of controllerDescriptors instead of a function

move aliases into each registrable controller

b768967

move start controller pre- and post- checks/actions out of StartContr…

44cac26

…ollers into StartController function the function is reused by ServiceAccountTokenController

add unit tests for NewControllerDescriptors

1591a0e

- controller descriptors should not be feature gated - aliases should not be defined for new controllers and have only a canonical name

atiratree force-pushed the encapsulate-kcm-controllers branch from c91e15f to 1591a0e Compare October 27, 2023 11:28

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 27, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 27, 2023

k8s-ci-robot merged commit ee474e6 into kubernetes:master Oct 27, 2023
6 of 14 checks passed

k8s-ci-robot added this to the v1.29 milestone Oct 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encapsulate KCM controllers with their metadata #120371

Encapsulate KCM controllers with their metadata #120371

atiratree commented Sep 1, 2023 •

edited

atiratree Sep 1, 2023

atiratree commented Sep 1, 2023

thockin left a comment

alexzielenski commented Sep 5, 2023

atiratree commented Sep 18, 2023

aojea Sep 18, 2023

atiratree Sep 18, 2023

aojea Sep 19, 2023 •

edited

atiratree Sep 19, 2023 •

edited

atiratree commented Sep 20, 2023

atiratree commented Oct 10, 2023

atiratree commented Oct 10, 2023

atiratree commented Oct 16, 2023

thockin left a comment

thockin commented Oct 27, 2023

k8s-ci-robot commented Oct 27, 2023

k8s-ci-robot commented Oct 27, 2023

atiratree commented Oct 30, 2023

Encapsulate KCM controllers with their metadata #120371

Encapsulate KCM controllers with their metadata #120371

Conversation

atiratree commented Sep 1, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

atiratree Sep 1, 2023

Choose a reason for hiding this comment

atiratree commented Sep 1, 2023

thockin left a comment

Choose a reason for hiding this comment

alexzielenski commented Sep 5, 2023

atiratree commented Sep 18, 2023

aojea Sep 18, 2023

Choose a reason for hiding this comment

atiratree Sep 18, 2023

Choose a reason for hiding this comment

aojea Sep 19, 2023 • edited

Choose a reason for hiding this comment

atiratree Sep 19, 2023 • edited

Choose a reason for hiding this comment

atiratree commented Sep 20, 2023

atiratree commented Oct 10, 2023

atiratree commented Oct 10, 2023

atiratree commented Oct 16, 2023

thockin left a comment

Choose a reason for hiding this comment

thockin commented Oct 27, 2023

k8s-ci-robot commented Oct 27, 2023

k8s-ci-robot commented Oct 27, 2023

atiratree commented Oct 30, 2023

atiratree commented Sep 1, 2023 •

edited

aojea Sep 19, 2023 •

edited

atiratree Sep 19, 2023 •

edited