Node status updater now deletes the node entry in attach updates... #45923

verult · 2017-05-17T00:18:04Z

… when node is missing in NodeInformer cache.

Added RemoveNodeFromAttachUpdates as part of node status updater operations.

What this PR does / why we need it: Fixes issue of unnecessary node status updates when node is deleted.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #42438

Special notes for your reviewer: Unit tested added, but a more comprehensive test involving the attach detach controller requires certain testing functionality that is currently absent, and will require larger effort. Will be added at a later time.

There is an edge case caused by the following steps:

A node is deleted and restarted. The node exists, but is not yet recognized by Kubernetes.
A pod requiring a volume attach with nodeName specifically set to this node.

This would make the pod stuck in ContainerCreating state. This is low-pri since it's a specific edge case that can be avoided.

Release note:

Fix log spam due to unnecessary status update when node is deleted.

k8s-ci-robot · 2017-05-17T00:18:12Z

Hi @verult. 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 @k8s-bot 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.

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. I understand the commands that are listed here.

verult · 2017-05-17T00:19:17Z

/assign @saad-ali @jingxu97

jingxu97 · 2017-05-17T00:58:16Z

pkg/controller/volume/attachdetach/statusupdater/node_status_updater.go

 				nodeName,
 				err)
-			nsu.actualStateOfWorld.SetNodeStatusUpdateNeeded(nodeName)
+			nsu.actualStateOfWorld.RemoveNodeFromAttachUpdates(nodeName)


I think you need to take different actions for nodeObj==nil and err != nil
When nodeObj==nil, it shows that API server does not have this object anymore, it should be safe to removeNode. But for err != nil, that indicates something wrong then retrieving the node object and node status updater should try it again.

nodeLister.Get() returns nil object only when there's an error, and returns an error only when the object is nil. However I can add in as a safeguard against future changes.

The error is different if node does not exist errors.NewNotFound(v1.Resource("node"), name)
you can check the error type to determine whether it is because of node exist or not.

verult · 2017-05-19T17:23:23Z

@k8s-bot ok to test

k8s-ci-robot · 2017-05-19T17:23:37Z

@verult: you can't request testing unless you are a kubernetes member.

In response to this:

@k8s-bot ok to test

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. I understand the commands that are listed here.

msau42 · 2017-05-19T17:29:47Z

@k8s-bot ok to test

msau42 · 2017-05-19T17:30:28Z

/assign @saad-ali @jingxu97

jingxu97 · 2017-05-19T17:38:34Z

@k8s-bot ok to test

jingxu97 · 2017-05-19T17:45:23Z

/approve

saad-ali

A couple of comments.

saad-ali · 2017-05-24T18:32:49Z

pkg/controller/volume/attachdetach/statusupdater/node_status_updater.go

