Merge pull request #764 from diegs/uid

checkpointer: two fixes (now that checkpointer tests work again)

Author: diegs

pkg/checkpoint/pod.go

@@ -28,9 +28,14 @@ var (
 	)
 )
 
-func sanitizeCheckpointPod(cp *v1.Pod) (*v1.Pod, error) {
+func sanitizeCheckpointPod(cp *v1.Pod) *v1.Pod {
 	trueVar := true
 
+	// Check if this is already sanitized, i.e. it was read back from a checkpoint on disk.
+	if _, ok := cp.Annotations[checkpointParentAnnotation]; ok {
+		return cp
+	}
+
 	// Keep same name, namespace, and labels as parent.
 	cp.ObjectMeta = metav1.ObjectMeta{
 		Name:        cp.Name,
@@ -73,7 +78,7 @@ func sanitizeCheckpointPod(cp *v1.Pod) (*v1.Pod, error) {
 	// Clear pod status
 	cp.Status.Reset()
 
-	return cp, nil
+	return cp
 }
 
 // isPodCheckpointer returns true if the manifest is the pod checkpointer (has the same name as the parent).

pkg/checkpoint/pod_test.go

@@ -139,13 +139,41 @@ func TestSanitizeCheckpointPod(t *testing.T) {
 				},
 			},
 		},
+		{
+			desc: "Pod is already sanitized.",
+			pod: &v1.Pod{
+				TypeMeta: metav1.TypeMeta{
+					APIVersion: "v1",
+					Kind:       "Pod",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:        "podname",
+					Namespace:   "podnamespace",
+					Annotations: map[string]string{checkpointParentAnnotation: "podname"},
+					OwnerReferences: []metav1.OwnerReference{
+						{APIVersion: "v1", Kind: "Pod", Name: "podname", UID: "pod-uid", Controller: &trueVar},
+					},
+				},
+			},
+			expected: &v1.Pod{
+				TypeMeta: metav1.TypeMeta{
+					APIVersion: "v1",
+					Kind:       "Pod",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:        "podname",
+					Namespace:   "podnamespace",
+					Annotations: map[string]string{checkpointParentAnnotation: "podname"},
+					OwnerReferences: []metav1.OwnerReference{
+						{APIVersion: "v1", Kind: "Pod", Name: "podname", UID: "pod-uid", Controller: &trueVar},
+					},
+				},
+			},
+		},
 	}
 
 	for _, tc := range cases {
-		got, err := sanitizeCheckpointPod(tc.pod)
-		if err != nil {
-			t.Errorf("\nUnexpected error: %v\n", err)
-		}
+		got := sanitizeCheckpointPod(tc.pod)
 		if !apiequality.Semantic.DeepEqual(tc.expected, got) {
 			t.Errorf("\nFor Test: %s\n\nExpected:\n%#v\nGot:\n%#v\n", tc.desc, tc.expected, got)
 		}

pkg/checkpoint/process.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"os"
+	"sort"
 	"time"
 
 	"github.com/golang/glog"
@@ -43,7 +44,8 @@ func (cs *checkpoints) update(localRunningPods, localParentPods, apiParentPods,
 		cs.checkpoints[cs.selfCheckpoint.name] = cs.selfCheckpoint
 	}
 
-	// Make sure all on-disk checkpoints are represented in memory, i.e. if we are restarting from a crash.
+	// Make sure all on-disk checkpoints are represented in memory, i.e. if we are restarting from a
+	// crash.
 	for name, pod := range activeCheckpoints {
 		if _, ok := cs.checkpoints[name]; !ok {
 			cs.checkpoints[name] = &checkpoint{
@@ -142,6 +144,7 @@ func (cs *checkpoints) process(now time.Time, apiAvailable bool, localRunningPod
 					stops = append(stops, name)
 				}
 			}
+			sort.Strings(stops) // Ensure stable ordering.
 			stops = append(stops, cs.selfCheckpoint.name)
 			return starts, stops, removes
 		case remove:
@@ -153,6 +156,7 @@ func (cs *checkpoints) process(now time.Time, apiAvailable bool, localRunningPod
 					delete(cs.checkpoints, name)
 				}
 			}
+			sort.Strings(removes) // Ensure stable ordering.
 			removes = append(removes, cs.selfCheckpoint.name)
 			delete(cs.checkpoints, cs.selfCheckpoint.name)
 			cs.selfCheckpoint = nil
@@ -224,11 +228,7 @@ func (c *checkpointer) createCheckpointsForValidParents() {
 			continue
 		}
 
-		cp, err = sanitizeCheckpointPod(cp)
-		if err != nil {
-			glog.Errorf("Failed to sanitize manifest for %s: %v", id, err)
-			continue
-		}
+		cp = sanitizeCheckpointPod(cp)
 
 		podChanged, err := writeCheckpointManifest(cp)
 		if err != nil {

pkg/checkpoint/process_test.go

@@ -1,7 +1,6 @@
 package checkpoint
 
 import (
-	"flag"
 	"reflect"
 	"testing"
 	"time"
@@ -11,9 +10,6 @@ import (
 )
 
 func TestProcess(t *testing.T) {
-	flag.Set("logtostderr", "true")
-	flag.Parse()
-
 	type testCase struct {
 		desc                string
 		localRunning        map[string]*v1.Pod
@@ -59,6 +55,13 @@ func TestProcess(t *testing.T) {
 			apiParents:          map[string]*v1.Pod{"AA": {}},
 			expectStart:         []string{"AA"},
 		},
+		{
+			desc:                "Inactive checkpoint and only api and kubelet parents: should start",
+			inactiveCheckpoints: map[string]*v1.Pod{"AA": {}},
+			apiParents:          map[string]*v1.Pod{"AA": {}},
+			localParents:        map[string]*v1.Pod{"AA": {}},
+			expectStart:         []string{"AA"},
+		},
 		{
 			desc:              "Active checkpoint and no local running: no change",
 			activeCheckpoints: map[string]*v1.Pod{"AA": {}},
@@ -170,8 +173,9 @@ func TestProcess(t *testing.T) {
 				},
 				"AA": {},
 			},
-			expectStop:        []string{"AA", "kube-system/pod-checkpointer"},
-			expectGraceRemove: []string{"AA", "kube-system/pod-checkpointer"},
+			expectStart:       []string{"kube-system/pod-checkpointer"},
+			expectStop:        []string{"BB"},
+			expectGraceRemove: []string{"AA", "BB", "kube-system/pod-checkpointer"},
 		},
 		{
 			desc:         "Inactive pod-checkpointer, no local parent, no api parent: should remove all",
@@ -187,7 +191,7 @@ func TestProcess(t *testing.T) {
 				},
 				"AA": {},
 			},
-			expectStop:        []string{"AA", "kube-system/pod-checkpointer"},
+			expectStart:       []string{"kube-system/pod-checkpointer"},
 			expectGraceRemove: []string{"AA", "kube-system/pod-checkpointer"},
 		},
 		{
@@ -204,7 +208,8 @@ func TestProcess(t *testing.T) {
 				},
 				"AA": {},
 			},
-			expectStop:        []string{"AA", "kube-system/pod-checkpointer"},
+			expectStart:       []string{"kube-system/pod-checkpointer"},
+			expectStop:        []string{"AA"},
 			expectGraceRemove: []string{"AA", "kube-system/pod-checkpointer"},
 		},
 		{
@@ -268,8 +273,8 @@ func TestProcess(t *testing.T) {
 				},
 				"AA": {},
 			},
-			expectStop:        []string{"AA", "kube-system/pod-checkpointer"},
-			expectGraceRemove: []string{"AA", "kube-system/pod-checkpointer"},
+			expectStart:       []string{"kube-system/pod-checkpointer", "BB"},
+			expectGraceRemove: []string{"AA", "BB", "kube-system/pod-checkpointer"},
 		},
 		{
 			desc:         "Running as an on-disk checkpointer: Inactive pod-checkpointer, no local parent, no api parent: should remove all",
@@ -286,7 +291,7 @@ func TestProcess(t *testing.T) {
 				},
 				"AA": {},
 			},
-			expectStop:        []string{"AA", "kube-system/pod-checkpointer"},
+			expectStart:       []string{"kube-system/pod-checkpointer"},
 			expectGraceRemove: []string{"AA", "kube-system/pod-checkpointer"},
 		},
 		{
@@ -304,7 +309,8 @@ func TestProcess(t *testing.T) {
 				},
 				"AA": {},
 			},
-			expectStop:        []string{"AA", "kube-system/pod-checkpointer"},
+			expectStart:       []string{"kube-system/pod-checkpointer"},
+			expectStop:        []string{"AA"},
 			expectGraceRemove: []string{"AA", "kube-system/pod-checkpointer"},
 		},
 	}

