Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix bug in status/manager_test.go #19690

Merged
merged 1 commit into from
Jan 18, 2016
Merged

Fix bug in status/manager_test.go #19690

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
94 changes: 49 additions & 45 deletions pkg/kubelet/status/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,15 @@ import (
"k8s.io/kubernetes/pkg/runtime"
)

var testPod *api.Pod = &api.Pod{
ObjectMeta: api.ObjectMeta{
UID: "12345678",
Name: "foo",
Namespace: "new",
},
// Generate new instance of test pod with the same initial value.
func getTestPod() *api.Pod {
return &api.Pod{
ObjectMeta: api.ObjectMeta{
UID: "12345678",
Name: "foo",
Namespace: "new",
},
}
}

// After adding reconciliation, if status in pod manager is different from the cached status, a reconciliation
Expand All @@ -64,7 +67,7 @@ func (m *manager) testSyncBatch() {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a new function getTestPod() return a new test pod every time? This will be more fool proof than asking each test to make a copy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I've added a TODO about that. This PR mainly aims at fixing the bug. I'll send a next PR to refactor the test with a newTestPod(), :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do you think it is not that urgent, we could do it in this PR? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha...I saw your respond, the page is not refreshed just now. If so I'll send a following PR to refactor the test a little.

func newTestManager(kubeClient client.Interface) *manager {
podManager := kubepod.NewBasicPodManager(kubepod.NewFakeMirrorClient())
podManager.AddPod(testPod)
podManager.AddPod(getTestPod())
return NewManager(kubeClient, podManager).(*manager)
}

Expand Down Expand Up @@ -117,6 +120,7 @@ func verifyUpdates(t *testing.T, manager *manager, expectedUpdates int) {

func TestNewStatus(t *testing.T) {
syncer := newTestManager(&testclient.Fake{})
testPod := getTestPod()
syncer.SetPodStatus(testPod, getRandomPodStatus())
verifyUpdates(t, syncer, 1)

Expand Down Expand Up @@ -180,13 +184,15 @@ func TestNewStatusSetsReadyTransitionTime(t *testing.T) {

func TestChangedStatus(t *testing.T) {
syncer := newTestManager(&testclient.Fake{})
testPod := getTestPod()
syncer.SetPodStatus(testPod, getRandomPodStatus())
syncer.SetPodStatus(testPod, getRandomPodStatus())
verifyUpdates(t, syncer, 2)
}

func TestChangedStatusKeepsStartTime(t *testing.T) {
syncer := newTestManager(&testclient.Fake{})
testPod := getTestPod()
now := unversioned.Now()
firstStatus := getRandomPodStatus()
firstStatus.StartTime = &now
Expand Down Expand Up @@ -234,6 +240,7 @@ func TestChangedStatusUpdatesLastTransitionTime(t *testing.T) {

func TestUnchangedStatus(t *testing.T) {
syncer := newTestManager(&testclient.Fake{})
testPod := getTestPod()
podStatus := getRandomPodStatus()
syncer.SetPodStatus(testPod, podStatus)
syncer.SetPodStatus(testPod, podStatus)
Expand Down Expand Up @@ -276,7 +283,7 @@ func TestSyncBatchIgnoresNotFound(t *testing.T) {
client.AddReactor("get", "pods", func(action testclient.Action) (bool, runtime.Object, error) {
return true, nil, errors.NewNotFound(api.Resource("pods"), "test-pod")
})
syncer.SetPodStatus(testPod, getRandomPodStatus())
syncer.SetPodStatus(getTestPod(), getRandomPodStatus())
syncer.testSyncBatch()

verifyActions(t, syncer.kubeClient, []testclient.Action{
Expand All @@ -286,6 +293,7 @@ func TestSyncBatchIgnoresNotFound(t *testing.T) {

func TestSyncBatch(t *testing.T) {
syncer := newTestManager(&testclient.Fake{})
testPod := getTestPod()
syncer.kubeClient = testclient.NewSimpleFake(testPod)
syncer.SetPodStatus(testPod, getRandomPodStatus())
syncer.testSyncBatch()
Expand All @@ -298,14 +306,14 @@ func TestSyncBatch(t *testing.T) {

func TestSyncBatchChecksMismatchedUID(t *testing.T) {
syncer := newTestManager(&testclient.Fake{})
pod := *testPod
pod := getTestPod()
pod.UID = "first"
syncer.podManager.AddPod(&pod)
differentPod := *testPod
syncer.podManager.AddPod(pod)
differentPod := getTestPod()
differentPod.UID = "second"
syncer.podManager.AddPod(&differentPod)
syncer.kubeClient = testclient.NewSimpleFake(&pod)
syncer.SetPodStatus(&differentPod, getRandomPodStatus())
syncer.podManager.AddPod(differentPod)
syncer.kubeClient = testclient.NewSimpleFake(pod)
syncer.SetPodStatus(differentPod, getRandomPodStatus())
syncer.testSyncBatch()
verifyActions(t, syncer.kubeClient, []testclient.Action{
testclient.GetActionImpl{ActionImpl: testclient.ActionImpl{Verb: "get", Resource: "pods"}},
Expand All @@ -315,24 +323,23 @@ func TestSyncBatchChecksMismatchedUID(t *testing.T) {
func TestSyncBatchNoDeadlock(t *testing.T) {
client := &testclient.Fake{}
m := newTestManager(client)
pod := getTestPod()

// Setup fake client.
var ret api.Pod
var err error
client.AddReactor("*", "pods", func(action testclient.Action) (bool, runtime.Object, error) {
switch action := action.(type) {
case testclient.GetAction:
assert.Equal(t, testPod.Name, action.GetName(), "Unexpeted GetAction: %+v", action)
assert.Equal(t, pod.Name, action.GetName(), "Unexpeted GetAction: %+v", action)
case testclient.UpdateAction:
assert.Equal(t, testPod.Name, action.GetObject().(*api.Pod).Name, "Unexpeted UpdateAction: %+v", action)
assert.Equal(t, pod.Name, action.GetObject().(*api.Pod).Name, "Unexpeted UpdateAction: %+v", action)
default:
assert.Fail(t, "Unexpected Action: %+v", action)
}
return true, &ret, err
})

pod := new(api.Pod)
*pod = *testPod
pod.Status.ContainerStatuses = []api.ContainerStatus{{State: api.ContainerState{Running: &api.ContainerStateRunning{}}}}

getAction := testclient.GetActionImpl{ActionImpl: testclient.ActionImpl{Verb: "get", Resource: "pods"}}
Expand Down Expand Up @@ -385,16 +392,16 @@ func TestSyncBatchNoDeadlock(t *testing.T) {
}

func TestStaleUpdates(t *testing.T) {
pod := *testPod
client := testclient.NewSimpleFake(&pod)
pod := getTestPod()
client := testclient.NewSimpleFake(pod)
m := newTestManager(client)

status := api.PodStatus{Message: "initial status"}
m.SetPodStatus(&pod, status)
m.SetPodStatus(pod, status)
status.Message = "first version bump"
m.SetPodStatus(&pod, status)
m.SetPodStatus(pod, status)
status.Message = "second version bump"
m.SetPodStatus(&pod, status)
m.SetPodStatus(pod, status)
verifyUpdates(t, m, 3)

t.Logf("First sync pushes latest status.")
Expand All @@ -412,13 +419,13 @@ func TestStaleUpdates(t *testing.T) {
}

t.Log("Unchanged status should not send an update.")
m.SetPodStatus(&pod, status)
m.SetPodStatus(pod, status)
verifyUpdates(t, m, 0)

t.Log("... unless it's stale.")
m.apiStatusVersions[pod.UID] = m.apiStatusVersions[pod.UID] - 1

m.SetPodStatus(&pod, status)
m.SetPodStatus(pod, status)
m.testSyncBatch()
verifyActions(t, m.kubeClient, []testclient.Action{
testclient.GetActionImpl{ActionImpl: testclient.ActionImpl{Verb: "get", Resource: "pods"}},
Expand Down Expand Up @@ -462,29 +469,29 @@ func TestStatusEquality(t *testing.T) {
}

func TestStaticPodStatus(t *testing.T) {
staticPod := *testPod
staticPod := getTestPod()
staticPod.Annotations = map[string]string{kubetypes.ConfigSourceAnnotationKey: "file"}
mirrorPod := *testPod
mirrorPod := getTestPod()
mirrorPod.UID = "mirror-12345678"
mirrorPod.Annotations = map[string]string{
kubetypes.ConfigSourceAnnotationKey: "api",
kubetypes.ConfigMirrorAnnotationKey: "mirror",
}
client := testclient.NewSimpleFake(&mirrorPod)
client := testclient.NewSimpleFake(mirrorPod)
m := newTestManager(client)
m.podManager.AddPod(&staticPod)
m.podManager.AddPod(&mirrorPod)
m.podManager.AddPod(staticPod)
m.podManager.AddPod(mirrorPod)
// Verify setup.
assert.True(t, kubepod.IsStaticPod(&staticPod), "SetUp error: staticPod")
assert.True(t, kubepod.IsMirrorPod(&mirrorPod), "SetUp error: mirrorPod")
assert.True(t, kubepod.IsStaticPod(staticPod), "SetUp error: staticPod")
assert.True(t, kubepod.IsMirrorPod(mirrorPod), "SetUp error: mirrorPod")
assert.Equal(t, m.podManager.TranslatePodUID(mirrorPod.UID), staticPod.UID)

status := getRandomPodStatus()
now := unversioned.Now()
status.StartTime = &now

m.SetPodStatus(&staticPod, status)
retrievedStatus := expectPodStatus(t, m, &staticPod)
m.SetPodStatus(staticPod, status)
retrievedStatus := expectPodStatus(t, m, staticPod)
assert.True(t, isStatusEqual(&status, &retrievedStatus), "Expected: %+v, Got: %+v", status, retrievedStatus)
retrievedStatus, _ = m.GetPodStatus(mirrorPod.UID)
assert.True(t, isStatusEqual(&status, &retrievedStatus), "Expected: %+v, Got: %+v", status, retrievedStatus)
Expand All @@ -505,10 +512,10 @@ func TestStaticPodStatus(t *testing.T) {
verifyActions(t, m.kubeClient, []testclient.Action{})

// Mirror pod identity changes.
m.podManager.DeletePod(&mirrorPod)
m.podManager.DeletePod(mirrorPod)
mirrorPod.UID = "new-mirror-pod"
mirrorPod.Status = api.PodStatus{}
m.podManager.AddPod(&mirrorPod)
m.podManager.AddPod(mirrorPod)
// Expect update to new mirrorPod.
m.testSyncBatch()
verifyActions(t, m.kubeClient, []testclient.Action{
Expand Down Expand Up @@ -542,8 +549,7 @@ func TestSetContainerReadiness(t *testing.T) {
Status: api.ConditionFalse,
}},
}
pod := new(api.Pod)
*pod = *testPod
pod := getTestPod()
pod.Spec.Containers = []api.Container{{Name: "c1"}, {Name: "c2"}}

// Verify expected readiness of containers & pod.
Expand Down Expand Up @@ -611,7 +617,8 @@ func TestSetContainerReadiness(t *testing.T) {

func TestSyncBatchCleanupVersions(t *testing.T) {
m := newTestManager(&testclient.Fake{})
mirrorPod := *testPod
testPod := getTestPod()
mirrorPod := getTestPod()
mirrorPod.UID = "mirror-uid"
mirrorPod.Name = "mirror_pod"
mirrorPod.Annotations = map[string]string{
Expand All @@ -632,11 +639,11 @@ func TestSyncBatchCleanupVersions(t *testing.T) {

// Non-orphaned pods should not be removed.
m.SetPodStatus(testPod, getRandomPodStatus())
m.podManager.AddPod(&mirrorPod)
m.podManager.AddPod(mirrorPod)
staticPod := mirrorPod
staticPod.UID = "static-uid"
staticPod.Annotations = map[string]string{kubetypes.ConfigSourceAnnotationKey: "file"}
m.podManager.AddPod(&staticPod)
m.podManager.AddPod(staticPod)
m.apiStatusVersions[testPod.UID] = 100
m.apiStatusVersions[mirrorPod.UID] = 200
m.testSyncBatch()
Expand All @@ -649,13 +656,13 @@ func TestSyncBatchCleanupVersions(t *testing.T) {
}

func TestReconcilePodStatus(t *testing.T) {
testPod := getTestPod()
client := testclient.NewSimpleFake(testPod)
syncer := newTestManager(client)
syncer.SetPodStatus(testPod, getRandomPodStatus())
// Call syncBatch directly to test reconcile
syncer.syncBatch() // The apiStatusVersions should be set now

originalStatus := testPod.Status
podStatus, ok := syncer.GetPodStatus(testPod.UID)
if !ok {
t.Fatal("Should find pod status for pod: %+v", testPod)
Expand Down Expand Up @@ -698,9 +705,6 @@ func TestReconcilePodStatus(t *testing.T) {
testclient.GetActionImpl{ActionImpl: testclient.ActionImpl{Verb: "get", Resource: "pods"}},
testclient.UpdateActionImpl{ActionImpl: testclient.ActionImpl{Verb: "update", Resource: "pods", Subresource: "status"}},
})

// Just in case that testPod is shared among different test functions, set it back.
testPod.Status = originalStatus
}

func expectPodStatus(t *testing.T, m *manager, pod *api.Pod) api.PodStatus {
Expand Down