Fix to handle hash collisions correctly for DaemonSets #66476

mortent · 2018-07-21T22:53:13Z

What this PR does / why we need it: This adds an integration test for the case where there is a hash collision when creating a ControllerRevision for a DaemonSet. It also fixes a shadowed variable that prevented this functionality from working as intended.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #62519

Special notes for your reviewer:
/sig apps

Release note:

Fixes issue when updating a DaemonSet causes a hash collision.

k8s-ci-robot · 2018-07-21T22:53:25Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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.

mortent · 2018-07-21T23:13:29Z

CLA should be fixed now

dims · 2018-07-22T02:41:39Z

/ok-to-test

mikedanese · 2018-07-24T13:30:35Z

pkg/controller/daemon/update.go

-		done, err := Match(ds, existedHistory)
-		if err != nil {
-			return nil, err
+		done, matchErr := Match(ds, existedHistory)


I feel like this is bound to happen again. Can you do something like

if outerErr := err; errors.IsAlreadExists(outerErr) { ... return outerErr }

mikedanese · 2018-07-24T13:47:21Z

test/integration/daemonset/daemonset_test.go

+		t.Fatalf("Failed to update DaemonSet: %v", err)
+	}
+
+	validateDaemonSetPodsAndMarkReady(podClient, podInformer, 1, t)


should you validate that the collision counter is bumped and that the pod has TerminationGracePeriodSeconds set to one?

mortent · 2018-07-25T17:22:21Z

Updated to address comments from @mikedanese.

janetkuo

Thanks for catching the bug!

janetkuo · 2018-07-25T23:38:44Z

test/integration/daemonset/daemonset_test.go

+	}
+
+	validateDaemonSetPodsAndMarkReady(podClient, podInformer, 1, t)
+	validateDaemonSetStatus(dsClient, ds.Name, 1, t)


validateDaemonSetPodsAndMarkReady checks DS has X number of pods and mark them Ready. It doesn't check if the Pods are updated or not. We can skip this step because we don't care if the pods are ready or not.

validateDaemonSetStatus checks whether the status matches current # of Pods or not, but it doesn't check if the Pods are updated or not, either. We can also skip this one.

There's a possible race that the Pods haven't been updated before we validate collision count and Pods. Suggest instead wait for new pods and new controllerrevision to be created, and then check DS's collision count.

janetkuo · 2018-07-25T23:39:58Z

test/integration/daemonset/daemonset_test.go

+	one := int64(1)
+	ds.Spec.Template.Spec.TerminationGracePeriodSeconds = &one
+
+	newHash, newName := hashAndNameForDaemonSet(ds)


Suggest add comments for each step (or some steps).

mortent · 2018-07-26T05:07:09Z

@janetkuo Thanks for the detailed review.
I have updated the validation part of the test to wait for a pod to come up with the updated value for TerminationGracePeriodSeconds and then check that the CollisionCount is the expected value. I think this should avoid the race.
Also added some comments on the key steps in the test code.

janetkuo · 2018-08-03T17:29:43Z

test/integration/daemonset/daemonset_test.go

+
+	// Look up the ControllerRevision for the DaemonSet
+	_, name := hashAndNameForDaemonSet(ds)
+	revision, err := clientset.AppsV1().ControllerRevisions(ds.Namespace).Get(name, metav1.GetOptions{})


It's possible that the CR haven't been created yet after the DS is created, so wait for CR to be created to avoid possible test flake.

janetkuo · 2018-08-03T17:31:29Z

test/integration/daemonset/daemonset_test.go

@@ -372,6 +374,33 @@ func waitForPodsCreated(podInformer cache.SharedIndexInformer, num int) error {
 	})
 }

