Merge pull request #114231 from helio/fix-windows-cpu-maximum

win: fix cpu count to calculate cpu_maximum

pkg/kubelet/kuberuntime/kuberuntime_container_windows.go

@@ -20,12 +20,12 @@ limitations under the License.
 package kuberuntime
 
 import (
-	"runtime"
-
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1"
 	"k8s.io/klog/v2"
 	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
+	"k8s.io/kubernetes/pkg/kubelet/winstats"
 	"k8s.io/kubernetes/pkg/securitycontext"
 )
 
@@ -50,11 +50,6 @@ func (m *kubeGenericRuntimeManager) generateWindowsContainerConfig(container *v1
 
 	cpuLimit := container.Resources.Limits.Cpu()
 	if !cpuLimit.IsZero() {
-		// Note that sysinfo.NumCPU() is limited to 64 CPUs on Windows due to Processor Groups,
-		// as only 64 processors are available for execution by a given process. This causes
-		// some oddities on systems with more than 64 processors.
-		// Refer https://msdn.microsoft.com/en-us/library/windows/desktop/dd405503(v=vs.85).aspx.
-
 		// Since Kubernetes doesn't have any notion of weight in the Pod/Container API, only limits/reserves, then applying CpuMaximum only
 		// will better follow the intent of the user. At one point CpuWeights were set, but this prevented limits from having any effect.
 
@@ -78,17 +73,7 @@ func (m *kubeGenericRuntimeManager) generateWindowsContainerConfig(container *v1
 		//   https://github.com/kubernetes/kubernetes/blob/56d1c3b96d0a544130a82caad33dd57629b8a7f8/staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto#L681-L682
 		//   https://github.com/opencontainers/runtime-spec/blob/ad53dcdc39f1f7f7472b10aa0a45648fe4865496/config-windows.md#cpu
 		//   If both CpuWeight and CpuMaximum are set - ContainerD catches this invalid case and returns an error instead.
-
+		wc.Resources.CpuMaximum = calculateCPUMaximum(cpuLimit, int64(winstats.ProcessorCount()))
-		cpuMaximum := 10000 * cpuLimit.MilliValue() / int64(runtime.NumCPU()) / 1000
-
-		// ensure cpuMaximum is in range [1, 10000].
-		if cpuMaximum < 1 {
-			cpuMaximum = 1
-		} else if cpuMaximum > 10000 {
-			cpuMaximum = 10000
-		}
-
-		wc.Resources.CpuMaximum = cpuMaximum
 	}
 
 	// The processor resource controls are mutually exclusive on
@@ -128,3 +113,16 @@ func (m *kubeGenericRuntimeManager) generateWindowsContainerConfig(container *v1
 
 	return wc, nil
 }
+
+// calculateCPUMaximum calculates the maximum CPU given a limit and a number of cpus while ensuring it's in range [1,10000].
+func calculateCPUMaximum(cpuLimit *resource.Quantity, cpuCount int64) int64 {
+	cpuMaximum := 10 * cpuLimit.MilliValue() / cpuCount
+
+	// ensure cpuMaximum is in range [1, 10000].
+	if cpuMaximum < 1 {
+		cpuMaximum = 1
+	} else if cpuMaximum > 10000 {
+		cpuMaximum = 10000
+	}
+	return cpuMaximum
+}

pkg/kubelet/kuberuntime/kuberuntime_container_windows_test.go

@@ -20,7 +20,6 @@ limitations under the License.
 package kuberuntime
 
 import (
-	"runtime"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -29,10 +28,8 @@ import (
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	utilfeature "k8s.io/apiserver/pkg/util/feature"
-	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1"
-	"k8s.io/kubernetes/pkg/features"
+	"k8s.io/kubernetes/pkg/kubelet/winstats"
 )
 
 func TestApplyPlatformSpecificContainerConfig(t *testing.T) {
@@ -87,7 +84,8 @@ func TestApplyPlatformSpecificContainerConfig(t *testing.T) {
 	err = fakeRuntimeSvc.applyPlatformSpecificContainerConfig(containerConfig, &pod.Spec.Containers[0], pod, new(int64), "foo", nil)
 	require.NoError(t, err)
 
-	expectedCpuMax := ((10000 * 3000) / int64(runtime.NumCPU()) / 1000)
+	limit := int64(3000)
+	expectedCpuMax := 10 * limit / int64(winstats.ProcessorCount())
 	expectedWindowsConfig := &runtimeapi.WindowsContainerConfig{
 		Resources: &runtimeapi.WindowsContainerResources{
 			CpuMaximum:         expectedCpuMax,
@@ -101,3 +99,48 @@ func TestApplyPlatformSpecificContainerConfig(t *testing.T) {
 	}
 	assert.Equal(t, expectedWindowsConfig, containerConfig.Windows)
 }
+
+func TestCalculateCPUMaximum(t *testing.T) {
+	tests := []struct {
+		name     string
+		cpuLimit resource.Quantity
+		cpuCount int64
+		want     int64
+	}{
+		{
+			name:     "max range when same amount",
+			cpuLimit: resource.MustParse("1"),
+			cpuCount: 1,
+			want:     10000,
+		},
+		{
+			name:     "percentage calculation is working as intended",
+			cpuLimit: resource.MustParse("94"),
+			cpuCount: 96,
+			want:     9791,
+		},
+		{
+			name:     "half range when half amount",
+			cpuLimit: resource.MustParse("1"),
+			cpuCount: 2,
+			want:     5000,
+		},
+		{
+			name:     "max range when more requested than available",
+			cpuLimit: resource.MustParse("2"),
+			cpuCount: 1,
+			want:     10000,
+		},
+		{
+			name:     "min range when less than minimum",
+			cpuLimit: resource.MustParse("1m"),
+			cpuCount: 100,
+			want:     1,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, calculateCPUMaximum(&tt.cpuLimit, tt.cpuCount))
+		})
+	}
+}

pkg/kubelet/winstats/perfcounter_nodestats.go

@@ -157,7 +157,7 @@ func (p *perfCounterNodeStatsClient) getMachineInfo() (*cadvisorapi.MachineInfo,
 	}
 
 	return &cadvisorapi.MachineInfo{
-		NumCores:       processorCount(),
+		NumCores:       ProcessorCount(),
 		MemoryCapacity: p.nodeInfo.memoryPhysicalCapacityBytes,
 		MachineID:      hostname,
 		SystemUUID:     systemUUID,
@@ -173,7 +173,7 @@ func (p *perfCounterNodeStatsClient) getMachineInfo() (*cadvisorapi.MachineInfo,
 // more notes for this issue:
 // same issue in moby: https://github.com/moby/moby/issues/38935#issuecomment-744638345
 // solution in hcsshim: https://github.com/microsoft/hcsshim/blob/master/internal/processorinfo/processor_count.go
-func processorCount() int {
+func ProcessorCount() int {
 	if amount := getActiveProcessorCount(allProcessorGroups); amount != 0 {
 		return int(amount)
 	}