From 738c4c6fa5198fe087fae831d71a1944cba8df7e Mon Sep 17 00:00:00 2001
From: James Sturtevant <jstur@microsoft.com>
Date: Thu, 1 Jun 2023 15:12:36 -0700
Subject: [PATCH] Fix issue for HPC pod metrics

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 <jstur@microsoft.com>
---
 pkg/cri/sbserver/sandbox_stats_windows.go     |  4 ++-
 .../sbserver/sandbox_stats_windows_test.go    | 32 +++++++++++++++++++
 pkg/cri/server/sandbox_stats_windows.go       |  4 ++-
 pkg/cri/server/sandbox_stats_windows_test.go  | 32 +++++++++++++++++++
 4 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/pkg/cri/sbserver/sandbox_stats_windows.go b/pkg/cri/sbserver/sandbox_stats_windows.go
index 88162a6f1..77f48d908 100644
--- 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
index ca88e0c6a..0b5c0ce24 100644
--- a/pkg/cri/sbserver/sandbox_stats_windows_test.go
+++ b/pkg/cri/sbserver/sandbox_stats_windows_test.go
@@ -200,6 +200,38 @@ func Test_criService_podSandboxStats(t *testing.T) {
 			},
 			expectError: false,
 		},
+		{
+			desc: "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,
+		},
 	} {
 		test := test
 		t.Run(test.desc, func(t *testing.T) {
diff --git a/pkg/cri/server/sandbox_stats_windows.go b/pkg/cri/server/sandbox_stats_windows.go
index 5d6b11193..5de4a4295 100644
--- 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
index 40bb82d7a..bba56c68e 100644
--- a/pkg/cri/server/sandbox_stats_windows_test.go
+++ b/pkg/cri/server/sandbox_stats_windows_test.go
@@ -200,6 +200,38 @@ func Test_criService_podSandboxStats(t *testing.T) {
 			},
 			expectError: false,
 		},
+		{
+			desc: "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,
+		},
 	} {
 		test := test
 		t.Run(test.desc, func(t *testing.T) {