Support Kubernetes extended resources for container components through attributes that override container and pod specification

[l0rd]

/area api

Which functionality do you think we should add?

Why is this needed? Is your feature request related to a problem?

Devfiles don't allow to specify request/limits for resources other than memory and CPU. But there are cases where developers workloads require extended resources (e.g. nvidia.com/gpu).

Detailed description:

Kubernetes nodes can be assigned with extended resources (besides memory and CPU) and workloads can request and limit the use of those resources.

For example NVIDIA GPU operator advertises nvidia.com/gpu resources on the cluster nodes so that a Pod that requires them can specify the request and limit quantities:

  apiVersion: v1
  kind: Pod
  metadata:
    name: python
  spec:
    containers:
      - name: python
        image: tensorflow/tensorflow:latest-gpu-jupyter
        resources:
          limits:
            memory: 4Gi
            nvidia.com/gpu: 1 # limiting to 1 GPU
          requests:
            nvidia.com/gpu: 1 # requesting 1 GPU

Describe the solution you'd like

Use devfile and components attributes to override pod and container spec:

JSON container overrides

schemaVersion: 2.1.0
metadata:
    name: python
components:
  - name: python
+   attributes:
+     container-overrides: {"resources": {"limits": {"nvidia.com/gpu": 1"}}}
    container:
      image: tensorflow/tensorflow:latest-gpu-jupyter
      (...)

YAML container overrides

schemaVersion: 2.1.0
metadata:
    name: python
components:
  - name: python
+   attributes:
+     container-overrides:
+       resources:
+         limits:
+           nvidia.com/gpu: 1
    container:
      image: tensorflow/tensorflow:latest-gpu-jupyter
      (...)

pod overrides at devfile attributes level (both YAML and JSON are supported)

schemaVersion: 2.1.0
metadata:
    name: python
+ attributes:
+   pod-overrides: {"spec": {"securityContext": {"allowPrivilegeEscalation": false}}}
components:
  - name: python
    container:
      image: tensorflow/tensorflow:latest-gpu-jupyter
      (...)

pod overrides at container attributes level (both YAML and JSON are supported)

schemaVersion: 2.1.0
metadata:
    name: python
components:
  - name: python
+   attributes:
+      pod-overrides: {"spec": {"schedulerName": "stork"}}
    container:
      image: tensorflow/tensorflow:latest-gpu-jupyter
      (...)

Additional context

The request comes from OpenShift Dev Spaces customers that were able to achieve that with devfile v1 kubernetes components. With devfile v2 kubernetes components run in their own pod and may not have access to the source code (that is mounted in the main pod).

[Javatar81]

A drawback of the approach to support only a specific subset of the resources field is that the Devfile spec needs to follow the evolution of the Kubernetes pod resources field specification. This wouldn't be a big issue if it was stable and clearly defined. However, the spec defines limits and requests as type Object (https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.24/#resourcerequirements-v1-core). Hence, the definition of limits and requests is open which is also demonstrated by the variety of fields interpreted by Kubernetes as documented here: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/. Examples are:

• spec.containers[].resources.limits.cpu
• spec.containers[].resources.limits.memory
• spec.containers[].resources.limits.hugepages-
• spec.containers[].resources.requests.cpu
• spec.containers[].resources.requests.memory
• spec.containers[].resources.requests.hugepages-

When it comes to gpus, the docs https://kubernetes.io/docs/tasks/manage-gpus/scheduling-gpus/ introduces .com/gpu (e.g. amd.com/gpu or nvidia.com/gpu) for device plugins.

I just wanted to point this out to discuss whether it might make sense to just pass-through all the fields under the resources field. The devfile spec could just follow the Kubernetes spec and define it as object as well to leave open what is inside.

[kadel]

This makes me again think about the purpose of the container component.
Do we want to cover everything possible configuration from Kubernetes container definition? It looks like we are slowly heading this way :-( And if this is not intentional, we should draw a line somewhere.

We already a several issues about adding more fields from k8s container definition to devfile container definition.

• Readiness probes for endpoints #168
• ServiceAccount for a container component #903
• imagePullPolicy of Always leads to unnecessary pulls against Docker Hub rate limit, spec should specify pull policy and possibly provide other options #89

I think that need to decide between the following options and stick to it:

• Define that Devfile component is a simplified version of the Kubernetes component and stop adding new fields to it, unless there is a really good reason and 80% of users will benefit from it. If someone wants to use more advanced features, they will have to use kubernetes component and provide a full definition. In this case will have to make sure that kubernetes components can be used as a replacement for containers in the rest fo the Devfile spec (for example in exec command)
• Define that Devfile component can be used as a replacement for Kubernetes component. In this case we should map all fields from Kubernetes component to Devfile component definition and do it in some systematic effort rather than doing it one by one.

[miboettc]

From a user perspective container component and kubernetes component are quite different, at least from an Eclipse Che perspective.

• container components run within the workspace pod. In the Devfile spec they have the mountSources flag which allows for easy access to the source code.
• kubernetes components do not have the mountSources flag (it was removed in the Devfile 2.x spec, but existed in Devfile 1.x). So there is no convenient way to access sources from a container. This means, the container could request GPUs but would not have access to any source code to execute on them.

Overall, I prefer the idea of @Javatar81 of making the definition of ressources in the container component more flexible. Being able to request GPUs would open Eclipse Che, odo and other tools to MLOps, ML Engineering use cases. So I assume there is a sufficiently large number of users who would benefit from such a feature.

[kadel]

Overall, I prefer the idea of @Javatar81 of making the definition of ressources in the container component more flexible. Being able to request GPUs would open Eclipse Che, odo and other tools to MLOps, ML Engineering use cases. So I assume there is a sufficiently large number of users who would benefit from such a feature.

I'm ok with that, that means that we will be taking the first approach that I mentioned in #920 (comment)

But that also means that we should add the other things like ServiceAccount, ImagePullPolicy, and another important one that just came up is SecurityContext (#923) to the container definition.

[l0rd]

After discussions during today's devfile cabal we agreed on:

• not including this into the devfile spec
• adopt a convention for tools like odo and che to use components attributes to override pods and containers specification.

Example n.1: add nvidia.com/gpu: 1 to a container resources requirements limits

components:
  - name: go
+   attributes:
+     container-overrides: {"resources": {"limits": "nvidia.com/gpu": 1"}}
    container:
      image: ...

Example n.2: specify the pod securityContext

components:
  - name: go
+   attributes:
+     pod-overrides: {"spec":{"securityContext":{"runAsUser":1000,"runAsGroup":3000,"fsGroup":2000}}}
    container:
      image: ...
   ...

[miboettc]

This sounds like a very flexible solution to address the problem. :-)

Would this be part of the Devfile 2.2 spec?

How does it relate to #860 and devfile/devworkspace-operator#852 ?

[amisevsk]

I've opened DevWorkspace Operator PR devfile/devworkspace-operator#944 that implements support for the pod-overrides attribute as described above. Any comments are welcome.

Some additional assumptions this implementation makes, at the moment:

1. Pod overrides apply to the workspace deployment, but in the sample are defined on a container component. To allow specifying the attribute in either global attributes or on a component, I've defined an order of precedence for processing multiple instances of the attribute, with later definitions overriding earlier definitions
2. The format for the pod-overrides attribute matches the definition in Allow DevWorkspace CR to specify pod spec overrides #860, i.e. pod-overrides are specified either as yaml:

   components:
     - name: go
       attributes:
         pod-overrides: 
           spec:
             securityContext: 
               runAsUser: 1000
               runAsGroup: 3000
               fsGroup: 2000
       container:
         image: ...
     ...

   or as json (but not a string):

   components:
     - name: go
       attributes:
         pod-overrides: {"spec":{"securityContext":{"runAsUser":1000,"runAsGroup":3000,"fsGroup":2000}}}
       container:
         image: ...
     ...

   This is a departure from the sample as it's not possible to concatenate fields (spec.securityContext:). The definition for how to support the concatenated form is unclear -- e.g. how would we set annotation

   io.kubernetes.cri-o.userns-mode: "auto:size=65536;map-to-root=true"
   

   in pod-overrides?

[l0rd]

@amisevsk +1 to your proposal. I have updated the description and samples to reflect that.

@kadel hopefully this syntax works for you too.

@miboettc it won't be part of the spec. It's a convention to use free-form attributes to specify pod and container overrides. In other words it will be implented earlier (without change to the API) but there won't be syntax validation (the devfile will be considered correct even if the pod-spec is syntactically invalid). And we have closed #860 in favor of devfile/devworkspace-operator#944. And this will also address devfile/devworkspace-operator#852.

[amisevsk]

To add, devfile/devworkspace-operator#944 has been updated to support container-overrides via attribute on container components as well, in a similar way to pod-overrides. Fields that can be set through fields on container components are ignored (e.g. no env, endpoints, etc.) but this allows specifying e.g. securityContext for individual containers if required.

[kadel]

I have a few concerns about proposed json form.

Lets say that original pod spec contains something like this

spec:
  securityContext: 
    runAsUser: 1000
    runAsGroup: 3000
    fsGroup: 2000

and I want to change just the value of runAsUser to 1001
How would pod-overrides look like. I assume something like this

{"spec":{"securityContext":{"runAsUser":1001}}}

Now lets say that I want to change runAsUser and remove runAsGroup and fsGroup fields, essentially overriding while securityContext so it ends up looking like this

spec:
  securityContext: 
    runAsUser: 1001

How would I define this in this json notation? Wouldn't the pod-override look the same as in the previous example?

@amisevsk How would DevspaceOperator interpret pod-override: {"spec":{"securityContext":{"runAsUser":1001}}} if the original Pod had the following structure already in it?

spec:
  securityContext: 
    runAsUser: 1000
    runAsGroup: 3000
    fsGroup: 2000

I liked the original syntax that @l0rd had in #920 (comment) before.
As I understood that the overrides for two cases about would be

["spec.securityContext.runAsUser: 1001"]

and

["spec.securityContext: {'runAsUser': 1001}"]

[amisevsk]

The DevWorkspace Operator currently treats the values defined in pod-overrides and container-overrides as a strategic merge patch on top of the default pod spec. The JSON form described is what's required to be able to parse fields effectively -- taking the approach of using something like

["spec.securityContext.runAsUser: 1001"]

it would mean each implementation would require ad-hoc handling of every single field (e.g. we'd need to parse out spec.securityContext.runAsUser and do the appropriate action for it). Supporting formats like

["spec.securityContext.runAsUser: 1001"]

and

["spec.securityContext: {'runAsUser': 1001}"]

would require writing a custom parser that understands the Kubernetes pod spec API, which would lead to more confusion and be harder to use than the well-defined Kubernetes strategic patch format.

To support not merging fields and instead replacing, Kubernetes strategic merge supports directives like $patch: replace. Currently these are not supported in DWO but support could be added there as well, though I worry that we're focusing on supporting 1% of cases at the detriment of the other 99%.

[amisevsk]

PR devfile/devworkspace-operator#967 adds support for strategic patch directives to DWO (see doc)

[kadel]

To support not merging fields and instead replacing, Kubernetes strategic merge supports directives like $patch: replace. Currently these are not supported in DWO but support could be added there as well, though I worry that we're focusing on supporting 1% of cases at the detriment of the other 99%.

Ok makes sense. I was not aware that this is intended to work as Strategic Merge Patch.
My main concern was that we don't have a clear definition of how overrides should be interpreted. If we are saying that it will be Strategic Merge Patch, then it is ok.

We need to make sure that this is properly documented, so users know what to expect.

[valaparthvi]

What happens when pod-overrides is defined at both global and component level?
DWO's implementation applies merge patch from the component level first and then the global one. Shouldn't only one level be applied; component level perhaps since it is local level preference?

[amisevsk]

There's no "local level" since pod overrides apply to the pod that holds all containers -- pod-overrides applied in a container component are also applied equally to other containers. The idea behind merging multiple pod-overrides fields is to a) allow pod-overrides to be defined where it makes sense (e.g. this container requires a different security context) and b) make importing/copying components from other devfiles easier (e.g. I can copy this component to have a container that can build images) while still having a top-level field which makes sense to use.

Pod overrides in DWO are applied first in the order they appear within components, then from the top-level attribute, with later values merging over earlier ones.

[l0rd]

Closing as complete