node topology worker should run #617

bai3shuo4 · 2021-04-25T10:40:08Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind bug

What this PR does / why we need it:
bugfix for commit before, if we directly remove topology informer run, the topology worker will not run and do not sync. fix this PR #590
Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fix capacity information updates when topology changes. Only affected central deployment and network attached storage, not deployment on each node. This broke in v2.2.0 as part of a bug fix for capacity informer handling.

k8s-ci-robot · 2021-04-25T10:40:15Z

Hi @bai3shuo4. Thanks for your PR.

I'm waiting for a kubernetes-csi 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.

Details

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.

bai3shuo4 · 2021-04-25T10:41:19Z

@pohly sorry, this PR should be fix further. Do you remember this PR: #590. It will cause problem when topology change

bai3shuo4 · 2021-04-28T02:31:00Z

/assign pohly

pohly

Please squash your commits and reference the commit where this broke.

pohly · 2021-04-28T06:39:31Z

pkg/capacity/capacity.go

 	klog.Info("Starting Capacity Controller")
 	defer c.queue.ShutDown()

+	go c.topologyInformer.Run(ctx)


This looks like the wrong place to put this. To be consistent with how the other informers are handled, this should be in the location where the informer gets created (i.e. in csi-provisioner.go), not where it is used.

This is the place it was placed: ff439ee

According to this commit, if this line was removed directly, the node topology worker will not run. If you change the node label or csinode, nothing will happen. It will cause problem

I agree that removal of go c.topologyInformer.Run(ctx) in that commit introduced a bug. The question just is where that Run() call should go instead. I propose cmd/csi-provisioner/csi-provisioner.go.

In fact, c.topologyInformer.Run(ctx) is not an informer. It only starts a topology queue worker. the node informer and csinode informer are started by factory.Start() in cmd/csi-provisioner/csi-provisioner.go. Moreover, only capacity feature uses this worker and it should belong to capacity controller right?

Maybe I can change this method name to avoid ambiguity like changing Run to RunWorker

Yes, c.topologyInformer it is not a SharedInformer instance from an informer factory. But conceptually it is the same.

We can rename Run -> RunWorker to make that more obvious.

@pohly I have revised to this way. Can you review again? Thanks!

pohly · 2021-04-28T06:45:22Z

We have unit tests for "topology changes", but that doesn't help here because this is an issue about combining the various pieces into the final binary. An E2E test would be needed which:

deploys a CSI driver on multiple nodes, with capacity tracking
changes the node topology while the test runs

Right now we don't have a driver for testing this.

@bai3shuo4: You are using this with network attached storage, i.e. you have a central external-provisioner instance, right? Distributed provisioning uses a fixed topology, so it shouldn't matter there whether the informers run.

bai3shuo4 · 2021-04-28T06:47:22Z

We have unit tests for "topology changes", but that doesn't help here because this is an issue about combining the various pieces into the final binary. An E2E test would be needed which:

deploys a CSI driver on multiple nodes, with capacity tracking

changes the node topology while the test runs

Right now we don't have a driver for testing this.

@bai3shuo4: You are using this with network attached storage, i.e. you have a central external-provisioner instance, right? Distributed provisioning uses a fixed topology, so it shouldn't matter there whether the informers run.

Yes, I use central one and network storage

pohly · 2021-04-28T07:01:50Z

Yes, I use central one and network storage

How important is storage capacity tracking in such a setup? Are there some limitations which nodes have access to which storage?

bai3shuo4 · 2021-04-28T07:05:52Z

Yes, I use central one and network storage

How important is storage capacity tracking in such a setup? Are there some limitations which nodes have access to which storage?

We have different storage cluster with different capacity for node which is divided by topology. Your CSIStorageCapacities really help a lot

bai3shuo4 · 2021-04-28T07:09:40Z

Yes, I use central one and network storage

How important is storage capacity tracking in such a setup? Are there some limitations which nodes have access to which storage?

One more thing, I think CSIStorageCapacity should refresh when we expand volume or something. Now I'm working with csi-resizer to help this happening

