Remove cpu limits from tasks

[enkeefe00]

Closes: KFLUXINFRA-767

[cmoulliard]

By removing such CPU limits you will then accept that pods get more CPU (if available on the node) as documented here https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ ?

[chmeliik]

@enkeefe00 could you explain the motivation for this change? I don't think anyone here understands it, and it feels like the opposite direction of what @jhutar has been doing (adding resource requests and limits to tasks)

[kdudka]

@cmoulliard Thanks for the link! The question is whether this is the only place where the limit can be set. Are you sure it cannot happen that a lower limit is set elsewhere and we need to override the number in the sast-coverity-check task?

[jhutar]

Hello. Generally I'm more concerned about memory request and limit. @hugares , do you have a preference?

Regarding if the CPU limit will come from somewhere else: https://kubernetes.io/docs/tasks/configure-pod-container/assign-cpu-resource/#if-you-do-not-specify-a-cpu-limit

If you do not specify a CPU limit for a Container, then one of these situations applies:

• The Container has no upper bound on the CPU resources it can use. The Container could use all of the CPU resources available on the Node where it is running.
• The Container is running in a namespace that has a default CPU limit, and the Container is automatically assigned the default limit. Cluster administrators can use a LimitRange to specify a default value for the CPU limit.

So looking at stone-stg-rh01, I see:

$ oc -n jhutar-1-tenant get LimitRange/resource-limits -o yaml | yq .spec
limits:
  - default:
      memory: 2Gi
    defaultRequest:
      cpu: 100m
      memory: 256Mi
    type: Container

Looks like we only have default CPU request.

Also Tekton plays a role when assigning resources to containers, but looks like that will not be relevant here: https://tekton.dev/docs/pipelines/compute-resources/

[enkeefe00]

@enkeefe00 could you explain the motivation for this change? I don't think anyone here understands it, and it feels like the opposite direction of what @jhutar has been doing (adding resource requests and limits to tasks)

Yes, my apologies. My ticket aims to remove CPU limits from all resources (see this blog's rationale) as the user workloads have been moved to a dedicated node pool. Any resource requests and memory limits should not be affected by this change. @hugares should be able to provide more information if needed.

[chmeliik]

@enkeefe00 makes sense, thanks for linking the relevant resources.

The failing check is saying that the "trusted-artifacts" task generator wants to put some limits back in place. You can fix that in task-generator/trusted-artifacts/ta.go I think

[chmeliik]

/ok-to-test

[hugares]

@chmeliik Can we merge this PR? I am not familiar with review/merge process in build-definitions

[chmeliik]

@chmeliik Can we merge this PR? I am not familiar with review/merge process in build-definitions

We still need reviews from:

• @konflux-ci/integration-service-maintainers
• @ligangty or @yma96
• @cmoulliard

[dirgim]

LGTM

[enkeefe00]

@cmoulliard and @yma96 or @ligangty could you review this PR so we can try to merge it soon?

[chmeliik]

/retest

[chmeliik]

We're only missing reviews from @yma96 or @ligangty and I don't think they would be opposed. Let's force-merge this before we merge #1865 and cause a rebase to be needed