From 32794b18f06acb1a5adbbc616433209eef32f856 Mon Sep 17 00:00:00 2001 From: "Tim St. Clair" Date: Wed, 9 Dec 2015 10:20:57 -0800 Subject: [PATCH 1/2] Fix data race in prober test --- pkg/kubelet/prober/manager.go | 20 +++++++++++++++++--- pkg/kubelet/prober/manager_test.go | 1 + pkg/kubelet/prober/testing.go | 4 ++-- 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/pkg/kubelet/prober/manager.go b/pkg/kubelet/prober/manager.go index e6fa47dd2e1..bac1838b9cc 100644 --- a/pkg/kubelet/prober/manager.go +++ b/pkg/kubelet/prober/manager.go @@ -79,20 +79,34 @@ func NewManager( runner kubecontainer.ContainerCommandRunner, refManager *kubecontainer.RefManager, recorder record.EventRecorder) Manager { + m := newManager(statusManager, livenessManager, runner, refManager, recorder) + m.start() + return m +} + +// newManager is the internal version of NewManager for testing. +func newManager( + statusManager status.Manager, + livenessManager results.Manager, + runner kubecontainer.ContainerCommandRunner, + refManager *kubecontainer.RefManager, + recorder record.EventRecorder) *manager { + prober := newProber(runner, refManager, recorder) readinessManager := results.NewManager() - m := &manager{ + return &manager{ statusManager: statusManager, prober: prober, readinessManager: readinessManager, livenessManager: livenessManager, workers: make(map[probeKey]*worker), } +} +// start syncing probe status. +func (m *manager) start() { // Start syncing readiness. go util.Forever(m.updateReadiness, 0) - - return m } // Key uniquely identifying container probes diff --git a/pkg/kubelet/prober/manager_test.go b/pkg/kubelet/prober/manager_test.go index de9ccd1cc78..f97899a0793 100644 --- a/pkg/kubelet/prober/manager_test.go +++ b/pkg/kubelet/prober/manager_test.go @@ -251,6 +251,7 @@ func TestUpdatePodStatus(t *testing.T) { func TestUpdateReadiness(t *testing.T) { testPod := getTestPod(readiness, api.Probe{}) m := newTestManager() + m.start() m.statusManager.SetPodStatus(&testPod, getTestRunningStatus()) m.AddPod(&testPod) diff --git a/pkg/kubelet/prober/testing.go b/pkg/kubelet/prober/testing.go index fbdc77ffbda..7f1ed587694 100644 --- a/pkg/kubelet/prober/testing.go +++ b/pkg/kubelet/prober/testing.go @@ -95,13 +95,13 @@ func getTestPod(probeType probeType, probeSpec api.Probe) api.Pod { func newTestManager() *manager { refManager := kubecontainer.NewRefManager() refManager.SetRef(testContainerID, &api.ObjectReference{}) // Suppress prober warnings. - m := NewManager( + m := newManager( status.NewManager(&testclient.Fake{}, kubepod.NewBasicPodManager(nil)), results.NewManager(), nil, // runner refManager, &record.FakeRecorder{}, - ).(*manager) + ) // Don't actually execute probes. m.prober.exec = fakeExecProber{probe.Success, nil} return m From 246442514c71095bf227e9f515e0765cb68c251f Mon Sep 17 00:00:00 2001 From: "Tim St. Clair" Date: Wed, 9 Dec 2015 10:58:15 -0800 Subject: [PATCH 2/2] Expose Start method, don't call in constructor --- pkg/kubelet/kubelet.go | 3 ++- pkg/kubelet/prober/fake_manager.go | 1 + pkg/kubelet/prober/manager.go | 19 +++++-------------- pkg/kubelet/prober/manager_test.go | 2 +- pkg/kubelet/prober/testing.go | 4 ++-- 5 files changed, 11 insertions(+), 18 deletions(-) diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 7d3954bf28a..e6cf4bd8c1c 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -918,8 +918,9 @@ func (kl *Kubelet) Run(updates <-chan kubetypes.PodUpdate) { // handled by pod workers). go util.Until(kl.podKiller, 1*time.Second, util.NeverStop) - // Run the system oom watcher forever. + // Start component sync loops. kl.statusManager.Start() + kl.probeManager.Start() // Start the pod lifecycle event generator. kl.pleg.Start() kl.syncLoop(updates, kl) diff --git a/pkg/kubelet/prober/fake_manager.go b/pkg/kubelet/prober/fake_manager.go index 65742823cb2..a055c0056b0 100644 --- a/pkg/kubelet/prober/fake_manager.go +++ b/pkg/kubelet/prober/fake_manager.go @@ -29,6 +29,7 @@ var _ Manager = FakeManager{} func (_ FakeManager) AddPod(_ *api.Pod) {} func (_ FakeManager) RemovePod(_ *api.Pod) {} func (_ FakeManager) CleanupPods(_ []*api.Pod) {} +func (_ FakeManager) Start() {} func (_ FakeManager) UpdatePodStatus(_ types.UID, podStatus *api.PodStatus) { for i := range podStatus.ContainerStatuses { diff --git a/pkg/kubelet/prober/manager.go b/pkg/kubelet/prober/manager.go index bac1838b9cc..1c3c2498916 100644 --- a/pkg/kubelet/prober/manager.go +++ b/pkg/kubelet/prober/manager.go @@ -52,6 +52,9 @@ type Manager interface { // UpdatePodStatus modifies the given PodStatus with the appropriate Ready state for each // container based on container running status, cached probe results and worker states. UpdatePodStatus(types.UID, *api.PodStatus) + + // Start starts the Manager sync loops. + Start() } type manager struct { @@ -79,18 +82,6 @@ func NewManager( runner kubecontainer.ContainerCommandRunner, refManager *kubecontainer.RefManager, recorder record.EventRecorder) Manager { - m := newManager(statusManager, livenessManager, runner, refManager, recorder) - m.start() - return m -} - -// newManager is the internal version of NewManager for testing. -func newManager( - statusManager status.Manager, - livenessManager results.Manager, - runner kubecontainer.ContainerCommandRunner, - refManager *kubecontainer.RefManager, - recorder record.EventRecorder) *manager { prober := newProber(runner, refManager, recorder) readinessManager := results.NewManager() @@ -103,8 +94,8 @@ func newManager( } } -// start syncing probe status. -func (m *manager) start() { +// Start syncing probe status. This should only be called once. +func (m *manager) Start() { // Start syncing readiness. go util.Forever(m.updateReadiness, 0) } diff --git a/pkg/kubelet/prober/manager_test.go b/pkg/kubelet/prober/manager_test.go index f97899a0793..06a78d3e176 100644 --- a/pkg/kubelet/prober/manager_test.go +++ b/pkg/kubelet/prober/manager_test.go @@ -251,7 +251,7 @@ func TestUpdatePodStatus(t *testing.T) { func TestUpdateReadiness(t *testing.T) { testPod := getTestPod(readiness, api.Probe{}) m := newTestManager() - m.start() + m.Start() m.statusManager.SetPodStatus(&testPod, getTestRunningStatus()) m.AddPod(&testPod) diff --git a/pkg/kubelet/prober/testing.go b/pkg/kubelet/prober/testing.go index 7f1ed587694..fbdc77ffbda 100644 --- a/pkg/kubelet/prober/testing.go +++ b/pkg/kubelet/prober/testing.go @@ -95,13 +95,13 @@ func getTestPod(probeType probeType, probeSpec api.Probe) api.Pod { func newTestManager() *manager { refManager := kubecontainer.NewRefManager() refManager.SetRef(testContainerID, &api.ObjectReference{}) // Suppress prober warnings. - m := newManager( + m := NewManager( status.NewManager(&testclient.Fake{}, kubepod.NewBasicPodManager(nil)), results.NewManager(), nil, // runner refManager, &record.FakeRecorder{}, - ) + ).(*manager) // Don't actually execute probes. m.prober.exec = fakeExecProber{probe.Success, nil} return m