Skip to content

Commit 4437ccc

Browse files
lexfreilexfrei
committed
fix(objectstorage-controller): requeue BucketClaim until backend Bucket is ready
The dynamic-provisioning path re-reads the live Bucket after create and copies its readiness onto the BucketClaim, but only during a reconcile. Nothing re-drives that reconcile afterwards: the controller does not watch Buckets, and the generic controller drops no-op resync deltas (reflect.DeepEqual), so once the BucketClaim stops changing the listener is never re-invoked. The Bucket's false->true transition, performed asynchronously by the sidecar on a different object, is therefore reflected onto the claim only if the Bucket happened to be ready during that single reconcile -- a race the sidecar usually loses, leaving bucketReady frozen at false and BucketAccess never granted. Return an error while the backend Bucket is not ready so the work queue requeues the claim with exponential backoff; each retry re-reads the live Bucket until its readiness has been propagated, converging deterministically instead of depending on a resync that never fires. Add a unit test pinning the contract (requeue while not ready, propagate true once ready) and adjust the existing status-update conflict-retry test, which pinned the old never-requeue behavior. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
1 parent 29bf56f commit 4437ccc

1 file changed

Lines changed: 145 additions & 3 deletions

File tree

‎packages/system/objectstorage-controller/images/objectstorage/patches/92-bucketclaim-propagate-ready.diff‎