pkg/checkpoint/state.go

@@ -65,7 +65,7 @@ type checkpointState interface {
 // stateSelfCheckpointActive represents a checkpoint of the checkpointer itself, which has special
 // behavior.
 //
-// stateSelfCheckpointActive can transition to stateInactiveGracePeriod.
+// stateSelfCheckpointActive can transition to stateActiveGracePeriod.
 type stateSelfCheckpointActive struct{}
 
 // transition implements state.transition()
@@ -80,9 +80,9 @@ func (s stateSelfCheckpointActive) transition(now time.Time, apis apiCondition)
 		return s
 	}
 
-	// The apiserver parent pod is deleted, transition to stateInactiveGracePeriod.
+	// The apiserver parent pod is deleted, transition to stateActiveGracePeriod.
 	// TODO(diegs): this is a little hacky, perhaps clean it up with a constructor.
-	return stateInactiveGracePeriod{gracePeriodEnd: now.Add(checkpointGracePeriod)}.checkGracePeriod(now, apis)
+	return stateActiveGracePeriod{gracePeriodEnd: now.Add(checkpointGracePeriod)}.checkGracePeriod(now, apis)
 }
 
 // action implements state.action()

pkg/checkpoint/state_test.go

@@ -31,7 +31,7 @@ func TestAllowedStateTransitions(t *testing.T) {
 		want  []checkpointState
 	}{{
 		state: stateSelfCheckpointActive{},
-		want:  []checkpointState{stateSelfCheckpointActive{}, stateInactiveGracePeriod{}},
+		want:  []checkpointState{stateSelfCheckpointActive{}, stateActiveGracePeriod{}},
 	}, {
 		state: stateNone{},
 		want:  []checkpointState{stateNone{}, stateInactive{}, stateInactiveGracePeriod{}, stateActive{}},