Skip to content

Commit f224e9b

Browse files
vdemeestertwoGiants
authored and
pritidesai
committed
Refactor CreatePVCFromVolumClaimTemplate condition logic
Co-authored-by: Stanislav Jakuschevskij <stas@two-giants.com>
1 parent ea3dc05 commit f224e9b

File tree

5 files changed

+170
-19
lines changed

5 files changed

+170
-19
lines changed

‎pkg/reconciler/pipelinerun/affinity_assistant.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,9 @@ const (
5050
)
5151

5252
var (
53-
ErrPvcCreationFailed = volumeclaim.ErrPvcCreationFailed
53+
// Deprecated: use volumeclain.ErrPvcCreationFailed instead
54+
ErrPvcCreationFailed = volumeclaim.ErrPvcCreationFailed
55+
// Deprecated: use volumeclaim.ErrAffinityAssistantCreationFailed instead
5456
ErrPvcCreationFailedRetryable = volumeclaim.ErrPvcCreationFailedRetryable
5557
ErrAffinityAssistantCreationFailed = errors.New("Affinity Assistant creation error")
5658
)

‎pkg/reconciler/pipelinerun/affinity_assistant_test.go‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -613,12 +613,7 @@ func TestCreateOrUpdateAffinityAssistantsAndPVCs_Failure(t *testing.T) {
613613
t.Errorf("expect error from createOrUpdateAffinityAssistantsAndPVCs but got nil")
614614
}
615615

616-
switch tc.failureType {
617-
case "pvc":
618-
if !errors.Is(err, ErrPvcCreationFailed) {
619-
t.Errorf("expected err type mismatching, expecting %v but got: %v", ErrPvcCreationFailed, err)
620-
}
621-
case "statefulset":
616+
if tc.failureType == "statefulset" {
622617
if !errors.Is(err, ErrAffinityAssistantCreationFailed) {
623618
t.Errorf("expected err type mismatching, expecting %v but got: %v", ErrAffinityAssistantCreationFailed, err)
624619
}

‎pkg/reconciler/pipelinerun/pipelinerun.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -753,12 +753,12 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel
753753
}
754754
if err := c.createOrUpdateAffinityAssistantsAndPVCs(ctx, pr, aaBehavior); err != nil {
755755
switch {
756-
case errors.Is(err, ErrPvcCreationFailed):
756+
case errors.Is(err, volumeclaim.ErrPvcCreationFailed):
757757
logger.Errorf("Failed to create PVC for PipelineRun %s: %v", pr.Name, err)
758758
pr.Status.MarkFailed(volumeclaim.ReasonCouldntCreateWorkspacePVC,
759759
"Failed to create PVC for PipelineRun %s/%s correctly: %s",
760760
pr.Namespace, pr.Name, err)
761-
case errors.Is(err, ErrPvcCreationFailedRetryable):
761+
case errors.Is(err, volumeclaim.ErrPvcCreationFailedRetryable):
762762
logger.Errorf("Failed to create PVC for PipelineRun %s: %v", pr.Name, err)
763763
pr.Status.MarkRunning(ReasonPending, "Waiting for PVC creation to succeed: %v", err)
764764
return err // not a permanent error, will requeue
@@ -768,6 +768,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel
768768
"Failed to create StatefulSet for PipelineRun %s/%s correctly: %s",
769769
pr.Namespace, pr.Name, err)
770770
default:
771+
logger.Errorf("default error handling for PipelineRun %s: %v", pr.Name, err)
771772
}
772773
return controller.NewPermanentError(err)
773774
}

‎pkg/reconciler/volumeclaim/pvchandler.go‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -80,20 +80,17 @@ func (c *defaultPVCHandler) CreatePVCFromVolumeClaimTemplate(ctx context.Context
8080
if apierrors.IsAlreadyExists(err) {
8181
c.logger.Infof("Tried to create PersistentVolumeClaim %s in namespace %s, but it already exists",
8282
claim.Name, claim.Namespace)
83-
} else if apierrors.IsForbidden(err) {
84-
if strings.Contains(err.Error(), "exceeded quota") {
85-
// This is a retry-able error
86-
return fmt.Errorf("%w: %v", ErrPvcCreationFailedRetryable, err.Error())
87-
}
88-
return fmt.Errorf("%w for %s: %w", ErrPvcCreationFailed, claim.Name, err)
83+
} else if isRetryableError(err) {
84+
// This is a retry-able error
85+
return fmt.Errorf("%w for %s: %v", ErrPvcCreationFailedRetryable, claim.Name, err.Error())
8986
} else {
90-
return fmt.Errorf("%w for %s: %w", ErrPvcCreationFailed, claim.Name, err)
87+
return fmt.Errorf("%w for %s: %v", ErrPvcCreationFailed, claim.Name, err.Error())
9188
}
9289
} else {
9390
c.logger.Infof("Created PersistentVolumeClaim %s in namespace %s", claim.Name, claim.Namespace)
9491
}
9592
case err != nil:
96-
return fmt.Errorf("%w: failed to retrieve PVC %s: %w", ErrPvcCreationFailed, claim.Name, err)
93+
return fmt.Errorf("failed to retrieve PVC %s: %w", claim.Name, err)
9794
}
9895

