From a162a1d4879ccc1576550aecfc1ca07df5cee26c Mon Sep 17 00:00:00 2001 From: Jonas Pettersson Date: Tue, 11 Aug 2020 21:21:04 +0200 Subject: [PATCH] Add support for repeated PVC-claim but using subPath in AA-validation The validation for compatibility with the Affinity Assistant does not support the same PVC repeated, but using different subPaths. This patch adds support for this case and tests for the validation. Co-authored-by: Scott --- pkg/reconciler/taskrun/taskrun.go | 33 +++--------- pkg/workspace/validate.go | 22 ++++++++ pkg/workspace/validate_test.go | 88 +++++++++++++++++++++++++++++++ 3 files changed, 116 insertions(+), 27 deletions(-) diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 6da61b6f515..7c9e7dd573a 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -302,10 +302,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) + if _, usesAssistant := tr.Annotations[workspace.AnnotationAffinityAssistantName]; usesAssistant { + if err := workspace.ValidateOnlyOnePVCIsUsed(tr.Spec.Workspaces); err != nil { + logger.Errorf("TaskRun %q workspaces incompatible with Affinity Assistant: %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 @@ -775,26 +777,3 @@ 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/workspace/validate.go b/pkg/workspace/validate.go index 2738b270b7d..18ba1535451 100644 --- a/pkg/workspace/validate.go +++ b/pkg/workspace/validate.go @@ -48,3 +48,25 @@ func ValidateBindings(w []v1beta1.WorkspaceDeclaration, wb []v1beta1.WorkspaceBi } return nil } + +// ValidateOnlyOnePVCIsUsed checks that a list of WorkspaceBinding uses only one +// persistent volume claim. +// +// This is only useful to validate that WorkspaceBindings in TaskRuns are compatible +// with affinity rules enforced by the AffinityAssistant. +func ValidateOnlyOnePVCIsUsed(wb []v1beta1.WorkspaceBinding) error { + workspaceVolumes := make(map[string]bool) + for _, w := range wb { + if w.PersistentVolumeClaim != nil { + workspaceVolumes[w.PersistentVolumeClaim.ClaimName] = true + } + if w.VolumeClaimTemplate != nil { + workspaceVolumes[w.Name] = true + } + } + + if len(workspaceVolumes) > 1 { + return fmt.Errorf("more than one PersistentVolumeClaim is bound") + } + return nil +} diff --git a/pkg/workspace/validate_test.go b/pkg/workspace/validate_test.go index e79d104a509..7d115a93136 100644 --- a/pkg/workspace/validate_test.go +++ b/pkg/workspace/validate_test.go @@ -17,9 +17,11 @@ limitations under the License. package workspace import ( + "errors" "testing" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" corev1 "k8s.io/api/core/v1" ) @@ -131,3 +133,89 @@ func TestValidateBindingsInvalid(t *testing.T) { }) } } + +func TestValidateOnlyOnePVCIsUsed_Valid(t *testing.T) { + for _, tc := range []struct { + name string + bindings []v1beta1.WorkspaceBinding + }{{ + name: "an error is not returned when no bindings are given", + bindings: []v1beta1.WorkspaceBinding{}, + }, { + name: "an error is not returned when volume claims are not used", + bindings: []v1beta1.WorkspaceBinding{{ + EmptyDir: &corev1.EmptyDirVolumeSource{}, + }, { + Secret: &corev1.SecretVolumeSource{}, + }}, + }, { + name: "an error is not returned when one PV claim is used in two bindings", + bindings: []v1beta1.WorkspaceBinding{{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "foo", + }, + }, { + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "foo", + }, + }}, + }, { + name: "an error is not returned when one PV claim is used in two bindings with different subpaths", + bindings: []v1beta1.WorkspaceBinding{{ + SubPath: "/pathA", + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "foo", + }, + }, { + SubPath: "/pathB", + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "foo", + }, + }}, + }} { + t.Run(tc.name, func(t *testing.T) { + if err := ValidateOnlyOnePVCIsUsed(tc.bindings); err != nil { + t.Errorf("unexpected error: %v", err) + } + }) + } +} + +func TestValidateOnlyOnePVCIsUsed_Invalid(t *testing.T) { + validationError := errors.New("more than one PersistentVolumeClaim is bound") + for _, tc := range []struct { + name string + bindings []v1beta1.WorkspaceBinding + wantErr error + }{{ + name: "an error is returned when two different PV claims are used", + bindings: []v1beta1.WorkspaceBinding{{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "foo", + }, + }, { + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "bar", + }, + }}, + wantErr: validationError, + }, { + name: "an error is returned when a PVC and volume claim template are mixed", + bindings: []v1beta1.WorkspaceBinding{{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "foo", + }, + }, { + Name: "bar", + VolumeClaimTemplate: &corev1.PersistentVolumeClaim{}, + }}, + wantErr: validationError, + }} { + t.Run(tc.name, func(t *testing.T) { + err := ValidateOnlyOnePVCIsUsed(tc.bindings) + if err == nil || (tc.wantErr.Error() != err.Error()) { + t.Errorf("expected %v received %v", tc.wantErr, err) + } + }) + } +}