Merge pull request kubernetes#896 from smarterclayton/return_correct_…

…index_for_delete Delete should use current node, not prev node index
mattf · Aug 14, 2014 · 8daf8c7 · 8daf8c7
2 parents bf7f8a2 + 2872e56
commit 8daf8c7
Show file tree

Hide file tree

Showing 2 changed files with 83 additions and 32 deletions.
diff --git a/pkg/tools/etcd_tools.go b/pkg/tools/etcd_tools.go
@@ -491,7 +491,7 @@ func (w *etcdWatcher) sendResult(res *etcd.Response) {
 			return
 		}
 		data = []byte(res.PrevNode.Value)
-		index = res.PrevNode.ModifiedIndex
+		index = res.Node.ModifiedIndex
 		action = watch.Deleted
 	default:
 		glog.Errorf("unknown action: %v", res.Action)

diff --git a/pkg/tools/etcd_tools_test.go b/pkg/tools/etcd_tools_test.go
@@ -391,15 +391,20 @@ func TestWatchInterpretation_Delete(t *testing.T) {
 
 	go w.sendResult(&etcd.Response{
 		Action: "delete",
+		Node: &etcd.Node{
+			ModifiedIndex: 2,
+		},
 		PrevNode: &etcd.Node{
-			Value: string(podBytes),
+			Value:         string(podBytes),
+			ModifiedIndex: 1,
 		},
 	})
 
 	got := <-w.outgoing
 	if e, a := watch.Deleted, got.Type; e != a {
 		t.Errorf("Expected %v, got %v", e, a)
 	}
+	pod.ResourceVersion = 2
 	if e, a := pod, got.Object; !reflect.DeepEqual(e, a) {
 		t.Errorf("Expected %v, got %v", e, a)
 	}
@@ -497,43 +502,89 @@ func TestWatch(t *testing.T) {
 func TestWatchFromZeroIndex(t *testing.T) {
 	pod := &api.Pod{JSONBase: api.JSONBase{ID: "foo"}}
 
-	fakeClient := MakeFakeEtcdClient(t)
-	fakeClient.Data["/some/key"] = EtcdResponseWithError{
-		R: &etcd.Response{
-			Node: &etcd.Node{
-				Value:         api.EncodeOrDie(pod),
-				ModifiedIndex: 1,
+	testCases := map[string]struct {
+		Response        EtcdResponseWithError
+		ExpectedVersion uint64
+		ExpectedType    watch.EventType
+	}{
+		"last write was a modify": {
+			EtcdResponseWithError{
+				R: &etcd.Response{
+					Node: &etcd.Node{
+						Value:         api.EncodeOrDie(pod),
+						ModifiedIndex: 1,
+					},
+					Action:    "compareAndSwap",
+					EtcdIndex: 2,
+				},
+			},
+			1,
+			watch.Modified,
+		},
+		"last write was a delete": {
+			EtcdResponseWithError{
+				R: &etcd.Response{
+					Node: &etcd.Node{
+						Value:         api.EncodeOrDie(pod),
+						ModifiedIndex: 2,
+					},
+					PrevNode: &etcd.Node{
+						Value:         api.EncodeOrDie(pod),
+						ModifiedIndex: 1,
+					},
+					Action:    "delete",
+					EtcdIndex: 3,
+				},
+			},
+			2,
+			watch.Deleted,
+		},
+		"last write was a create": {
+			EtcdResponseWithError{
+				R: &etcd.Response{
+					Node: &etcd.Node{
+						Value:         api.EncodeOrDie(pod),
+						ModifiedIndex: 2,
+					},
+					Action:    "create",
+					EtcdIndex: 3,
+				},
 			},
-			Action:    "compareAndSwap",
-			EtcdIndex: 2,
+			2,
+			watch.Added,
 		},
 	}
-	h := EtcdHelper{fakeClient, codec, versioner}
 
-	watching, err := h.Watch("/some/key", 0)
-	if err != nil {
-		t.Fatalf("Unexpected error: %v", err)
-	}
+	for k, testCase := range testCases {
+		fakeClient := MakeFakeEtcdClient(t)
+		fakeClient.Data["/some/key"] = testCase.Response
+		h := EtcdHelper{fakeClient, codec, versioner}
 
-	fakeClient.WaitForWatchCompletion()
+		watching, err := h.Watch("/some/key", 0)
+		if err != nil {
+			t.Fatalf("%s: unexpected error: %v", k, err)
+		}
 
-	// the existing node is detected and the index set
-	event := <-watching.ResultChan()
-	if e, a := watch.Modified, event.Type; e != a {
-		t.Errorf("Expected %v, got %v", e, a)
-	}
-	actualPod, ok := event.Object.(*api.Pod)
-	if !ok {
-		t.Fatalf("expected a pod, got %#v", event.Object)
-	}
-	if actualPod.ResourceVersion != 1 {
-		t.Errorf("Expected pod with resource version %d, Got %#v", 1, actualPod)
-	}
-	pod.ResourceVersion = 1
-	if e, a := pod, event.Object; !reflect.DeepEqual(e, a) {
-		t.Errorf("Expected %v, got %v", e, a)
+		fakeClient.WaitForWatchCompletion()
+
+		// the existing node is detected and the index set
+		event := <-watching.ResultChan()
+		if e, a := testCase.ExpectedType, event.Type; e != a {
+			t.Errorf("%s: expected %v, got %v", k, e, a)
+		}
+		actualPod, ok := event.Object.(*api.Pod)
+		if !ok {
+			t.Fatalf("%s: expected a pod, got %#v", k, event.Object)
+		}
+		if actualPod.ResourceVersion != testCase.ExpectedVersion {
+			t.Errorf("%s: expected pod with resource version %d, Got %#v", k, testCase.ExpectedVersion, actualPod)
+		}
+		pod.ResourceVersion = testCase.ExpectedVersion
+		if e, a := pod, event.Object; !reflect.DeepEqual(e, a) {
+			t.Errorf("%s: expected %v, got %v", k, e, a)
+		}
+		watching.Stop()
 	}
-	watching.Stop()
 }
 
 func TestWatchListFromZeroIndex(t *testing.T) {