Skip to content

Commit

Permalink
add taskspec resources validation
Browse files Browse the repository at this point in the history
  • Loading branch information
@Yongxuanzhang @tekton-robot
Yongxuanzhang authored and tekton-robot committed Feb 9, 2022
1 parent 24099c3 commit c0ab5f3
Show file tree
Hide file tree
Showing 3 changed files with 248 additions and 0 deletions.
6 changes: 6 additions & 0 deletions pkg/reconciler/taskrun/taskrun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,12 @@ 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)
tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err)
return nil, nil, controller.NewPermanentError(err)
}

if err := ValidateResolvedTaskResources(ctx, tr.Spec.Params, rtr); err != nil {
logger.Errorf("TaskRun %q resources are invalid: %v", tr.Name, err)
tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err)
Expand Down
216 changes: 216 additions & 0 deletions pkg/reconciler/taskrun/taskrun_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3379,6 +3379,50 @@ func TestReconcileTaskResourceResolutionAndValidation(t *testing.T) {
"Warning Failed", // Event about the TaskRun state changed
"Warning InternalError", // Event about the error (generated by the genreconciler)
},
}, {
desc: "Fail ValidateTaskSpecRequestResources",
d: test.Data{
Tasks: []*v1beta1.Task{{
ObjectMeta: objectMeta("test-task-invalid-taskspec-resource", "foo"),
Spec: v1beta1.TaskSpec{
Workspaces: []v1beta1.WorkspaceDeclaration{{
Name: "ws1",
Description: "a test task workspace",
ReadOnly: true,
}},
Steps: []v1beta1.Step{{Container: corev1.Container{
Image: "image",
Command: []string{"cmd"},
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("8"),
corev1.ResourceMemory: resource.MustParse("8Gi"),
},
Limits: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("8"),
corev1.ResourceMemory: resource.MustParse("4Gi"),
},
},
}}}},
}},
TaskRuns: []*v1beta1.TaskRun{{
ObjectMeta: objectMeta("test-taskrun-invalid-taskspec-resource", "foo"),
Spec: v1beta1.TaskRunSpec{
TaskRef: &v1beta1.TaskRef{
Name: "test-task-invalid-taskspec-resource",
APIVersion: "a1",
},
},
}},
ClusterTasks: nil,
PipelineResources: nil,
},
wantFailedReason: podconvert.ReasonFailedValidation,
wantEvents: []string{
"Normal Started ",
"Warning Failed", // Event about the TaskRun state changed
"Warning InternalError", // Event about the error (generated by the genreconciler)
},
}} {
t.Run(tt.desc, func(t *testing.T) {
testAssets, cancel := getTaskRunController(t, tt.d)
Expand Down Expand Up @@ -4371,6 +4415,178 @@ func TestStopSidecars_NoClientGetPodForTaskSpecWithoutRunningSidecars(t *testing
}
}

func Test_validateTaskSpecRequestResources_ValidResources(t *testing.T) {
ctx := context.Background()

tcs := []struct {
name string
taskSpec *v1beta1.TaskSpec
}{{
name: "no requested resources",
taskSpec: &v1beta1.TaskSpec{
Steps: []v1beta1.Step{
{
Container: corev1.Container{
Image: "image",
Command: []string{"cmd"},
},
}},
StepTemplate: &corev1.Container{
Resources: corev1.ResourceRequirements{
Limits: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("4"),
corev1.ResourceMemory: resource.MustParse("8Gi"),
},
},
},
},
}, {
name: "no limit configured",
taskSpec: &v1beta1.TaskSpec{
Steps: []v1beta1.Step{{Container: corev1.Container{
Image: "image",
Command: []string{"cmd"},
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("8"),
corev1.ResourceMemory: resource.MustParse("4Gi"),
},
},
}}},
},
}, {
name: "request less or equal than step limit but larger than steptemplate limit",
taskSpec: &v1beta1.TaskSpec{
Steps: []v1beta1.Step{{Container: corev1.Container{
Image: "image",
Command: []string{"cmd"},
Resources: corev1.ResourceRequirements{
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"),
},
},
}}},
StepTemplate: &corev1.Container{
Resources: corev1.ResourceRequirements{
Limits: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("4"),
corev1.ResourceMemory: resource.MustParse("4Gi"),
},
},
},
},
}, {
name: "request less or equal than step limit",
taskSpec: &v1beta1.TaskSpec{
Steps: []v1beta1.Step{{Container: corev1.Container{
Image: "image",
Command: []string{"cmd"},
Resources: corev1.ResourceRequirements{
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"),
},
},
}}},
},
}, {
name: "request less or equal than steptemplate limit",
taskSpec: &v1beta1.TaskSpec{
Steps: []v1beta1.Step{{Container: corev1.Container{
Image: "image",
Command: []string{"cmd"},
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("8"),
corev1.ResourceMemory: resource.MustParse("4Gi"),
},
},
}}},
StepTemplate: &corev1.Container{
Resources: corev1.ResourceRequirements{
Limits: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("8"),
corev1.ResourceMemory: resource.MustParse("8Gi"),
},
},
},
},
}}

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")
}
})
}

}

func Test_validateTaskSpecRequestResources_InvalidResources(t *testing.T) {
ctx := context.Background()
tcs := []struct {
name string
taskSpec *v1beta1.TaskSpec
}{{
name: "step request larger than step limit",
taskSpec: &v1beta1.TaskSpec{
Steps: []v1beta1.Step{{Container: corev1.Container{
Image: "image",
Command: []string{"cmd"},
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("8"),
corev1.ResourceMemory: resource.MustParse("8Gi"),
},
Limits: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("8"),
corev1.ResourceMemory: resource.MustParse("4Gi"),
},
},
}}}},
}, {
name: "step request larger than steptemplate limit",
taskSpec: &v1beta1.TaskSpec{
Steps: []v1beta1.Step{{Container: corev1.Container{
Image: "image",
Command: []string{"cmd"},
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("8"),
corev1.ResourceMemory: resource.MustParse("8Gi"),
},
},
}}},
StepTemplate: &corev1.Container{
Resources: corev1.ResourceRequirements{
Limits: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("8"),
corev1.ResourceMemory: resource.MustParse("4Gi"),
},
},
},
},
}}

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")
}
})
}
}

func podVolumeMounts(idx, totalSteps int) []corev1.VolumeMount {
var mnts []corev1.VolumeMount
mnts = append(mnts, corev1.VolumeMount{
Expand Down
26 changes: 26 additions & 0 deletions pkg/reconciler/taskrun/validate_resources.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,3 +142,29 @@ 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 {
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 nil
}

0 comments on commit c0ab5f3

Please sign in to comment.