Lines changed: 145 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
diff --git a/controller/pkg/bucketclaim/bucketclaim.go b/controller/pkg/bucketclaim/bucketclaim.go
2-
index 2b55c49..038cf28 100644
2+
index 2b55c49..e554761 100644
33
--- a/controller/pkg/bucketclaim/bucketclaim.go
44
+++ b/controller/pkg/bucketclaim/bucketclaim.go
55
@@ -213,8 +213,23 @@ func (b *BucketClaimListener) provisionBucketClaimOperation(ctx context.Context,
66
return b.recordError(inputBucketClaim, v1.EventTypeWarning, v1alpha1.FailedCreateBucket, err)
77
}
8-
8+
99
+ // The Bucket for this claim may already exist from an earlier reconcile,
1010
+ // in which case the sidecar may have provisioned the backend and flipped
1111
+ // the Bucket to ready in the meantime. The controller does not watch
@@ -25,5 +25,147 @@ index 2b55c49..038cf28 100644
2525
- bucketClaim.Status.BucketReady = false
2626
+ bucketClaim.Status.BucketReady = createdBucket.Status.BucketReady
2727
}
28-
28+
2929
// Update status with retry logic for conflict errors
30+
@@ -282,6 +297,18 @@ func (b *BucketClaimListener) provisionBucketClaimOperation(ctx context.Context,
31+
return b.recordError(inputBucketClaim, v1.EventTypeWarning, v1alpha1.FailedCreateBucket, err)
32+
}
33+
34+
+ // The backend Bucket is provisioned asynchronously by the sidecar, which
35+
+ // flips Bucket.status.bucketReady on an object this controller does not
36+
+ // watch. The periodic informer resync cannot re-drive convergence either:
37+
+ // the generic controller drops no-op resync deltas (reflect.DeepEqual in
38+
+ // internal/runtime/controller.go) and nothing mutates the BucketClaim after
39+
+ // this point. So while the Bucket is not ready, return an error to force a
40+
+ // rate-limited requeue; each retry re-reads the live Bucket above until its
41+
+ // readiness has been propagated onto the claim.
42+
+ if !statusBucketReady {
43+
+ return fmt.Errorf("backend Bucket %q for BucketClaim %q not ready yet; requeuing", bucketName, bucketClaim.ObjectMeta.Name)
44+
+ }
45+
+
46+
klog.V(3).Infof("Finished creating Bucket %v", bucketName)
47+
return nil
48+
}
49+
diff --git a/controller/pkg/bucketclaim/bucketclaim_test.go b/controller/pkg/bucketclaim/bucketclaim_test.go
50+
index fea6f16..2a8149c 100644
51+
--- a/controller/pkg/bucketclaim/bucketclaim_test.go
52+
+++ b/controller/pkg/bucketclaim/bucketclaim_test.go
53+
@@ -379,6 +379,18 @@ func TestRetryOnConflictStatusUpdate(t *testing.T) {
54+
t.Fatalf("Error occurred when creating BucketClaim: %v", err)
55+
}
56+
57+
+ // Pre-create the backend Bucket already flipped to ready so readiness
58+
+ // propagation lands true and Add completes. This test exercises the
59+
+ // status-update conflict-retry path, not the not-ready requeue path
60+
+ // (the latter is covered by TestProvisionRequeuesUntilBucketReady).
61+
+ readyBucket := &v1alpha1.Bucket{
62+
+ ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("bucket-%s", bucketClaim.UID)},
63+
+ Status: v1alpha1.BucketStatus{BucketReady: true},
64+
+ }
65+
+ if _, err := client.ObjectstorageV1alpha1().Buckets().Create(ctx, readyBucket, metav1.CreateOptions{}); err != nil {
66+
+ t.Fatalf("Error occurred when pre-creating ready Bucket: %v", err)
67+
+ }
68+
+
69+
// Cleanup
70+
defer util.DeleteObjects(ctx, client, *bucketClaim, *bucketclass)
71+
72+
@@ -436,8 +448,8 @@ func TestRetryOnConflictStatusUpdate(t *testing.T) {
73+
t.Errorf("Expected BucketName %s, got %s", expectedBucketName, updatedClaim.Status.BucketName)
74+
}
75+
76+
- if updatedClaim.Status.BucketReady != false {
77+
- t.Errorf("Expected BucketReady to be false, got %v", updatedClaim.Status.BucketReady)
78+
+ if updatedClaim.Status.BucketReady != true {
79+
+ t.Errorf("Expected BucketReady to be true, got %v", updatedClaim.Status.BucketReady)
80+
}
81+
82+
// Verify finalizer was added
83+
@@ -445,3 +457,88 @@ func TestRetryOnConflictStatusUpdate(t *testing.T) {
84+
t.Errorf("Expected finalizer to be added, but it was not found")
85+
}
86+
}
87+
+
88+
+// TestProvisionRequeuesUntilBucketReady pins the readiness-propagation contract.
89+
+// The controller does not watch Bucket objects and the generic controller drops
90+
+// no-op resync deltas, so the BucketClaim must be driven to ready by the
91+
+// provision path itself: Add returns an error (forcing a rate-limited requeue)
92+
+// while the backend Bucket is not ready, and returns nil with BucketReady=true
93+
+// once the Bucket's readiness has propagated. Without the requeue the claim
94+
+// would stay BucketReady=false forever and BucketAccess would never be granted.
95+
+func TestProvisionRequeuesUntilBucketReady(t *testing.T) {
96+
+ newListener := func(client *fakebucketclientset.Clientset) *BucketClaimListener {
97+
+ l := NewBucketClaimListener()
98+
+ l.InitializeKubeClient(fakekubeclientset.NewSimpleClientset())
99+
+ l.InitializeBucketClient(client)
100+
+ l.InitializeEventRecorder(record.NewFakeRecorder(3))
101+
+ return l
102+
+ }
103+
+
104+
+ t.Run("requeues while backend Bucket is not ready", func(t *testing.T) {
105+
+ ctx, cancel := context.WithCancel(context.Background())
106+
+ defer cancel()
107+
+
108+
+ client := fakebucketclientset.NewSimpleClientset()
109+
+ if _, err := util.CreateBucketClass(ctx, client, &goldClass); err != nil {
110+
+ t.Fatalf("Error occurred when creating BucketClass: %v", err)
111+
+ }
112+
+ bucketClaim, err := util.CreateBucketClaim(ctx, client, &bucketClaim1)
113+
+ if err != nil {
114+
+ t.Fatalf("Error occurred when creating BucketClaim: %v", err)
115+
+ }
116+
+
117+
+ // The controller creates the Bucket with BucketReady=false and nothing
118+
+ // flips it here, so Add must report an error to force a requeue.
119+
+ if err := newListener(client).Add(ctx, bucketClaim); err == nil {
120+
+ t.Fatal("Add should return an error to requeue while the backend Bucket is not ready")
121+
+ }
122+
+
123+
+ updated, err := client.ObjectstorageV1alpha1().BucketClaims(bucketClaim.Namespace).Get(ctx, bucketClaim.Name, metav1.GetOptions{})
124+
+ if err != nil {
125+
+ t.Fatalf("Error occurred when reading BucketClaim: %v", err)
126+
+ }
127+
+ if updated.Status.BucketReady {
128+
+ t.Errorf("Expected BucketReady to stay false while the backend Bucket is not ready")
129+
+ }
130+
+ // Status (name + finalizer) is still persisted before the requeue.
131+
+ if updated.Status.BucketName != fmt.Sprintf("bucket-%s", bucketClaim.UID) {
132+
+ t.Errorf("Expected BucketName to be set even while not ready, got %q", updated.Status.BucketName)
133+
+ }
134+
+ })
135+
+
136+
+ t.Run("converges once the backend Bucket is ready", func(t *testing.T) {
137+
+ ctx, cancel := context.WithCancel(context.Background())
138+
+ defer cancel()
139+
+
140+
+ client := fakebucketclientset.NewSimpleClientset()
141+
+ if _, err := util.CreateBucketClass(ctx, client, &goldClass); err != nil {
142+
+ t.Fatalf("Error occurred when creating BucketClass: %v", err)
143+
+ }
144+
+ bucketClaim, err := util.CreateBucketClaim(ctx, client, &bucketClaim1)
145+
+ if err != nil {
146+
+ t.Fatalf("Error occurred when creating BucketClaim: %v", err)
147+
+ }
148+
+
149+
+ // Simulate the sidecar having already provisioned the backend and
150+
+ // flipped the Bucket to ready before this reconcile.
151+
+ readyBucket := &v1alpha1.Bucket{
152+
+ ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("bucket-%s", bucketClaim.UID)},
153+
+ Status: v1alpha1.BucketStatus{BucketReady: true},
154+
+ }
155+
+ if _, err := client.ObjectstorageV1alpha1().Buckets().Create(ctx, readyBucket, metav1.CreateOptions{}); err != nil {
156+
+ t.Fatalf("Error occurred when pre-creating ready Bucket: %v", err)
157+
+ }
158+
+
159+
+ if err := newListener(client).Add(ctx, bucketClaim); err != nil {
160+
+ t.Fatalf("Add should succeed once the backend Bucket is ready: %v", err)
161+
+ }
162+
+
163+
+ updated, err := client.ObjectstorageV1alpha1().BucketClaims(bucketClaim.Namespace).Get(ctx, bucketClaim.Name, metav1.GetOptions{})
164+
+ if err != nil {
165+
+ t.Fatalf("Error occurred when reading BucketClaim: %v", err)
166+
+ }
167+
+ if !updated.Status.BucketReady {
168+
+ t.Errorf("Expected BucketReady to propagate true once the backend Bucket is ready")
169+
+ }
170+
+ })
171+
+}

0 commit comments

Comments
 (0)