SetNodeUpdateStatusNeeded whenever nodeAdd event is received #37727

rkouj · 2016-11-30T23:55:09Z

What this PR does / why we need it:
Bug fix and SetNodeStatusUpdateNeeded for a node whenever its api object is added. This is to ensure that we don't lose the attached list of volumes in the node when its api object is deleted and recreated.

fixes #37586
#37585

Special notes for your reviewer:

k8s-oncall · 2016-11-30T23:55:16Z

This change is

saad-ali

Couple small comments

saad-ali · 2016-12-01T02:03:35Z

pkg/controller/volume/attachdetach/cache/actual_state_of_world.go

@@ -457,6 +460,7 @@ func (asw *actualStateOfWorld) updateNodeStatusUpdateNeeded(nodeName types.NodeN
 			"Failed to set statusUpdateNeeded to needed %t because nodeName=%q  does not exist",
 			needed,
 			nodeName)
+		return


nit: Errors should not be silently swallowed, consumers should decide if they want to swallow the error. So I'd suggest modifying this chain to bubble the error up the caller and let the caller decide to ignore it.

I agree and I wanted to change it back to return fmt.Errorf() as it previously was. However that would involve changing the return type of the function and everywhere where it is being called (twice in this case) which might also affect some tests.
It might make sense to not touch production code that has been tested before. Also this would involve an additional overhead on the intended fix of this PR.
I can definitely create a separate issue and PR to refactor this code and spend some time with the changes that it would potentially affect.

Ack. We can do this during normal 1.6 development.

Created #39056

saad-ali · 2016-12-01T02:05:59Z

pkg/controller/volume/attachdetach/cache/actual_state_of_world_test.go

@@ -1094,6 +1094,17 @@ func Test_OneVolumeTwoNodes_TwoDevicePaths(t *testing.T) {
 	verifyAttachedVolume(t, attachedVolumes, generatedVolumeName2, string(volumeName), node2Name, devicePath2, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */)
 }

+func Test_SetNodeStatusUpdateNeededError(t *testing.T) {


Write a comment before the test summarizing what it is testing (expected behavior).

Then inside the test I like to break it down in to three sections:

// Arrange // Act // Assert

This makes it easier to follow the test. Basically you setup, do something, then verify it. Up to you if you want to follow this pattern.

rkouj · 2016-12-01T02:18:36Z

After stubbing out code that deletes pods, I was able to verify that the upgrade works and I'm able to see the data that was written before the upgrade.

kubectl describe pods
https://gist.github.com/rkouj/dc642237b390b77f6db891bfb11255f7

k8s-ci-robot · 2016-12-01T04:35:18Z

Jenkins GCI GKE smoke e2e failed for commit b25a426. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

saad-ali · 2016-12-01T04:40:16Z

After stubbing out code that deletes pods, I was able to verify that the upgrade works and I'm able to see the data that was written before the upgrade.

Awesome.

rkouj · 2016-12-01T04:42:03Z

@k8s-bot gci gke e2e test this

saad-ali

One more nit

saad-ali · 2016-12-01T04:44:51Z

pkg/controller/volume/attachdetach/attach_detach_controller.go

+	}
+	nodeName := types.NodeName(node.Name)
+	adc.nodeUpdate(nil, obj)
+	adc.actualStateOfWorld.SetNodeStatusUpdateNeeded(nodeName)


nit: add a comment why we are doing this since it is not obvious

saad-ali · 2016-12-01T05:18:08Z

/lgtm

saad-ali · 2016-12-01T06:01:54Z

Removing LGTM while @rkouj investigates another upgrade failure.

rkouj · 2016-12-01T07:17:00Z

I retested it and it's working fine. It looks like first time around I didn't have the fix in my branch when I was doing the test. I'll retest this again tomorrow morning to be absolutely sure.

rkouj · 2016-12-01T22:50:40Z

Retested. The fix works.

This is when the node object got recreated

E1201 22:26:42.684050 3486 kubelet_node_status.go:131] Previously node "kubernetes-minion-group-k3b4" had externalID "6192076403268984273"; now it is "6725016659983934100"; will delete and recreate.

