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

As an operator I would like to be able to define init-container resources #2931

Closed
qu1queee opened this issue Jul 13, 2020 · 14 comments
Closed
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@qu1queee
Copy link
Contributor

Feature request

Similar to how the resources values (cpu/memory) are managed by the pipeline controller for TaskRun containers(see docs) . It would be interesting to see if this pattern could apply to the TaskRun initContainers.

The current default behaviour when a LimitRange exists on the namespace is that the initContainers get the LimitRange defaultRequest values when resources are not specified, while the containers get the min values, when the resources are not specified.

Use case

The use-case is around how can I control the TaskRun pod's effective request/limit resource. I think this use-case arises from the need to have full control on the pod effective request/limit in order to define proper pricing during the pod execution. When I say pricing I meant some sort of number that is calculated from the effective request/limit resource during a certain amount of time.

To achieve this, ideally the init-container resources should be also configurable as the containers and follow the same pattern as the TaskRun containers, when resources are not defined. I think the pattern should be the same, in order to have a common set of resources values for both containers and init-containers.

@qu1queee qu1queee added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 13, 2020
@danielhelfand
Copy link
Member

I think the main issue here is that the resolveResourceRequest() func currently isn't being applied to init containers as it is for steps.

There could maybe be a separate func created for init containers to apply limit range minimums if present as resolveResourceRequest also searches for the max values for CPU, memory, and ephemeral storage, which I don't think would apply to init containers.

A question I have is if the defaultRequest is enough to satisfy this behavior, and maybe a resolution to this issue could be documentation to clarify the difference of why limitrange min is used for steps but not init containers? Originally minimums were chosen over defaults for steps because the goal of resolveResourceRequests is to use the smallest amount of resources possible for step containers part of a TaskRun.

Another possible approach could be to add support for defaultRequest for steps to keep the experience consistent.

@qu1queee
Copy link
Contributor Author

thanks for the answers @danielhelfand .

It is good that you explain the rational behind why Tekton chose limitrange min instead of defaultRequest for the containers. In general if a container does not specify the resource request , it always falls back to the LimitRange defaultRequest(if the LimitRange exists), therefore sticking to this behaviour could make more sense for the Tekton steps use-case, rather than using min. I think your following statement inline with it:

Another possible approach could be to add support for defaultRequest for steps to keep the experience consistent.

When you say adding support for defaultRequest in the steps, you envision support for both min or defaultRequest?, e.g. by allowing the user to define some sort of flag on the steps to specify which one to use? Or just a single default behaviour?

@danielhelfand
Copy link
Member

My personal opinion on the ways step resource requests are handled is that it is a confusing user experience. The way the algorithm works for pipelines is to only accept the max resource request values across all step containers part of a TaskRun. If a limitrange minimum is present, the min is applied to all steps without max values for CPU, memory, and storage. If no min is present, all containers are zeroed out. The original implementation of this came because of #598. So in the context of steps, supporting a defaultRequest would be difficult to justify in my opinion.

What's confusing about steps is that each step can be defined with resource requests, but the only requests acknowledged are the max values. I am wondering if pipelines should support some way of only having to define resource requests once as opposed to supporting the ability to do so for each step.

And while the user experience for init containers is inconsistent in the case of how to define resource requests, maybe it makes sense to keep the defaultRequest. I am unaware of it init containers with pipelines support specifying resource requests, but I think having the ability to specify those requests individually would also be important here.

@qu1queee
Copy link
Contributor Author

qu1queee commented Jul 22, 2020

So in the context of steps, supporting a defaultRequest would be difficult to justify in my opinion.

Yes, I agree, you are totally right. Specially around the fact that containers(steps) are sequentially executed.

What's confusing about steps is that each step can be defined with resource requests, but the only requests acknowledged are the max values. I am wondering if pipelines should support some way of only having to define resource requests once as opposed to supporting the ability to do so for each step.

Ok, it make sense as an additional feature. Maybe this inlines with #2984 (comment) ?

And while the user experience for init containers is inconsistent in the case of how to define resource requests, maybe it makes sense to keep the defaultRequest. I am unaware of it init containers with pipelines support specifying resource requests, but I think having the ability to specify those requests individually would also be important here.

