Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

win: fix cpu count to calculate cpu_maximum #114231

Merged
merged 1 commit into from Jan 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 16 additions & 18 deletions pkg/kubelet/kuberuntime/kuberuntime_container_windows.go
Expand Up @@ -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"
)

Expand All @@ -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.

Expand All @@ -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.

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
wc.Resources.CpuMaximum = calculateCPUMaximum(cpuLimit, int64(winstats.ProcessorCount()))
}

// The processor resource controls are mutually exclusive on
Expand Down Expand Up @@ -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
mweibel marked this conversation as resolved.
Show resolved Hide resolved

// ensure cpuMaximum is in range [1, 10000].
if cpuMaximum < 1 {
cpuMaximum = 1
} else if cpuMaximum > 10000 {
cpuMaximum = 10000
}
return cpuMaximum
}
53 changes: 48 additions & 5 deletions pkg/kubelet/kuberuntime/kuberuntime_container_windows_test.go
Expand Up @@ -20,7 +20,6 @@ limitations under the License.
package kuberuntime

import (
"runtime"
"testing"

"github.com/stretchr/testify/assert"
Expand All @@ -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) {
Expand Down Expand Up @@ -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,
Expand All @@ -101,3 +99,48 @@ func TestApplyPlatformSpecificContainerConfig(t *testing.T) {
}
assert.Equal(t, expectedWindowsConfig, containerConfig.Windows)
}

func TestCalculateCPUMaximum(t *testing.T) {
mweibel marked this conversation as resolved.
Show resolved Hide resolved
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))
})
}
}
4 changes: 2 additions & 2 deletions pkg/kubelet/winstats/perfcounter_nodestats.go
Expand Up @@ -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,
Expand All @@ -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)
}
Expand Down