+func waitForDaemonSetCreated(c clientset.Interface, name string, namespace string) error {
+	return wait.Poll(2*time.Second, 30*time.Second, func() (bool, error) {


Use PollImmediate() instead of Poll(). Use shorter intervals (e.g. 100ms) and shorter timeouts (10s).

I'd like to change all Poll() to PollImmediate() and make interval and timeout constants, e.g.

const ( interval = 100 * time.Millisecond shortTimeout = 10 * time.Second longTimeout = 60 * time.Second )

but that should be a separate PR.

I've updated the test to use PollImmediate and shorter intervals and timeouts. I can do a separate PR to pull out the constants and switch the other tests in the file to use PollImmediate.

janetkuo · 2018-08-03T17:53:18Z

test/integration/daemonset/daemonset_test.go

+	}
+
+	// Wait for the pod with the latest Spec to become available
+	err = wait.Poll(10*time.Second, 60*time.Second, func() (bool, error) {


janetkuo · 2018-08-03T17:55:17Z

test/integration/daemonset/daemonset_test.go

+		t.Fatalf("Failed to update DaemonSet: %v", err)
+	}
+
+	// Wait for the pod with the latest Spec to become available


"Wait for any pod ... to exist"

janetkuo · 2018-08-03T17:56:59Z

test/integration/daemonset/daemonset_test.go

+		objects := podInformer.GetIndexer().List()
+		for _, object := range objects {
+			pod := object.(*v1.Pod)
+			if *pod.Spec.TerminationGracePeriodSeconds == int64(1) {


nil: == *ds.Spec.Template.Spec.TerminationGracePeriodSeconds

janetkuo · 2018-08-03T18:00:00Z

test/integration/daemonset/daemonset_test.go

+		return false, nil
+	})
+	if err != nil {
+		t.Fatal(err)


t.Fatalf("Failed to wait for Pods with the latest Spec to be created: %v", err)

janetkuo · 2018-08-03T18:01:42Z

test/integration/daemonset/daemonset_test.go

+	if err != nil {
+		t.Fatalf("Failed to create ControllerRevision: %v", err)
+	}
+


nit: save ds's collision count here for comparison later. The count should be count+1 at the end of this test.

mortent · 2018-08-03T22:28:30Z

@janetkuo I have updated the PR based on the comments.

mortent · 2018-08-03T23:31:14Z

/retest

janetkuo · 2018-08-08T20:14:05Z

test/integration/daemonset/daemonset_test.go

+	var orgCollisionCount int32
+	if ds.Status.CollisionCount != nil {
+		orgCollisionCount = *ds.Status.CollisionCount
+	}


Move this down to L846 after DS's CR is created and a fresh DS is got back from the server. This is because collision count might change after CR creation.

janetkuo · 2018-08-09T18:19:01Z

/lgtm

k8s-ci-robot · 2018-08-09T18:19:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: janetkuo, mortent

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:

~~pkg/controller/daemon/OWNERS~~ [janetkuo]
~~test/integration/daemonset/OWNERS~~ [janetkuo]

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

k8s-github-robot · 2018-08-09T22:11:40Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-08-09T23:30:57Z

Automatic merge from submit-queue (batch tested with PRs 67090, 67159, 66866, 62111, 66476). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from lukaszo and mikedanese July 21, 2018 22:53

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 21, 2018

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 21, 2018

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 Jul 22, 2018

janetkuo self-assigned this Jul 23, 2018

mikedanese reviewed Jul 24, 2018

View reviewed changes

janetkuo reviewed Jul 25, 2018

View reviewed changes

janetkuo reviewed Aug 3, 2018

View reviewed changes

mortent force-pushed the IntegrationTestForDaemonSetHashCollision branch from 737cfdc to 0e28b6c Compare August 3, 2018 22:23

mortent force-pushed the IntegrationTestForDaemonSetHashCollision branch 2 times, most recently from 57ff30a to ab23bc4 Compare August 4, 2018 16:54

janetkuo reviewed Aug 8, 2018

View reviewed changes

Fix to handle hash collisions correctly for DaemonSet

a93ea43

mortent force-pushed the IntegrationTestForDaemonSetHashCollision branch from ab23bc4 to a93ea43 Compare August 8, 2018 20:54

janetkuo approved these changes Aug 9, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 9, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 9, 2018

k8s-github-robot merged commit e26f5d1 into kubernetes:master Aug 9, 2018

neolit123 mentioned this pull request Aug 10, 2018

test/integration/daemonset TestLaunchWithHashCollision is flaking #67273

Closed

tnozicka mentioned this pull request Aug 13, 2018

Fix conflict race in TestLaunchWithHashCollision #67346

Closed

mortent mentioned this pull request Oct 3, 2018

REQUEST: New membership for mortent kubernetes/org#136

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to handle hash collisions correctly for DaemonSets #66476

Fix to handle hash collisions correctly for DaemonSets #66476

mortent commented Jul 21, 2018

k8s-ci-robot commented Jul 21, 2018

mortent commented Jul 21, 2018

dims commented Jul 22, 2018

mikedanese Jul 24, 2018

mikedanese Jul 24, 2018

mortent commented Jul 25, 2018

janetkuo left a comment

janetkuo Jul 25, 2018 •

edited

Loading

janetkuo Jul 25, 2018

mortent commented Jul 26, 2018

janetkuo Aug 3, 2018

janetkuo Aug 3, 2018

mortent Aug 3, 2018

janetkuo Aug 3, 2018

janetkuo Aug 3, 2018

janetkuo Aug 3, 2018

janetkuo Aug 3, 2018

janetkuo Aug 3, 2018

mortent commented Aug 3, 2018

mortent commented Aug 3, 2018

janetkuo Aug 8, 2018

janetkuo commented Aug 9, 2018

k8s-ci-robot commented Aug 9, 2018

k8s-github-robot commented Aug 9, 2018

k8s-github-robot commented Aug 9, 2018

Fix to handle hash collisions correctly for DaemonSets #66476

Fix to handle hash collisions correctly for DaemonSets #66476

Conversation

mortent commented Jul 21, 2018

k8s-ci-robot commented Jul 21, 2018

mortent commented Jul 21, 2018

dims commented Jul 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mortent commented Jul 25, 2018

janetkuo left a comment

Choose a reason for hiding this comment

janetkuo Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mortent commented Jul 26, 2018

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

mortent commented Aug 3, 2018

mortent commented Aug 3, 2018

Choose a reason for hiding this comment

janetkuo commented Aug 9, 2018

k8s-ci-robot commented Aug 9, 2018

k8s-github-robot commented Aug 9, 2018

k8s-github-robot commented Aug 9, 2018

janetkuo Jul 25, 2018 •

edited

Loading