Introduce controller aliases for KCM and CCM #115813

atiratree · 2023-02-15T22:44:06Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

There are many inconsistencies in the naming of KCM and CCM controllers. We would like to have consistent naming of controllers now when we switch to contextual logging . This is important for

more readable logging where each line has a familiar component prefix
makes names consistent across all controllers
the same name can be referenced when using CLI help / docs / in initiliazers

This PR introduces aliases for estabilished controllers like podgc. User has two options when specifying --controllers flag:

use the new name pod-garbage-collector-controller
use the old podgc name (alias) which will internally resolve to pod-garbage-collector-controller

a followup will look into modifying each controller to pick up the new names for:

integration with contextual logging where a logger will automatically obtain a component prefix for its component
logging should use a canonical controller name when referencing a controller (Eg. Starting X, Shutting down X)
this also can be done for running_managed_controllers metric (ControllerStarted, ControllerStopped)
logging of WaitForNamedCacheSync
controller events

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

for more details please see this discussion: https://groups.google.com/a/kubernetes.io/g/dev/c/I9l8SSM599s/m/PtzEMIS8BAAJ

Does this PR introduce a user-facing change?

kube-controller-manager and cloud-controller-manager have changed the name of controllers that can be turned off/on that are passed to the `--controllers` flag (eg `pod-garbage-collector-controller` ). The old names (eg `podgc`) are also accepted and aliased to the new names

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

cmd/kube-controller-manager/app/names/controller_names.go

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

staging/src/k8s.io/cloud-provider/app/names/controller_names.go

staging/src/k8s.io/cloud-provider/options/kubecloudshared.go

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

atiratree · 2023-02-16T00:19:23Z

cmd/cloud-controller-manager/.import-restrictions

@@ -3,6 +3,7 @@ rules:
 allowedPrefixes:
 - k8s.io/kubernetes/cmd/kube-controller-manager/app/options
 - k8s.io/kubernetes/cmd/kube-controller-manager/app/config
+ - k8s.io/kubernetes/cmd/kube-controller-manager/app/names


For nodeipam controller. Should be safe.

#115813 (comment)

cmd/kube-controller-manager/app/names/controller_names.go

liggitt · 2023-02-16T15:15:07Z

cmd/kube-controller-manager/app/names/controller_names.go

+
+import cpnames "k8s.io/cloud-provider/app/names"
+
+// canonical controller names


can this explain where these names are used (command-line invocations, events, contextual logging...?), and the policy around changing them (I think the policy will be "don't")

good idea, I have added a documentation for these names inside both of the names packages

staging/src/k8s.io/cloud-provider/app/names/controller_names.go

liggitt · 2023-02-16T15:20:20Z

cmd/cloud-controller-manager/main.go

 // "node-controller" is the shared identity of all node controllers, including node, node lifecycle, and node ipam.
 // See https://github.com/kubernetes/kubernetes/pull/72764#issuecomment-453300990 for more context.
 InitContext: app.ControllerInitContext{
 ClientName: "node-controller",
 },
 Constructor: nodeIpamController.StartNodeIpamControllerWrapper,
 }
+ controllerAliases["nodeipam"] = kcmnames.NodeIpamControllerName


it's a little strange to use the kcmnames.NodeIpamControllerName for the name but define the alias locally here

I thought about this in a way that the old name belonged to CCM when the aliases did not exist yet. Theoretically it could have been a different name from KCM.

due to import restrictions I am not defining it in CCMControllerAliases

all the nodeipam controller relevant bits are in CCM main package so it is in a relevant place

I suppose I could also import and use the KCMControllerAliases, if we don't want to have a lonely string around.

liggitt · 2023-02-16T15:21:32Z

cmd/cloud-controller-manager/main.go

 "k8s.io/cloud-provider/options"
 "k8s.io/component-base/cli"
 cliflag "k8s.io/component-base/cli/flag"
 _ "k8s.io/component-base/metrics/prometheus/clientgo" // load all the prometheus client-go plugins
 _ "k8s.io/component-base/metrics/prometheus/version" // for version metric registration
 "k8s.io/klog/v2"
+ kcmnames "k8s.io/kubernetes/cmd/kube-controller-manager/app/names"


I'm pretty sure we don't want references to k8s.io/kubernetes/cmd/kube-controller-manager from cloud-controller-manager... should the nodeipam name be in k8s.io/cloud-provider/app/names instead?

I would like to have the controller name defined in the same place/module where the controller lives. And the nodeipam controller lives in KCM and is imported by CCM

kubernetes/cmd/cloud-controller-manager/nodeipamcontroller.go

Lines 37 to 39 in 894cfdf

nodeipamcontrolleroptions "k8s.io/kubernetes/cmd/kube-controller-manager/app/options"

nodeipamcontroller "k8s.io/kubernetes/pkg/controller/nodeipam"

nodeipamconfig "k8s.io/kubernetes/pkg/controller/nodeipam/config"

