Repository navigation
Support Kubernetes extended resources for container components through attributes that override container and pod specification #920
Description
/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 GPUDescribe 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).
Activity
Javatar81 commentedon Sep 7, 2022
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:
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 commentedon Sep 9, 2022
This makes me again think about the purpose of the
containercomponent.Do we want to cover everything possible configuration from Kubernetes
containerdefinition? 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.
I think that need to decide between the following options and stick to it:
componentis a simplified version of the Kubernetescomponentand 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 usekubernetescomponent and provide a full definition. In this case will have to make sure thatkubernetescomponents can be used as a replacement forcontainersin the rest fo the Devfile spec (for example inexeccommand)componentcan 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 commentedon Sep 12, 2022
From a user perspective
containercomponent andkubernetescomponent are quite different, at least from an Eclipse Che perspective.containercomponents run within the workspace pod. In the Devfile spec they have themountSourcesflag which allows for easy access to the source code.kubernetescomponents do not have themountSourcesflag (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
containercomponent 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 commentedon Sep 12, 2022
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 isSecurityContext(#923) to thecontainerdefinition.l0rd commentedon Sep 12, 2022
After discussions during today's devfile cabal we agreed on:
odoandcheto use components attributes to override pods and containers specification.Example n.1: add
nvidia.com/gpu: 1to a container resources requirements limitsExample n.2: specify the pod
securityContextmiboettc commentedon Sep 13, 2022
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 commentedon Oct 4, 2022
I've opened DevWorkspace Operator PR devfile/devworkspace-operator#944 that implements support for the
pod-overridesattribute as described above. Any comments are welcome.Some additional assumptions this implementation makes, at the moment:
pod-overridesattribute matches the definition in Allow DevWorkspace CR to specify pod spec overrides #860, i.e.pod-overridesare specified either as yaml:spec.securityContext:). The definition for how to support the concatenated form is unclear -- e.g. how would we set annotation[-]Support Kubernetes extended resources for container components[/-][+]Support Kubernetes extended resources for container components through attributes that override container and pod specification[/+]l0rd commentedon Oct 14, 2022
@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 commentedon Oct 14, 2022
To add, devfile/devworkspace-operator#944 has been updated to support
container-overridesvia attribute on container components as well, in a similar way topod-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.securityContextfor individual containers if required.kadel commentedon Nov 3, 2022
I have a few concerns about proposed json form.
Lets say that original pod spec contains something like this
and I want to change just the value of
runAsUserto1001How would
pod-overrideslook like. I assume something like this{"spec":{"securityContext":{"runAsUser":1001}}}Now lets say that I want to change
runAsUserand removerunAsGroupandfsGroupfields, essentially overriding whilesecurityContextso it ends up looking like thisHow would I define this in this json notation? Wouldn't the
pod-overridelook 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?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 commentedon Nov 3, 2022
The DevWorkspace Operator currently treats the values defined in
pod-overridesandcontainer-overridesas 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 likeit would mean each implementation would require ad-hoc handling of every single field (e.g. we'd need to parse out
spec.securityContext.runAsUserand do the appropriate action for it). Supporting formats likeand
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 commentedon Nov 3, 2022
PR devfile/devworkspace-operator#967 adds support for strategic patch directives to DWO (see doc)
kadel commentedon Nov 8, 2022
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 commentedon Nov 28, 2022
What happens when
pod-overridesis 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 commentedon Nov 28, 2022
There's no "local level" since pod overrides apply to the pod that holds all containers --
pod-overridesapplied 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 commentedon Jun 15, 2023
Closing as complete