[TEP-0135] Revert PVC creation - e2e

Part of [tektoncd#6740] and unblocks [tektoncd#6635]. `PVCs` are created if a user specifies `VolumeClaimTemplate` as the source of a `pipelinerun workspace`.
Prior to this change, such `pvcs` are created via `affinity assistant statefulset` when `affinity assistant` is enabled (in both `workspaces` or `pipelineruns`
coschedule mode).

To delete such pvcs when the owning `pipelinerun` is completed, we must explicitly delete those pvcs in the reconciler since the owner of such pvcs is the `affinity assistant statefulset`
instead of the `pipelinerun`. The problem of such deletion mechanism is that such `pvcs` are left in `terminating` state when the owning `pipelinerun` is `completed` but not `deleted`.
This is because the pvcs are protected by `kubernetes.io/pvc-protection` `finalizer`, which does not allow the `pvcs` to be deleted until the `pipelinerun` consuming the `pvc` is deleted.
Leaving pvcs in `terminating` state is confusing to cluster operators. Instead of changing the pvc deletion behavior in such backward incompatible way,
it is better to make it configurable so that it is backward compatible, as prototyped in [tektoncd#6635].

This commit reverts the pvc creation behavior `per-workspace` coschedule mode, which changes the owner of the `pvcs` back to the `pipelinerun` instead of the
`affinity assistant statefulset`. After the commit, the pvcs created by specifying `VolumeClaimTemplate` are left in `bounded` state instead of `terminating`.
This commit is the prerequisite of [tektoncd#6635].

This commit does NOT reverts the pvc creation behavior `per-pipelinerun` coschedule mode as we will enforce the deletion of pvcs when the owning `pipelinerun` is completed.
This is a better practice and there is no backward compatability concern since `per-pipelinerun` coschedule mode is a new feature.

[tektoncd#6740]: tektoncd#6740
[tektoncd#6635]: tektoncd#6635

Author: QuanZhang-William

pkg/reconciler/pipelinerun/affinity_assistant.go

@@ -40,9 +40,9 @@ import (
 )
 
 const (
-	// ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSetPerWorkspace indicates that a PipelineRun uses workspaces with PersistentVolumeClaim
+	// ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSet indicates that a PipelineRun uses workspaces with PersistentVolumeClaim
 	// as a volume source and expect an Assistant StatefulSet in AffinityAssistantPerWorkspace behavior, but couldn't create a StatefulSet.
-	ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSetPerWorkspace = "ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSetPerWorkspace"
+	ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSet = "ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSet"
 
 	featureFlagDisableAffinityAssistantKey = "disable-affinity-assistant"
 )
@@ -103,18 +103,21 @@ func (c *Reconciler) createOrUpdateAffinityAssistantsAndPVCs(ctx context.Context
 			errs = append(errs, err...)
 		}
 	case aa.AffinityAssistantPerPipelineRun, aa.AffinityAssistantPerPipelineRunWithIsolation:
-		if claims != nil || claimTemplates != nil {
-			aaName := GetAffinityAssistantName("", pr.Name)
-			// In AffinityAssistantPerPipelineRun or AffinityAssistantPerPipelineRunWithIsolation modes, the PVCs are created via StatefulSet for volume scheduling.
-			// PVCs from pipelinerun's VolumeClaimTemplate are enforced to be deleted at pipelinerun completion time,
-			// so we don't need to worry the OwnerReference of the PVCs
-			err := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, claimTemplates, claims, unschedulableNodes)
-			errs = append(errs, err...)
-		}
+		aaName := GetAffinityAssistantName("", pr.Name)
+		// In AffinityAssistantPerPipelineRun or AffinityAssistantPerPipelineRunWithIsolation modes, the PVCs are created via StatefulSet for volume scheduling.
+		// PVCs from pipelinerun's VolumeClaimTemplate are enforced to be deleted at pipelinerun completion time,
+		// so we don't need to worry the OwnerReference of the PVCs
+		err := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, claimTemplates, claims, unschedulableNodes)
+		errs = append(errs, err...)
 	case aa.AffinityAssistantDisabled:
 	}
 
-	return errorutils.NewAggregate(errs)
+	failReason := ""
+	if errs != nil {
+		failReason = ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSet
+	}
+
+	return errorutils.NewAggregate(errs), failReason
 }
 
 // createOrUpdateAffinityAssistant creates an Affinity Assistant Statefulset with the provided affinityAssistantName and pipelinerun information.
@@ -178,29 +181,39 @@ func (c *Reconciler) createOrUpdateAffinityAssistant(ctx context.Context, affini
 	return errs
 }
 
-// TODO(#6740)(WIP) implement cleanupAffinityAssistants for AffinityAssistantPerPipelineRun and AffinityAssistantPerPipelineRunWithIsolation affinity assistant modes
+// cleanupAffinityAssistants deletes affinity assistant StatefulSet
 func (c *Reconciler) cleanupAffinityAssistants(ctx context.Context, pr *v1.PipelineRun) error {
-	// omit cleanup if the feature is disabled
-	if c.isAffinityAssistantDisabled(ctx) {
-		return nil
+	aaBehavior, err := aa.GetAffinityAssistantBehavior(ctx)
+	if err != nil {
+		return err
 	}
 
 	var errs []error
-	for _, w := range pr.Spec.Workspaces {
-		if w.PersistentVolumeClaim != nil || w.VolumeClaimTemplate != nil {
-			affinityAssistantStsName := GetAffinityAssistantName(w.Name, pr.Name)
-			if err := c.KubeClientSet.AppsV1().StatefulSets(pr.Namespace).Delete(ctx, affinityAssistantStsName, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
-				errs = append(errs, fmt.Errorf("failed to delete StatefulSet %s: %w", affinityAssistantStsName, err))
+	switch aaBehavior {
+	case aa.AffinityAssistantPerWorkspace:
+		for _, w := range pr.Spec.Workspaces {
+			if w.PersistentVolumeClaim != nil || w.VolumeClaimTemplate != nil {
+				affinityAssistantStsName := GetAffinityAssistantName(w.Name, pr.Name)
+				if err := c.KubeClientSet.AppsV1().StatefulSets(pr.Namespace).Delete(ctx, affinityAssistantStsName, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
+					errs = append(errs, fmt.Errorf("failed to delete StatefulSet %s: %w", affinityAssistantStsName, err))
+				}
 			}
 		}
+	case aa.AffinityAssistantPerPipelineRun, aa.AffinityAssistantPerPipelineRunWithIsolation:
+		affinityAssistantStsName := GetAffinityAssistantName("", pr.Name)
+		if err := c.KubeClientSet.AppsV1().StatefulSets(pr.Namespace).Delete(ctx, affinityAssistantStsName, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
+			errs = append(errs, fmt.Errorf("failed to delete StatefulSet %s: %w", affinityAssistantStsName, err))
+		}
+	case aa.AffinityAssistantDisabled:
+		return nil
 	}
+
 	return errorutils.NewAggregate(errs)
 }
 
 // getPersistentVolumeClaimNameWithAffinityAssistant returns the PersistentVolumeClaim name that is
 // created by the Affinity Assistant StatefulSet VolumeClaimTemplate when Affinity Assistant is enabled.
 // The PVCs created by StatefulSet VolumeClaimTemplates follow the format `<pvcName>-<affinityAssistantName>-0`
-// TODO(#6740)(WIP): use this function when adding end-to-end support for AffinityAssistantPerPipelineRun mode
 func getPersistentVolumeClaimNameWithAffinityAssistant(pipelineWorkspaceName, prName string, wb v1.WorkspaceBinding, owner metav1.OwnerReference) string {
 	pvcName := volumeclaim.GetPVCNameWithoutAffinityAssistant(wb.VolumeClaimTemplate.Name, wb, owner)
 	affinityAssistantName := GetAffinityAssistantName(pipelineWorkspaceName, prName)
@@ -322,17 +335,6 @@ func affinityAssistantStatefulSet(name string, pr *v1.PipelineRun, claimTemplate
 	}
 }
 
-// isAffinityAssistantDisabled returns a bool indicating whether an Affinity Assistant should
-// be created for each PipelineRun that use workspaces with PersistentVolumeClaims
-// as volume source. The default behaviour is to enable the Affinity Assistant to
-// provide Node Affinity for TaskRuns that share a PVC workspace.
-//
-// TODO(#6740)(WIP): replace this function with GetAffinityAssistantBehavior
-func (c *Reconciler) isAffinityAssistantDisabled(ctx context.Context) bool {
-	cfg := config.FromContextOrDefaults(ctx)
-	return cfg.FeatureFlags.DisableAffinityAssistant
-}
-
 // getAssistantAffinityMergedWithPodTemplateAffinity return the affinity that merged with PipelineRun PodTemplate affinity.
 func getAssistantAffinityMergedWithPodTemplateAffinity(pr *v1.PipelineRun) *corev1.Affinity {
 	// use podAntiAffinity to repel other affinity assistants

pkg/reconciler/pipelinerun/affinity_assistant_test.go

@@ -754,53 +754,6 @@ func TestThatCleanupIsAvoidedIfAssistantIsDisabled(t *testing.T) {
 	}
 }
 
-func TestDisableAffinityAssistant(t *testing.T) {
-	for _, tc := range []struct {
-		description string
-		configMap   *corev1.ConfigMap
-		expected    bool
-	}{{
-		description: "Default behaviour: A missing disable-affinity-assistant flag should result in false",
-		configMap: &corev1.ConfigMap{
-			ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()},
-			Data:       map[string]string{},
-		},
-		expected: false,
-	}, {
-		description: "Setting disable-affinity-assistant to false should result in false",
-		configMap: &corev1.ConfigMap{
-			ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()},
-			Data: map[string]string{
-				featureFlagDisableAffinityAssistantKey: "false",
-			},
-		},
-		expected: false,
-	}, {
-		description: "Setting disable-affinity-assistant to true should result in true",
-		configMap: &corev1.ConfigMap{
-			ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()},
-			Data: map[string]string{
-				featureFlagDisableAffinityAssistantKey: "true",
-			},
-		},
-		expected: true,
-	}} {
-		t.Run(tc.description, func(t *testing.T) {
-			c := Reconciler{
-				KubeClientSet: fakek8s.NewSimpleClientset(
-					tc.configMap,
-				),
-				Images: pipeline.Images{},
-			}
-			store := config.NewStore(logtesting.TestLogger(t))
-			store.OnConfigChanged(tc.configMap)
-			if result := c.isAffinityAssistantDisabled(store.ToContext(context.Background())); result != tc.expected {
-				t.Errorf("Expected %t Received %t", tc.expected, result)
-			}
-		})
-	}
-}
-
 func TestGetAssistantAffinityMergedWithPodTemplateAffinity(t *testing.T) {
 	assistantPodAffinityTerm := corev1.WeightedPodAffinityTerm{
 		Weight: 100,

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -611,25 +611,12 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel
 			return controller.NewPermanentError(err)
 		}
 
-		switch aaBehavior {
-		case affinityassistant.AffinityAssistantPerWorkspace, affinityassistant.AffinityAssistantDisabled:
-			if err = c.createOrUpdateAffinityAssistantsAndPVCs(ctx, pr, aaBehavior); err != nil {
-				logger.Errorf("Failed to create PVC or affinity assistant StatefulSet for PipelineRun %s: %v", pr.Name, err)
-				if aaBehavior == affinityassistant.AffinityAssistantPerWorkspace {
-					pr.Status.MarkFailed(ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSetPerWorkspace,
-						"Failed to create StatefulSet for PipelineRun %s/%s correctly: %s",
-						pr.Namespace, pr.Name, err)
-				} else {
-					pr.Status.MarkFailed(volumeclaim.ReasonCouldntCreateWorkspacePVC,
-						"Failed to create PVC for PipelineRun %s/%s Workspaces correctly: %s",
-						pr.Namespace, pr.Name, err)
-				}
-
-				return controller.NewPermanentError(err)
-			}
-		case affinityassistant.AffinityAssistantPerPipelineRun, affinityassistant.AffinityAssistantPerPipelineRunWithIsolation:
-			// TODO(#6740)(WIP): implement end-to-end support for AffinityAssistantPerPipelineRun and AffinityAssistantPerPipelineRunWithIsolation modes
-			return controller.NewPermanentError(fmt.Errorf("affinity assistant behavior: %v is not implemented", aaBehavior))
+		// TODO: better error message
+		if err, reason := c.createOrUpdateAffinityAssistantsAndPVCsPerAABehavior(ctx, pr, aaBehavior); err != nil {
+			logger.Errorf("Failed to create PVC or affinity assistant StatefulSet for PipelineRun %s: %v", pr.Name, err)
+			pr.Status.MarkFailed(reason,
+				"Failed to create PVC or StatefulSet for PipelineRun %s/%s correctly: %s",
+				pr.Namespace, pr.Name, err)
 		}
 	}
 
@@ -876,8 +863,20 @@ func (c *Reconciler) createTaskRun(ctx context.Context, taskRunName string, para
 		return nil, err
 	}
 
-	if !c.isAffinityAssistantDisabled(ctx) && pipelinePVCWorkspaceName != "" {
-		tr.Annotations[workspace.AnnotationAffinityAssistantName] = GetAffinityAssistantName(pipelinePVCWorkspaceName, pr.Name)
+	aaBehavior, err := affinityassistant.GetAffinityAssistantBehavior(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	switch aaBehavior {
+	case affinityassistant.AffinityAssistantPerWorkspace:
+		if pipelinePVCWorkspaceName != "" {
+			tr.Annotations[workspace.AnnotationAffinityAssistantName] = GetAffinityAssistantName(pipelinePVCWorkspaceName, pr.Name)
+		}
+	case affinityassistant.AffinityAssistantPerPipelineRun, affinityassistant.AffinityAssistantPerPipelineRunWithIsolation:
+		tr.Annotations[workspace.AnnotationAffinityAssistantName] = GetAffinityAssistantName("", pr.Name)
+
+	case affinityassistant.AffinityAssistantDisabled:
 	}
 
 	logger.Infof("Creating a new TaskRun object %s for pipeline task %s", taskRunName, rpt.PipelineTask.Name)
@@ -997,10 +996,23 @@ func (c *Reconciler) createCustomRun(ctx context.Context, runName string, params
 			},
 		}
 	}
+
 	// Set the affinity assistant annotation in case the custom task creates TaskRuns or Pods
 	// that can take advantage of it.
-	if !c.isAffinityAssistantDisabled(ctx) && pipelinePVCWorkspaceName != "" {
-		r.Annotations[workspace.AnnotationAffinityAssistantName] = GetAffinityAssistantName(pipelinePVCWorkspaceName, pr.Name)
+	aaBehavior, err := affinityassistant.GetAffinityAssistantBehavior(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	switch aaBehavior {
+	case affinityassistant.AffinityAssistantPerWorkspace:
+		if pipelinePVCWorkspaceName != "" {
+			r.Annotations[workspace.AnnotationAffinityAssistantName] = GetAffinityAssistantName(pipelinePVCWorkspaceName, pr.Name)
+		}
+	case affinityassistant.AffinityAssistantPerPipelineRun, affinityassistant.AffinityAssistantPerPipelineRunWithIsolation:
+		r.Annotations[workspace.AnnotationAffinityAssistantName] = GetAffinityAssistantName("", pr.Name)
+
+	case affinityassistant.AffinityAssistantDisabled:
 	}
 
 	logger.Infof("Creating a new CustomRun object %s", runName)
@@ -1116,9 +1128,11 @@ func (c *Reconciler) taskWorkspaceByWorkspaceVolumeSource(ctx context.Context, p
 	}
 	binding.Name = taskWorkspaceName
 
-	// TODO(#6740)(WIP): get binding for AffinityAssistantPerPipelineRun and AffinityAssistantPerPipelineRunWithIsolation mode
-	if aaBehavior == affinityassistant.AffinityAssistantDisabled || aaBehavior == affinityassistant.AffinityAssistantPerWorkspace {
+	switch aaBehavior {
+	case affinityassistant.AffinityAssistantPerWorkspace, affinityassistant.AffinityAssistantDisabled:
 		binding.PersistentVolumeClaim.ClaimName = volumeclaim.GetPVCNameWithoutAffinityAssistant(wb.VolumeClaimTemplate.Name, wb, owner)
+	case affinityassistant.AffinityAssistantPerPipelineRun, affinityassistant.AffinityAssistantPerPipelineRunWithIsolation:
+		binding.PersistentVolumeClaim.ClaimName = getPersistentVolumeClaimNameWithAffinityAssistant("", prName, wb, owner)
 	}
 
 	return binding

pkg/reconciler/taskrun/taskrun.go

@@ -418,7 +418,11 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec,
 		return nil, nil, controller.NewPermanentError(err)
 	}
 
-	if _, usesAssistant := tr.Annotations[workspace.AnnotationAffinityAssistantName]; usesAssistant {
+	aaBehavior, err := affinityassistant.GetAffinityAssistantBehavior(ctx)
+	if err != nil {
+		return nil, nil, controller.NewPermanentError(err)
+	}
+	if aaBehavior == affinityassistant.AffinityAssistantPerWorkspace {
 		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)