KCM imports CCM controllers/options/names as well. We prevent circular dependency by choosing the relevant packages for nodeipam controller and only importing them in the main package of CCM

kubernetes/cmd/cloud-controller-manager/.import-restrictions

Line 4 in 894cfdf

- k8s.io/kubernetes/cmd/kube-controller-manager/app/options

I'm not really sure if anyone uses CCM from K/K. It seems that it was some step in Cloud Provider Extraction/Migration that is not-used anymore.

@cheftako and @nckturner, do you know the exact context about this CCM implementation and if anyone uses it?

No one (afaik) uses the cloud-controller-manager binary thats built from the cmd/cloud-controller-manager, but everyone uses the cloud-provider library in staging.

Do we want to deprecate or remove the support for the binary and the relevant code in parallel with this PR?

Yes, we can propose it in a PR.

I am proposing a deprecation in #117564 - more details in the PR.

So are we OK with this because it is going away?

Some extra content: cmd/cloud-controller-manager previously stays as an example for new incoming cloud providers. Do we still plan to have a way to educate the incoming cloud provider if this is removed?

Please correct me if I am wrong, but I do not see an issue with the imports as I mentioned in the earlier comment, even if the example CCM does not go away.

We can start with the deprecation and have some time to discuss where to move it before the removal. We can also keep the README in cmd/cloud-controller-manager and reference the new location after that.

liggitt · 2023-02-16T15:23:32Z

cmd/kube-controller-manager/app/names/controller_names.go

