From 1cd4d8cc0cde351bf1a2a96a096c53a1fe133591 Mon Sep 17 00:00:00 2001 From: Jonas Pettersson Date: Mon, 29 Jun 2020 12:12:31 +0200 Subject: [PATCH] Validate TaskRun compatibility with the Affinity Assistant A TaskRun that mount more than one PVC-backed workspace is incompatible with the Affinity Assistant. But there is no validation if the TaskRun is compatible - so the TaskRun Pod is stuck with little information on why to the user. This commit adds validation of TaskRuns. When a TaskRun is associated with an Affinity Assistant, it is checked that not more than one PVC workspace is used - if so, the TaskRun will fail with a TaskRunValidationFailed condition. Proposed in https://github.com/tektoncd/pipeline/issues/2829#issuecomment-645868118 Closes https://github.com/tektoncd/pipeline/issues/2864 --- pkg/reconciler/taskrun/taskrun.go | 29 +++++++++++ pkg/reconciler/taskrun/taskrun_test.go | 68 ++++++++++++++++++++++++++ 2 files changed, 97 insertions(+) diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index ba31bb751a0..690abcf6dd9 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -294,6 +294,12 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1beta1.TaskRun) (*v1beta1 return nil, nil, controller.NewPermanentError(err) } + if err := validateWorkspaceCompatibilityWithAffinityAssistant(tr); err != nil { + logger.Errorf("TaskRun %q workspaces are invalid: %v", tr.Name, err) + tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err) + return nil, nil, controller.NewPermanentError(err) + } + // Initialize the cloud events if at least a CloudEventResource is defined // and they have not been initialized yet. // FIXME(afrittoli) This resource specific logic will have to be replaced @@ -702,3 +708,26 @@ func storeTaskSpec(ctx context.Context, tr *v1beta1.TaskRun, ts *v1beta1.TaskSpe } return nil } + +// validateWorkspaceCompatibilityWithAffinityAssistant validates the TaskRun's compatibility +// with the Affinity Assistant - if associated with an Affinity Assistant. +// No more than one PVC-backed workspace can be used for a TaskRun that is associated with an +// Affinity Assistant. +func validateWorkspaceCompatibilityWithAffinityAssistant(tr *v1beta1.TaskRun) error { + _, isAssociatedWithAnAffinityAssistant := tr.Annotations[workspace.AnnotationAffinityAssistantName] + if !isAssociatedWithAnAffinityAssistant { + return nil + } + + pvcWorkspaces := 0 + for _, w := range tr.Spec.Workspaces { + if w.PersistentVolumeClaim != nil || w.VolumeClaimTemplate != nil { + pvcWorkspaces++ + } + } + + if pvcWorkspaces > 1 { + return fmt.Errorf("TaskRun mounts more than one PersistentVolumeClaim - that is forbidden when the Affinity Assistant is enabled") + } + return nil +} diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index d2adf189573..20b2d1930d2 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -38,6 +38,7 @@ import ( "github.com/tektoncd/pipeline/pkg/reconciler/volumeclaim" "github.com/tektoncd/pipeline/pkg/system" "github.com/tektoncd/pipeline/pkg/timeout" + "github.com/tektoncd/pipeline/pkg/workspace" test "github.com/tektoncd/pipeline/test" "github.com/tektoncd/pipeline/test/diff" "github.com/tektoncd/pipeline/test/names" @@ -2820,6 +2821,73 @@ func TestReconcileTaskResourceResolutionAndValidation(t *testing.T) { } } +// TestReconcileWithWorkspacesIncompatibleWithAffinityAssistant tests that a TaskRun used with an associated +// Affinity Assistant is validated and that the validation fails for a TaskRun that is incompatible with +// Affinity Assistant; e.g. using more than one PVC-backed workspace. +func TestReconcileWithWorkspacesIncompatibleWithAffinityAssistant(t *testing.T) { + taskWithTwoWorkspaces := tb.Task("test-task-two-workspaces", tb.TaskNamespace("foo"), + tb.TaskSpec( + tb.TaskWorkspace("ws1", "task workspace", "", true), + tb.TaskWorkspace("ws2", "another workspace", "", false), + )) + taskRun := tb.TaskRun("taskrun-with-two-workspaces", tb.TaskRunNamespace("foo"), tb.TaskRunSpec( + tb.TaskRunTaskRef(taskWithTwoWorkspaces.Name, tb.TaskRefAPIVersion("a1")), + tb.TaskRunWorkspacePVC("ws1", "", "pvc1"), + tb.TaskRunWorkspaceVolumeClaimTemplate("ws2", "", &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pvc2", + }, + Spec: corev1.PersistentVolumeClaimSpec{}, + }), + )) + + // associate the TaskRun with a dummy Affinity Assistant + taskRun.Annotations[workspace.AnnotationAffinityAssistantName] = "dummy-affinity-assistant" + + d := test.Data{ + Tasks: []*v1beta1.Task{taskWithTwoWorkspaces}, + TaskRuns: []*v1beta1.TaskRun{taskRun}, + ClusterTasks: nil, + PipelineResources: nil, + } + names.TestingSeed() + testAssets, cancel := getTaskRunController(t, d) + defer cancel() + clients := testAssets.Clients + + t.Logf("Creating SA %s in %s", "default", "foo") + if _, err := clients.Kube.CoreV1().ServiceAccounts("foo").Create(&corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "foo", + }, + }); err != nil { + t.Fatal(err) + } + + _ = testAssets.Controller.Reconciler.Reconcile(context.Background(), getRunName(taskRun)) + + _, err := clients.Pipeline.TektonV1beta1().Tasks(taskRun.Namespace).Get(taskWithTwoWorkspaces.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("krux: %v", err) + } + + ttt, err := clients.Pipeline.TektonV1beta1().TaskRuns(taskRun.Namespace).Get(taskRun.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("expected TaskRun %s to exist but instead got error when getting it: %v", taskRun.Name, err) + } + + if len(ttt.Status.Conditions) != 1 { + t.Errorf("unexpected number of Conditions, expected 1 Condition") + } + + for _, cond := range ttt.Status.Conditions { + if cond.Reason != podconvert.ReasonFailedValidation { + t.Errorf("unexpected Reason on the Condition, expected: %s, got: %s", podconvert.ReasonFailedValidation, cond.Reason) + } + } +} + // TestReconcileWorkspaceWithVolumeClaimTemplate tests a reconcile of a TaskRun that has // a Workspace with VolumeClaimTemplate and check that it is translated to a created PersistentVolumeClaim. func TestReconcileWorkspaceWithVolumeClaimTemplate(t *testing.T) {