9996
return nil
@@ -181,3 +178,10 @@ func getPersistentVolumeClaimIdentity(workspaceName, ownerName string) string {
181178
hashString := hex.EncodeToString(hashBytes[:])
182179
return hashString[:10]
183180
}
181+
182+
func isRetryableError(err error) bool {
183+
if (apierrors.IsForbidden(err) && strings.Contains(err.Error(), "exceeded quota")) || apierrors.IsConflict(err) {
184+
return true
185+
}
186+
return false
187+
}

‎pkg/reconciler/volumeclaim/pvchandler_test.go‎

Lines changed: 151 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,15 @@ package volumeclaim
1818

1919
import (
2020
"context"
21+
"errors"
2122
"fmt"
23+
"strings"
2224
"testing"
2325

2426
v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
2527
"go.uber.org/zap"
2628
corev1 "k8s.io/api/core/v1"
27-
"k8s.io/apimachinery/pkg/api/errors"
29+
apierrors "k8s.io/apimachinery/pkg/api/errors"
2830
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2931
"k8s.io/apimachinery/pkg/runtime"
3032
"k8s.io/apimachinery/pkg/runtime/schema"
@@ -207,7 +209,7 @@ func TestCreateExistPersistentVolumeClaims(t *testing.T) {
207209
fn := func(action client_go_testing.Action) (bool, runtime.Object, error) {
208210
switch action := action.(type) {
209211
case client_go_testing.GetActionImpl:
210-
return true, nil, errors.NewNotFound(schema.GroupResource{}, action.Name)
212+
return true, nil, apierrors.NewNotFound(schema.GroupResource{}, action.Name)
211213
default:
212214
return false, nil, fmt.Errorf("no reaction implemented for %s", action)
213215
}
@@ -295,3 +297,150 @@ func TestPurgeFinalizerAndDeletePVCForWorkspace(t *testing.T) {
295297
t.Errorf("pvc %s kubernetes.io/pvc-protection finalizer is not removed properly", pvcName)
296298
}
297299
}
300+
301+
// TestCreatePVCFromVolumeClaimTemplate_GetError tests error handling when getting existing PVC fails
302+
func TestCreatePVCFromVolumeClaimTemplate_GetError(t *testing.T) {
303+
ownerRef := metav1.OwnerReference{UID: types.UID("test-owner")}
304+
namespace := "test-ns"
305+
306+
wb := v1.WorkspaceBinding{
307+
Name: "test-workspace",
308+
VolumeClaimTemplate: &corev1.PersistentVolumeClaim{
309+
ObjectMeta: metav1.ObjectMeta{Name: "test-claim"},
310+
Spec: corev1.PersistentVolumeClaimSpec{},
311+
},
312+
}
313+
314+
fakekubeclient := fakek8s.NewSimpleClientset()
315+
pvcHandler := defaultPVCHandler{fakekubeclient, zap.NewExample().Sugar()}
316+
317+
// Mock Get to return an error that's not NotFound
318+
fakekubeclient.Fake.PrependReactor("get", "persistentvolumeclaims",
319+
func(action client_go_testing.Action) (bool, runtime.Object, error) {
320+
return true, nil, apierrors.NewInternalError(errors.New("internal server error"))
321+
})
322+
323+
err := pvcHandler.CreatePVCFromVolumeClaimTemplate(t.Context(), wb, ownerRef, namespace)
324+
325+
if err == nil {
326+
t.Fatal("Expected error when Get fails with error")
327+
}
328+
329+
if !strings.Contains(err.Error(), "failed to retrieve PVC") {
330+
t.Errorf("Expected error message about failed to retrieve PVC, got: %v", err)
331+
}
332+
}
333+
334+
// TestCreatePVCFromVolumeClaimTemplate_CreateRetryableError tests retryable error handling
335+
func TestCreatePVCFromVolumeClaimTemplate_CreateRetryableError(t *testing.T) {
336+
ownerRef := metav1.OwnerReference{UID: types.UID("test-owner")}
337+
namespace := "test-ns"
338+
339+
wb := v1.WorkspaceBinding{
340+
Name: "test-workspace",
341+
VolumeClaimTemplate: &corev1.PersistentVolumeClaim{
342+
ObjectMeta: metav1.ObjectMeta{Name: "test-claim"},
343+
Spec: corev1.PersistentVolumeClaimSpec{},
344+
},
345+
}
346+
347+
fakekubeclient := fakek8s.NewSimpleClientset()
348+
pvcHandler := defaultPVCHandler{fakekubeclient, zap.NewExample().Sugar()}
349+
350+
// Mock Get to return NotFound, then Create to return quota exceeded error
351+
fakekubeclient.Fake.PrependReactor("get", "persistentvolumeclaims",
352+
func(action client_go_testing.Action) (bool, runtime.Object, error) {
353+
return true, nil, apierrors.NewNotFound(schema.GroupResource{}, "test-claim")
354+
})
355+
356+
fakekubeclient.Fake.PrependReactor("create", "persistentvolumeclaims",
357+
func(action client_go_testing.Action) (bool, runtime.Object, error) {
358+
return true, nil, apierrors.NewForbidden(schema.GroupResource{}, "test-claim", errors.New("foo exceeded quota"))
359+
})
360+
361+
err := pvcHandler.CreatePVCFromVolumeClaimTemplate(t.Context(), wb, ownerRef, namespace)
362+
363+
if err == nil {
364+
t.Fatal("Expected retryable error")
365+
}
366+
367+
if !strings.Contains(err.Error(), ErrPvcCreationFailedRetryable.Error()) {
368+
t.Errorf("Expected retryable error, got: %v", err)
369+
}
370+
}
371+
372+
// TestCreatePVCFromVolumeClaimTemplate_CreateNonRetryableError tests non-retryable error handling
373+
func TestCreatePVCFromVolumeClaimTemplate_CreateNonRetryableError(t *testing.T) {
374+
ownerRef := metav1.OwnerReference{UID: types.UID("test-owner")}
375+
namespace := "test-ns"
376+
377+
wb := v1.WorkspaceBinding{
378+
Name: "test-workspace",
379+
VolumeClaimTemplate: &corev1.PersistentVolumeClaim{
380+
ObjectMeta: metav1.ObjectMeta{Name: "test-claim"},
381+
Spec: corev1.PersistentVolumeClaimSpec{},
382+
},
383+
}
384+
385+
fakekubeclient := fakek8s.NewSimpleClientset()
386+
pvcHandler := defaultPVCHandler{fakekubeclient, zap.NewExample().Sugar()}
387+
388+
// Mock Get to return NotFound, then Create to return non-retryable error
389+
fakekubeclient.Fake.PrependReactor("get", "persistentvolumeclaims",
390+
func(action client_go_testing.Action) (bool, runtime.Object, error) {
391+
return true, nil, apierrors.NewNotFound(schema.GroupResource{}, "test-claim")
392+
})
393+
394+
fakekubeclient.Fake.PrependReactor("create", "persistentvolumeclaims",
395+
func(action client_go_testing.Action) (bool, runtime.Object, error) {
396+
return true, nil, apierrors.NewInternalError(errors.New("internal server error"))
397+
})
398+
399+
err := pvcHandler.CreatePVCFromVolumeClaimTemplate(t.Context(), wb, ownerRef, namespace)
400+
401+
if err == nil {
402+
t.Fatal("Expected non-retryable error")
403+
}
404+
405+
if !strings.Contains(err.Error(), ErrPvcCreationFailed.Error()) {
406+
t.Errorf("Expected non-retryable error, got: %v", err)
407+
}
408+
}
409+
410+
// TestCreatePVCFromVolumeClaimTemplate_ConflictError tests conflict error handling (retryable)
411+
func TestCreatePVCFromVolumeClaimTemplate_ConflictError(t *testing.T) {
412+
ownerRef := metav1.OwnerReference{UID: types.UID("test-owner")}
413+
namespace := "test-ns"
414+
415+
wb := v1.WorkspaceBinding{
416+
Name: "test-workspace",
417+
VolumeClaimTemplate: &corev1.PersistentVolumeClaim{
418+
ObjectMeta: metav1.ObjectMeta{Name: "test-claim"},
419+
Spec: corev1.PersistentVolumeClaimSpec{},
420+
},
421+
}
422+
423+
fakekubeclient := fakek8s.NewSimpleClientset()
424+
pvcHandler := defaultPVCHandler{fakekubeclient, zap.NewExample().Sugar()}
425+
426+
// Mock Get to return NotFound, then Create to return conflict error
427+
fakekubeclient.Fake.PrependReactor("get", "persistentvolumeclaims",
428+
func(action client_go_testing.Action) (bool, runtime.Object, error) {
429+
return true, nil, apierrors.NewNotFound(schema.GroupResource{}, "test-claim")
430+
})
431+
432+
fakekubeclient.Fake.PrependReactor("create", "persistentvolumeclaims",
433+
func(action client_go_testing.Action) (bool, runtime.Object, error) {
434+
return true, nil, apierrors.NewConflict(schema.GroupResource{}, "test-claim", errors.New("conflict"))
435+
})
436+
437+
err := pvcHandler.CreatePVCFromVolumeClaimTemplate(t.Context(), wb, ownerRef, namespace)
438+
439+
if err == nil {
440+
t.Fatal("Expected conflict error (retryable)")
441+
}
442+
443+
if !strings.Contains(err.Error(), ErrPvcCreationFailedRetryable.Error()) {
444+
t.Errorf("Expected retryable conflict error, got: %v", err)
445+
}
446+
}

0 commit comments

Comments
 (0)