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?

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?

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

yes, adding..

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 {

Perhaps @marosset can comment if this is needed.

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

SG. Adding with rebase fix.

fixed

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

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?

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

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).

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.

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

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

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;
}

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.

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

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

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.

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