Skip to content

Commit d4323c3

Browse files
matejvasekmatejvasek
authored
Revert "Fix PVC cleanup between on-cluster builds (knative#3676)" (knative#3853)
This reverts commit e70861e.
1 parent e7d7dcf commit d4323c3

3 files changed

Lines changed: 23 additions & 154 deletions

File tree

‎pkg/k8s/persistent_volumes.go‎

Lines changed: 0 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -72,47 +72,6 @@ func DeletePersistentVolumeClaims(ctx context.Context, namespaceOverride string,
7272
return client.CoreV1().PersistentVolumeClaims(namespace).DeleteCollection(ctx, metav1.DeleteOptions{}, listOptions)
7373
}
7474

75-
// DeletePersistentVolumeClaim deletes a single PVC by name
76-
func DeletePersistentVolumeClaim(ctx context.Context, name, namespaceOverride string) error {
77-
client, namespace, err := NewClientAndResolvedNamespace(namespaceOverride)
78-
if err != nil {
79-
return err
80-
}
81-
82-
err = client.CoreV1().PersistentVolumeClaims(namespace).Delete(ctx, name, metav1.DeleteOptions{})
83-
if err != nil && !k8serrors.IsNotFound(err) {
84-
return fmt.Errorf("failed to delete PVC %s: %w", name, err)
85-
}
86-
return nil
87-
}
88-
89-
// WaitForPVCDeletion waits for a PVC to be fully deleted (not just in Terminating state)
90-
func WaitForPVCDeletion(ctx context.Context, name, namespaceOverride string) error {
91-
client, namespace, err := NewClientAndResolvedNamespace(namespaceOverride)
92-
if err != nil {
93-
return err
94-
}
95-
96-
// Poll until PVC is gone
97-
for {
98-
select {
99-
case <-ctx.Done():
100-
return ctx.Err()
101-
default:
102-
_, err := client.CoreV1().PersistentVolumeClaims(namespace).Get(ctx, name, metav1.GetOptions{})
103-
if k8serrors.IsNotFound(err) {
104-
// PVC is fully deleted
105-
return nil
106-
}
107-
if err != nil {
108-
return fmt.Errorf("error checking PVC deletion status: %w", err)
109-
}
110-
// PVC still exists (possibly Terminating), wait and retry
111-
time.Sleep(time.Second)
112-
}
113-
}
114-
}
115-
11675
var TarImage = "ghcr.io/knative/func-utils:v2"
11776

11877
// UploadToVolume uploads files (passed in form of tar stream) into volume.

‎pkg/pipelines/tekton/pipelines_provider.go‎

Lines changed: 6 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -578,46 +578,18 @@ func findNewestPipelineRunWithRetry(ctx context.Context, f fn.Function, namespac
578578

579579
// allows simple mocking in unit tests, use with caution regarding concurrency
580580
var createPersistentVolumeClaim = k8s.CreatePersistentVolumeClaim
581-
var getPersistentVolumeClaim = k8s.GetPersistentVolumeClaim
582-
var deletePersistentVolumeClaim = k8s.DeletePersistentVolumeClaim
583-
var waitForPVCDeletion = k8s.WaitForPVCDeletion
584581

585582
func createPipelinePersistentVolumeClaim(ctx context.Context, f fn.Function, namespace string, labels map[string]string) error {
586-
pvcName := getPipelinePvcName(f)
587-
588-
// Check if PVC already exists
589-
existingPVC, err := getPersistentVolumeClaim(ctx, pvcName, namespace)
590-
if err != nil && !k8serrors.IsNotFound(err) {
591-
return fmt.Errorf("failed to check existing PVC: %w", err)
592-
}
593-
594-
// If PVC exists, delete it and wait for full deletion to ensure clean workspace
595-
if existingPVC != nil {
596-
err = deletePersistentVolumeClaim(ctx, pvcName, namespace)
597-
if err != nil {
598-
return fmt.Errorf("failed to delete existing PVC: %w", err)
599-
}
600-
601-
// Wait for PVC to be fully deleted (not just Terminating)
602-
err = waitForPVCDeletion(ctx, pvcName, namespace)
603-
if err != nil {
604-
return fmt.Errorf("failed waiting for PVC deletion: %w", err)
605-
}
606-
}
607-
608-
// Create fresh PVC
609-
var pvcs resource.Quantity
610-
pvcs = DefaultPersistentVolumeClaimSize
583+
var err error
584+
pvcs := DefaultPersistentVolumeClaimSize
611585
if f.Build.PVCSize != "" {
612586
if pvcs, err = resource.ParseQuantity(f.Build.PVCSize); err != nil {
613-
return fmt.Errorf("PVC size value could not be parsed: %w", err)
587+
return fmt.Errorf("PVC size value could not be parsed. %w", err)
614588
}
615589
}
616-
617-
err = createPersistentVolumeClaim(ctx, pvcName, namespace, labels, f.Deploy.Annotations, corev1.ReadWriteOnce, pvcs, f.Build.RemoteStorageClass)
618-
if err != nil {
619-
return fmt.Errorf("problem creating persistent volume claim: %w", err)
590+
err = createPersistentVolumeClaim(ctx, getPipelinePvcName(f), namespace, labels, f.Deploy.Annotations, corev1.ReadWriteOnce, pvcs, f.Build.RemoteStorageClass)
591+
if err != nil && !k8serrors.IsAlreadyExists(err) {
592+
return fmt.Errorf("problem creating persistent volume claim: %v", err)
620593
}
621-
622594
return nil
623595
}

‎pkg/pipelines/tekton/pipelines_provider_test.go‎

Lines changed: 17 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -82,10 +82,7 @@ func TestSourcesAsTarStream(t *testing.T) {
8282
}
8383

8484
func Test_createPipelinePersistentVolumeClaim(t *testing.T) {
85-
type mockCreateType func(ctx context.Context, name, namespaceOverride string, labels map[string]string, annotations map[string]string, accessMode corev1.PersistentVolumeAccessMode, resourceRequest resource.Quantity, storageClass string) (err error)
86-
type mockGetType func(ctx context.Context, name, namespaceOverride string) (*corev1.PersistentVolumeClaim, error)
87-
type mockDeleteType func(ctx context.Context, name, namespaceOverride string) error
88-
type mockWaitType func(ctx context.Context, name, namespaceOverride string) error
85+
type mockType func(ctx context.Context, name, namespaceOverride string, labels map[string]string, annotations map[string]string, accessMode corev1.PersistentVolumeAccessMode, resourceRequest resource.Quantity, storageClass string) (err error)
8986

9087
type args struct {
9188
ctx context.Context
@@ -95,13 +92,10 @@ func Test_createPipelinePersistentVolumeClaim(t *testing.T) {
9592
size string
9693
}
9794
tests := []struct {
98-
name string
99-
args args
100-
mockCreate mockCreateType
101-
mockGet mockGetType
102-
mockDelete mockDeleteType
103-
mockWait mockWaitType
104-
wantErr bool
95+
name string
96+
args args
97+
mock mockType
98+
wantErr bool
10599
}{
106100
{
107101
name: "returns error if pvc creation failed",
@@ -112,102 +106,46 @@ func Test_createPipelinePersistentVolumeClaim(t *testing.T) {
112106
labels: nil,
113107
size: DefaultPersistentVolumeClaimSize.String(),
114108
},
115-
mockGet: func(ctx context.Context, name, namespaceOverride string) (*corev1.PersistentVolumeClaim, error) {
116-
return nil, &apiErrors.StatusError{ErrStatus: metav1.Status{Reason: metav1.StatusReasonNotFound}}
117-
},
118-
mockCreate: func(ctx context.Context, name, namespaceOverride string, labels map[string]string, annotations map[string]string, accessMode corev1.PersistentVolumeAccessMode, resourceRequest resource.Quantity, storageClass string) (err error) {
109+
mock: func(ctx context.Context, name, namespaceOverride string, labels map[string]string, annotations map[string]string, accessMode corev1.PersistentVolumeAccessMode, resourceRequest resource.Quantity, storageClass string) (err error) {
119110
return errors.New("creation of pvc failed")
120111
},
121112
wantErr: true,
122113
},
123114
{
124-
name: "deletes and recreates if pvc already exists",
115+
name: "returns nil if pvc already exists",
125116
args: args{
126117
ctx: t.Context(),
127118
f: fn.Function{},
128119
namespace: "test-ns",
129120
labels: nil,
130121
size: DefaultPersistentVolumeClaimSize.String(),
131122
},
132-
mockGet: func(ctx context.Context, name, namespaceOverride string) (*corev1.PersistentVolumeClaim, error) {
133-
return &corev1.PersistentVolumeClaim{}, nil
134-
},
135-
mockDelete: func(ctx context.Context, name, namespaceOverride string) error {
136-
return nil
137-
},
138-
mockWait: func(ctx context.Context, name, namespaceOverride string) error {
139-
return nil
140-
},
141-
mockCreate: func(ctx context.Context, name, namespaceOverride string, labels map[string]string, annotations map[string]string, accessMode corev1.PersistentVolumeAccessMode, resourceRequest resource.Quantity, storageClass string) (err error) {
142-
return nil
123+
mock: func(ctx context.Context, name, namespaceOverride string, labels map[string]string, annotations map[string]string, accessMode corev1.PersistentVolumeAccessMode, resourceRequest resource.Quantity, storageClass string) (err error) {
124+
return &apiErrors.StatusError{ErrStatus: metav1.Status{Reason: metav1.StatusReasonAlreadyExists}}
143125
},
144126
wantErr: false,
145127
},
146128
{
147-
name: "returns error if deletion fails",
148-
args: args{
149-
ctx: t.Context(),
150-
f: fn.Function{},
151-
namespace: "test-ns",
152-
labels: nil,
153-
size: DefaultPersistentVolumeClaimSize.String(),
154-
},
155-
mockGet: func(ctx context.Context, name, namespaceOverride string) (*corev1.PersistentVolumeClaim, error) {
156-
return &corev1.PersistentVolumeClaim{}, nil
157-
},
158-
mockDelete: func(ctx context.Context, name, namespaceOverride string) error {
159-
return errors.New("deletion failed")
160-
},
161-
wantErr: true,
162-
},
163-
{
164-
name: "returns error if waiting for deletion fails",
129+
name: "returns err if namespace not defined and default returns an err",
165130
args: args{
166131
ctx: t.Context(),
167132
f: fn.Function{},
168-
namespace: "test-ns",
133+
namespace: "",
169134
labels: nil,
170135
size: DefaultPersistentVolumeClaimSize.String(),
171136
},
172-
mockGet: func(ctx context.Context, name, namespaceOverride string) (*corev1.PersistentVolumeClaim, error) {
173-
return &corev1.PersistentVolumeClaim{}, nil
174-
},
175-
mockDelete: func(ctx context.Context, name, namespaceOverride string) error {
176-
return nil
177-
},
178-
mockWait: func(ctx context.Context, name, namespaceOverride string) error {
179-
return errors.New("wait for deletion failed")
137+
mock: func(ctx context.Context, name, namespaceOverride string, labels map[string]string, annotations map[string]string, accessMode corev1.PersistentVolumeAccessMode, resourceRequest resource.Quantity, storageClass string) (err error) {
138+
return errors.New("no namespace defined")
180139
},
181140
wantErr: true,
182141
},
183142
}
184143
for _, tt := range tests {
185-
t.Run(tt.name, func(t *testing.T) {
186-
// save current functions and restore them at the end
187-
oldCreate := createPersistentVolumeClaim
188-
oldGet := getPersistentVolumeClaim
189-
oldDelete := deletePersistentVolumeClaim
190-
oldWait := waitForPVCDeletion
191-
defer func() {
192-
createPersistentVolumeClaim = oldCreate
193-
getPersistentVolumeClaim = oldGet
194-
deletePersistentVolumeClaim = oldDelete
195-
waitForPVCDeletion = oldWait
196-
}()
197-
198-
if tt.mockCreate != nil {
199-
createPersistentVolumeClaim = tt.mockCreate
200-
}
201-
if tt.mockGet != nil {
202-
getPersistentVolumeClaim = tt.mockGet
203-
}
204-
if tt.mockDelete != nil {
205-
deletePersistentVolumeClaim = tt.mockDelete
206-
}
207-
if tt.mockWait != nil {
208-
waitForPVCDeletion = tt.mockWait
209-
}
144+
t.Run(tt.name, func(t *testing.T) { // save current function and restore it at the end
145+
old := createPersistentVolumeClaim
146+
defer func() { createPersistentVolumeClaim = old }()
210147

148+
createPersistentVolumeClaim = tt.mock
211149
tt.args.f.Build.PVCSize = tt.args.size
212150
if err := createPipelinePersistentVolumeClaim(tt.args.ctx, tt.args.f, tt.args.namespace, tt.args.labels); (err != nil) != tt.wantErr {
213151
t.Errorf("createPipelinePersistentVolumeClaim() error = %v, wantErr %v", err, tt.wantErr)

0 commit comments

Comments
 (0)