pohly · 2021-04-28T07:19:54Z

One more thing, I think CSIStorageCapacity should refresh when we expand volume or something. Now I'm working with csi-resizer to help this happening

The periodic refresh will cover that, but I agree, something that is actually responding to changes would be better. If you have some idea then that would be great.

The modular design of the CSI sidecars is working against us here. If all functionality was in a single "controller sidecar", it would be easier.

pohly · 2021-04-28T11:34:05Z

pkg/capacity/topology/mock.go


+func (mt *Mock) RunWorker(ctx context.Context) {
+}
+


This replaces the Run method, doesn't it?

Yes, you mean I can remove topology informer Run method directly?

Yes. I thought that was the proposal: clarify that this is not a SharedInformer by naming the "Run" method not "Run". Nothing else calls it, so we are free to choose an arbitrary name.

If that makes the patch very large, then perhaps we should simply stick with Run.

pohly · 2021-04-28T11:34:43Z

pkg/capacity/topology/nodes.go

+	klog.Infof("Started node topology worker")
+	<-ctx.Done()
+	klog.Infof("Shutting node topology worker")
+}


Now that we have simplified this function like this, one other enhancement becomes possible: we no longer need the separate runWorker function and can save one goroutine:

func (nt *nodeTopology) RunWorker(ctx context.Context) { klog.Info("Started node topology informer") for nt.processNextWorkItem(ctx) { } klog.Info("Stopped node topology worker") }

pohly · 2021-04-28T11:35:04Z

pkg/capacity/topology/topology.go

 	HasSynced() bool
+
+	// RunWorker starts a worker to process queue.
+	RunWorker(ctx context.Context)


pohly · 2021-04-28T11:35:34Z

pkg/capacity/capacity.go

 	klog.Info("Starting Capacity Controller")
 	defer c.queue.ShutDown()

+	go c.topologyInformer.RunWorker(ctx)


I still think this belongs into csi-provisioner.go. We should add a comment to NewCentralCapacityController that "all informers are expected to be started by the caller".

Node topology worker only works for Capacity controller. If something goes wrong before CapacityController Run, I think we don't even need to start this worker. Therefore, why not put it into controller run function :)

Because it is consistent conceptually ("all informers are expected to be started by the caller"- we can even add "started and stopped").

If something goes wrong in csi-provisioner.go, the binary will exit. It'll make no difference how many goroutines get killed by that and the overhead is also irrelevant.

pohly

Getting close... now we need to describe the change better in the release note.

Can you change that into:

Fix capacity information updates when topology changes. Only affected central deployment and network attached storage, not deployment on each node. This broke in v2.2.0 as part of a bug fix for capacity informer handling.

pohly · 2021-04-29T15:54:20Z

pkg/capacity/topology/nodes.go

 	go nt.runWorker(ctx)

-	klog.Info("Started node topology informer")
+	klog.Infof("Started node topology worker")


Please stick with Info, the string is not a format string.

pohly · 2021-04-29T15:54:34Z

pkg/capacity/topology/nodes.go

+	klog.Infof("Started node topology worker")
 	<-ctx.Done()
-	klog.Info("Shutting node topology informer")
+	klog.Infof("Shutting node topology worker")


pohly · 2021-04-29T15:56:48Z

cmd/csi-provisioner/csi-provisioner.go

 			klog.Infof("producing CSIStorageCapacity objects with fixed topology segment %s", segment)
 			topologyInformer = topology.NewFixedNodeTopology(&segment)
 		}
+		go topologyInformer.RunWorker(context.TODO())


Please use context.Background() here. context.TODO() implies that further changes are needed, which is not the case here.

pohly · 2021-04-30T12:40:38Z

/kind bug

pohly · 2021-04-30T12:40:49Z

/ok-to-test

…h RunWorker

pohly

Sorry, one more idea...

pohly · 2021-05-03T09:16:11Z

pkg/capacity/topology/nodes.go

+	klog.Infof("Started node topology worker")
+	<-ctx.Done()
+	klog.Infof("Shutting node topology worker")
+}


