Merge pull request #86728 from mattjmcnaughton/mattjmcnaughton/add-test-coverage-oom-watcher

Add test coverage for oom watcher

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 {

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))
 }