@@ -64,14 +66,19 @@ func (nsu *nodeStatusUpdater) UpdateNodeStatuses() error {
 	nodesToUpdate := nsu.actualStateOfWorld.GetVolumesToReportAttached()
 	for nodeName, attachedVolumes := range nodesToUpdate {
 		nodeObj, err := nsu.nodeLister.Get(string(nodeName))
-		if nodeObj == nil || err != nil {
+		statusErr, isStatusError := err.(*errors.StatusError)
+		if isStatusError && statusErr.Status().Reason == metav1.StatusReasonNotFound {
 			// If node does not exist, its status cannot be updated, log error and
 			// reset flag statusUpdateNeeded back to true to indicate this node status
 			// needs to be updated again


nit: Update this comment.

saad-ali · 2017-05-24T18:36:42Z

pkg/volume/util/operationexecutor/operation_executor.go

+
+	// Removes the given node from the record of attach updates. The node's entire
+	// volumesToReportAsAttached list is removed.
+	RemoveNodeFromAttachUpdates(nodeName types.NodeName) error


Since this operation does not apply to both attachdetach and volumemanager actual_state_of_the_world, just add it to the ASW interface in controller/volume/attachdetach/cache/actual_state_of_world.go instead. Then you don't need to put a noop version of the method in volumemanager/cache/actual_state_of_world.go

saad-ali · 2017-05-24T18:37:56Z

Make sure to keep the 1.5 version in sync with any changes in this PR as well

jingxu97 · 2017-05-24T19:46:27Z

pkg/controller/volume/attachdetach/statusupdater/node_status_updater.go

@@ -64,14 +66,19 @@ func (nsu *nodeStatusUpdater) UpdateNodeStatuses() error {
 	nodesToUpdate := nsu.actualStateOfWorld.GetVolumesToReportAttached()
 	for nodeName, attachedVolumes := range nodesToUpdate {
 		nodeObj, err := nsu.nodeLister.Get(string(nodeName))
-		if nodeObj == nil || err != nil {
+		statusErr, isStatusError := err.(*errors.StatusError)
+		if isStatusError && statusErr.Status().Reason == metav1.StatusReasonNotFound {


I think you can use error.IsNotFound() to check this

saad-ali · 2017-05-24T21:15:57Z

/lgtm
/approve

saad-ali · 2017-05-24T21:16:44Z

Make sure to fix the release note in your first comment on this page or change the release note label to no release note.

verult · 2017-05-24T22:48:09Z

/release-note-none

verult · 2017-05-25T00:03:59Z

@k8s-bot pull-kubernetes-kubemark-e2e-gce test this

… node is missing in NodeInformer cache. Fixes kubernetes#42438. - Added RemoveNodeFromAttachUpdates as part of node status updater operations.

k8s-ci-robot · 2017-05-25T02:00:02Z

@verult: The following test(s) failed:

Test name	Commit	Details	Rerun command
pull-kubernetes-federation-e2e-gce	`f9dc2d5`	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

jingxu97 · 2017-05-26T18:10:32Z

/lgtm

k8s-github-robot · 2017-05-26T18:10:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jingxu97, saad-ali, verult

Needs approval from an approver in each of these OWNERS Files:

~~pkg/controller/volume/attachdetach/OWNERS~~ [saad-ali]
~~pkg/volume/util/operationexecutor/OWNERS~~ [saad-ali]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-05-26T19:58:01Z

Automatic merge from submit-queue (batch tested with PRs 46383, 45645, 45923, 44884, 46294)

…-upstream-release-1.6 Automatic merge from submit-queue Automated cherry pick of #45923 Cherry pick of #45923 on release-1.6. #45923: Node status updater now deletes the node entry in attach

k8s-cherrypick-bot · 2017-06-12T11:43:05Z

Commit found in the "release-1.6" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

Automatic merge from submit-queue Node status updater now deletes the node entry in attach updates when node is missing in NodeInformer cache. - Added RemoveNodeFromAttachUpdates as part of node status updater operations. **What this PR does / why we need it**: Fixes issue of unnecessary node status updates when node is deleted. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #42438 **Special notes for your reviewer**: v1.5 version of the fix addressed by PR #45923. This is necessary because NodeLister did not exist prior to 1.6, thus node status updater requires a slightly different node existence check. **Release note**: ```release-note NONE ```

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 17, 2017

k8s-github-robot assigned smarterclayton and gnufied May 17, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 17, 2017

jingxu97 reviewed May 17, 2017

View reviewed changes

verult force-pushed the cxing/NodeStatusUpdaterFix branch from 8bc9fee to f41c953 Compare May 17, 2017 02:42

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 19, 2017

k8s-ci-robot assigned jingxu97 and saad-ali May 19, 2017

verult mentioned this pull request May 23, 2017

Node status updater now deletes the node entry in attach updates when node is missing in NodeInformer cache. #46301

Merged

verult force-pushed the cxing/NodeStatusUpdaterFix branch 2 times, most recently from e5f5944 to 5c28946 Compare May 24, 2017 17:25

saad-ali suggested changes May 24, 2017

View reviewed changes

jingxu97 reviewed May 24, 2017

View reviewed changes

verult force-pushed the cxing/NodeStatusUpdaterFix branch from 5c28946 to 455dba2 Compare May 24, 2017 21:08

saad-ali approved these changes May 24, 2017

View reviewed changes

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

saad-ali added this to the v1.7 milestone May 24, 2017

verult force-pushed the cxing/NodeStatusUpdaterFix branch from 455dba2 to 0a0d758 Compare May 24, 2017 22:32

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2017

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 24, 2017

Node status updater now deletes the node entry in attach updates when…

f9dc2d5

… node is missing in NodeInformer cache. Fixes kubernetes#42438. - Added RemoveNodeFromAttachUpdates as part of node status updater operations.

verult force-pushed the cxing/NodeStatusUpdaterFix branch from 0a0d758 to f9dc2d5 Compare May 25, 2017 01:32

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

k8s-github-robot merged commit c34b359 into kubernetes:master May 26, 2017

saad-ali added the cherrypick-candidate label May 26, 2017

saad-ali modified the milestones: v1.6, v1.7 May 26, 2017

verult mentioned this pull request May 26, 2017

Automated cherry pick of #45923 #46541

Merged

enisoc added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 30, 2017

saad-ali mentioned this pull request Jun 7, 2017

node not exist failure during node status update flush controller's log #30898

Closed

This was referenced Jun 9, 2017

Node deleted but still streaming DeletingNode events kubernetes/autoscaler#109

Closed

ElasticSearch statefulset broken after 1 hour kz8s/tack#192

Closed

k8s-cherrypick-bot removed the cherrypick-candidate label Jun 12, 2017

verult mentioned this pull request Aug 16, 2017

Pod stuck in ContainerCreating with log message "Volume is not yet attached according to node" #50721

Closed

verult deleted the cxing/NodeStatusUpdaterFix branch March 24, 2018 01:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node status updater now deletes the node entry in attach updates... #45923

Node status updater now deletes the node entry in attach updates... #45923

verult commented May 17, 2017 •

edited by enisoc

Loading

k8s-ci-robot commented May 17, 2017

verult commented May 17, 2017 •

edited

Loading

jingxu97 May 17, 2017

verult May 17, 2017

jingxu97 May 19, 2017

verult commented May 19, 2017

k8s-ci-robot commented May 19, 2017

msau42 commented May 19, 2017

msau42 commented May 19, 2017

jingxu97 commented May 19, 2017

jingxu97 commented May 19, 2017

saad-ali left a comment

saad-ali May 24, 2017

saad-ali May 24, 2017

saad-ali commented May 24, 2017

jingxu97 May 24, 2017

saad-ali commented May 24, 2017

saad-ali commented May 24, 2017

verult commented May 24, 2017

verult commented May 25, 2017

k8s-ci-robot commented May 25, 2017 •

edited

Loading

jingxu97 commented May 26, 2017

k8s-github-robot commented May 26, 2017

k8s-github-robot commented May 26, 2017

k8s-cherrypick-bot commented Jun 12, 2017

Node status updater now deletes the node entry in attach updates... #45923

Node status updater now deletes the node entry in attach updates... #45923

Conversation

verult commented May 17, 2017 • edited by enisoc Loading

k8s-ci-robot commented May 17, 2017

verult commented May 17, 2017 • edited Loading

jingxu97 May 17, 2017

Choose a reason for hiding this comment

verult May 17, 2017

Choose a reason for hiding this comment

jingxu97 May 19, 2017

Choose a reason for hiding this comment

verult commented May 19, 2017

k8s-ci-robot commented May 19, 2017

msau42 commented May 19, 2017

msau42 commented May 19, 2017

jingxu97 commented May 19, 2017

jingxu97 commented May 19, 2017

saad-ali left a comment

Choose a reason for hiding this comment

saad-ali May 24, 2017

Choose a reason for hiding this comment

saad-ali May 24, 2017

Choose a reason for hiding this comment

saad-ali commented May 24, 2017

jingxu97 May 24, 2017

Choose a reason for hiding this comment

saad-ali commented May 24, 2017

saad-ali commented May 24, 2017

verult commented May 24, 2017

verult commented May 25, 2017

k8s-ci-robot commented May 25, 2017 • edited Loading

jingxu97 commented May 26, 2017

k8s-github-robot commented May 26, 2017

k8s-github-robot commented May 26, 2017

k8s-cherrypick-bot commented Jun 12, 2017

verult commented May 17, 2017 •

edited by enisoc

Loading

verult commented May 17, 2017 •

edited

Loading

k8s-ci-robot commented May 25, 2017 •

edited

Loading