Now that we have simplified this function like this, one other enhancement becomes possible: we no longer need the separate runWorker function and can save one goroutine:

func (nt *nodeTopology) RunWorker(ctx context.Context) { klog.Info("Started node topology informer") for nt.processNextWorkItem(ctx) { } klog.Info("Stopped node topology worker") }

pohly · 2021-05-05T17:59:15Z

Sorry, one more idea...

That can be added later. Let me merge it as-is.

/lgtm
/approve

pohly · 2021-05-05T18:04:47Z

/approve

msau42 · 2021-05-05T18:35:46Z

/approve

k8s-ci-robot · 2021-05-05T18:35:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bai3shuo4, msau42, pohly

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [msau42]

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

bai3shuo4 · 2021-05-07T02:18:49Z

Sorry, one more idea...

@pohly Sorry, I just check my github at this moment....yeap, I think we can simplify this func, maybe I can add this commit in further bugfix

k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 25, 2021

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 Apr 25, 2021

k8s-ci-robot requested review from davidz627 and lpabon April 25, 2021 10:40

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 25, 2021

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 26, 2021

k8s-ci-robot assigned pohly Apr 28, 2021

pohly requested changes Apr 28, 2021

View reviewed changes

pohly reviewed Apr 28, 2021

View reviewed changes

bai3shuo4 force-pushed the bai3shuo4/fix-topology-worker branch from f9b41df to aaa456d Compare April 28, 2021 06:40

bai3shuo4 force-pushed the bai3shuo4/fix-topology-worker branch from aaa456d to 3a29b8c Compare April 28, 2021 08:37

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 28, 2021

bai3shuo4 requested a review from pohly April 28, 2021 10:08

pohly requested changes Apr 28, 2021

View reviewed changes

bai3shuo4 force-pushed the bai3shuo4/fix-topology-worker branch from 3a29b8c to d175b80 Compare April 29, 2021 12:11

bai3shuo4 requested a review from pohly April 29, 2021 12:11

pohly requested changes Apr 29, 2021

View reviewed changes

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 30, 2021

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 Apr 30, 2021

node topology worker should run and replace topology informer Run wit…

38ce9c3

…h RunWorker

bai3shuo4 force-pushed the bai3shuo4/fix-topology-worker branch from d175b80 to 38ce9c3 Compare May 1, 2021 03:36

pohly requested changes May 3, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 2021

pohly mentioned this pull request May 5, 2021

optimization for node.go #625

Closed

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2021

k8s-ci-robot merged commit f71827b into kubernetes-csi:master May 5, 2021

pohly mentioned this pull request May 7, 2021

capacity: simplify topology informer #629

Merged

msau42 mentioned this pull request Aug 11, 2021

Add changelog for v3.0.0 #665

Merged

wongma7 mentioned this pull request Sep 15, 2021

Startup Dependency No CSINode Data kubernetes-sigs/aws-ebs-csi-driver#1030

Closed

node topology worker should run #617

node topology worker should run #617

Uh oh!

Conversation

bai3shuo4 commented Apr 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Apr 25, 2021

Uh oh!

bai3shuo4 commented Apr 25, 2021

Uh oh!

bai3shuo4 commented Apr 28, 2021

Uh oh!

pohly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bai3shuo4 Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bai3shuo4 Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pohly commented Apr 28, 2021

Uh oh!

bai3shuo4 commented Apr 28, 2021

Uh oh!

pohly commented Apr 28, 2021

Uh oh!

bai3shuo4 commented Apr 28, 2021

Uh oh!

bai3shuo4 commented Apr 28, 2021

Uh oh!

pohly commented Apr 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pohly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pohly commented Apr 30, 2021

Uh oh!

pohly commented Apr 30, 2021

Uh oh!

pohly left a comment

bai3shuo4 commented Apr 25, 2021 •

edited

Loading

bai3shuo4 Apr 28, 2021 •

edited

Loading

bai3shuo4 Apr 28, 2021 •

edited

Loading