From ae9ee067a5f0449cfc12118f6009c195c34d6744 Mon Sep 17 00:00:00 2001 From: Austin Zhao Date: Thu, 12 May 2022 01:00:14 +0000 Subject: [PATCH] Support Task-level Resources Requirements: Part #1 Required fields and related webhook validations are added to support a user to configure the computing resources per Task/Pod which significantly reduces the over-asked amount configured by the step-level. --- docs/compute-resources.md | 245 ++++++++++++++++++ .../pipeline/v1beta1/openapi_generated.go | 41 ++- .../pipeline/v1beta1/pipelinerun_types.go | 6 + .../v1beta1/pipelinerun_validation.go | 15 ++ .../v1beta1/pipelinerun_validation_test.go | 50 +++- pkg/apis/pipeline/v1beta1/resource_types.go | 11 + pkg/apis/pipeline/v1beta1/swagger.json | 21 ++ pkg/apis/pipeline/v1beta1/task_validation.go | 37 +++ .../pipeline/v1beta1/task_validation_test.go | 137 +++++++++- .../pipeline/v1beta1/taskrun_validation.go | 14 + .../v1beta1/taskrun_validation_test.go | 53 +++- .../pipeline/v1beta1/zz_generated.deepcopy.go | 15 ++ 12 files changed, 638 insertions(+), 7 deletions(-) diff --git a/docs/compute-resources.md b/docs/compute-resources.md index 87559ac930c..8845b3f2721 100644 --- a/docs/compute-resources.md +++ b/docs/compute-resources.md @@ -52,6 +52,251 @@ requirements for `Step`containers, they must be treated as if they are running i Tekton adjusts `Step` resource requirements to comply with [LimitRanges](#limitrange-support). [ResourceQuotas](#resourcequota-support) are not currently supported. +Kubernetes interprets `Pod` resources requirements by taking the value summed up by each container's resources requirements. It leads Tekton to hoard resources computed by all `Steps`(containers), which can only be used on each sequentially executed `Step`. This observation forms the interest to support a `Task`/`Pod` level resources requirements, which naturally matches Kubernetes computing logic and can help users save significantly on requested resources. + +`Task`-level resources requirements can be configured in `Task.spec.resources` or `PipelineRun.spec.taskRunSpecs.resources`. + +e.g. + +```yaml +apiVersion: tekton.dev/v1beta1 +kind: Task +metadata: + name: foo +spec: + steps: + - name: foo1 + - name: foo2 + resources: + requests: + cpu: 1 + limits: + cpu: 2 +``` + +### Applying Resources Requirements + +**Requests** +`Task`(`Pod`) level resources requests will be applied only on one `Step`(container) to acquire the same effective request for the `Task`. (An exception with `LimitRange` will be explained in the below section.) + +**Limits** +Different from requests, `Task` level resources limits will be applied on each `Step` as a non-limits-configured `Step` will have higher limits than configured ones by Kubernetes. Although the summed-up limits will lead to a value higher than the desired for `Task`, the effective limits will not be used for scheduling a `Pod` but applied in container runtime. So during any time of the `Task`, this limit will be enforced on each `Step`. + +### Interaction with Step-level Resources Requirements + +When Tekton now supports configuring step-level resources requirements, from `Task.spec.step.resources`, `Task.spec.stepTemplate.resources`, `TaskRun.spec.stepOverrides.resources`, and `PipelineRun.spec.taskRunSpecs.stepOverrides.resources`, either the task-level or step-level will be allowed to configure resources requirements, therefore avoiding possible conflicts. + +The following configurations will be rejected during the webhook validation: + +Case #1: `Task.spec.step.resources` and `Task.spec.resources` + +```yaml +kind: Task +spec: + steps: + - name: foo + requests: + cpu: 1 + resources: + requests: + cpu: 2 +``` + +Case #2: `Task.spec.stepTemplate.resources` and `Task.spec.resources` + +```yaml +kind: Task +spec: + steps: + - name: foo + stepTemplate: + requests: + cpu: 1 + resources: + requests: + cpu: 2 +``` + +Case #3: `TaskRun.spec.stepOverrides.resources` and `TaskRun.spec.taskSpec.resources` + +```yaml +kind: TaskRun +spec: + stepOverrides: + - name: foo + resources: + requests: + cpu: 1 + taskSpec: + resources: + requests: + cpu: 2 +``` + +Case #4: `PipelineRun.spec.taskRunSpecs.stepOverrides.resources` and `PipelineRun.spec.taskRunSpecs.resources` + +```yaml +kind: PipelineRun +spec: + taskRunSpecs: + - pipelineTaskName: foo + stepOverrides: + - name: foo + resources: + requests: + cpu: 1 + resources: + requests: + cpu: 2 +``` + +### Interaction with Runtime Overwriting + +As runtime specifications are based on an execution context, Tekton allows users to overwrite certain statically configured fields in the runtime. For task-level resources requirements, there are `TaskRun.spec.taskSpec.resources` and `PipelineRun.spec.taskRunSpecs.resources`. + +e.g. + +```yaml +kind: Task +metadata: + name: foo +spec: + resources: + requests: + cpu: 1 +--- +kind: TaskRun +spec: + taskRef: + name: foo + taskSpec: + resources: + requests: + cpu: 2 +``` + +The final task cpu resources request will be 2 by the overwriting. + +### Interaction with LimitRanges + +Min or max resources requirements can be configured by `LimitRange`. For a min request on certain containers, the task-level request can subtract this amount and assign the rest to one `Step`(container) as stated before. Furthermore, if a container has a value more than the max limit after being set with task-level requests, the request amount will be spread out to each container (the number will be rounded to ignore decimals). + +Case #1: Applying Min + +```yaml +kind: LimitRange +spec: + limits: + - min: + cpu: 200m +--- +kind: Task +spec: + steps: + - name: step1 # applied with min + - name: step2 # applied with min + - name: step3 # not applied + resources: + requests: + cpu: 1 +``` + +| Step name | CPU request | +| --------- | ----------- | +| step1 | 800m | +| step2 | 200m | +| step3 | N/A | + +Here the 800m on step1 comes from `200m + (1 - 200m * 2)`. + +Case #2: Applying Max + +```yaml +kind: LimitRange +spec: + limits: + - max: + cpu: 800m +--- +kind: Task +spec: + steps: + - name: step1 # applied with max + - name: step2 # applied with max + - name: step3 # not applied + resources: + requests: + cpu: 1 +``` + +| Step name | CPU request | +| --------- | ----------- | +| step1 | 333m | +| step2 | 333m | +| step3 | 333m | + +Here the 333m comes from `1 / 3` with number rounding to leave decimals out. + +Case #3: Applying Min and Max + +```yaml +kind: LimitRange +spec: + limits: + - min: + cpu: 200m + - max: + cpu: 700m +--- +kind: Task +spec: + steps: + - name: step1 # applied with min and max + - name: step2 # applied with min and max + - name: step3 # not applied + resources: + requests: + cpu: 1 +``` + +| Step name | CPU request | +| --------- | ----------- | +| step1 | 400m | +| step2 | 400m | +| step3 | 200m |   + +Here the 400m comes from the min `200` + the spread out `(1 - 200m * 2) / 3`. + +### Interaction with Sidecar Resources Requirements + +A `Sidecar` container will run with sequentially executed `Steps` in parallel, which leads to a summed up resources requirements for the `Task`(`Pod`) from the 2 parts. In order to avoid possible conflicts on limits, resources requirements will be configured separately for `Steps` and `Sidecar`. + +e.g. + +```yaml +kind: Task +spec: + steps: + - name: step1 + - name: step2 + sidecars: + - name: sidecar1 + resources: + requests: + cpu: 750m + limits: + cpu: 1 + resources: + requests: + cpu: 2 +``` + +| Step/Sidecar name | CPU request | CPU limit | +| ----------------- | ----------- | --------- | +| step1 | 2 | N/A | +| step2 | N/A | N/A | +| sidecar1 | 750m | 1 | + ## LimitRange Support Kubernetes allows users to configure [LimitRanges]((https://kubernetes.io/docs/concepts/policy/limit-range/)), diff --git a/pkg/apis/pipeline/v1beta1/openapi_generated.go b/pkg/apis/pipeline/v1beta1/openapi_generated.go index bf89f787a11..16f44d1fb2e 100644 --- a/pkg/apis/pipeline/v1beta1/openapi_generated.go +++ b/pkg/apis/pipeline/v1beta1/openapi_generated.go @@ -2881,11 +2881,18 @@ func schema_pkg_apis_pipeline_v1beta1_PipelineTaskRunSpec(ref common.ReferenceCa }, }, }, + "resources": { + SchemaProps: spec.SchemaProps{ + Description: "Compute Resources required by this container. Cannot be updated. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/", + Default: map[string]interface{}{}, + Ref: ref("k8s.io/api/core/v1.ResourceRequirements"), + }, + }, }, }, }, Dependencies: []string{ - "github.com/tektoncd/pipeline/pkg/apis/pipeline/pod.Template", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.TaskRunSidecarOverride", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.TaskRunStepOverride"}, + "github.com/tektoncd/pipeline/pkg/apis/pipeline/pod.Template", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.TaskRunSidecarOverride", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.TaskRunStepOverride", "k8s.io/api/core/v1.ResourceRequirements"}, } } @@ -4335,11 +4342,41 @@ func schema_pkg_apis_pipeline_v1beta1_TaskResources(ref common.ReferenceCallback }, }, }, + "limits": { + SchemaProps: spec.SchemaProps{ + Description: "Limits describes the maximum amount of compute resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/", + Type: []string{"object"}, + AdditionalProperties: &spec.SchemaOrBool{ + Allows: true, + Schema: &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Default: map[string]interface{}{}, + Ref: ref("k8s.io/apimachinery/pkg/api/resource.Quantity"), + }, + }, + }, + }, + }, + "requests": { + SchemaProps: spec.SchemaProps{ + Description: "Requests describes the minimum amount of compute resources required. If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, otherwise to an implementation-defined value. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/", + Type: []string{"object"}, + AdditionalProperties: &spec.SchemaOrBool{ + Allows: true, + Schema: &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Default: map[string]interface{}{}, + Ref: ref("k8s.io/apimachinery/pkg/api/resource.Quantity"), + }, + }, + }, + }, + }, }, }, }, Dependencies: []string{ - "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.TaskResource"}, + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.TaskResource", "k8s.io/apimachinery/pkg/api/resource.Quantity"}, } } diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_types.go b/pkg/apis/pipeline/v1beta1/pipelinerun_types.go index 31dd1d94ea0..69730ac1137 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_types.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_types.go @@ -20,6 +20,7 @@ import ( "context" "time" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" "github.com/tektoncd/pipeline/pkg/apis/config" @@ -600,6 +601,11 @@ type PipelineTaskRunSpec struct { StepOverrides []TaskRunStepOverride `json:"stepOverrides,omitempty"` // +listType=atomic SidecarOverrides []TaskRunSidecarOverride `json:"sidecarOverrides,omitempty"` + // Compute Resources required by the Task/TaskRun (Pod). + // Cannot be updated. + // More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ + // +optional + Resources corev1.ResourceRequirements `json:"resources,omitempty" protobuf:"bytes,8,opt,name=resources"` } // GetTaskRunSpec returns the task specific spec for a given diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go index 482cda177eb..f095bf4ce93 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go @@ -195,6 +195,9 @@ func validateTaskRunSpec(ctx context.Context, trs PipelineTaskRunSpec) (errs *ap if trs.SidecarOverrides != nil { errs = errs.Also(validateSidecarOverrides(trs.SidecarOverrides).ViaField("sidecarOverrides")) } + if trs.Resources.Size() > 0 { + errs = errs.Also(validateResourcesRequirements(trs).ViaField("resources")) + } } else { if trs.StepOverrides != nil { errs = errs.Also(apis.ErrDisallowedFields("stepOverrides")) @@ -205,3 +208,15 @@ func validateTaskRunSpec(ctx context.Context, trs PipelineTaskRunSpec) (errs *ap } return errs } + +// validateResourcesRequirements() validates if only step-level or task-level resources requirements are configured +func validateResourcesRequirements(trs PipelineTaskRunSpec) (errs *apis.FieldError) { + for _, taskRunStepOverride := range trs.StepOverrides { + if taskRunStepOverride.Resources.Size() > 0 { + return &apis.FieldError{ + Message: "PipelineTaskRunSpec can't be configured with both step-level(stepOverrides.resources) and task-level(taskRunSpecs.resources) resources requirements", + } + } + } + return nil +} diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go index 6fc9f58c306..db034d97241 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go @@ -610,6 +610,29 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) { }, wantErr: apis.ErrMissingField("taskRunSpecs[0].sidecarOverrides[0].name"), withContext: enableAlphaAPIFields, + }, { + name: "invalid both step-level(stepOverrides.resources) and task-level(taskRunSpecs.resources) resources requirements configured", + spec: v1beta1.PipelineRunSpec{ + PipelineRef: &v1beta1.PipelineRef{Name: "foo"}, + TaskRunSpecs: []v1beta1.PipelineTaskRunSpec{ + { + PipelineTaskName: "foo", + StepOverrides: []v1beta1.TaskRunStepOverride{{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: corev1resources.MustParse("1Gi")}, + }}, + }, + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: corev1resources.MustParse("2Gi")}, + }, + }, + }, + }, + wantErr: &apis.FieldError{ + Message: "PipelineTaskRunSpec can't be configured with both step-level(stepOverrides.resources) and task-level(taskRunSpecs.resources) resources requirements", + }, + withContext: enableAlphaAPIFields, }} for _, ps := range tests { t.Run(ps.name, func(t *testing.T) { @@ -627,8 +650,9 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) { func TestPipelineRunSpec_Validate(t *testing.T) { tests := []struct { - name string - spec v1beta1.PipelineRunSpec + name string + spec v1beta1.PipelineRunSpec + withContext func(context.Context) context.Context }{{ name: "PipelineRun without pipelineRef", spec: v1beta1.PipelineRunSpec{ @@ -641,10 +665,30 @@ func TestPipelineRunSpec_Validate(t *testing.T) { }}, }, }, + }, { + name: "valid task-level(taskRunSpecs.resources) resources requirements configured", + spec: v1beta1.PipelineRunSpec{ + PipelineRef: &v1beta1.PipelineRef{Name: "foo"}, + TaskRunSpecs: []v1beta1.PipelineTaskRunSpec{{ + PipelineTaskName: "foo", + StepOverrides: []v1beta1.TaskRunStepOverride{{ + Name: "foo", + }}, + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: corev1resources.MustParse("2Gi")}, + }, + }}, + }, + withContext: enableAlphaAPIFields, }} + for _, ps := range tests { t.Run(ps.name, func(t *testing.T) { - if err := ps.spec.Validate(context.Background()); err != nil { + ctx := context.Background() + if ps.withContext != nil { + ctx = ps.withContext(ctx) + } + if err := ps.spec.Validate(ctx); err != nil { t.Error(err) } }) diff --git a/pkg/apis/pipeline/v1beta1/resource_types.go b/pkg/apis/pipeline/v1beta1/resource_types.go index 0eda026817a..7f41c53f33c 100644 --- a/pkg/apis/pipeline/v1beta1/resource_types.go +++ b/pkg/apis/pipeline/v1beta1/resource_types.go @@ -61,6 +61,7 @@ var AllResourceTypes = resource.AllResourceTypes // TaskResources allows a Pipeline to declare how its DeclaredPipelineResources // should be provided to a Task as its inputs and outputs. +// Also task-level(Pod) container computing resources can be configured by limits and requests. type TaskResources struct { // Inputs holds the mapping from the PipelineResources declared in // DeclaredPipelineResources to the input PipelineResources required by the Task. @@ -70,6 +71,16 @@ type TaskResources struct { // DeclaredPipelineResources to the input PipelineResources required by the Task. // +listType=atomic Outputs []TaskResource `json:"outputs,omitempty"` + // Limits describes the maximum amount of compute resources allowed for each Task (Pod). + // More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ + // +optional + Limits v1.ResourceList `json:"limits,omitempty"` + // Requests describes the minimum amount of compute resources required for each Task (Pod). + // If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, + // otherwise to an implementation-defined value. + // More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ + // +optional + Requests v1.ResourceList `json:"requests,omitempty"` } // TaskResource defines an input or output Resource declared as a requirement diff --git a/pkg/apis/pipeline/v1beta1/swagger.json b/pkg/apis/pipeline/v1beta1/swagger.json index f218f3f2a84..880add693e6 100644 --- a/pkg/apis/pipeline/v1beta1/swagger.json +++ b/pkg/apis/pipeline/v1beta1/swagger.json @@ -1612,6 +1612,11 @@ "pipelineTaskName": { "type": "string" }, + "resources": { + "description": "Compute Resources required by this container. Cannot be updated. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/", + "default": {}, + "$ref": "#/definitions/v1.ResourceRequirements" + }, "sidecarOverrides": { "type": "array", "items": { @@ -2424,6 +2429,14 @@ }, "x-kubernetes-list-type": "atomic" }, + "limits": { + "description": "Limits describes the maximum amount of compute resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/", + "type": "object", + "additionalProperties": { + "default": {}, + "$ref": "#/definitions/k8s.io.apimachinery.pkg.api.resource.Quantity" + } + }, "outputs": { "description": "Outputs holds the mapping from the PipelineResources declared in DeclaredPipelineResources to the input PipelineResources required by the Task.", "type": "array", @@ -2432,6 +2445,14 @@ "$ref": "#/definitions/v1beta1.TaskResource" }, "x-kubernetes-list-type": "atomic" + }, + "requests": { + "description": "Requests describes the minimum amount of compute resources required. If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, otherwise to an implementation-defined value. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/", + "type": "object", + "additionalProperties": { + "default": {}, + "$ref": "#/definitions/k8s.io.apimachinery.pkg.api.resource.Quantity" + } } } }, diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go index acea12ba00d..df96860e2de 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation.go +++ b/pkg/apis/pipeline/v1beta1/task_validation.go @@ -78,6 +78,7 @@ func (ts *TaskSpec) Validate(ctx context.Context) (errs *apis.FieldError) { errs = errs.Also(ValidateParameterTypes(ctx, ts.Params).ViaField("params")) errs = errs.Also(ValidateParameterVariables(ts.Steps, ts.Params)) errs = errs.Also(ValidateResourcesVariables(ts.Steps, ts.Resources)) + errs = errs.Also(ValidateResourcesRequirements(ctx, ts)) errs = errs.Also(validateTaskContextVariables(ts.Steps)) errs = errs.Also(validateResults(ctx, ts.Results).ViaField("results")) return errs @@ -374,6 +375,42 @@ func ValidateResourcesVariables(steps []Step, resources *TaskResources) *apis.Fi return validateVariables(steps, "resources.(?:inputs|outputs)", resourceNames) } +// ValidateResourcesRequirements() validates if only step-level or task-level resources requirements are configured +func ValidateResourcesRequirements(ctx context.Context, taskSpec *TaskSpec) *apis.FieldError { + if isResourcesRequirementsInTaskLevel(taskSpec) { + if err := ValidateEnabledAPIFields(ctx, "task-level resources requirements", config.AlphaAPIFields); err != nil { + return err + } + } + + if isResourcesRequirementsInStepLevel(taskSpec) && isResourcesRequirementsInTaskLevel(taskSpec) { + return &apis.FieldError{ + Message: "TaskSpec can't be configured with both step-level(Task.spec.step.resources or Task.spec.stepTemplate.resources) and task-level(Task.spec.resources) resources requirements", + Paths: []string{"resources"}, + } + } + + return nil +} + +// isResourcesRequirementsInStepLevel() checks if any resources requirements specified under step-level +func isResourcesRequirementsInStepLevel(taskSpec *TaskSpec) bool { + for _, step := range taskSpec.Steps { + if step.Resources.Size() > 0 { + return true + } + } + return taskSpec.StepTemplate != nil && taskSpec.StepTemplate.Resources.Size() > 0 +} + +// isResourcesRequirementsInTaskLevel() checks if any resources requirements specified under task-level +func isResourcesRequirementsInTaskLevel(taskSpec *TaskSpec) bool { + if taskSpec == nil || taskSpec.Resources == nil { + return false + } + return taskSpec.Resources.Limits != nil || taskSpec.Resources.Requests != nil +} + // TODO (@chuangw6): Make sure an object param is not used as a whole when providing values for strings. // https://github.com/tektoncd/community/blob/main/teps/0075-object-param-and-result-types.md#variable-replacement-with-object-params // "When providing values for strings, Task and Pipeline authors can access diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go index a19e50f11af..031cca30aa1 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/task_validation_test.go @@ -28,6 +28,7 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/test/diff" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "knative.dev/pkg/apis" ) @@ -372,7 +373,72 @@ func TestTaskSpecValidate(t *testing.T) { hello "$(context.taskRun.namespace)"`, }}, }, + }, { + name: "valid step-level(step.resources) resources requirements", + fields: fields{ + Steps: []v1beta1.Step{{ + Name: "step-1", + Image: "my-image", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("2"), + }, + }, + }}, + }, + }, { + name: "valid step-level(stepTemplate.resources) resources requirements", + fields: fields{ + Steps: []v1beta1.Step{{ + Name: "step-1", + Image: "my-image", + }}, + StepTemplate: &v1beta1.StepTemplate{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("2"), + }, + }, + }, + }, + }, { + name: "valid step-level(step.resources and stepTemplate.resources) resources requirements", + fields: fields{ + Steps: []v1beta1.Step{{ + Name: "step-1", + Image: "my-image", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1"), + }, + }, + }}, + StepTemplate: &v1beta1.StepTemplate{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1"), + }, + }, + }, + }, + }, { + name: "valid task-level(spec.resources) resources requirements", + fields: fields{ + Steps: []v1beta1.Step{{ + Name: "step-1", + Image: "my-image", + }}, + Resources: &v1beta1.TaskResources{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("4"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + }, + }, + }, }} + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ts := &v1beta1.TaskSpec{ @@ -406,6 +472,7 @@ func TestTaskSpecValidateError(t *testing.T) { name string fields fields expectedError apis.FieldError + withContext func(context.Context) context.Context }{{ name: "empty spec", expectedError: apis.FieldError{ @@ -529,6 +596,7 @@ func TestTaskSpecValidateError(t *testing.T) { Paths: []string{"params"}, Details: "Names: \nMust only contain alphanumeric characters, hyphens (-), underscores (_), and dots (.)\nMust begin with a letter or an underscore (_)", }, + withContext: enableAlphaAPIFields, }, { name: "duplicated param names", fields: fields{ @@ -633,6 +701,7 @@ func TestTaskSpecValidateError(t *testing.T) { Message: `"object" type does not match default value's type: "string"`, Paths: []string{"params.task.type", "params.task.default.type"}, }, + withContext: enableAlphaAPIFields, }, { name: "the spec of object type parameter misses the definition of properties", fields: fields{ @@ -644,6 +713,7 @@ func TestTaskSpecValidateError(t *testing.T) { Steps: validSteps, }, expectedError: *apis.ErrMissingField(fmt.Sprintf("params.task.properties")), + withContext: enableAlphaAPIFields, }, { name: "PropertySpec type is set with unsupported type", fields: fields{ @@ -662,6 +732,7 @@ func TestTaskSpecValidateError(t *testing.T) { Message: fmt.Sprintf("The value type specified for these keys %v is invalid", []string{"key1"}), Paths: []string{"params.task.properties"}, }, + withContext: enableAlphaAPIFields, }, { name: "keys defined in properties are missed in default", fields: fields{ @@ -683,6 +754,7 @@ func TestTaskSpecValidateError(t *testing.T) { Message: fmt.Sprintf("Required key(s) %s for the parameter %s are not provided in default.", []string{"key2"}, "myobjectParam"), Paths: []string{"myobjectParam.properties", "myobjectParam.default"}, }, + withContext: enableAlphaAPIFields, }, { name: "invalid step", fields: fields{ @@ -853,6 +925,7 @@ func TestTaskSpecValidateError(t *testing.T) { Message: `variable is not properly isolated in "not isolated: $(params.baz)"`, Paths: []string{"steps[0].args[0]"}, }, + withContext: enableAlphaAPIFields, }, { name: "inferred array star not properly isolated", fields: fields{ @@ -875,6 +948,7 @@ func TestTaskSpecValidateError(t *testing.T) { Message: `variable is not properly isolated in "not isolated: $(params.baz[*])"`, Paths: []string{"steps[0].args[0]"}, }, + withContext: enableAlphaAPIFields, }, { name: "Inexistent param variable in volumeMount with existing", fields: fields{ @@ -897,6 +971,7 @@ func TestTaskSpecValidateError(t *testing.T) { Message: `non-existent variable in "$(params.inexistent)-foo"`, Paths: []string{"steps[0].volumeMount[0].name"}, }, + withContext: enableAlphaAPIFields, }, { name: "Inexistent param variable with existing", fields: fields{ @@ -915,6 +990,7 @@ func TestTaskSpecValidateError(t *testing.T) { Message: `non-existent variable in "$(params.foo) && $(params.inexistent)"`, Paths: []string{"steps[0].args[0]"}, }, + withContext: enableAlphaAPIFields, }, { name: "Multiple volumes with same name", fields: fields{ @@ -1135,6 +1211,60 @@ func TestTaskSpecValidateError(t *testing.T) { Message: "invalid value: -10s", Paths: []string{"steps[0].negative timeout"}, }, + }, { + name: "invalid both step-level(step.resources) and task-level resources requirements", + fields: fields{ + Steps: []v1beta1.Step{{ + Name: "step-1", + Image: "my-image", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("2"), + }, + }, + }}, + Resources: &v1beta1.TaskResources{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("4"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + }, + }, + }, + expectedError: apis.FieldError{ + Message: "TaskSpec can't be configured with both step-level(Task.spec.step.resources or Task.spec.stepTemplate.resources) and task-level(Task.spec.resources) resources requirements", + Paths: []string{"resources"}, + }, + withContext: enableAlphaAPIFields, + }, { + name: "invalid both step-level(stepTemplate.resources) and task-level resources requirements", + fields: fields{ + Steps: []v1beta1.Step{{ + Name: "step-1", + Image: "my-image", + }}, + StepTemplate: &v1beta1.StepTemplate{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1"), + }, + }, + }, + Resources: &v1beta1.TaskResources{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("4"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + }, + }, + }, + expectedError: apis.FieldError{ + Message: "TaskSpec can't be configured with both step-level(Task.spec.step.resources or Task.spec.stepTemplate.resources) and task-level(Task.spec.resources) resources requirements", + Paths: []string{"resources"}, + }, + withContext: enableAlphaAPIFields, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1147,8 +1277,13 @@ func TestTaskSpecValidateError(t *testing.T) { Workspaces: tt.fields.Workspaces, Results: tt.fields.Results, } - ctx := getContextBasedOnFeatureFlag("alpha") + + ctx := context.Background() + if tt.withContext != nil { + ctx = tt.withContext(ctx) + } ts.SetDefaults(ctx) + err := ts.Validate(ctx) if err == nil { t.Fatalf("Expected an error, got nothing for %v", ts) diff --git a/pkg/apis/pipeline/v1beta1/taskrun_validation.go b/pkg/apis/pipeline/v1beta1/taskrun_validation.go index e4fa03228e7..4cf3f114a0f 100644 --- a/pkg/apis/pipeline/v1beta1/taskrun_validation.go +++ b/pkg/apis/pipeline/v1beta1/taskrun_validation.go @@ -66,6 +66,7 @@ func (ts *TaskRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) { if ts.StepOverrides != nil { errs = errs.Also(ValidateEnabledAPIFields(ctx, "stepOverrides", config.AlphaAPIFields).ViaField("stepOverrides")) errs = errs.Also(validateStepOverrides(ts.StepOverrides).ViaField("stepOverrides")) + errs = errs.Also(validateStepOverridesResourcesRequirements(ts.TaskSpec, ts.StepOverrides).ViaField("stepOverrides")) } if ts.SidecarOverrides != nil { errs = errs.Also(ValidateEnabledAPIFields(ctx, "sidecarOverrides", config.AlphaAPIFields).ViaField("sidecarOverrides")) @@ -138,6 +139,19 @@ func validateStepOverrides(overrides []TaskRunStepOverride) (errs *apis.FieldErr return errs } +// validateStepOverridesResourcesRequirements() validates if both step-level and task-level resources requirements are configured +func validateStepOverridesResourcesRequirements(taskSpec *TaskSpec, overrides []TaskRunStepOverride) (errs *apis.FieldError) { + for _, override := range overrides { + if override.Resources.Size() > 0 && isResourcesRequirementsInTaskLevel(taskSpec) { + return &apis.FieldError{ + Message: "TaskRun can't be configured with both step-level(stepOverrides.resources) and task-level(taskSpec.resources) resources requirements", + Paths: []string{"resources"}, + } + } + } + return nil +} + func validateSidecarOverrides(overrides []TaskRunSidecarOverride) (errs *apis.FieldError) { var names []string for i, o := range overrides { diff --git a/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go b/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go index 89bfffe91ed..4e142b03c03 100644 --- a/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go @@ -515,7 +515,36 @@ func TestTaskRunSpec_Invalidate(t *testing.T) { }, wantErr: apis.ErrMissingField("sidecarOverrides[0].name"), wc: enableAlphaAPIFields, + }, { + name: "invalid both step-level(stepOverrides.resources) and task-level(taskSpec.resources) resources requirements", + spec: v1beta1.TaskRunSpec{ + TaskSpec: &v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{ + Name: "step-1", + Image: "my-image", + }}, + Resources: &v1beta1.TaskResources{ + Requests: corev1.ResourceList{ + corev1.ResourceMemory: corev1resources.MustParse("2Gi"), + }, + }, + }, + StepOverrides: []v1beta1.TaskRunStepOverride{{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceMemory: corev1resources.MustParse("1Gi"), + }, + }, + }}, + }, + wantErr: &apis.FieldError{ + Message: "TaskRun can't be configured with both step-level(stepOverrides.resources) and task-level(taskSpec.resources) resources requirements", + Paths: []string{"stepOverrides.resources"}, + }, + wc: enableAlphaAPIFields, }} + for _, ts := range tests { t.Run(ts.name, func(t *testing.T) { ctx := context.Background() @@ -534,6 +563,7 @@ func TestTaskRunSpec_Validate(t *testing.T) { tests := []struct { name string spec v1beta1.TaskRunSpec + wc func(context.Context) context.Context }{{ name: "taskspec without a taskRef", spec: v1beta1.TaskRunSpec{ @@ -581,10 +611,31 @@ func TestTaskRunSpec_Validate(t *testing.T) { }}, }, }, + }, { + name: "valid task-level(taskSpec.resources) resources requirements", + spec: v1beta1.TaskRunSpec{ + TaskSpec: &v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{ + Name: "step-1", + Image: "my-image", + }}, + Resources: &v1beta1.TaskResources{ + Requests: corev1.ResourceList{ + corev1.ResourceMemory: corev1resources.MustParse("2Gi"), + }, + }, + }, + }, + wc: enableAlphaAPIFields, }} + for _, ts := range tests { t.Run(ts.name, func(t *testing.T) { - if err := ts.spec.Validate(context.Background()); err != nil { + ctx := context.Background() + if ts.wc != nil { + ctx = ts.wc(ctx) + } + if err := ts.spec.Validate(ctx); err != nil { t.Error(err) } }) diff --git a/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go index 43ea81e4520..7f60e4fcf80 100644 --- a/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go @@ -1226,6 +1226,7 @@ func (in *PipelineTaskRunSpec) DeepCopyInto(out *PipelineTaskRunSpec) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + in.Resources.DeepCopyInto(&out.Resources) return } @@ -1774,6 +1775,20 @@ func (in *TaskResources) DeepCopyInto(out *TaskResources) { *out = make([]TaskResource, len(*in)) copy(*out, *in) } + if in.Limits != nil { + in, out := &in.Limits, &out.Limits + *out = make(v1.ResourceList, len(*in)) + for key, val := range *in { + (*out)[key] = val.DeepCopy() + } + } + if in.Requests != nil { + in, out := &in.Requests, &out.Requests + *out = make(v1.ResourceList, len(*in)) + for key, val := range *in { + (*out)[key] = val.DeepCopy() + } + } return }