Skip to content

Commit d26bf1b

Browse files
AndrewChubatiukAndrewChubatiuk
committed
e2e: do not enclose CR update into Eventually as it's expected to be applied immediately
1 parent 345de1a commit d26bf1b

File tree

17 files changed

+93
-202
lines changed

17 files changed

+93
-202
lines changed

‎Makefile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -370,7 +370,7 @@ CONTROLLER_TOOLS_VERSION ?= v0.18.0
370370
ENVTEST_VERSION ?= release-0.20
371371
GOLANGCI_LINT_VERSION ?= v2.4.0
372372
CODEGENERATOR_VERSION ?= v0.32.2
373-
KIND_VERSION ?= v0.30.0
373+
KIND_VERSION ?= v0.27.0
374374
OLM_VERSION ?= 0.31.0
375375
OPERATOR_SDK_VERSION ?= v1.39.1
376376
OPM_VERSION ?= v1.51.0

‎internal/controller/operator/controllers.go‎

Lines changed: 21 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -358,52 +358,50 @@ func reconcileAndTrackStatus[T client.Object, ST reconcile.StatusWithMetadata[ST
358358
}
359359
return
360360
}
361-
currentStatus := vmv1beta1.UpdateStatusExpanding
362361
specChanged, err := object.HasSpecChanges()
363362
if err != nil {
364363
resultErr = fmt.Errorf("cannot parse exist spec changes")
365364
return
366365
}
366+
resultStatus := vmv1beta1.UpdateStatusOperational
367367
if specChanged {
368-
if err := reconcile.UpdateObjectStatus(ctx, c, object, currentStatus, nil); err != nil {
368+
if err := reconcile.UpdateObjectStatus(ctx, c, object, vmv1beta1.UpdateStatusExpanding, nil); err != nil {
369369
resultErr = fmt.Errorf("failed to update object status: %w", err)
370370
return
371371
}
372372
if err := createGenericEventForObject(ctx, c, object, "starting object update"); err != nil {
373373
logger.WithContext(ctx).Error(err, " cannot create k8s api event")
374374
}
375-
defer func() {
376-
if currentStatus == vmv1beta1.UpdateStatusExpanding {
377-
return
378-
}
375+
logger.WithContext(ctx).Info("object has changes with previous state, applying changes")
376+
}
377+
defer func() {
378+
prevStatus := object.GetStatus().GetStatusMetadata().UpdateStatus
379+
if specChanged && prevStatus != resultStatus {
379380
newPatch, err := object.LastAppliedSpecAsPatch()
380381
if err != nil {
381-
resultErr = fmt.Errorf("cannot parse last applied spec for cluster: %w", err)
382+
resultErr = fmt.Errorf("cannot parse last applied spec: %w", err)
382383
return
383384
}
384385
if err := c.Patch(ctx, object, newPatch); err != nil {
385-
resultErr = fmt.Errorf("cannot update cluster with last applied spec: %w", err)
386+
resultErr = fmt.Errorf("cannot update resource with last applied spec: %w", err)
386387
return
387388
}
388-
}()
389-
logger.WithContext(ctx).Info("object has changes with previous state, applying changes")
390-
}
389+
}
390+
if err := reconcile.UpdateObjectStatus(ctx, c, object, resultStatus, resultErr); err != nil {
391+
resultErr = fmt.Errorf("failed to update object status: %w", err)
392+
return
393+
}
394+
}()
391395

392396
result, err = cb()
393397
if err != nil {
394-
// do not change status on conflict to failed
398+
// do not change status on conflict or timeout to failed
395399
// it should be retried on the next loop
396-
if k8serrors.IsConflict(err) {
397-
return
398-
}
399-
currentStatus = vmv1beta1.UpdateStatusFailed
400-
if reconcile.IsErrorWaitTimeout(err) {
401-
currentStatus = vmv1beta1.UpdateStatusExpanding
402-
err = nil
403-
}
404-
if updateErr := reconcile.UpdateObjectStatus(ctx, c, object, currentStatus, err); updateErr != nil {
405-
resultErr = fmt.Errorf("failed to update object status: %q, origin err: %w", updateErr, err)
406-
return
400+
if k8serrors.IsConflict(err) || reconcile.IsErrorWaitTimeout(err) {
401+
resultStatus = vmv1beta1.UpdateStatusExpanding
402+
} else {
403+
resultStatus = vmv1beta1.UpdateStatusFailed
404+
resultErr = err
407405
}
408406
return
409407
}
@@ -413,11 +411,5 @@ func reconcileAndTrackStatus[T client.Object, ST reconcile.StatusWithMetadata[ST
413411
}
414412
logger.WithContext(ctx).Info("object was successfully reconciled")
415413
}
416-
currentStatus = vmv1beta1.UpdateStatusOperational
417-
if err := reconcile.UpdateObjectStatus(ctx, c, object, currentStatus, nil); err != nil {
418-
resultErr = fmt.Errorf("failed to update object status: %w", err)
419-
return
420-
}
421-
422414
return result, nil
423415
}