This is when the nodeStatusUpdateNeeded got called.

I1201 22:26:42.977372 5 attach_detach_controller.go:254] [LOGTOREMOVE] about to call SetNodeStatusUpdateNeeded for nodeName kubernetes-minion-group-k3b4

rkouj · 2016-12-01T22:58:19Z

Controller and kubelet logs

https://gist.github.com/anonymous/a4a6bf1bf1a42d697bbc46abc628f258
https://gist.github.com/anonymous/de50717fbe8380694bf7287440eb0012

rkouj · 2016-12-01T23:01:30Z

Frequency of updates on the nodeAdd

https://gist.github.com/anonymous/bf6ea5f3e465d261f918a7e062966515

rkouj · 2016-12-01T23:01:56Z

@saad-ali PTAL

saad-ali · 2016-12-01T23:06:44Z

Reviewed 1 of 4 files at r1, 2 of 3 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

saad-ali · 2016-12-01T23:08:30Z

Thanks for revalidating!

/lgtm

Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

k8s-github-robot · 2016-12-02T11:31:47Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-ci-robot · 2016-12-02T12:39:54Z

Jenkins kops AWS e2e failed for commit 638ef1b. Full PR test history.

The magic incantation to run this job again is @k8s-bot kops aws e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-02T13:03:47Z

Jenkins GCE e2e failed for commit 638ef1b. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

rkouj · 2016-12-02T13:24:22Z

@k8s-bot cvm gce e2e test this

rkouj · 2016-12-02T13:24:43Z

@k8s-bot kops aws e2e test this

k8s-github-robot · 2016-12-02T13:44:57Z

Automatic merge from submit-queue

…upstream-release-1.5 Automatic merge from submit-queue Automated cherry pick of #37727 Cherry pick of #37727 on release-1.5.

k8s-cherrypick-bot · 2016-12-02T23:47:30Z

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 30, 2016

k8s-github-robot assigned saad-ali Dec 1, 2016

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Dec 1, 2016

rkouj force-pushed the bug-fix-upgrade-test branch from 93fccb9 to 3418cbd Compare December 1, 2016 00:43

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 1, 2016

saad-ali added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Dec 1, 2016

saad-ali added this to the v1.5 milestone Dec 1, 2016

saad-ali added cherrypick-candidate do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Dec 1, 2016

saad-ali reviewed Dec 1, 2016

View reviewed changes

rkouj mentioned this pull request Dec 1, 2016

Pods with PVCs stuck in ContainerCreating after Node upgrade #37585

Closed

rkouj force-pushed the bug-fix-upgrade-test branch from 3418cbd to b25a426 Compare December 1, 2016 03:26

saad-ali reviewed Dec 1, 2016

View reviewed changes

SetNodeUpdateStatusNeeded whenever nodeAdd event is received

638ef1b

rkouj force-pushed the bug-fix-upgrade-test branch from b25a426 to 638ef1b Compare December 1, 2016 05:12

saad-ali removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Dec 1, 2016

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 1, 2016

saad-ali removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 1, 2016

dims mentioned this pull request Dec 1, 2016

GCE PD unable to attach when node is upgraded to 1.5 from 1.4 #37586

Closed

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 1, 2016

k8s-github-robot merged commit c552f89 into kubernetes:master Dec 2, 2016

rkouj mentioned this pull request Dec 2, 2016

Automated cherry pick of #37727 #37973

Merged

saad-ali added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Dec 2, 2016

k8s-github-robot pushed a commit that referenced this pull request Dec 2, 2016

Merge pull request #37973 from rkouj/automated-cherry-pick-of-#37727-…

92991e7

…upstream-release-1.5 Automatic merge from submit-queue Automated cherry pick of #37727 Cherry pick of #37727 on release-1.5.

k8s-cherrypick-bot removed the cherrypick-candidate label Dec 2, 2016

rootfs mentioned this pull request Dec 6, 2016

