Fix issue for HPC pod metrics

jsturtevant · jsturtevant · commit 89415fe36162 · 2023-06-02T16:29:26.000-07:00
The initial PR had a check for nil metrics but after some refactoring in the PR the test case that was suppose cover HPC was missing a scenario where the metric was not nil but didn't contain any metrics. This fixes that case and adds a testcase to cover it.

Signed-off-by: James Sturtevant &lt;jstur@microsoft.com&gt;
diff --git a/pkg/cri/sbserver/sandbox_stats_windows.go b/pkg/cri/sbserver/sandbox_stats_windows.go
@@ -267,7 +267,9 @@ func (c *criService) listWindowsMetricsForSandbox(ctx context.Context, sandbox s
 
 func (c *criService) convertToCRIStats(stats *wstats.Statistics) (*runtime.WindowsContainerStats, error) {
 	var cs runtime.WindowsContainerStats
-	if stats != nil {
+	// the metric should exist but stats or stats.container will be nil for HostProcess sandbox containers
+	// this can also be the case when the container has not started yet
+	if stats != nil && stats.Container != nil {
 		wstats := stats.GetWindows()
 		if wstats == nil {
 			return nil, fmt.Errorf("windows stats is empty")
diff --git a/pkg/cri/sbserver/sandbox_stats_windows_test.go b/pkg/cri/sbserver/sandbox_stats_windows_test.go
@@ -192,6 +192,37 @@ func Test_criService_podSandboxStats(t *testing.T) {
 			},
 			expectError: false,
 		},
+		"pod sandbox with empty stats still works (hostprocess container scenario)": {
+			metrics: map[string]*wstats.Statistics{
+				"c1": {
+					Container: windowsStat(currentStatsTimestamp, 400, 20),
+				},
+				"s1": {},
+			},
+			sandbox: sandboxPod("s1", initialStatsTimestamp, 200),
+			containers: []containerstore.Container{
+				{
+					Metadata: containerstore.Metadata{ID: "c1"},
+					Stats: &stats.ContainerStats{
+						Timestamp:            initialStatsTimestamp,
+						UsageCoreNanoSeconds: 200,
+					},
+				},
+			},
+			expectedPodStats: expectedStats{
+				UsageCoreNanoSeconds: 400,
+				UsageNanoCores:       200,
+				WorkingSetBytes:      20,
+			},
+			expectedContainerStats: []expectedStats{
+				{
+					UsageCoreNanoSeconds: 400,
+					UsageNanoCores:       200,
+					WorkingSetBytes:      20,
+				},
+			},
+			expectError: false,
+		},
 	} {
 		t.Run(desc, func(t *testing.T) {
 			actualPodStats, actualContainerStats, err := c.toPodSandboxStats(test.sandbox, test.metrics, test.containers, currentStatsTimestamp)
diff --git a/pkg/cri/server/sandbox_stats_windows.go b/pkg/cri/server/sandbox_stats_windows.go
@@ -266,7 +266,9 @@ func (c *criService) listWindowsMetricsForSandbox(ctx context.Context, sandbox s
 
 func (c *criService) convertToCRIStats(stats *wstats.Statistics) (*runtime.WindowsContainerStats, error) {
 	var cs runtime.WindowsContainerStats
-	if stats != nil {
+	// the metric should exist but stats or stats.container will be nil for HostProcess sandbox containers
+	// this can also be the case when the container has not started yet
+	if stats != nil && stats.Container != nil {
 		wstats := stats.GetWindows()
 		if wstats == nil {
 			return nil, fmt.Errorf("windows stats is empty")
diff --git a/pkg/cri/server/sandbox_stats_windows_test.go b/pkg/cri/server/sandbox_stats_windows_test.go
@@ -192,6 +192,37 @@ func Test_criService_podSandboxStats(t *testing.T) {
 			},
 			expectError: false,
 		},
+		"pod sandbox with empty stats still works (hostprocess container scenario)": {
+			metrics: map[string]*wstats.Statistics{
+				"c1": {
+					Container: windowsStat(currentStatsTimestamp, 400, 20),
+				},
+				"s1": {},
+			},
+			sandbox: sandboxPod("s1", initialStatsTimestamp, 200),
+			containers: []containerstore.Container{
+				{
+					Metadata: containerstore.Metadata{ID: "c1"},
+					Stats: &stats.ContainerStats{
+						Timestamp:            initialStatsTimestamp,
+						UsageCoreNanoSeconds: 200,
+					},
+				},
+			},
+			expectedPodStats: expectedStats{
+				UsageCoreNanoSeconds: 400,
+				UsageNanoCores:       200,
+				WorkingSetBytes:      20,
+			},
+			expectedContainerStats: []expectedStats{
+				{
+					UsageCoreNanoSeconds: 400,
+					UsageNanoCores:       200,
+					WorkingSetBytes:      20,
+				},
+			},
+			expectError: false,
+		},
 	} {
 		t.Run(desc, func(t *testing.T) {
 			actualPodStats, actualContainerStats, err := c.toPodSandboxStats(test.sandbox, test.metrics, test.containers, currentStatsTimestamp)
