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

Specify LimitRange Minimum as Part of Task Definition #1920

Closed
danielhelfand opened this issue Jan 22, 2020 · 5 comments · Fixed by #1991
Closed

Specify LimitRange Minimum as Part of Task Definition #1920

danielhelfand opened this issue Jan 22, 2020 · 5 comments · Fixed by #1991
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. kind/question Issues or PRs that are questions around the project or a particular feature

Comments

@danielhelfand
Copy link
Member

danielhelfand commented Jan 22, 2020

Related to #1045

As documented in #1045, Tekton pipeline does not work when a LimitRange minimum is specified. This is due to how container resource requests are "zeroed out" in resolveResourceRequests(containers).

The proposal in #1045 is to have a way for a user to set a minimum for container resource requests. I am wondering if it makes sense to allow a user to define this as part of a Task definition. If the user defines this field, resolveResourceRequests will accept the LimitRange minimum values passed in. If no minimum is specified, the current behavior of zeroing out the resource requests would be applied.

This would allow the user control to honor a LimitRange minimum set within a namespace. The values could be pulled from a TaskRun and passed and used instead of allZeroQty().

Also open to alternative suggestions, but I think this provides the user an option. LimitRange minimums are probably somewhat of an edge case, but we should still honor the request.

/kind question
/kind feature

@tekton-robot tekton-robot added kind/question Issues or PRs that are questions around the project or a particular feature kind/feature Categorizes issue or PR as related to a new feature. labels Jan 22, 2020
@imjasonh
Copy link
Member

Is there a way to get the LimitRange that would be applied to a Pod, before creating that Pod? Or is the proposal that we have some new key/value in config-defaults.yaml to set LimitRange minimums? (Maximums should still be rejected, not right-sized, correct?)

@danielhelfand
Copy link
Member Author

danielhelfand commented Jan 22, 2020

Is there a way to get the LimitRange that would be applied to a Pod, before creating that Pod?

If there is a way that the user does not have to specify the minimum via the resource definition and the correct LimitRange can be identified in the namespace where the TaskRun is, that would be the ideal scenario. I am still trying to figure out what the best approach for that would be. Open to any suggestions on that and can look into the pod approach as you're suggesting.

Or is the proposal that we have some new key/value in config-defaults.yaml to set LimitRange minimums? (Maximums should still be rejected, not right-sized, correct?)

Yes, the alternative scenario is to put the responsibility on the user to define these values. If we did allow maximum, we could potentially fail the task sooner if it does go over the max, but for this proposal I am more interested in specifically the minimum. If config defaults is the better approach, open to that.

@danielhelfand
Copy link
Member Author

Possible thought here too is to allow users to specify the LimitRange name to use. Using that information, the LimitRange could be grabbed and its minimum values could be applied.

@danielhelfand
Copy link
Member Author

/assign

@danielhelfand
Copy link
Member Author

So my thought is that there should be a way to specify a LimitRange name as part of a TaskRun. This would make sense for factoring in resource quotas as part of a TaskRun.

My proposal would be to add aLimitRangeName as part of the TaskRun spec. This can be used to get the LimitRange using client-go, which requires the namespace and LimitRange name to retrieve it. Having this property on the TaskRun spec would allow the data to be more easily moved to where it is needed and allows the user to explicitly set this property on the TaskRun itself.

This property could also be added to config defaults and applied to all TaskRuns in a namespace.

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. kind/question Issues or PRs that are questions around the project or a particular feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants