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.

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

Make every workspace containers and init containers configurable #22036

Closed
l0rd opened this issue Mar 6, 2023 · 9 comments
Closed

Make every workspace containers and init containers configurable #22036

l0rd opened this issue Mar 6, 2023 · 9 comments
Assignees
Labels
area/che-operator Issues and PRs related to Eclipse Che Kubernetes Operator area/devworkspace-operator kind/enhancement A feature request - must adhere to the feature request template. severity/P1 Has a major impact to usage or development of the system. sprint/current team/B This team is responsible for the Web Terminal, the DevWorkspace Operator and the IDEs.
Milestone

Comments

@l0rd
Copy link
Contributor

l0rd commented Mar 6, 2023

Is your enhancement related to a problem? Please describe

The spec of these workspace pod containers is not configurable:

  • gateway container
  • project-clone container

This can be annoying if we want to remove the CPULimit of these containers for example.

Describe the solution you'd like

Add the fields gatewayContainer and projectCloneContainer under CheCluster devEnvironments:

spec:
  devEnvironments:
    gatewayContainer:
        (...container settings...)
    projectCloneContainer:
        (...container settings...)
  • The container API is already defined in the CheCluster API.

  • Setting a container field empty should result in the property not being set. For example, to unset the gateway CPU limit (and keep the other container properties):

    spec:
      devEnvironments:
        gatewayContainer:
          resources:
            limits:
              cpu: '0'

Additional context

@l0rd l0rd added kind/enhancement A feature request - must adhere to the feature request template. area/che-operator Issues and PRs related to Eclipse Che Kubernetes Operator area/devworkspace-operator severity/P1 Has a major impact to usage or development of the system. labels Mar 6, 2023
@ibuziuk ibuziuk added sprint/next team/A This team is responsible for the Che Operator and all its operands as well as chectl and Hosted Che team/B This team is responsible for the Web Terminal, the DevWorkspace Operator and the IDEs. and removed team/A This team is responsible for the Che Operator and all its operands as well as chectl and Hosted Che labels Mar 6, 2023
@l0rd l0rd added the team/A This team is responsible for the Che Operator and all its operands as well as chectl and Hosted Che label Mar 8, 2023
@l0rd
Copy link
Contributor Author

l0rd commented Mar 8, 2023

The gateway changes should be Team A (as that's configured by Che operator), the projectClone changes should be Team B (as that's configured by DW operator).

@l0rd l0rd mentioned this issue Mar 8, 2023
7 tasks
@l0rd
Copy link
Contributor Author

l0rd commented Mar 9, 2023

@mkuznyetsov working on this issue, gateway side, may be a good task for DevTools Week (cc @ibuziuk @tolusha)

@mkuznyetsov mkuznyetsov self-assigned this Mar 13, 2023
@ibuziuk ibuziuk added sprint/current and removed team/A This team is responsible for the Che Operator and all its operands as well as chectl and Hosted Che sprint/next labels Mar 22, 2023
@tolusha
Copy link
Contributor

tolusha commented Mar 31, 2023

@l0rd

Setting a container field empty should result in the property not being set

What is the reason behind this? It is recommended to set some limits into a container.
I suggest to set defaults if property is not set in CheCluster CR.

@nickboldt
Copy link
Contributor

PLEASE don't make this another change in CheCluster we have to then remove like in https://issues.redhat.com/browse/CRW-4077

Let's keep those out of the CR so that updates between versions don't keep breaking as we add/remove/change defaults in the CheCluster.

@l0rd
Copy link
Contributor Author

l0rd commented Apr 3, 2023

What is the reason behind this? It is recommended to set some limits into a container. I suggest to set defaults if property is not set in CheCluster CR.

We want to keep the default as it is today. But we want to allow admins to specify that the workspace containers will have no limits/requests. This is considered a good practice in some cases.

@tolusha
Copy link
Contributor

tolusha commented Apr 6, 2023

We want to keep the default as it is today. But we want to allow admins to specify that the workspace containers will have no limits/requests. This is considered a good practice in some cases.

I suggest doing this in the following way:

  1. if limits/requests are set in CheCluster CR, then use it
  2. if they are not set (nil), then use defaults
  3. if they are set to "0" (empty can't be used), then don't set limits/requests

@mkuznyetsov
Copy link
Contributor

gatewayContainer related PR is merged, associated Docs PR will be merged by the docs team

@l0rd
Copy link
Contributor Author

l0rd commented May 3, 2023

To close this issue we are waiting for devfile/devworkspace-operator#1080 that should be addressed in DWO 0.21.0. After that we need a correspondent issue/PR on che-operator side.

@l0rd l0rd assigned amisevsk and unassigned tolusha and mkuznyetsov May 3, 2023
@cgruver
Copy link

cgruver commented May 18, 2023

@l0rd

Setting a container field empty should result in the property not being set

What is the reason behind this? It is recommended to set some limits into a container. I suggest to set defaults if property is not set in CheCluster CR.

@tolusha Setting CPU Limits is not always a good practice. We crashed into the the default CPU limit of 500m recently while trying to debug why a workspace pod did not seem to be able to burst much CPU.

We also discovered that if a devfile component has a request of 500m (or greater), but no limit set, then the workspace fails to create.

IMO, we should allow for the Che admins to turn off the auto-injection of a CPU Limit.

Here's a really well written explaination for not setting CPU limits: https://home.robusta.dev/blog/stop-using-cpu-limits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/che-operator Issues and PRs related to Eclipse Che Kubernetes Operator area/devworkspace-operator kind/enhancement A feature request - must adhere to the feature request template. severity/P1 Has a major impact to usage or development of the system. sprint/current team/B This team is responsible for the Web Terminal, the DevWorkspace Operator and the IDEs.
Projects
None yet
Development

No branches or pull requests

7 participants