I think defining also resources for the init containers should be possible, what would be the next step on that particular topic(init containers resources definition)?

@imjasonh
Copy link
Member

Reading through the issue, it sounds like the specific concrete request is for Tekton's injected initContainers -- which the user has no control of or visibility into -- to be given the LimitRange's defaultRequest instead of the min. This seems fine to me, and in line with how defaultRequest is intended to be used. In fact, is it possible we could just not set initContainers' requests and let some LimitRange admission controller set the defaultRequest for us?

Step resource requests are indeed confusing, but this issue is specifically about Tekton's initContainers, which I think we can change in the way you're requesting.

@danielhelfand
Copy link
Member

danielhelfand commented Jul 23, 2020

I think it’s actually that the defaultRequest is applied currently for initContainers but the min is applied for steps, which is leading to confusion dealing with requests.

Unless I am misinterpreting this, which I thought meant that defaultRequest is currently applied for initContainers:

The current default behaviour when a LimitRange exists on the namespace is that the initContainers get the LimitRange defaultRequest values when resources are not specified, while the containers get the min values, when the resources are not specified.

@zhangtbj
Copy link
Contributor

Yep, it is better that Tekton init and user containers use the same resource settings to avoid the confusing. Because we have different defaultRequest and min settings in LimitRange, I always need to think what is init container settings, what is the tekton user container settings :(

After tekton has the confirmed solution for init container, it is also better to show the example in this doc:
https://docs.google.com/presentation/d/1-FNMMbRuxckAInO2aJPtzuINqV-9pr-M25OXQoGbV0s/edit#slide=id.g8dbce12b8f_0_68 (FYI @imjasonh :))

@qu1queee
Copy link
Contributor Author

what @danielhelfand mentioned is correct:

I think it’s actually that the defaultRequest is applied currently for initContainers but the min is applied for steps, which is leading to confusion dealing with requests.

For the initContainers resource request, the defaultRequest is used, not the min as in the steps. In general, I think due to the Pod's effective request/limit way of working , initContainers should not matter that much, because I doubt their resources will be higher than the sum of all containers in the Taskrun(based on max(max(initContainers),sum(containers))) .

Some open questions:

  1. Should we move from defaultRequest to min in the initContainers resource request?

I think that no, if I get this right, the min for the steps was used to prevent resource hoarding. For init-containers it is fine to use defaultRequest because we know that only the highest initContainer resource request will be taken into account, not the sum.

  1. Should we allow users to be able to define resources requests for the initContainers?

I do not see a reason why not to do this.

@imjasonh
Copy link
Member

I do not see a reason why not to do this.

Users and operators should not care what resources we assign to initContainers -- they are an implementation detail. In the future there may not be any initContainers at all, or there may end up being 20 of them. Users and operators shouldn't have to care. Tekton should just be responsible for making sure they're valid requests on the cluster, not in violation of any LimitRanges, which is what happens today AFAIK.

For init-containers it is fine to use defaultRequest because we know that only the highest initContainer resource request will be taken into account, not the sum.

If Tekton's doing what it's supposed to, this should also apply to steps, since we know they'll run sequentially and we can take the max(steps).

@qu1queee
Copy link
Contributor Author

@imjasonh are you saying that for steps(containers) we should use defaultRequest instead of min from the LimitRange?

@imjasonh
Copy link
Member

@imjasonh are you saying that for steps(containers) we should use defaultRequest instead of min from the LimitRange?

For steps, we should assign one container the max(steps), and all the rest can be min.

@qu1queee
Copy link
Contributor Author

ok, which is already the case.

@qu1queee
Copy link
Contributor Author

qu1queee commented Jul 27, 2020

im closing this issue in favor of 2986, thanks all for the nice discussions.

@vdemeester
Copy link
Member

For the initContainers resource request, the defaultRequest is used, not the min as in the steps. In general, I think due to the Pod's effective request/limit way of working , initContainers should not matter that much, because I doubt their resources will be higher than the sum of all containers in the Taskrun(based on max(max(initContainers),sum(containers))) .

This really depends on what are those values though. If the min is 50Mi and the defaultRequest is 500Mi, then we will be requesting 500Mi for any task that has less than 11 steps which is probably most of them. That said, that might be fine as is then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

5 participants