Check for LimitRange Minimum for TaskRun Container Requests #1991

danielhelfand · 2020-02-01T03:17:54Z

Closes #1045
Closes #1920

Changes

This pull request adds the ability to specify a LimitRange name as part of how a TaskRun handles container resource requests. Currently, pipelines/tasks do not work when a LimitRange minimum is specified because all container requests that do not have the max values are set to 0. Since these requests are 0, they fail to meet the minimum specified and the TaskRun fails.

In order to specify a LimitRange name, a LimitRangeName property has been added to TaskRun/PipelineRun specs. The LimitRange name is then used to get the LimitRange, which is passed to resolveResourceRequests. LimitRanges were added to the 200-clusterrole.yaml to allow LimitRanges to be accessed.

Open to suggestions on more robust testing and can also include docs/examples if helpful.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Allow pipelines/tasks to work when LimitRange minimum is set by specifying a LimitRangeName as part of TaskRun/PipelineRun specs

danielhelfand · 2020-02-01T03:50:43Z

/test pull-tekton-pipeline-integration-tests

danielhelfand · 2020-02-03T04:12:36Z

pkg/pod/pod.go

+	// Get LimitRange by name if present in ConfigMap tekton-limitrange-config.
+	// Otherwise, pass empty request to resolveResourceRequests.
+	limitRangeName := ""
+	limitRangeConfig, err := kubeclient.CoreV1().ConfigMaps(taskRun.Namespace).Get("tekton-limitrange-config", metav1.GetOptions{})


Would be open to changing tekton-limitrange-config to something else in case this approach could be used to support other namespaced default values.

pkg/pod/resource_request.go

pkg/pod/pod.go

ghost

Looks good! Would definitely like to see the configmap name and purpose documented as well.

pkg/pod/resource_request.go

tekton-robot · 2020-02-03T19:47:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sbwsg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

danielhelfand · 2020-02-03T19:56:45Z

Looks good! Would definitely like to see the configmap name and purpose documented as well.

Thanks! I'll add the last changes you added and a doc.

ghost · 2020-02-04T20:16:23Z

docs/tekton-limitrange-config.md

+minimum defined for container requests, a ConfigMap named `tekton-limitrange-config` can be 
+created that holds the name of the LimitRange in the namespace where your TaskRuns are to be ran. 
+
+An example `tekton-limitrange-config` is shown below:


danielhelfand · 2020-02-05T01:27:41Z

@sbwsg Sorry to do this, but I changed this up a bit. While I was hesitant to do this because I am not sure how commonly used this will be, I think it makes more sense to add the LimitRangeName to the pipelinerun/taskrun specs and avoid using a separate config.

If it is preferred to not add this field to the spec, I can go back to the config implementation. Otherwise, I can squash these commits and go with this new approach.

ghost · 2020-02-05T13:28:36Z

pkg/apis/pipeline/v1alpha1/pipelinerun_types.go

