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.

Sign up for GitHub

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

Jump to bottom

Adding Windows support for InPlace Pod Vertical Scaling #112599

Merged
merged 2 commits into from
Oct 31, 2023
Merged

Adding Windows support for InPlace Pod Vertical Scaling #112599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2ad4304
Add Windows support for IPPVS
fabi200123 Apr 4, 2023
868adeb
Update IPPVS e2e tests for containerd version above 1.6.9
fabi200123 Jun 19, 2023
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
52 changes: 38 additions & 14 deletions pkg/kubelet/kuberuntime/kuberuntime_container_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,14 +260,25 @@ func TestToKubeContainerStatusWithResources(t *testing.T) {
State: runtimeapi.ContainerState_CONTAINER_RUNNING,
CreatedAt: createdAt,
StartedAt: startedAt,
Resources: &runtimeapi.ContainerResources{
Linux: &runtimeapi.LinuxContainerResources{
CpuQuota: 25000,
CpuPeriod: 100000,
MemoryLimitInBytes: 524288000,
OomScoreAdj: -998,
},
},
Resources: func() *runtimeapi.ContainerResources {
if goruntime.GOOS == "windows" {
return &runtimeapi.ContainerResources{
Windows: &runtimeapi.WindowsContainerResources{
CpuMaximum: 2500,
CpuCount: 1,
MemoryLimitInBytes: 524288000,
},
}
}
return &runtimeapi.ContainerResources{
Linux: &runtimeapi.LinuxContainerResources{
CpuQuota: 25000,
CpuPeriod: 100000,
MemoryLimitInBytes: 524288000,
OomScoreAdj: -998,
},
}
}(),
},
expected: &kubecontainer.Status{
ID: *cid,
Expand All @@ -289,12 +300,22 @@ func TestToKubeContainerStatusWithResources(t *testing.T) {
State: runtimeapi.ContainerState_CONTAINER_RUNNING,
CreatedAt: createdAt,
StartedAt: startedAt,
Resources: &runtimeapi.ContainerResources{
Linux: &runtimeapi.LinuxContainerResources{
CpuQuota: 50000,
CpuPeriod: 100000,
},
},
Resources: func() *runtimeapi.ContainerResources {
if goruntime.GOOS == "windows" {
return &runtimeapi.ContainerResources{
Windows: &runtimeapi.WindowsContainerResources{
CpuMaximum: 2500,
CpuCount: 2,
},
}
}
return &runtimeapi.ContainerResources{
Linux: &runtimeapi.LinuxContainerResources{
CpuQuota: 50000,
CpuPeriod: 100000,
},
}
}(),
},
expected: &kubecontainer.Status{
ID: *cid,
Expand All @@ -320,6 +341,9 @@ func TestToKubeContainerStatusWithResources(t *testing.T) {
MemoryLimitInBytes: 524288000,
OomScoreAdj: -998,
},
Windows: &runtimeapi.WindowsContainerResources{
MemoryLimitInBytes: 524288000,
},
},
},
expected: &kubecontainer.Status{
Expand Down
74 changes: 55 additions & 19 deletions pkg/kubelet/kuberuntime/kuberuntime_container_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,19 +42,24 @@ func (m *kubeGenericRuntimeManager) applyPlatformSpecificContainerConfig(config

// generateContainerResources generates platform specific (windows) container resources config for runtime
func (m *kubeGenericRuntimeManager) generateContainerResources(pod *v1.Pod, container *v1.Container) *runtimeapi.ContainerResources {
//TODO: Add windows support
return nil
return &runtimeapi.ContainerResources{
Windows: m.generateWindowsContainerResources(pod, container),
}
}

// generateWindowsContainerConfig generates windows container config for kubelet runtime v1.
// Refer https://git.k8s.io/design-proposals-archive/node/cri-windows.md.
func (m *kubeGenericRuntimeManager) generateWindowsContainerConfig(container *v1.Container, pod *v1.Pod, uid *int64, username string) (*runtimeapi.WindowsContainerConfig, error) {
wc := &runtimeapi.WindowsContainerConfig{
Resources: &runtimeapi.WindowsContainerResources{},
SecurityContext: &runtimeapi.WindowsContainerSecurityContext{},
}
// generateWindowsContainerResources generates windows container resources config for runtime
func (m *kubeGenericRuntimeManager) generateWindowsContainerResources(pod *v1.Pod, container *v1.Container) *runtimeapi.WindowsContainerResources {
wcr := m.calculateWindowsResources(container.Resources.Limits.Cpu(), container.Resources.Limits.Memory())

return wcr
}

// calculateWindowsResources will create the windowsContainerResources type based on the provided CPU and memory resource requests, limits
func (m *kubeGenericRuntimeManager) calculateWindowsResources(cpuLimit, memoryLimit *resource.Quantity) *runtimeapi.WindowsContainerResources {
resources := runtimeapi.WindowsContainerResources{}

memLimit := memoryLimit.Value()

cpuLimit := container.Resources.Limits.Cpu()
if !cpuLimit.IsZero() {
// 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 @@ -79,22 +84,32 @@ 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()))
resources.CpuMaximum = calculateCPUMaximum(cpuLimit, int64(winstats.ProcessorCount()))
}

// The processor resource controls are mutually exclusive on
// Windows Server Containers, the order of precedence is
// CPUCount first, then CPUMaximum.
if wc.Resources.CpuCount > 0 {
if wc.Resources.CpuMaximum > 0 {
wc.Resources.CpuMaximum = 0
if resources.CpuCount > 0 {
if resources.CpuMaximum > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we ever enter this block?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resources.CpuCount is never set, so L93 can't be True. This issue existed before this though. Do we have anything related to CPU count in the cpuLimit above that we can use?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The part with if wc.Resources.CpuMaximum > 0' was introduced here [#97141](https://github.com/kubernetes/kubernetes/pull/97141). Before that, the !isolatedByHyperv` was created in #85856 based on this issue #84804

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should untangle this as a separate change IMO

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG

resources.CpuMaximum = 0
klog.InfoS("Mutually exclusive options: CPUCount priority > CPUMaximum priority on Windows Server Containers. CPUMaximum should be ignored")
}
}

memoryLimit := container.Resources.Limits.Memory().Value()
if memoryLimit != 0 {
wc.Resources.MemoryLimitInBytes = memoryLimit
if memLimit != 0 {
resources.MemoryLimitInBytes = memLimit
}

return &resources
}

// generateWindowsContainerConfig generates windows container config for kubelet runtime v1.
// Refer https://github.com/kubernetes/community/blob/master/contributors/design-proposals/node/cri-windows.md.
func (m *kubeGenericRuntimeManager) generateWindowsContainerConfig(container *v1.Container, pod *v1.Pod, uid *int64, username string) (*runtimeapi.WindowsContainerConfig, error) {
wc := &runtimeapi.WindowsContainerConfig{
Resources: m.generateWindowsContainerResources(pod, container),
SecurityContext: &runtimeapi.WindowsContainerSecurityContext{},
}

// setup security context
Expand Down Expand Up @@ -134,6 +149,27 @@ func calculateCPUMaximum(cpuLimit *resource.Quantity, cpuCount int64) int64 {
}

func toKubeContainerResources(statusResources *runtimeapi.ContainerResources) *kubecontainer.ContainerResources {
//TODO: Add windows support
return nil
var cStatusResources *kubecontainer.ContainerResources
runtimeStatusResources := statusResources.GetWindows()
if runtimeStatusResources != nil {
var memLimit, cpuLimit *resource.Quantity

// Used the reversed formula from the calculateCPUMaximum function
if runtimeStatusResources.CpuMaximum > 0 {
cpuLimitValue := runtimeStatusResources.CpuMaximum * int64(winstats.ProcessorCount()) / 10
cpuLimit = resource.NewMilliQuantity(cpuLimitValue, resource.DecimalSI)
}

if runtimeStatusResources.MemoryLimitInBytes > 0 {
memLimit = resource.NewQuantity(runtimeStatusResources.MemoryLimitInBytes, resource.BinarySI)
}

if cpuLimit != nil || memLimit != nil {
cStatusResources = &kubecontainer.ContainerResources{
CPULimit: cpuLimit,
MemoryLimit: memLimit,
}
}
}
return cStatusResources
}
44 changes: 44 additions & 0 deletions pkg/kubelet/kuberuntime/kuberuntime_container_windows_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,3 +149,47 @@ func TestCalculateCPUMaximum(t *testing.T) {
})
}
}

func TestCalculateWindowsResources(t *testing.T) {
_, _, fakeRuntimeSvc, err := createTestRuntimeManager()
require.NoError(t, err)

tests := []struct {
name string
cpuLim resource.Quantity
memLim resource.Quantity
expected *runtimeapi.WindowsContainerResources
}{
{
name: "Request128MBLimit256MB",
cpuLim: resource.MustParse("2"),
memLim: resource.MustParse("128Mi"),
expected: &runtimeapi.WindowsContainerResources{
CpuMaximum: 2500,
MemoryLimitInBytes: 134217728,
},
},
{
name: "RequestNoMemory",
cpuLim: resource.MustParse("8"),
memLim: resource.MustParse("0"),
expected: &runtimeapi.WindowsContainerResources{
CpuMaximum: 10000,
MemoryLimitInBytes: 0,
},
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to add a test case requesting zero CPU limit for windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, I have added it

{
name: "RequestZeroCPU",
cpuLim: resource.MustParse("0"),
memLim: resource.MustParse("128Mi"),
expected: &runtimeapi.WindowsContainerResources{
CpuMaximum: 1,
MemoryLimitInBytes: 134217728,
},
},
}
for _, test := range tests {
windowsContainerResources := fakeRuntimeSvc.calculateWindowsResources(&test.cpuLim, &test.memLim)
assert.Equal(t, test.expected, windowsContainerResources)
}
}