‎internal/controller/operator/factory/reconcile/status.go‎

Lines changed: 17 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package reconcile
22

33
import (
44
"context"
5-
"encoding/json"
65
"fmt"
76
"math/rand/v2"
87
"reflect"
@@ -192,72 +191,40 @@ type StatusWithMetadata[T any] interface {
192191
GetStatusMetadata() *vmv1beta1.StatusMetadata
193192
}
194193

195-
// UpdateStatus reconcile provided object status with given actualStatus status
196-
func UpdateObjectStatus[T client.Object, ST StatusWithMetadata[STC], STC any](ctx context.Context, rclient client.Client, object ObjectWithDeepCopyAndStatus[T, ST, STC], actualStatus vmv1beta1.UpdateStatus, maybeErr error) error {
197-
currentStatus := object.GetStatus()
198-
prevStatus := currentStatus.DeepCopy()
199-
currMeta := currentStatus.GetStatusMetadata()
200-
newUpdateStatus := actualStatus
194+
// UpdateStatus reconcile provided object status with given newUpdateStatus status
195+
func UpdateObjectStatus[T client.Object, ST StatusWithMetadata[STC], STC any](ctx context.Context, rclient client.Client, object ObjectWithDeepCopyAndStatus[T, ST, STC], newUpdateStatus vmv1beta1.UpdateStatus, maybeErr error) error {
196+
patch := client.MergeFrom(object.DeepCopy())
197+
newObjectStatus := object.GetStatus()
198+
prevObjectStatus := newObjectStatus.DeepCopy()
199+
newObjectMeta := newObjectStatus.GetStatusMetadata()
201200

202-
switch actualStatus {
201+
switch newUpdateStatus {
203202
case vmv1beta1.UpdateStatusExpanding, vmv1beta1.UpdateStatusOperational:
204-
currMeta.Reason = ""
203+
newObjectMeta.Reason = ""
205204
case vmv1beta1.UpdateStatusPaused:
206205
case vmv1beta1.UpdateStatusFailed:
207206
if maybeErr != nil {
208-
currMeta.Reason = maybeErr.Error()
207+
newObjectMeta.Reason = maybeErr.Error()
209208
}
210209
default:
211-
panic(fmt.Sprintf("BUG: not expected status=%q", actualStatus))
210+
panic(fmt.Sprintf("BUG: not expected status=%q", newUpdateStatus))
212211
}
213212

214-
currMeta.ObservedGeneration = object.GetGeneration()
215-
object.DefaultStatusFields(currentStatus)
216-
// if opts.mutateCurrentBeforeCompare != nil {
217-
// opts.mutateCurrentBeforeCompare(opts.crStatus.(ST))
218-
// }
213+
object.DefaultStatusFields(newObjectStatus)
214+
newObjectMeta.ObservedGeneration = object.GetGeneration()
215+
newObjectMeta.UpdateStatus = newUpdateStatus
219216
// compare before send update request
220217
// it reduces load at kubernetes api-server
221-
if equality.Semantic.DeepEqual(currentStatus, prevStatus) && currMeta.UpdateStatus == actualStatus {
218+
if equality.Semantic.DeepEqual(newObjectStatus, prevObjectStatus) {
222219
return nil
223220
}
224-
currMeta.UpdateStatus = newUpdateStatus
225221

226-
// make a deep copy before passing object to Patch function
227-
// it reload state of the object from API server
228-
// which is not desired behaviour
229-
objecToUpdate := object.DeepCopy()
230-
pr, err := buildStatusPatch(currentStatus)
231-
if err != nil {
232-
return err
233-
}
234-
if err := rclient.Status().Patch(ctx, objecToUpdate, pr); err != nil {
222+
objectToUpdate := object.DeepCopy()
223+
if err := rclient.Status().Patch(ctx, objectToUpdate, patch); err != nil {
235224
return fmt.Errorf("cannot update resource status with patch: %w", err)
236225
}
237226
// Update ResourceVersion in order to resolve future conflicts
238-
object.SetResourceVersion(objecToUpdate.GetResourceVersion())
227+
object.SetResourceVersion(objectToUpdate.GetResourceVersion())
239228

240229
return nil
241230
}
242-
243-
func buildStatusPatch(currentStatus any) (client.Patch, error) {
244-
type patch struct {
245-
OP string `json:"op"`
246-
Path string `json:"path"`
247-
Value any `json:"value"`
248-
}
249-
ops := []patch{
250-
{
251-
OP: "replace",
252-
Path: "/status",
253-
Value: currentStatus,
254-
},
255-
}
256-
data, err := json.Marshal(ops)
257-
if err != nil {
258-
return nil, fmt.Errorf("possible bug, cannot serialize patch specification as json :%w", err)
259-
}
260-
261-
return client.RawPatch(types.JSONPatchType, data), nil
262-
263-
}

‎test/e2e/childobjects/vmalertmanagerconfig_test.go‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ var _ = Describe("test vmalertmanagerconfig Controller", Label("vm", "child", "a
2424
amCfgs []*vmv1beta1.VMAlertmanagerConfig
2525
}
2626
type step struct {
27-
setup func()
2827
modify func()
2928
verify func()
3029
}
@@ -46,9 +45,6 @@ var _ = Describe("test vmalertmanagerconfig Controller", Label("vm", "child", "a
4645
}
4746
})
4847
step := steps[0]
49-
if step.setup != nil {
50-
step.setup()
51-
}
5248
for _, am := range args.ams {
5349
Expect(k8sClient.Create(ctx, am)).To(Succeed())
5450
}
@@ -70,9 +66,6 @@ var _ = Describe("test vmalertmanagerconfig Controller", Label("vm", "child", "a
7066
}
7167
step.verify()
7268
for _, step := range steps[1:] {
73-
if step.setup != nil {
74-
step.setup()
75-
}
7669
if step.modify != nil {
7770
step.modify()
7871
}

‎test/e2e/childobjects/vmrule_test.go‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ var _ = Describe("test vmrule Controller", Label("vm", "child", "alert"), func()
2424
rules []*vmv1beta1.VMRule
2525
}
2626
type step struct {
27-
setup func()
2827
modify func()
2928
verify func()
3029
}
@@ -46,9 +45,6 @@ var _ = Describe("test vmrule Controller", Label("vm", "child", "alert"), func()
4645
}
4746
})
4847
step := steps[0]
49-
if step.setup != nil {
50-
step.setup()
51-
}
5248
for _, alert := range args.alerts {
5349
Expect(k8sClient.Create(ctx, alert)).To(Succeed())
5450
}
@@ -70,9 +66,6 @@ var _ = Describe("test vmrule Controller", Label("vm", "child", "alert"), func()
7066
}
7167
step.verify()
7268
for _, step := range steps[1:] {
73-
if step.setup != nil {
74-
step.setup()
75-
}
7669
if step.modify != nil {
7770
step.modify()
7871
}

‎test/e2e/vlagent_test.go‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -373,12 +373,10 @@ var _ = Describe("test vlagent Controller", Label("vl", "agent", "vlagent"), fun
373373
step.setup(initCR)
374374
}
375375
// update and wait ready
376-
Eventually(func() error {
377-
var toUpdate vmv1.VLAgent
378-
Expect(k8sClient.Get(ctx, nsn, &toUpdate)).To(Succeed())
379-
step.modify(&toUpdate)
380-
return k8sClient.Update(ctx, &toUpdate)
381-
}, eventualExpandingTimeout).Should(Succeed())
376+
var toUpdate vmv1.VLAgent
377+
Expect(k8sClient.Get(ctx, nsn, &toUpdate)).To(Succeed())
378+
step.modify(&toUpdate)
379+
Expect(k8sClient.Update(ctx, &toUpdate)).To(Succeed())
382380
Eventually(func() error {
383381
return expectObjectStatusOperational(ctx, k8sClient, &vmv1.VLAgent{}, nsn)
384382
}, eventualStatefulsetAppReadyTimeout).Should(Succeed())

‎test/e2e/vlcluster_test.go‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,6 @@ var _ = Describe("test vlcluster Controller", Label("vl", "cluster", "vlcluster"
5555
},
5656
}
5757
type testStep struct {
58-
setup func(*vmv1.VLCluster)
5958
modify func(*vmv1.VLCluster)
6059
verify func(*vmv1.VLCluster)
6160
}
@@ -72,16 +71,11 @@ var _ = Describe("test vlcluster Controller", Label("vl", "cluster", "vlcluster"
7271
}, eventualDeploymentAppReadyTimeout).Should(Succeed())
7372

7473
for _, step := range steps {
75-
if step.setup != nil {
76-
step.setup(initCR)
77-
}
7874
// perform update
79-
Eventually(func() error {
80-
var toUpdate vmv1.VLCluster
81-
Expect(k8sClient.Get(ctx, nsn, &toUpdate)).To(Succeed())
82-
step.modify(&toUpdate)
83-
return k8sClient.Update(ctx, &toUpdate)
84-
}, eventualExpandingTimeout).Should(Succeed())
75+
var toUpdate vmv1.VLCluster
76+
Expect(k8sClient.Get(ctx, nsn, &toUpdate)).To(Succeed())
77+
step.modify(&toUpdate)
78+
Expect(k8sClient.Update(ctx, &toUpdate)).To(Succeed())
8579
Eventually(func() error {
8680
return expectObjectStatusOperational(ctx, k8sClient, &vmv1.VLCluster{}, nsn)
8781
}, eventualDeploymentAppReadyTimeout).Should(Succeed())

‎test/e2e/vlsingle_test.go‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,6 @@ var _ = Describe("test vlsingle Controller", Label("vl", "single", "vlsingle"),
174174
},
175175
}
176176
type testStep struct {
177-
setup func(*vmv1.VLSingle)
178177
modify func(*vmv1.VLSingle)
179178
verify func(*vmv1.VLSingle)
180179
}
@@ -191,16 +190,11 @@ var _ = Describe("test vlsingle Controller", Label("vl", "single", "vlsingle"),
191190
}, eventualDeploymentAppReadyTimeout).Should(Succeed())
192191

193192
for _, step := range steps {
194-
if step.setup != nil {
195-
step.setup(initCR)
196-
}
197193
// perform update
198-
Eventually(func() error {
199-
var toUpdate vmv1.VLSingle
200-
Expect(k8sClient.Get(ctx, nsn, &toUpdate)).To(Succeed())
201-
step.modify(&toUpdate)
202-
return k8sClient.Update(ctx, &toUpdate)
203-
}, eventualExpandingTimeout).Should(Succeed())
194+
var toUpdate vmv1.VLSingle
195+
Expect(k8sClient.Get(ctx, nsn, &toUpdate)).To(Succeed())
196+
step.modify(&toUpdate)
197+
Expect(k8sClient.Update(ctx, &toUpdate)).To(Succeed())
204198
Eventually(func() error {
205199
return expectObjectStatusOperational(ctx, k8sClient, &vmv1.VLSingle{}, nsn)
206200
}, eventualDeploymentAppReadyTimeout).Should(Succeed())

‎test/e2e/vmagent_test.go‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -420,12 +420,10 @@ var _ = Describe("test vmagent Controller", Label("vm", "agent", "vmagent"), fun
420420
step.setup(initCR)
421421
}
422422
// update and wait ready
423-
Eventually(func() error {
424-
var toUpdate vmv1beta1.VMAgent
425-
Expect(k8sClient.Get(ctx, nsn, &toUpdate)).To(Succeed())
426-
step.modify(&toUpdate)
427-
return k8sClient.Update(ctx, &toUpdate)
428-
}, eventualExpandingTimeout).Should(Succeed())
423+
var toUpdate vmv1beta1.VMAgent
424+
Expect(k8sClient.Get(ctx, nsn, &toUpdate)).To(Succeed())
425+
step.modify(&toUpdate)
426+
Expect(k8sClient.Update(ctx, &toUpdate)).To(Succeed())
429427
Eventually(func() error {
430428
return expectObjectStatusOperational(ctx, k8sClient, &vmv1beta1.VMAgent{}, nsn)
431429
}, eventualStatefulsetAppReadyTimeout).Should(Succeed())

‎test/e2e/vmalert_test.go‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -299,12 +299,10 @@ var _ = Describe("test vmalert Controller", Label("vm", "alert"), func() {
299299
return expectObjectStatusOperational(ctx, k8sClient, &vmv1beta1.VMAlert{}, nsn)
300300
}, eventualStatefulsetAppReadyTimeout).Should(Succeed())
301301
// update and wait ready
302-
Eventually(func() error {
303-
var toUpdate vmv1beta1.VMAlert
304-
Expect(k8sClient.Get(ctx, nsn, &toUpdate)).To(Succeed())
305-
modify(&toUpdate)
306-
return k8sClient.Update(ctx, &toUpdate)
307-
}, eventualExpandingTimeout).Should(Succeed())
302+
var toUpdate vmv1beta1.VMAlert
303+
Expect(k8sClient.Get(ctx, nsn, &toUpdate)).To(Succeed())
304+
modify(&toUpdate)
305+
Expect(k8sClient.Update(ctx, &toUpdate)).To(Succeed())
308306
Eventually(func() error {
309307
return expectObjectStatusOperational(ctx, k8sClient, &vmv1beta1.VMAlert{}, nsn)
310308
}, eventualStatefulsetAppReadyTimeout).Should(Succeed())

0 commit comments

Comments
 (0)