+// 1. use of shortcuts should be avoided
+// 2. kubernetes resources should be written together without a dash ("-")
+const (
+ ServiceAccountTokenControllerName = "serviceaccount-token-controller"


nit: is it stuttery to end every constant with Name when we're already in a package called names?

can we do anything in a unit test to enforce consistency on these names (all lowercase, - instead of _, end in -controller, etc)?

+1 for both

I have updated controllermanager_test to enforce the consistency via a regex. Should I also limit the length of the controller name as we do in DNS1123 validations? Not sure if that is useful.

liggitt · 2023-02-16T15:24:48Z

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

+}
+
+func TestControllerNamesDeclaration(t *testing.T) {
+ declaredControllers := sets.New(


should this set of all declared controller names live in the names package?

I would like to keep one source of truth for this set, which would be KnownControllers. This is just to test that this source is well defined. I do not see a benefit of exposing additional set (source) in the names package at this moment.

atiratree · 2023-02-17T12:01:51Z

attachdetach unit test failed which should not be affected by these changes
/test pull-kubernetes-unit

sftim · 2023-02-17T13:28:05Z

Ideally: add a list of old and new name mappings (adding these to a new page within https://k8s.io/docs/reference/)

atiratree · 2023-02-17T16:38:49Z

I am not sure if it is necessary to advertise these mappings that much (as a new page). Can we rather enhance existing kube-controller-manager page and inject a note there mentioning the aliases behaviour with a link to 1.26 doc for the old names? (they can be found in the help for --controllers flag)

Nevertheless I can open a doc PR once this one gets resolved and continue the discussion there.

sftim · 2023-02-17T16:45:35Z

Editing the existing (auto generated) page means editing the generator code, which may not be simpler. Of course the help for --controllers gets copied there automatically.

cici37 · 2023-02-21T21:11:28Z

/triage accepted

cici37 · 2023-02-23T17:38:25Z

/triage accepted

atiratree · 2023-06-13T21:26:23Z

manually rebased - needed to fix an error for the tests

…ionOptions

and move the to cloud-provider module

atiratree · 2023-06-19T15:12:33Z

observed flake #114852
/test pull-kubernetes-unit

soltysh

/lgtm

k8s-ci-robot · 2023-06-22T10:58:38Z

LGTM label has been added.

Git tree hash: 99586855218ffbf9bb158fb4805c2ea758c5cb68

k8s-ci-robot · 2023-06-22T10:58:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atiratree, soltysh, 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/cloud-controller-manager/OWNERS~~ [thockin]
~~cmd/kube-controller-manager/OWNERS~~ [soltysh,thockin]
~~staging/src/k8s.io/cloud-provider/OWNERS~~ [thockin]
~~staging/src/k8s.io/controller-manager/OWNERS~~ [thockin]
~~vendor/OWNERS~~ [soltysh,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-09-01T19:38:43Z

I have opened a followup to this PR #120371 which tries to encapsulate the controllers as suggested by @thockin in #115813 (comment)

k8s-ci-robot requested review from andrewsykim and cheftako February 15, 2023 22:45

atiratree commented Feb 15, 2023

View reviewed changes

atiratree changed the title ~~Controller aliases~~ Introduce controller aliases for KCM and CCM Feb 15, 2023

atiratree mentioned this pull request Feb 15, 2023

contextual logging kubernetes/enhancements#3077

Open

12 tasks

atiratree force-pushed the controller-aliases branch from e53f1ca to 0182918 Compare February 16, 2023 00:13

atiratree commented Feb 16, 2023

View reviewed changes

liggitt reviewed Feb 16, 2023

View reviewed changes

atiratree force-pushed the controller-aliases branch 3 times, most recently from 83b9a01 to 033edc2 Compare February 16, 2023 22:22

atiratree mentioned this pull request Feb 16, 2023

Migrate pkg/controller/bootstrap to contextual logging #113464

Merged

k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Feb 23, 2023

atiratree force-pushed the controller-aliases branch from d0d4758 to 7f6083f Compare June 13, 2023 21:25

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2023

k8s-ci-robot requested a review from thockin June 13, 2023 21:25

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

atiratree added 9 commits June 19, 2023 14:59

support controller name aliases in GenericControllerManagerConfigurat…

ba17551

…ionOptions

introduce KCM controller aliases and unify controller names

9730951

simplify TestValidateControllersOptions test

94792d8

introduce CCM controller aliases and unify controller names

9fd8f56

reference controllers correctly from CLI help

cd56748

use controller names for CLI FlagSets to show the names in help

e5f5a9b

deduplicate cloud provider controller names

4e6a3fb

and move the to cloud-provider module

update vendor

3b55f20

add KCM names into CCM cmd allowed import-restrictions

85d9339

atiratree force-pushed the controller-aliases branch from 7f6083f to 85d9339 Compare June 19, 2023 13:05

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

soltysh approved these changes Jun 22, 2023

View reviewed changes

k8s-ci-robot assigned soltysh Jun 22, 2023

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

k8s-ci-robot merged commit 4f6a750 into kubernetes:master Jun 22, 2023
13 checks passed

k8s-ci-robot added this to the v1.28 milestone Jun 22, 2023

This was referenced Jun 25, 2023

Fixing the binary names for windows plugins kubernetes/cloud-provider-gcp#571

Merged

cloud-provider-gcp and kubernetes 1.28 out of sync kubernetes/cloud-provider-gcp#574

Closed

wyike mentioned this pull request Aug 23, 2023

Bump kubernetes 1.28 kubernetes/cloud-provider-vsphere#751

Merged

wwentland mentioned this pull request Aug 23, 2023

[occm] Use standard service account name in OCCM helm chart kubernetes/cloud-provider-openstack#2332

Merged

atiratree mentioned this pull request Sep 1, 2023

Encapsulate KCM controllers with their metadata #120371

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce controller aliases for KCM and CCM #115813

Introduce controller aliases for KCM and CCM #115813

atiratree commented Feb 15, 2023 •

edited

atiratree Feb 16, 2023

atiratree Feb 16, 2023

liggitt Feb 16, 2023

atiratree Feb 16, 2023

liggitt Feb 16, 2023

atiratree Feb 16, 2023

liggitt Feb 16, 2023

atiratree Feb 16, 2023

jprzychodzen Mar 6, 2023

nckturner Apr 17, 2023

atiratree Apr 18, 2023 •

edited

nckturner Apr 20, 2023

atiratree Apr 24, 2023

thockin Apr 27, 2023

cici37 Apr 27, 2023

atiratree May 9, 2023

liggitt Feb 16, 2023

atiratree Feb 16, 2023

liggitt Feb 16, 2023

atiratree Feb 16, 2023

atiratree commented Feb 17, 2023

sftim commented Feb 17, 2023

atiratree commented Feb 17, 2023

sftim commented Feb 17, 2023

cici37 commented Feb 21, 2023

cici37 commented Feb 23, 2023

atiratree commented Jun 13, 2023

atiratree commented Jun 19, 2023

soltysh left a comment

k8s-ci-robot commented Jun 22, 2023

k8s-ci-robot commented Jun 22, 2023

atiratree commented Sep 1, 2023


		import cpnames "k8s.io/cloud-provider/app/names"

		// canonical controller names

	nodeipamcontrolleroptions "k8s.io/kubernetes/cmd/kube-controller-manager/app/options"
	nodeipamcontroller "k8s.io/kubernetes/pkg/controller/nodeipam"
	nodeipamconfig "k8s.io/kubernetes/pkg/controller/nodeipam/config"

Introduce controller aliases for KCM and CCM #115813

Introduce controller aliases for KCM and CCM #115813

Conversation

atiratree commented Feb 15, 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.:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree Apr 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree commented Feb 17, 2023

sftim commented Feb 17, 2023

atiratree commented Feb 17, 2023

sftim commented Feb 17, 2023

cici37 commented Feb 21, 2023

cici37 commented Feb 23, 2023

atiratree commented Jun 13, 2023

atiratree commented Jun 19, 2023

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 22, 2023

k8s-ci-robot commented Jun 22, 2023

atiratree commented Sep 1, 2023

atiratree commented Feb 15, 2023 •

edited

atiratree Apr 18, 2023 •

edited