Merge pull request #90216 from DataDog/nayef/fix-container-statuses-race

Avoid overwriting podStatus ContainerStatuses in convertToAPIContainerStatuses

pkg/kubelet/kubelet_pods.go

@@ -1695,11 +1695,15 @@ func (kl *Kubelet) convertToAPIContainerStatuses(pod *v1.Pod, podStatus *kubecon
 		statuses[container.Name] = status
 	}
 
+	// Copy the slice before sorting it
+	containerStatusesCopy := make([]*kubecontainer.Status, len(podStatus.ContainerStatuses))
+	copy(containerStatusesCopy, podStatus.ContainerStatuses)
+
 	// Make the latest container status comes first.
-	sort.Sort(sort.Reverse(kubecontainer.SortContainerStatusesByCreationTime(podStatus.ContainerStatuses)))
+	sort.Sort(sort.Reverse(kubecontainer.SortContainerStatusesByCreationTime(containerStatusesCopy)))
 	// Set container statuses according to the statuses seen in pod status
 	containerSeen := map[string]int{}
-	for _, cStatus := range podStatus.ContainerStatuses {
+	for _, cStatus := range containerStatusesCopy {
 		cName := cStatus.Name
 		if _, ok := statuses[cName]; !ok {
 			// This would also ignore the infra container.

pkg/kubelet/kubelet_pods_test.go

@@ -26,6 +26,7 @@ import (
 	"reflect"
 	"sort"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -3220,3 +3221,33 @@ func TestSortPodIPs(t *testing.T) {
 		})
 	}
 }
+
+func TestConvertToAPIContainerStatusesDataRace(t *testing.T) {
+	pod := podWithUIDNameNs("12345", "test-pod", "test-namespace")
+
+	testTimestamp := time.Unix(123456789, 987654321)
+
+	criStatus := &kubecontainer.PodStatus{
+		ID:        pod.UID,
+		Name:      pod.Name,
+		Namespace: pod.Namespace,
+		ContainerStatuses: []*kubecontainer.Status{
+			{Name: "containerA", CreatedAt: testTimestamp},
+			{Name: "containerB", CreatedAt: testTimestamp.Add(1)},
+		},
+	}
+
+	testKubelet := newTestKubelet(t, false)
+	defer testKubelet.Cleanup()
+	kl := testKubelet.kubelet
+
+	// convertToAPIContainerStatuses is purely transformative and shouldn't alter the state of the kubelet
+	// as there are no synchronisation events in that function (no locks, no channels, ...) each test routine
+	// should have its own vector clock increased independently. Golang race detector uses pure happens-before
+	// detection, so would catch a race condition consistently, despite only spawning 2 goroutines
+	for i := 0; i < 2; i++ {
+		go func() {
+			kl.convertToAPIContainerStatuses(pod, criStatus, []v1.ContainerStatus{}, []v1.Container{}, false, false)
+		}()
+	}
+}