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

CRI changes to support in-place pod resize #111645

Merged
merged 3 commits into from Aug 3, 2022
Merged

CRI changes to support in-place pod resize #111645

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0ef263c
CRI changes to support implementation of in-place pod resize.
vinaykul Aug 2, 2022
007d93a
Handle UpdateContainerResources for Windows in v1alpha2
vinaykul Aug 2, 2022
09fb5da
CRI changes to support implementation of in-place pod resize (generat…
vinaykul Aug 2, 2022
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
8 changes: 5 additions & 3 deletions pkg/kubelet/cm/cpumanager/cpu_manager.go
View file
Open in desktop
Expand Up @@ -42,7 +42,7 @@ import (
type ActivePodsFunc func() []*v1.Pod

type runtimeService interface {
UpdateContainerResources(id string, resources *runtimeapi.LinuxContainerResources) error
Copy link
Contributor

Choose a reason for hiding this comment

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

not a blocker but rather just to confirm, changing this parameter from LinuxContainerResources to ContainerResources could be decoupled from the CRI changes right? remote_runtime would just need to call UpdateContainerResources with LinuxContainerResources directly (windows would not be supported tho).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does runtime call this? IIUC, runtime responds to it, and yes it does not have to change. The UpdateContainerResourcesRequest.Linux field will contain what it needs same as before. Windows runtime looks at UpdateContainerResourcesRequest.Windows.

Copy link
Member

@mikebrow mikebrow Aug 2, 2022 •
edited

Choose a reason for hiding this comment

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

because of the way the changes went in (not changing the CRI api except for the additional status field and some additional context) the client side manager proto should only impact cri-tools implementing the manager and container runtimes doing integration by implementing a manager for test

Copy link
Member

@mikebrow mikebrow Aug 2, 2022 •
edited

Choose a reason for hiding this comment

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

I'd be good with decoupling the client side manager changes as well.. if desired..

Copy link
Member

@mikebrow mikebrow Aug 2, 2022 •
edited

Choose a reason for hiding this comment

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

noting windows was there in the request before, but was not being used:

message UpdateContainerResourcesRequest {
     // ID of the container to update.
     string container_id = 1;
    // Resource configuration specific to Linux containers.
    LinuxContainerResources linux = 2;
    // Resource configuration specific to Windows containers.
    WindowsContainerResources windows = 3;
    // Unstructured key-value map holding arbitrary additional information for
    // container resources updating. This can be used for specifying experimental
    // resources to update or other options to use when updating the container.
    map<string, string> annotations = 4;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, runtime responds to it.

That I understand. I was just trying to confirm the parameters here did not need to change.

the client side manager proto should only impact cri-tools implementing the manager and container runtimes doing integration by implementing a manager for test

Gotcha. Yeah in that case this should have very little risk. I am okay with leaving this as the current PR is.

UpdateContainerResources(id string, resources *runtimeapi.ContainerResources) error
}

type policyName string
Expand Down Expand Up @@ -515,8 +515,10 @@ func (m *manager) updateContainerCPUSet(containerID string, cpus cpuset.CPUSet)
// this patch-like partial resources.
return m.containerRuntime.UpdateContainerResources(
containerID,
&runtimeapi.LinuxContainerResources{
CpusetCpus: cpus.String(),
&runtimeapi.ContainerResources{
Copy link
Member

Choose a reason for hiding this comment

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

note to self: need to carry these param changes forward to the main/containerd/containerd/integration bucket for local testing

Linux: &runtimeapi.LinuxContainerResources{
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a Windows field in here too for completeness?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mark, thanks for the quick response. Does Windows have an equivalent for Cpuset? Unless I missed something, I don't see an equivalent in WindowsContainerResources.
In any case, we are less than 2 hours away from code freeze and I don't want to reset CI unless I have to. If this can come in with changes for adding windows support in or before beta, lets do it at that point.

Copy link
Contributor

Choose a reason for hiding this comment

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

There isn't an equivalent for cpuset on Windows (today)

CpusetCpus: cpus.String(),
},
})
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/kubelet/cm/cpumanager/cpu_manager_test.go
View file
Open in desktop
Expand Up @@ -128,7 +128,7 @@ type mockRuntimeService struct {
err error
}

func (rt mockRuntimeService) UpdateContainerResources(id string, resources *runtimeapi.LinuxContainerResources) error {
func (rt mockRuntimeService) UpdateContainerResources(id string, resources *runtimeapi.ContainerResources) error {
return rt.err
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/kubelet/cm/memorymanager/memory_manager.go
View file
Open in desktop
Expand Up @@ -43,7 +43,7 @@ const memoryManagerStateFileName = "memory_manager_state"
type ActivePodsFunc func() []*v1.Pod

type runtimeService interface {
UpdateContainerResources(id string, resources *runtimeapi.LinuxContainerResources) error
UpdateContainerResources(id string, resources *runtimeapi.ContainerResources) error
}

type sourcesReadyStub struct{}
Expand Down
2 changes: 1 addition & 1 deletion pkg/kubelet/cm/memorymanager/memory_manager_test.go
View file
Open in desktop
Expand Up @@ -122,7 +122,7 @@ type mockRuntimeService struct {
err error
}

func (rt mockRuntimeService) UpdateContainerResources(id string, resources *runtimeapi.LinuxContainerResources) error {
func (rt mockRuntimeService) UpdateContainerResources(id string, resources *runtimeapi.ContainerResources) error {
return rt.err
}

Expand Down
4 changes: 4 additions & 0 deletions pkg/kubelet/cri/remote/conversion.go
View file
Open in desktop
Expand Up @@ -113,6 +113,10 @@ func v1alpha2LinuxContainerResources(from *runtimeapi.LinuxContainerResources) *
return (*v1alpha2.LinuxContainerResources)(unsafe.Pointer(from))
}

func v1alpha2WindowsContainerResources(from *runtimeapi.WindowsContainerResources) *v1alpha2.WindowsContainerResources {
return (*v1alpha2.WindowsContainerResources)(unsafe.Pointer(from))
}

func v1alpha2ExecRequest(from *runtimeapi.ExecRequest) *v1alpha2.ExecRequest {
// If this function changes, also adapt the corresponding Exec code in
// pkg/kubelet/cri/remote/remote_runtime.go
Expand Down
2 changes: 1 addition & 1 deletion pkg/kubelet/cri/remote/fake/fake_runtime.go
View file
Open in desktop
Expand Up @@ -305,7 +305,7 @@ func (f *RemoteRuntime) Status(ctx context.Context, req *kubeapi.StatusRequest)

// UpdateContainerResources updates ContainerConfig of the container.
func (f *RemoteRuntime) UpdateContainerResources(ctx context.Context, req *kubeapi.UpdateContainerResourcesRequest) (*kubeapi.UpdateContainerResourcesResponse, error) {
err := f.RuntimeService.UpdateContainerResources(req.ContainerId, req.Linux)
err := f.RuntimeService.UpdateContainerResources(req.ContainerId, &kubeapi.ContainerResources{Linux: req.Linux})
if err != nil {
return nil, err
}
Expand Down
8 changes: 5 additions & 3 deletions pkg/kubelet/cri/remote/remote_runtime.go
View file
Open in desktop
Expand Up @@ -641,20 +641,22 @@ func (r *remoteRuntimeService) containerStatusV1(ctx context.Context, containerI
}

// UpdateContainerResources updates a containers resource config
func (r *remoteRuntimeService) UpdateContainerResources(containerID string, resources *runtimeapi.LinuxContainerResources) (err error) {
func (r *remoteRuntimeService) UpdateContainerResources(containerID string, resources *runtimeapi.ContainerResources) (err error) {
klog.V(10).InfoS("[RemoteRuntimeService] UpdateContainerResources", "containerID", containerID, "timeout", r.timeout)
ctx, cancel := getContextWithTimeout(r.timeout)
defer cancel()

if r.useV1API() {
_, err = r.runtimeClient.UpdateContainerResources(ctx, &runtimeapi.UpdateContainerResourcesRequest{
ContainerId: containerID,
Linux: resources,
Linux: resources.GetLinux(),
Windows: resources.GetWindows(),
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks to me the ContainerResources object declared below is unused. I think we should specify Linux, Windows and ContainerResources{Linux, Windows} to allow for some backwards compatibliity for containerd (whose old version expect just the Linux and Windows fields, and not yet the ContainerResources field). Am I interpreting this right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC, originally I had a separate ContainerResources with Linux and windows - it is still in the KEP , but WindowsContainerResources was added to UCRR and made that unnecessary, and I removed it recently after @mikebrow spotted it Please see comment

Did I misunderstand you?

})
} else {
_, err = r.runtimeClientV1alpha2.UpdateContainerResources(ctx, &runtimeapiV1alpha2.UpdateContainerResourcesRequest{
ContainerId: containerID,
Linux: v1alpha2LinuxContainerResources(resources),
Linux: v1alpha2LinuxContainerResources(resources.GetLinux()),
Copy link
Contributor

Choose a reason for hiding this comment

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

We updated v1alpha2 with the new ContainerResources structure, so I think we should do the same we do above. If we don't update v1alpha2, then we can leave this as it is now IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, adding..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no conversion function for v1alpha2LinuxContainerResources.
Is it wise to just add an unsafe pointer conversion func? I don't have a way to test/verify.
I'm not very sure about this below. Does it look right to you?

diff --git a/pkg/kubelet/cri/remote/conversion.go b/pkg/kubelet/cri/remote/conversion.go
index 35539397755..120b718cbf5 100644
--- a/pkg/kubelet/cri/remote/conversion.go
+++ b/pkg/kubelet/cri/remote/conversion.go
@@ -113,6 +113,10 @@ func v1alpha2LinuxContainerResources(from *runtimeapi.LinuxContainerResources) *
        return (*v1alpha2.LinuxContainerResources)(unsafe.Pointer(from))
 }
 
+func v1alpha2WindowsContainerResources(from *runtimeapi.WindowsContainerResources) *v1alpha2.WindowsContainerResources {
+       return (*v1alpha2.WindowsContainerResources)(unsafe.Pointer(from))
+}
+
 func v1alpha2ExecRequest(from *runtimeapi.ExecRequest) *v1alpha2.ExecRequest {
        // If this function changes, also adapt the corresponding Exec code in
        // pkg/kubelet/cri/remote/remote_runtime.go
diff --git a/pkg/kubelet/cri/remote/remote_runtime.go b/pkg/kubelet/cri/remote/remote_runtime.go
index a09879b346a..976f845e011 100644
--- a/pkg/kubelet/cri/remote/remote_runtime.go
+++ b/pkg/kubelet/cri/remote/remote_runtime.go
@@ -639,6 +639,7 @@ func (r *remoteRuntimeService) UpdateContainerResources(containerID string, reso
                _, err = r.runtimeClientV1alpha2.UpdateContainerResources(ctx, &runtimeapiV1alpha2.UpdateContainerResourcesRequest{
                        ContainerId: containerID,
                        Linux:       v1alpha2LinuxContainerResources(resources.GetLinux()),
+                       Windows:     v1alpha2WindowsContainerResources(resources.GetWindows()),
                })
        }
        if err != nil {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps @marosset can comment if this is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah that looks correct. this could also take the approach of other recent CRI changes and just drop v1alpha2 support

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SG. Adding with rebase fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like containerd has had support updating Windows resources in UpdateContainerResources since the 1.6 release (which I'm pretty sure still used v1alpha2).
Because of this I think we should include the Windows changes in v1alpha2 and v1.
x-ref containerd/containerd#5778

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually the changes I just linked use the v1 API.
I don't think we need to support v1alpha2 but since the changes are already in might as well leave them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can remove it with the main feature changes in 1.26. I've added a TODO tracking item for this.

Windows: v1alpha2WindowsContainerResources(resources.GetWindows()),
})
}
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion pkg/kubelet/kuberuntime/instrumented_services.go
View file
Open in desktop
Expand Up @@ -131,7 +131,7 @@ func (in instrumentedRuntimeService) ContainerStatus(containerID string, verbose
return out, err
}

func (in instrumentedRuntimeService) UpdateContainerResources(containerID string, resources *runtimeapi.LinuxContainerResources) error {
func (in instrumentedRuntimeService) UpdateContainerResources(containerID string, resources *runtimeapi.ContainerResources) error {
const operation = "update_container"
defer recordOperation(operation, time.Now())

Expand Down