Attach/detach controller does not recover from missed pod deletion #34242

Closed

rkouj mentioned this pull request Dec 21, 2016

Refactor to address the following discussion #39056

Closed

saad-ali mentioned this pull request Jan 25, 2017

Do not swallow error in asw.updateNodeStatusUpdateNeeded #40126

Merged

carlory mentioned this pull request Apr 11, 2024

AD controller remove this behavior: call SetNodeUpdateStatusNeeded whenever nodeAdd event is received #124265

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SetNodeUpdateStatusNeeded whenever nodeAdd event is received #37727

SetNodeUpdateStatusNeeded whenever nodeAdd event is received #37727

rkouj commented Nov 30, 2016 •

edited

k8s-oncall commented Nov 30, 2016

saad-ali left a comment

saad-ali Dec 1, 2016

rkouj Dec 1, 2016 •

edited

saad-ali Dec 1, 2016

rkouj Dec 21, 2016

saad-ali Dec 1, 2016

rkouj Dec 1, 2016

rkouj commented Dec 1, 2016

k8s-ci-robot commented Dec 1, 2016

saad-ali commented Dec 1, 2016

rkouj commented Dec 1, 2016

saad-ali left a comment

saad-ali Dec 1, 2016

rkouj Dec 1, 2016

saad-ali commented Dec 1, 2016

saad-ali commented Dec 1, 2016

rkouj commented Dec 1, 2016

rkouj commented Dec 1, 2016 •

edited

rkouj commented Dec 1, 2016

rkouj commented Dec 1, 2016

rkouj commented Dec 1, 2016

saad-ali commented Dec 1, 2016

saad-ali commented Dec 1, 2016

k8s-github-robot commented Dec 2, 2016

k8s-ci-robot commented Dec 2, 2016

k8s-ci-robot commented Dec 2, 2016

rkouj commented Dec 2, 2016

rkouj commented Dec 2, 2016

k8s-github-robot commented Dec 2, 2016

k8s-cherrypick-bot commented Dec 2, 2016

SetNodeUpdateStatusNeeded whenever nodeAdd event is received #37727

SetNodeUpdateStatusNeeded whenever nodeAdd event is received #37727

Conversation

rkouj commented Nov 30, 2016 • edited

k8s-oncall commented Nov 30, 2016

saad-ali left a comment

Choose a reason for hiding this comment

saad-ali Dec 1, 2016

Choose a reason for hiding this comment

rkouj Dec 1, 2016 • edited

Choose a reason for hiding this comment

saad-ali Dec 1, 2016

Choose a reason for hiding this comment

rkouj Dec 21, 2016

Choose a reason for hiding this comment

saad-ali Dec 1, 2016

Choose a reason for hiding this comment

rkouj Dec 1, 2016

Choose a reason for hiding this comment

rkouj commented Dec 1, 2016

k8s-ci-robot commented Dec 1, 2016

saad-ali commented Dec 1, 2016

rkouj commented Dec 1, 2016

saad-ali left a comment

Choose a reason for hiding this comment

saad-ali Dec 1, 2016

Choose a reason for hiding this comment

rkouj Dec 1, 2016

Choose a reason for hiding this comment

saad-ali commented Dec 1, 2016

saad-ali commented Dec 1, 2016

rkouj commented Dec 1, 2016

rkouj commented Dec 1, 2016 • edited

rkouj commented Dec 1, 2016

rkouj commented Dec 1, 2016

rkouj commented Dec 1, 2016

saad-ali commented Dec 1, 2016

saad-ali commented Dec 1, 2016

k8s-github-robot commented Dec 2, 2016

k8s-ci-robot commented Dec 2, 2016

k8s-ci-robot commented Dec 2, 2016

rkouj commented Dec 2, 2016

rkouj commented Dec 2, 2016

k8s-github-robot commented Dec 2, 2016

k8s-cherrypick-bot commented Dec 2, 2016

rkouj commented Nov 30, 2016 •

edited

rkouj Dec 1, 2016 •

edited

rkouj commented Dec 1, 2016 •

edited