cloud node controller: implement with workqueues and node lister #94736

nicolehanjing · 2020-09-11T23:32:45Z

As title

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Refactor the node controller to use Workqueue and Node Lister so that Informer watches for changes on the current state of Kubernetes objects and sends events to Workqueue where events are then popped up by worker(s) to process.
If we can use Workqueue in the implementation, it will be more readable and gain better maintenance.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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

k8s-ci-robot · 2020-09-11T23:32:52Z

Welcome @HaibaraAi96!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-09-11T23:32:53Z

Hi @HaibaraAi96. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

nicolehanjing · 2020-09-11T23:33:05Z

/assign @andrewsykim

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

andrewsykim · 2020-09-12T00:56:38Z

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

+		// Finally, if no error occurs we Forget this item so it does not
+		// get queued again until another change happens.
+		cnc.workqueue.Forget(obj)
+		klog.Infof("Successfully synced '%s'", key)


This is probably too verbose for Infof, maybe increase verbosity here:

klog.V(4).Infof("Successfully synced '%s'", key)

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

andrewsykim · 2020-09-12T01:07:04Z

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

 	// Start a loop to periodically update the node addresses obtained from the cloud
-	wait.Until(func() { cnc.UpdateNodeStatus(context.TODO()) }, cnc.nodeStatusUpdateFrequency, stopCh)
+	go wait.Until(cnc.runWorker, cnc.nodeStatusUpdateFrequency, stopCh)


I think we still want the periodic run of UpdateNodeStatus based on cnc.nodeStatusUpdateFrequency and syncHandler should actually only call UpdateCloudNode and not UpdateNodeStatus. So this should look like this instead:

go wait.Until(func() { cnc.UpdateNodeStatus(context.TODO()) }, cnc.nodeStatusUpdateFrequency, stopCh) go wait.Until(c.runWorker, time.Second, stopCh)

andrewsykim · 2020-09-12T01:11:56Z

Thanks for the PR @HaibaraAi96!

I think overall we need this PR to update cloud node controller to use workqueues so that failed syncs aren't dropped. I left some initial feedback.

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

nicolehanjing · 2020-09-13T21:09:39Z

/retest

k8s-ci-robot · 2020-09-13T21:09:52Z

@HaibaraAi96: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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/test-infra repository.

andrewsykim · 2020-09-14T01:47:19Z

/ok-to-test

nicolehanjing · 2020-09-14T17:34:11Z

/retest

andrewsykim · 2020-09-14T17:41:40Z

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

+		return nil
+	}
+
+	// Get the Node resource with this namespace/name


"Get the Node resource with this name" since nodes do not have namespaces.

andrewsykim · 2020-09-14T17:42:28Z

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

