Skip to content

Commit f88f1bf

Browse files
QuanZhang-WilliamQuanZhang-William
committed
[TEP-0135] Revert PVC creation
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
1 parent d2cb90d commit f88f1bf

File tree

4 files changed

+76
-66
lines changed

4 files changed

+76
-66
lines changed

‎pkg/reconciler/pipelinerun/affinity_assistant.go‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ import (
4141

4242
const (
4343
// ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSetPerWorkspace indicates that a PipelineRun uses workspaces with PersistentVolumeClaim
44-
// as a volume source and expect an Assistant StatefulSet, but couldn't create a StatefulSet.
44+
// as a volume source and expect an Assistant StatefulSet in AffinityAssistantPerWorkspace behavior, but couldn't create a StatefulSet.
4545
ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSetPerWorkspace = "ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSetPerWorkspace"
4646

4747
featureFlagDisableAffinityAssistantKey = "disable-affinity-assistant"
@@ -88,12 +88,18 @@ func (c *Reconciler) createOrUpdateAffinityAssistantsPerAABehavior(ctx context.C
8888
}
8989
for claimTemplate, workspaceName := range claimTemplatesToWorkspace {
9090
aaName := getAffinityAssistantName(workspaceName, pr.Name)
91-
err := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, []corev1.PersistentVolumeClaim{*claimTemplate}, nil, unschedulableNodes)
91+
// To support PVC auto deletion at pipelinerun deletion time, the OwnerReference of the PVCs should be set to the owning pipelinerun.
92+
// In AffinityAssistantPerWorkspace mode, the reconciler has created PVCs (owned by pipelinerun) from pipelinerun's VolumeClaimTemplate at this point,
93+
// so the VolumeClaimTemplates are pass in as PVCs when creating affinity assistant StatefulSet for volume scheduling.
94+
// If passed in as VolumeClaimTemplates, the PVCs are owned by Affinity Assistant StatefulSet instead of the pipelinerun.
95+
err := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, nil, []corev1.PersistentVolumeClaimVolumeSource{{ClaimName: claimTemplate.Name}}, unschedulableNodes)
9296
errs = append(errs, err...)
9397
}
9498
case aa.AffinityAssistantPerPipelineRun, aa.AffinityAssistantPerPipelineRunWithIsolation:
9599
if claims != nil || claimTemplates != nil {
96100
aaName := getAffinityAssistantName("", pr.Name)
101+
// PVCs from pipelinerun's VolumeClaimTemplate are enforced to be deleted at pipelinerun completion time,
102+
// so we don't need to worry the OwnerReference of the PVCs
97103
err := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, claimTemplates, claims, unschedulableNodes)
98104
errs = append(errs, err...)
99105
}
@@ -179,14 +185,6 @@ func (c *Reconciler) cleanupAffinityAssistants(ctx context.Context, pr *v1.Pipel
179185
if err := c.KubeClientSet.AppsV1().StatefulSets(pr.Namespace).Delete(ctx, affinityAssistantStsName, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
180186
errs = append(errs, fmt.Errorf("failed to delete StatefulSet %s: %w", affinityAssistantStsName, err))
181187
}
182-
183-
// cleanup PVCs created by Affinity Assistants
184-
if w.VolumeClaimTemplate != nil {
185-
pvcName := getPersistentVolumeClaimNameWithAffinityAssistant(w.Name, pr.Name, w, *kmeta.NewControllerRef(pr))
186-
if err := c.KubeClientSet.CoreV1().PersistentVolumeClaims(pr.Namespace).Delete(ctx, pvcName, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
187-
errs = append(errs, fmt.Errorf("failed to delete PersistentVolumeClaim %s: %w", pvcName, err))
188-
}
189-
}
190188
}
191189
}
192190
return errorutils.NewAggregate(errs)
@@ -195,6 +193,7 @@ func (c *Reconciler) cleanupAffinityAssistants(ctx context.Context, pr *v1.Pipel
195193
// getPersistentVolumeClaimNameWithAffinityAssistant returns the PersistentVolumeClaim name that is
196194
// created by the Affinity Assistant StatefulSet VolumeClaimTemplate when Affinity Assistant is enabled.
197195
// The PVCs created by StatefulSet VolumeClaimTemplates follow the format `<pvcName>-<affinityAssistantName>-0`
196+
// TODO(#6740)(WIP): use this function when adding end-to-end support for AffinityAssistantPerPipelineRun mode
198197
func getPersistentVolumeClaimNameWithAffinityAssistant(pipelineWorkspaceName, prName string, wb v1.WorkspaceBinding, owner metav1.OwnerReference) string {
199198
pvcName := volumeclaim.GetPVCNameWithoutAffinityAssistant(wb.VolumeClaimTemplate.Name, wb, owner)
200199
affinityAssistantName := getAffinityAssistantName(pipelineWorkspaceName, prName)

‎pkg/reconciler/pipelinerun/affinity_assistant_test.go‎

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -208,17 +208,31 @@ func TestCreateAndDeleteOfAffinityAssistantPerWorkspace(t *testing.T) {
208208
name: "VolumeClaimTemplate Workspace type",
209209
pr: testPRWithVolumeClaimTemplate,
210210
expectStatefulSetSpec: []*appsv1.StatefulSetSpec{{
211-
VolumeClaimTemplates: []corev1.PersistentVolumeClaim{{
212-
ObjectMeta: metav1.ObjectMeta{Name: "pvc-b9eea16dce"},
213-
}},
211+
Template: corev1.PodTemplateSpec{
212+
Spec: corev1.PodSpec{
213+
Volumes: []corev1.Volume{{
214+
Name: "workspace-0",
215+
VolumeSource: corev1.VolumeSource{
216+
PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ClaimName: "pvc-b9eea16dce"},
217+
},
218+
}},
219+
},
220+
},
214221
}},
215222
}, {
216223
name: "VolumeClaimTemplate and PersistentVolumeClaim Workspaces",
217224
pr: testPRWithVolumeClaimTemplateAndPVC,
218225
expectStatefulSetSpec: []*appsv1.StatefulSetSpec{{
219-
VolumeClaimTemplates: []corev1.PersistentVolumeClaim{{
220-
ObjectMeta: metav1.ObjectMeta{Name: "pvc-b9eea16dce"},
221-
}}}, {
226+
Template: corev1.PodTemplateSpec{
227+
Spec: corev1.PodSpec{
228+
Volumes: []corev1.Volume{{
229+
Name: "workspace-0",
230+
VolumeSource: corev1.VolumeSource{
231+
PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ClaimName: "pvc-b9eea16dce"},
232+
},
233+
}},
234+
},
235+
}}, {
222236
Template: corev1.PodTemplateSpec{
223237
Spec: corev1.PodSpec{
224238
Volumes: []corev1.Volume{{
@@ -642,14 +656,12 @@ func TestCleanupAffinityAssistants_Success(t *testing.T) {
642656
ReadyReplicas: 1,
643657
},
644658
}}
645-
646659
expectedPVCName := getPersistentVolumeClaimNameWithAffinityAssistant(workspace.Name, pr.Name, workspace, *kmeta.NewControllerRef(pr))
647660
pvc := []*corev1.PersistentVolumeClaim{{
648661
ObjectMeta: metav1.ObjectMeta{
649662
Name: expectedPVCName,
650663
}},
651664
}
652-
653665
data := Data{
654666
StatefulSets: aa,
655667
PVCs: pvc,
@@ -667,9 +679,11 @@ func TestCleanupAffinityAssistants_Success(t *testing.T) {
667679
if !apierrors.IsNotFound(err) {
668680
t.Errorf("expected a NotFound response of StatefulSet, got: %v", err)
669681
}
682+
683+
// the PVCs are NOT expected to be deleted at Affinity Assistant cleanup time
670684
_, err = c.KubeClientSet.CoreV1().PersistentVolumeClaims(pr.Namespace).Get(ctx, expectedPVCName, metav1.GetOptions{})
671-
if !apierrors.IsNotFound(err) {
672-
t.Errorf("expected a NotFound response of PersistentVolumeClaims, got: %v", err)
685+
if err != nil {
686+
t.Errorf("unexpected err when getting PersistentVolumeClaims, err: %v", err)
673687
}
674688
}
675689

@@ -692,14 +706,9 @@ func TestCleanupAffinityAssistants_Failure(t *testing.T) {
692706
func(action testing2.Action) (handled bool, ret runtime.Object, err error) {
693707
return true, &corev1.NodeList{}, errors.New("error deleting statefulsets")
694708
})
695-
c.KubeClientSet.CoreV1().(*fake.FakeCoreV1).PrependReactor("delete", "persistentvolumeclaims",
696-
func(action testing2.Action) (handled bool, ret runtime.Object, err error) {
697-
return true, &corev1.Pod{}, errors.New("error deleting persistentvolumeclaims")
698-
})
699709

700710
expectedErrs := errorutils.NewAggregate([]error{
701711
errors.New("failed to delete StatefulSet affinity-assistant-e3b0c44298: error deleting statefulsets"),
702-
errors.New("failed to delete PersistentVolumeClaim pvc-e3b0c44298-affinity-assistant-e3b0c44298-0: error deleting persistentvolumeclaims"),
703712
})
704713

705714
errs := c.cleanupAffinityAssistants(ctx, pr)

‎pkg/reconciler/pipelinerun/pipelinerun.go‎

Lines changed: 36 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -605,27 +605,35 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel
605605
return controller.NewPermanentError(err)
606606
}
607607

608-
// Make an attempt to create Affinity Assistant if it does not exist
609-
// if the Affinity Assistant already exists, handle the possibility of assigned node becoming unschedulable by deleting the pod
610-
if !c.isAffinityAssistantDisabled(ctx) {
611-
// create Affinity Assistant (StatefulSet) so that taskRun pods that share workspace PVC achieve Node Affinity
612-
// TODO(#6740)(WIP): We only support AffinityAssistantPerWorkspace at the point. Implement different AffinityAssitantBehaviors based on `coscheduling` feature flag when adding end-to-end support.
613-
if err = c.createOrUpdateAffinityAssistantsPerAABehavior(ctx, pr, affinityassistant.AffinityAssistantPerWorkspace); err != nil {
614-
logger.Errorf("Failed to create affinity assistant StatefulSet for PipelineRun %s: %v", pr.Name, err)
615-
pr.Status.MarkFailed(ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSetPerWorkspace,
616-
"Failed to create StatefulSet for PipelineRun %s/%s correctly: %s",
617-
pr.Namespace, pr.Name, err)
618-
return controller.NewPermanentError(err)
608+
aaBehavior, err := affinityassistant.GetAffinityAssistantBehavior(ctx)
609+
if err != nil {
610+
return controller.NewPermanentError(err)
611+
}
612+
613+
switch aaBehavior {
614+
case affinityassistant.AffinityAssistantPerWorkspace, affinityassistant.AffinityAssistantDisabled:
615+
if pr.HasVolumeClaimTemplate() {
616+
// create workspace PVC from template
617+
if err = c.pvcHandler.CreatePVCsForWorkspacesWithoutAffinityAssistant(ctx, pr.Spec.Workspaces, *kmeta.NewControllerRef(pr), pr.Namespace); err != nil {
618+
logger.Errorf("Failed to create PVC for PipelineRun %s: %v", pr.Name, err)
619+
pr.Status.MarkFailed(volumeclaim.ReasonCouldntCreateWorkspacePVC,
620+
"Failed to create PVC for PipelineRun %s/%s Workspaces correctly: %s",
621+
pr.Namespace, pr.Name, err)
622+
return controller.NewPermanentError(err)
623+
}
619624
}
620-
} else if pr.HasVolumeClaimTemplate() {
621-
// create workspace PVC from template
622-
if err = c.pvcHandler.CreatePVCsForWorkspacesWithoutAffinityAssistant(ctx, pr.Spec.Workspaces, *kmeta.NewControllerRef(pr), pr.Namespace); err != nil {
623-
logger.Errorf("Failed to create PVC for PipelineRun %s: %v", pr.Name, err)
624-
pr.Status.MarkFailed(volumeclaim.ReasonCouldntCreateWorkspacePVC,
625-
"Failed to create PVC for PipelineRun %s/%s Workspaces correctly: %s",
626-
pr.Namespace, pr.Name, err)
627-
return controller.NewPermanentError(err)
625+
if aaBehavior == affinityassistant.AffinityAssistantPerWorkspace {
626+
if err = c.createOrUpdateAffinityAssistantsPerAABehavior(ctx, pr, affinityassistant.AffinityAssistantPerWorkspace); err != nil {
627+
logger.Errorf("Failed to create affinity assistant StatefulSet for PipelineRun %s: %v", pr.Name, err)
628+
pr.Status.MarkFailed(ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSetPerWorkspace,
629+
"Failed to create StatefulSet for PipelineRun %s/%s correctly: %s",
630+
pr.Namespace, pr.Name, err)
631+
return controller.NewPermanentError(err)
632+
}
628633
}
634+
case affinityassistant.AffinityAssistantPerPipelineRun, affinityassistant.AffinityAssistantPerPipelineRunWithIsolation:
635+
// TODO(#6740)(WIP): implement end-to-end support for AffinityAssistantPerPipelineRun and AffinityAssistantPerPipelineRunWithIsolation modes
636+
return controller.NewPermanentError(fmt.Errorf("affinity assistant behavior: %v is not implemented", aaBehavior))
629637
}
630638
}
631639

@@ -1049,7 +1057,12 @@ func (c *Reconciler) getTaskrunWorkspaces(ctx context.Context, pr *v1.PipelineRu
10491057
pipelinePVCWorkspaceName = pipelineWorkspace
10501058
}
10511059

1052-
workspace := c.taskWorkspaceByWorkspaceVolumeSource(ctx, pipelinePVCWorkspaceName, pr.Name, b, taskWorkspaceName, pipelineTaskSubPath, *kmeta.NewControllerRef(pr))
1060+
aaBehavior, err := affinityassistant.GetAffinityAssistantBehavior(ctx)
1061+
if err != nil {
1062+
return nil, "", err
1063+
}
1064+
1065+
workspace := c.taskWorkspaceByWorkspaceVolumeSource(ctx, pipelinePVCWorkspaceName, pr.Name, b, taskWorkspaceName, pipelineTaskSubPath, *kmeta.NewControllerRef(pr), aaBehavior)
10531066
workspaces = append(workspaces, workspace)
10541067
} else {
10551068
workspaceIsOptional := false
@@ -1084,7 +1097,7 @@ func (c *Reconciler) getTaskrunWorkspaces(ctx context.Context, pr *v1.PipelineRu
10841097
// taskWorkspaceByWorkspaceVolumeSource returns the WorkspaceBinding to be bound to each TaskRun in the Pipeline Task.
10851098
// If the PipelineRun WorkspaceBinding is a volumeClaimTemplate, the returned WorkspaceBinding references a PersistentVolumeClaim created for the PipelineRun WorkspaceBinding based on the PipelineRun as OwnerReference.
10861099
// Otherwise, the returned WorkspaceBinding references the same volume as the PipelineRun WorkspaceBinding, with the file path joined with pipelineTaskSubPath as the binding subpath.
1087-
func (c *Reconciler) taskWorkspaceByWorkspaceVolumeSource(ctx context.Context, pipelineWorkspaceName string, prName string, wb v1.WorkspaceBinding, taskWorkspaceName string, pipelineTaskSubPath string, owner metav1.OwnerReference) v1.WorkspaceBinding {
1100+
func (c *Reconciler) taskWorkspaceByWorkspaceVolumeSource(ctx context.Context, pipelineWorkspaceName string, prName string, wb v1.WorkspaceBinding, taskWorkspaceName string, pipelineTaskSubPath string, owner metav1.OwnerReference, aaBehavior affinityassistant.AffinityAssitantBehavior) v1.WorkspaceBinding {
10881101
if wb.VolumeClaimTemplate == nil {
10891102
binding := *wb.DeepCopy()
10901103
binding.Name = taskWorkspaceName
@@ -1098,9 +1111,8 @@ func (c *Reconciler) taskWorkspaceByWorkspaceVolumeSource(ctx context.Context, p
10981111
}
10991112
binding.Name = taskWorkspaceName
11001113

1101-
if !c.isAffinityAssistantDisabled(ctx) {
1102-
binding.PersistentVolumeClaim.ClaimName = getPersistentVolumeClaimNameWithAffinityAssistant(pipelineWorkspaceName, prName, wb, owner)
1103-
} else {
1114+
// TODO(#6740)(WIP): get binding for AffinityAssistantPerPipelineRun and AffinityAssistantPerPipelineRunWithIsolation mode
1115+
if aaBehavior == affinityassistant.AffinityAssistantDisabled || aaBehavior == affinityassistant.AffinityAssistantPerWorkspace {
11041116
binding.PersistentVolumeClaim.ClaimName = volumeclaim.GetPVCNameWithoutAffinityAssistant(wb.VolumeClaimTemplate.Name, wb, owner)
11051117
}
11061118

‎pkg/reconciler/pipelinerun/pipelinerun_test.go‎

Lines changed: 7 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ import (
4040
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
4141
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
4242
resolutionv1beta1 "github.com/tektoncd/pipeline/pkg/apis/resolution/v1beta1"
43+
"github.com/tektoncd/pipeline/pkg/internal/affinityassistant"
4344
resolutionutil "github.com/tektoncd/pipeline/pkg/internal/resolution"
4445
"github.com/tektoncd/pipeline/pkg/reconciler/events/cloudevent"
4546
"github.com/tektoncd/pipeline/pkg/reconciler/events/k8sevent"
@@ -7673,7 +7674,7 @@ func Test_taskWorkspaceByWorkspaceVolumeSource(t *testing.T) {
76737674
name, taskWorkspaceName, pipelineWorkspaceName, prName string
76747675
wb v1.WorkspaceBinding
76757676
expectedBinding v1.WorkspaceBinding
7676-
disableAffinityAssistant bool
7677+
aaBehavior affinityassistant.AffinityAssitantBehavior
76777678
}{
76787679
{
76797680
name: "PVC Workspace with Affinity Assistant",
@@ -7687,9 +7688,10 @@ func Test_taskWorkspaceByWorkspaceVolumeSource(t *testing.T) {
76877688
expectedBinding: v1.WorkspaceBinding{
76887689
Name: "task-workspace",
76897690
PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
7690-
ClaimName: "pvc-2c26b46b68-affinity-assistant-e011a5ef79-0",
7691+
ClaimName: "pvc-2c26b46b68",
76917692
},
76927693
},
7694+
aaBehavior: affinityassistant.AffinityAssistantPerWorkspace,
76937695
},
76947696
{
76957697
name: "PVC Workspace without Affinity Assistant",
@@ -7705,7 +7707,7 @@ func Test_taskWorkspaceByWorkspaceVolumeSource(t *testing.T) {
77057707
ClaimName: "pvc-2c26b46b68",
77067708
},
77077709
},
7708-
disableAffinityAssistant: true,
7710+
aaBehavior: affinityassistant.AffinityAssistantDisabled,
77097711
},
77107712
{
77117713
name: "non-PVC Workspace",
@@ -7718,27 +7720,15 @@ func Test_taskWorkspaceByWorkspaceVolumeSource(t *testing.T) {
77187720
Name: "task-workspace",
77197721
EmptyDir: &corev1.EmptyDirVolumeSource{},
77207722
},
7723+
aaBehavior: affinityassistant.AffinityAssistantPerWorkspace,
77217724
},
77227725
}
77237726

77247727
for _, tc := range tests {
77257728
t.Run(tc.name, func(t *testing.T) {
77267729
c := Reconciler{}
77277730
ctx := context.Background()
7728-
if tc.disableAffinityAssistant {
7729-
featureFlags, err := config.NewFeatureFlagsFromMap(map[string]string{
7730-
"disable-affinity-assistant": "true",
7731-
})
7732-
if err != nil {
7733-
t.Fatalf("error creating feature flag disable-affinity-assistant from map: %v", err)
7734-
}
7735-
cfg := &config.Config{
7736-
FeatureFlags: featureFlags,
7737-
}
7738-
ctx = config.ToContext(context.Background(), cfg)
7739-
}
7740-
7741-
binding := c.taskWorkspaceByWorkspaceVolumeSource(ctx, tc.pipelineWorkspaceName, tc.prName, tc.wb, tc.taskWorkspaceName, "", *kmeta.NewControllerRef(testPr))
7731+
binding := c.taskWorkspaceByWorkspaceVolumeSource(ctx, tc.pipelineWorkspaceName, tc.prName, tc.wb, tc.taskWorkspaceName, "", *kmeta.NewControllerRef(testPr), tc.aaBehavior)
77427732
if d := cmp.Diff(tc.expectedBinding, binding); d != "" {
77437733
t.Errorf("WorkspaceBinding diff: %s", diff.PrintWantGot(d))
77447734
}

0 commit comments

Comments
 (0)