From fcd18c34b2d99d2f79d2a44029d7248b95728fef 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 --- .../pipeline/v1beta1/openapi_generated.go | 32 ++++- pkg/apis/pipeline/v1beta1/resource_types.go | 10 ++ pkg/apis/pipeline/v1beta1/swagger.json | 16 +++ .../pipeline/v1beta1/zz_generated.deepcopy.go | 14 +++ pkg/reconciler/taskrun/taskrun.go | 17 ++- pkg/reconciler/taskrun/taskrun_test.go | 27 ++--- pkg/reconciler/taskrun/validate_resources.go | 79 ++++++++++--- .../taskrun/validate_resources_test.go | 110 ++++++++++++++++++ 8 files changed, 272 insertions(+), 33 deletions(-) diff --git a/pkg/apis/pipeline/v1beta1/openapi_generated.go b/pkg/apis/pipeline/v1beta1/openapi_generated.go index 146dd506b98..a1e77cfe462 100644 --- a/pkg/apis/pipeline/v1beta1/openapi_generated.go +++ b/pkg/apis/pipeline/v1beta1/openapi_generated.go @@ -4335,11 +4335,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/resource_types.go b/pkg/apis/pipeline/v1beta1/resource_types.go index 0eda026817a..242f3c3e2b1 100644 --- a/pkg/apis/pipeline/v1beta1/resource_types.go +++ b/pkg/apis/pipeline/v1beta1/resource_types.go @@ -70,6 +70,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. + // 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. + // 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 c1e8ea1f3e7..f1e16f34943 100644 --- a/pkg/apis/pipeline/v1beta1/swagger.json +++ b/pkg/apis/pipeline/v1beta1/swagger.json @@ -2424,6 +2424,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 +2440,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/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go index 70b62ad2027..4941132015d 100644 --- a/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go @@ -1774,6 +1774,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 } diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 00a5519c2c8..d3d7bd15835 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -333,10 +333,23 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1beta1.TaskRun) (*v1beta1 return nil, nil, controller.NewPermanentError(err) } - if err := validateTaskSpecRequestResources(ctx, taskSpec); err != nil { - logger.Errorf("TaskRun %s taskSpec request resources are invalid: %v", tr.Name, err) + if isResourcesRequirementsInStepLevel(taskSpec) && isResourcesRequirementsInTaskLevel(taskSpec) { + err := fmt.Errorf("TaskRun %s can't be configured with both step-level and task-level resources requirements", tr.Name) tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err) return nil, nil, controller.NewPermanentError(err) + } else if isResourcesRequirementsInStepLevel(taskSpec) { + if err := validateStepLevelResourcesRequirements(taskSpec); err != nil { + tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err) + return nil, nil, controller.NewPermanentError(err) + } + } else if isResourcesRequirementsInTaskLevel(taskSpec) { + // validate task level + if err := validateTaskLevelResourcesRequirements(taskSpec); err != nil { + tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err) + return nil, nil, controller.NewPermanentError(err) + } + // updatewithTask + // updatewithTaskrun } if err := ValidateResolvedTaskResources(ctx, tr.Spec.Params, []v1beta1.Param{}, rtr); err != nil { diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index b6ed0f2add7..f6cd25a01ba 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -3912,10 +3912,8 @@ status: } } -func Test_validateTaskSpecRequestResources_ValidResources(t *testing.T) { - ctx := context.Background() - - tcs := []struct { +func Test_validateStepLevelResourcesRequirements_validResourcesRequirements(t *testing.T) { + taskSpecs := []struct { name string taskSpec *v1beta1.TaskSpec }{{ @@ -4017,19 +4015,18 @@ func Test_validateTaskSpecRequestResources_ValidResources(t *testing.T) { }, }} - for _, tc := range tcs { - t.Run(tc.name, func(t *testing.T) { - if err := validateTaskSpecRequestResources(ctx, tc.taskSpec); err != nil { - t.Fatalf("Expected to see error when validating invalid TaskSpec resources but saw none") + for _, taskSpec := range taskSpecs { + t.Run(taskSpec.name, func(t *testing.T) { + if err := validateStepLevelResourcesRequirements(taskSpec.taskSpec); err != nil { + t.Errorf("Expected no errors when validating valid step-level resources requirements, but got: %v.", err) } }) } } -func Test_validateTaskSpecRequestResources_InvalidResources(t *testing.T) { - ctx := context.Background() - tcs := []struct { +func Test_validateStepLevelResourcesRequirements_invalidResourcesRequirements(t *testing.T) { + taskSpecs := []struct { name string taskSpec *v1beta1.TaskSpec }{{ @@ -4073,10 +4070,10 @@ func Test_validateTaskSpecRequestResources_InvalidResources(t *testing.T) { }, }} - for _, tc := range tcs { - t.Run(tc.name, func(t *testing.T) { - if err := validateTaskSpecRequestResources(ctx, tc.taskSpec); err == nil { - t.Fatalf("Expected to see error when validating invalid TaskSpec resources but saw none") + for _, taskSpec := range taskSpecs { + t.Run(taskSpec.name, func(t *testing.T) { + if err := validateStepLevelResourcesRequirements(taskSpec.taskSpec); err == nil { + t.Errorf("Expected errors when validating invalid step-level resources requirements, but got none.") } }) } diff --git a/pkg/reconciler/taskrun/validate_resources.go b/pkg/reconciler/taskrun/validate_resources.go index 96d3a3589bb..537c69c55fe 100644 --- a/pkg/reconciler/taskrun/validate_resources.go +++ b/pkg/reconciler/taskrun/validate_resources.go @@ -223,23 +223,26 @@ func ValidateResolvedTaskResources(ctx context.Context, params []v1beta1.Param, return nil } -func validateTaskSpecRequestResources(ctx context.Context, taskSpec *v1beta1.TaskSpec) error { - if taskSpec != nil { - for _, step := range taskSpec.Steps { - for k, request := range step.Resources.Requests { - // First validate the limit in step - if limit, ok := step.Resources.Limits[k]; ok { +// validateStepLevelResourcesRequirements() validates step-level resources requirements under 'Task.TaskSpec.Step' and 'Task.TaskSpec.StepTemplate' +func validateStepLevelResourcesRequirements(taskSpec *v1beta1.TaskSpec) error { + if !isResourcesRequirementsInStepLevel(taskSpec) { + return nil + } + + for _, step := range taskSpec.Steps { + for key, request := range step.Resources.Requests { + // First validate the limit in step + if limit, ok := step.Resources.Limits[key]; ok { + // Check if limit < request (so out of limit) + if (&limit).Cmp(request) == -1 { + return fmt.Errorf("invalid request resource value: %v must be less or equal to limit %v", request.String(), limit.String()) + } + } else if taskSpec.StepTemplate != nil { + // If step doesn't configure the limit, validate the limit in stepTemplate + if limit, ok := taskSpec.StepTemplate.Resources.Limits[key]; ok { if (&limit).Cmp(request) == -1 { - return fmt.Errorf("Invalid request resource value: %v must be less or equal to limit %v", request.String(), limit.String()) - } - } else if taskSpec.StepTemplate != nil { - // If step doesn't configure the limit, validate the limit in stepTemplate - if limit, ok := taskSpec.StepTemplate.Resources.Limits[k]; ok { - if (&limit).Cmp(request) == -1 { - return fmt.Errorf("Invalid request resource value: %v must be less or equal to limit %v", request.String(), limit.String()) - } + return fmt.Errorf("invalid request resource value: %v must be less or equal to limit %v", request.String(), limit.String()) } - } } @@ -249,6 +252,52 @@ func validateTaskSpecRequestResources(ctx context.Context, taskSpec *v1beta1.Tas return nil } +// isResourcesRequirementsInStepLevel() checks if any resources requirements specified under step-level +func isResourcesRequirementsInStepLevel(taskSpec *v1beta1.TaskSpec) bool { + if taskSpec == nil { + return false + } + + for _, step := range taskSpec.Steps { + if step.Resources.String() != "" { + return true + } + } + + return taskSpec.StepTemplate != nil && taskSpec.StepTemplate.Resources.String() != "" +} + +// isResourcesRequirementsInTaskLevel() checks if any resources requirements specified under task-level +func isResourcesRequirementsInTaskLevel(taskSpec *v1beta1.TaskSpec) bool { + if taskSpec == nil || taskSpec.Resources == nil { + return false + } + return taskSpec.Resources.Limits != nil || taskSpec.Resources.Requests != nil +} + +// validateTaskLevelResourcesRequirements() validates task-level resource requirements under 'Task.TaskSpec' +func validateTaskLevelResourcesRequirements(taskSpec *v1beta1.TaskSpec) error { + if !isResourcesRequirementsInTaskLevel(taskSpec) { + return nil + } + + for key, request := range taskSpec.Resources.Requests { + if limit, ok := taskSpec.Resources.Limits[key]; ok { + // Check if limit < request (so out of limit) + if (&limit).Cmp(request) == -1 { + return fmt.Errorf("invalid request resource value: %v must be less or equal to limit %v", request.String(), limit.String()) + } + } + } + + return nil +} + +// +func updateByTaskLevelRequestResources(taskSpec *v1beta1.TaskSpec) { + +} + // validateOverrides validates that all stepOverrides map to valid steps, and likewise for sidecarOverrides func validateOverrides(ctx context.Context, ts *v1beta1.TaskSpec, trs *v1beta1.TaskRunSpec) error { stepErr := validateStepOverrides(ctx, ts, trs) diff --git a/pkg/reconciler/taskrun/validate_resources_test.go b/pkg/reconciler/taskrun/validate_resources_test.go index 6feae3f9044..ed09a52c3d7 100644 --- a/pkg/reconciler/taskrun/validate_resources_test.go +++ b/pkg/reconciler/taskrun/validate_resources_test.go @@ -21,6 +21,8 @@ import ( "testing" "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/tektoncd/pipeline/pkg/apis/config" @@ -550,3 +552,111 @@ func TestValidateOverrides(t *testing.T) { }) } } + +func Test_validateTaskLevelResourcesRequirements_validResourcesRequirements(t *testing.T) { + taskSpecs := []struct { + name string + taskSpec *v1beta1.TaskSpec + }{{ + name: "only valid 'limits' configured", + taskSpec: &v1beta1.TaskSpec{ + Resources: &v1beta1.TaskResources{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("4"), + corev1.ResourceMemory: resource.MustParse("8Gi"), + }, + }, + }, + }, { + name: "only valid 'requests' configured", + taskSpec: &v1beta1.TaskSpec{ + Resources: &v1beta1.TaskResources{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + }, + }, + }, { + name: "valid 'requests' <= 'limits' configured", + taskSpec: &v1beta1.TaskSpec{ + Resources: &v1beta1.TaskResources{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("8Gi"), + }, + }, + }, + }} + + for _, taskSpec := range taskSpecs { + t.Run(taskSpec.name, func(t *testing.T) { + if err := validateTaskLevelResourcesRequirements(taskSpec.taskSpec); err != nil { + t.Errorf("Expected no errors when validating valid task-level resources requirements, but got: %v.", err) + } + }) + } +} + +func Test_validateTaskLevelResourcesRequirements_invalidResourcesRequirements(t *testing.T) { + taskSpecs := []struct { + name string + taskSpec *v1beta1.TaskSpec + }{{ + name: "invalid 'cpu' requests > limits configured", + taskSpec: &v1beta1.TaskSpec{ + Resources: &v1beta1.TaskResources{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("4"), + }, + }, + }, + }, { + name: "invalid 'memory' requests > limits configured", + taskSpec: &v1beta1.TaskSpec{ + Resources: &v1beta1.TaskResources{ + Requests: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("8Gi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + }, + }, + }, { + name: "at least one invalid 'requests' > 'limits' configured", + taskSpec: &v1beta1.TaskSpec{ + Resources: &v1beta1.TaskResources{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8.1"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + }, + }, + }} + + for _, taskSpec := range taskSpecs { + t.Run(taskSpec.name, func(t *testing.T) { + if err := validateTaskLevelResourcesRequirements(taskSpec.taskSpec); err == nil { + t.Errorf("Expected errors when validating invalid task-level resources requirements, but got none.") + } + }) + } +} + +func Test_validateTaskSpecRequestResources_validateTaskLevelRequirementsPrecedence(t *testing.T) { + // only with step + // only with step template + // with both +}