+		// The Node resource may no longer exist, in which case we stop
+		// processing.
+		if apierrors.IsNotFound(err) {
+			utilruntime.HandleError(fmt.Errorf("Node '%s' in work queue no longer exists", key))


I don't think this line is necessary since nodes not existing is not an error case here.

sure, will remove that

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

andrewsykim · 2020-09-14T18:02:18Z

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

@@ -295,7 +432,8 @@ func (cnc *CloudNodeController) updateNodeAddress(ctx context.Context, node *v1.
 // in a retry-if-conflict loop.
 type nodeModifier func(*v1.Node)

-func (cnc *CloudNodeController) UpdateCloudNode(ctx context.Context, _, newObj interface{}) {
+// UpdateCloudNode handles updating existing nodes registered with the cloud taint.
+func (cnc *CloudNodeController) UpdateCloudNode(ctx context.Context, newObj interface{}) {


maybe just name this syncNode now?

Also thinking that syncNode should receive the name of the node instead of newObj interface{} and in initializeNode we fetch the latest node from the informer cache line 478, instead of from apiserver.

staging/src/k8s.io/cloud-provider/controllers/node/node_controller_test.go

nicolehanjing · 2020-09-16T03:49:00Z

/retest

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

andrewsykim · 2020-09-15T23:52:42Z

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

@@ -347,60 +433,51 @@ func (cnc *CloudNodeController) initializeNode(ctx context.Context, node *v1.Nod
 	})
 	if err != nil {
 		utilruntime.HandleError(err)


this can be removed as well

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

andrewsykim · 2020-09-16T21:18:18Z

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

-		if err != nil {
-			return err
+		var curNode *v1.Node
+


nit: remove line here

nicolehanjing · 2020-09-16T22:26:05Z

/test pull-kubernetes-e2e-gce-ubuntu-containerd

andrewsykim · 2020-09-16T23:17:23Z

/retest

andrewsykim

/approve
/lgtm

Thanks @HaibaraAi96!

andrewsykim · 2020-09-16T23:25:13Z

/retest

k8s-ci-robot · 2020-09-16T23:27:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, HaibaraAi96

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:

~~staging/src/k8s.io/cloud-provider/OWNERS~~ [andrewsykim]

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

andrewsykim · 2020-09-17T00:01:38Z

/milestone v1.20

andrewsykim · 2020-09-17T01:18:51Z

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

-		if err != nil {
-			return err
+		var curNode *v1.Node
+		if cnc.cloud.ProviderName() == "gce" {


FYI @cheftako we should probably discuss getting rid of this custom check for GCE, I can't recall the initial reason behind the GCE specific check. Would probably be nice to have something generic here.

+1. Most of these checks are because the test needs something which wasn't generally available. Not sure about this particular test but I've written a few of these. Its frequently something like the webhook tests. You need to create a special test container and deploy it to your registry. The intention was always that other cloud providers would follow adding the container to their registry.

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 11, 2020

k8s-ci-robot assigned andrewsykim Sep 11, 2020

k8s-ci-robot added the area/cloudprovider label Sep 11, 2020

k8s-ci-robot requested review from andrewsykim and davidopp September 11, 2020 23:35

k8s-ci-robot added sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 11, 2020

andrewsykim reviewed Sep 12, 2020

View reviewed changes

JornShen reviewed Sep 13, 2020

View reviewed changes

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go Show resolved Hide resolved

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go Show resolved Hide resolved

nicolehanjing force-pushed the mynodecontroller branch from 25bcd85 to 3646b54 Compare September 13, 2020 21:01

nicolehanjing commented Sep 13, 2020

View reviewed changes

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 14, 2020

nicolehanjing force-pushed the mynodecontroller branch from 3646b54 to 50bc5ab Compare September 14, 2020 17:34

andrewsykim reviewed Sep 14, 2020

View reviewed changes

nicolehanjing force-pushed the mynodecontroller branch from 50bc5ab to f18c219 Compare September 14, 2020 20:39

nicolehanjing commented Sep 15, 2020

View reviewed changes

staging/src/k8s.io/cloud-provider/controllers/node/node_controller_test.go Show resolved Hide resolved

nicolehanjing force-pushed the mynodecontroller branch from 039ebc3 to 4e6be1e Compare September 16, 2020 03:46

andrewsykim reviewed Sep 16, 2020

View reviewed changes

nicolehanjing force-pushed the mynodecontroller branch from 4e6be1e to 0fc1834 Compare September 16, 2020 17:38

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

nicolehanjing force-pushed the mynodecontroller branch from 0fc1834 to 914ff60 Compare September 16, 2020 17:46

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

nicolehanjing force-pushed the mynodecontroller branch 2 times, most recently from d0abb51 to 91c641b Compare September 16, 2020 20:37

andrewsykim reviewed Sep 16, 2020

View reviewed changes

nicolehanjing force-pushed the mynodecontroller branch 2 times, most recently from 1f587fb to d654864 Compare September 16, 2020 21:30

nicolehanjing changed the title ~~cloud provider: refactor node controller~~ cloud node controller: implement with workqueues and node lister Sep 16, 2020

nicolehanjing force-pushed the mynodecontroller branch from d654864 to 9931871 Compare September 16, 2020 21:37

cloud node controller: implement with workqueues and node lister

4ba09a8

nicolehanjing force-pushed the mynodecontroller branch from 9931871 to 4ba09a8 Compare September 16, 2020 22:42

andrewsykim reviewed Sep 16, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 16, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 16, 2020

k8s-ci-robot added this to the v1.20 milestone Sep 17, 2020

k8s-ci-robot merged commit aaccd5f into kubernetes:master Sep 17, 2020

andrewsykim reviewed Sep 17, 2020

View reviewed changes

nicolehanjing mentioned this pull request Oct 8, 2020

REQUEST: New membership for nicolehanjing kubernetes/org#2250

Closed

6 tasks

JornShen mentioned this pull request Nov 5, 2020

REQUEST: New membership for JornShen kubernetes/org#2313

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cloud node controller: implement with workqueues and node lister #94736

cloud node controller: implement with workqueues and node lister #94736

nicolehanjing commented Sep 11, 2020 •

edited

Loading

k8s-ci-robot commented Sep 11, 2020

k8s-ci-robot commented Sep 11, 2020

nicolehanjing commented Sep 11, 2020

andrewsykim Sep 12, 2020

andrewsykim Sep 12, 2020

andrewsykim commented Sep 12, 2020

nicolehanjing commented Sep 13, 2020

k8s-ci-robot commented Sep 13, 2020

andrewsykim commented Sep 14, 2020

nicolehanjing commented Sep 14, 2020

andrewsykim Sep 14, 2020

andrewsykim Sep 14, 2020

nicolehanjing Sep 14, 2020

andrewsykim Sep 14, 2020

andrewsykim Sep 14, 2020

nicolehanjing commented Sep 16, 2020

andrewsykim Sep 15, 2020

andrewsykim Sep 16, 2020

nicolehanjing commented Sep 16, 2020

andrewsykim commented Sep 16, 2020

andrewsykim left a comment

andrewsykim commented Sep 16, 2020

k8s-ci-robot commented Sep 16, 2020

andrewsykim commented Sep 17, 2020

andrewsykim Sep 17, 2020

cheftako Sep 17, 2020

cloud node controller: implement with workqueues and node lister #94736

cloud node controller: implement with workqueues and node lister #94736

Conversation

nicolehanjing commented Sep 11, 2020 • edited Loading

k8s-ci-robot commented Sep 11, 2020

k8s-ci-robot commented Sep 11, 2020

nicolehanjing commented Sep 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim commented Sep 12, 2020

nicolehanjing commented Sep 13, 2020

k8s-ci-robot commented Sep 13, 2020

andrewsykim commented Sep 14, 2020

nicolehanjing commented Sep 14, 2020

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

nicolehanjing commented Sep 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolehanjing commented Sep 16, 2020

andrewsykim commented Sep 16, 2020

andrewsykim left a comment

Choose a reason for hiding this comment

andrewsykim commented Sep 16, 2020

k8s-ci-robot commented Sep 16, 2020

andrewsykim commented Sep 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolehanjing commented Sep 11, 2020 •

edited

Loading