@@ -66,6 +66,8 @@ type PipelineRunSpec struct {
 	// with those declared in the pipeline.
 	// +optional
 	Workspaces []WorkspaceBinding `json:"workspaces,omitempty"`
+	// +optional


could use a comment here describing the field's purpose. // LimitRangeName specifies the limit range to use for minimum resource values in task pods or similar.

Same comment for taskrun_types in v1alpha1 and 2

Thanks! Added along with some docs for pipelineruns/taskruns.

ghost · 2020-02-05T14:05:13Z

docs/pipelineruns.md

@@ -51,6 +51,10 @@ following fields:
    follow the instruction [here](taskruns.md#Configuring-default-timeout) to configure the
    default timeout, the same way as `TaskRun`.
  - [`podTemplate`](#pod-template) - Specifies a [pod template](./podtemplates.md) that will be used as the basis for the `Task` pod.
+  - [`limitRangeName`](#limitrange-name) - Specifies the name of a LimitRange that exists in the namespace of the `PipelineRun`. This LimitRange's minimum 
+    for container resource requests will be used as part of requesting the appropriate amount of CPU, memory, and ephemeral storage for containers that are 
+    part of the `TaskRuns` of a `PipelineRun`. This proerty only needs to be specified if the `PipelineRun` is happening in a namespace with a LimitRange 


nit: proerty -> property

ghost · 2020-02-05T14:06:27Z

pkg/apis/pipeline/v1alpha1/pipelinerun_types.go

@@ -66,6 +66,8 @@ type PipelineRunSpec struct {
 	// with those declared in the pipeline.
 	// +optional
 	Workspaces []WorkspaceBinding `json:"workspaces,omitempty"`
+	// +optional


ghost · 2020-02-05T14:22:02Z

/lgtm

tekton-robot requested review from abayer and imjasonh February 1, 2020 03:17

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Feb 1, 2020

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 1, 2020

danielhelfand force-pushed the limit_range_configmap branch from 6704bcd to f94a45b Compare February 2, 2020 17:36

danielhelfand commented Feb 3, 2020

View reviewed changes

danielhelfand force-pushed the limit_range_configmap branch from f94a45b to cc91153 Compare February 3, 2020 13:21

ghost reviewed Feb 3, 2020

View reviewed changes

pkg/pod/resource_request.go Outdated Show resolved Hide resolved

pkg/pod/resource_request.go Outdated Show resolved Hide resolved

pkg/pod/pod.go Outdated Show resolved Hide resolved

danielhelfand force-pushed the limit_range_configmap branch from cc91153 to 0905278 Compare February 3, 2020 15:41

ghost approved these changes Feb 3, 2020

View reviewed changes

pkg/pod/resource_request.go Outdated Show resolved Hide resolved

pkg/pod/resource_request.go Outdated Show resolved Hide resolved

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2020

danielhelfand force-pushed the limit_range_configmap branch from 0905278 to 482085d Compare February 3, 2020 20:44

ghost reviewed Feb 4, 2020

View reviewed changes

ghost reviewed Feb 5, 2020

View reviewed changes

danielhelfand force-pushed the limit_range_configmap branch from 9deecad to 3991d39 Compare February 5, 2020 13:53

ghost reviewed Feb 5, 2020

View reviewed changes

check for limitrange minimum for taskrun container requests

8c0dab1

danielhelfand force-pushed the limit_range_configmap branch from 3991d39 to 8c0dab1 Compare February 5, 2020 14:21

tekton-robot assigned ghost Feb 5, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 5, 2020

tekton-robot merged commit 63e6cda into tektoncd:master Feb 5, 2020

This was referenced Feb 5, 2020

Add LimitRange Examples for PipelineRun and TaskRun #2004

Merged

Don't Require User Input to Account for LimitRange Min #2020

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for LimitRange Minimum for TaskRun Container Requests #1991

Check for LimitRange Minimum for TaskRun Container Requests #1991

danielhelfand commented Feb 1, 2020 •

edited

Loading

danielhelfand commented Feb 1, 2020

danielhelfand Feb 3, 2020

ghost left a comment

tekton-robot commented Feb 3, 2020

danielhelfand commented Feb 3, 2020

ghost Feb 4, 2020

danielhelfand commented Feb 5, 2020

ghost Feb 5, 2020

danielhelfand Feb 5, 2020

ghost Feb 5, 2020

ghost Feb 5, 2020

ghost Feb 5, 2020

ghost commented Feb 5, 2020

Check for LimitRange Minimum for TaskRun Container Requests #1991

Check for LimitRange Minimum for TaskRun Container Requests #1991

Conversation

danielhelfand commented Feb 1, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

danielhelfand commented Feb 1, 2020

danielhelfand Feb 3, 2020

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

tekton-robot commented Feb 3, 2020

danielhelfand commented Feb 3, 2020

ghost Feb 4, 2020

Choose a reason for hiding this comment

danielhelfand commented Feb 5, 2020

ghost Feb 5, 2020

Choose a reason for hiding this comment

danielhelfand Feb 5, 2020

Choose a reason for hiding this comment

ghost Feb 5, 2020

Choose a reason for hiding this comment

ghost Feb 5, 2020

Choose a reason for hiding this comment

ghost Feb 5, 2020

Choose a reason for hiding this comment

ghost commented Feb 5, 2020

danielhelfand commented Feb 1, 2020 •

edited

Loading