From 9077603b976230ea384d61544d7b18b4e6dd9543 Mon Sep 17 00:00:00 2001 From: mattjmcnaughton Date: Mon, 30 Dec 2019 10:30:20 -0500 Subject: [PATCH] Add richer unit tests for OomWatcher Add unit tests for OomWatcher that actually test the logic defined in the `Start` method. As a result of an earlier refactor, its now trivial to mock the OOMInstance events which the `oom_watcher` is supposed to be watching. --- pkg/kubelet/oom/oom_watcher_linux.go | 7 +- pkg/kubelet/oom/oom_watcher_linux_test.go | 147 +++++++++++++++++++++- 2 files changed, 146 insertions(+), 8 deletions(-) diff --git a/pkg/kubelet/oom/oom_watcher_linux.go b/pkg/kubelet/oom/oom_watcher_linux.go index 11724ad592f..698da28e5db 100644 --- a/pkg/kubelet/oom/oom_watcher_linux.go +++ b/pkg/kubelet/oom/oom_watcher_linux.go @@ -58,7 +58,10 @@ func NewWatcher(recorder record.EventRecorder) (Watcher, error) { return watcher, nil } -const systemOOMEvent = "SystemOOM" +const ( + systemOOMEvent = "SystemOOM" + recordEventContainerName = "/" +) // Start watches for system oom's and records an event for every system oom encountered. func (ow *realWatcher) Start(ref *v1.ObjectReference) error { @@ -69,7 +72,7 @@ func (ow *realWatcher) Start(ref *v1.ObjectReference) error { defer runtime.HandleCrash() for event := range outStream { - if event.ContainerName == "/" { + if event.ContainerName == recordEventContainerName { klog.V(1).Infof("Got sys oom event: %v", event) eventMsg := "System OOM encountered" if event.ProcessName != "" && event.Pid != 0 { diff --git a/pkg/kubelet/oom/oom_watcher_linux_test.go b/pkg/kubelet/oom/oom_watcher_linux_test.go index 8411df37269..c91164e75ba 100644 --- a/pkg/kubelet/oom/oom_watcher_linux_test.go +++ b/pkg/kubelet/oom/oom_watcher_linux_test.go @@ -17,7 +17,9 @@ limitations under the License. package oom import ( + "fmt" "testing" + "time" v1 "k8s.io/api/core/v1" "k8s.io/client-go/tools/record" @@ -26,19 +28,152 @@ import ( "github.com/stretchr/testify/assert" ) -// TestBasic verifies that the OOMWatch works without error. -func TestBasic(t *testing.T) { +type fakeStreamer struct { + oomInstancesToStream []*oomparser.OomInstance +} + +func (fs *fakeStreamer) StreamOoms(outStream chan<- *oomparser.OomInstance) { + for _, oomInstance := range fs.oomInstancesToStream { + outStream <- oomInstance + } +} + +// TestStartingWatcher tests that the watcher, using the actual streamer +// and not the fake, starts successfully. +func TestStartingWatcher(t *testing.T) { fakeRecorder := &record.FakeRecorder{} node := &v1.ObjectReference{} - // TODO: Substitute this `oomStreamer` out for a fake, and then write - // more comprehensive unit tests of the actual behavior. - oomStreamer, err := oomparser.New() + oomWatcher, err := NewWatcher(fakeRecorder) assert.NoError(t, err) + assert.NoError(t, oomWatcher.Start(node)) +} + +// TestWatcherRecordsEventsForOomEvents ensures that our OomInstances coming +// from `StreamOoms` are translated into events in our recorder. +func TestWatcherRecordsEventsForOomEvents(t *testing.T) { + oomInstancesToStream := []*oomparser.OomInstance{ + { + Pid: 1000, + ProcessName: "fakeProcess", + TimeOfDeath: time.Now(), + ContainerName: recordEventContainerName, + VictimContainerName: "some-container", + }, + } + numExpectedOomEvents := len(oomInstancesToStream) + + fakeStreamer := &fakeStreamer{ + oomInstancesToStream: oomInstancesToStream, + } + + fakeRecorder := record.NewFakeRecorder(numExpectedOomEvents) + node := &v1.ObjectReference{} oomWatcher := &realWatcher{ recorder: fakeRecorder, - oomStreamer: oomStreamer, + oomStreamer: fakeStreamer, } assert.NoError(t, oomWatcher.Start(node)) + + eventsRecorded := getRecordedEvents(fakeRecorder, numExpectedOomEvents) + assert.Equal(t, numExpectedOomEvents, len(eventsRecorded)) +} + +func getRecordedEvents(fakeRecorder *record.FakeRecorder, numExpectedOomEvents int) []string { + eventsRecorded := []string{} + + select { + case event := <-fakeRecorder.Events: + eventsRecorded = append(eventsRecorded, event) + + if len(eventsRecorded) == numExpectedOomEvents { + break + } + case <-time.After(10 * time.Second): + break + } + + return eventsRecorded +} + +// TestWatcherRecordsEventsForOomEventsCorrectContainerName verifies that we +// only record OOM events when the container name is the one for which we want +// to record events (i.e. /). +func TestWatcherRecordsEventsForOomEventsCorrectContainerName(t *testing.T) { + // By "incorrect" container name, we mean a container name for which we + // don't want to record an oom event. + numOomEventsWithIncorrectContainerName := 1 + oomInstancesToStream := []*oomparser.OomInstance{ + { + Pid: 1000, + ProcessName: "fakeProcess", + TimeOfDeath: time.Now(), + ContainerName: recordEventContainerName, + VictimContainerName: "some-container", + }, + { + Pid: 1000, + ProcessName: "fakeProcess", + TimeOfDeath: time.Now(), + ContainerName: "/dont-record-oom-event", + VictimContainerName: "some-container", + }, + } + numExpectedOomEvents := len(oomInstancesToStream) - numOomEventsWithIncorrectContainerName + + fakeStreamer := &fakeStreamer{ + oomInstancesToStream: oomInstancesToStream, + } + + fakeRecorder := record.NewFakeRecorder(numExpectedOomEvents) + node := &v1.ObjectReference{} + + oomWatcher := &realWatcher{ + recorder: fakeRecorder, + oomStreamer: fakeStreamer, + } + assert.NoError(t, oomWatcher.Start(node)) + + eventsRecorded := getRecordedEvents(fakeRecorder, numExpectedOomEvents) + assert.Equal(t, numExpectedOomEvents, len(eventsRecorded)) +} + +// TestWatcherRecordsEventsForOomEventsWithAdditionalInfo verifies that our the +// emitted event has the proper pid/process data when appropriate. +func TestWatcherRecordsEventsForOomEventsWithAdditionalInfo(t *testing.T) { + // The process and event info should appear in the event message. + eventPid := 1000 + processName := "fakeProcess" + + oomInstancesToStream := []*oomparser.OomInstance{ + { + Pid: eventPid, + ProcessName: processName, + TimeOfDeath: time.Now(), + ContainerName: recordEventContainerName, + VictimContainerName: "some-container", + }, + } + numExpectedOomEvents := len(oomInstancesToStream) + + fakeStreamer := &fakeStreamer{ + oomInstancesToStream: oomInstancesToStream, + } + + fakeRecorder := record.NewFakeRecorder(numExpectedOomEvents) + node := &v1.ObjectReference{} + + oomWatcher := &realWatcher{ + recorder: fakeRecorder, + oomStreamer: fakeStreamer, + } + assert.NoError(t, oomWatcher.Start(node)) + + eventsRecorded := getRecordedEvents(fakeRecorder, numExpectedOomEvents) + + assert.Equal(t, numExpectedOomEvents, len(eventsRecorded)) + assert.Contains(t, eventsRecorded[0], systemOOMEvent) + assert.Contains(t, eventsRecorded[0], fmt.Sprintf("pid: %d", eventPid)) + assert.Contains(t, eventsRecorded[0], fmt.Sprintf("victim process: %s", processName)) }