Skip to content

Commit e2a5311

Browse files
fwieselnotandy
fwiesel
authored and
notandy
committed
Eviction: Use Patch + FieldOwner
FieldOwner allows us to identify who maintains a field and detect if there is a conflict between multiple controllers.
1 parent 209d4dd commit e2a5311

File tree

1 file changed

+52
-56
lines changed

1 file changed

+52
-56
lines changed

‎internal/controller/eviction_controller.go‎

Lines changed: 52 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -100,14 +100,15 @@ func (r *EvictionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
100100

101101
statusCondition := meta.FindStatusCondition(eviction.Status.Conditions, kvmv1.ConditionTypeEvicting)
102102
if statusCondition == nil {
103-
// No status condition, so we need to add it
104-
if r.addCondition(ctx, eviction, metav1.ConditionTrue, "Running", kvmv1.ConditionReasonRunning) {
105-
log.Info("running")
106-
return ctrl.Result{}, nil
107-
}
108-
// We just checked if the condition is there, so this should never
109-
// be reached, but let's cover our bass
110-
return ctrl.Result{RequeueAfter: 1 * time.Second}, nil
103+
base := eviction.DeepCopy()
104+
meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
105+
Type: kvmv1.ConditionTypeEvicting,
106+
Status: metav1.ConditionTrue,
107+
Message: "Running",
108+
Reason: kvmv1.ConditionReasonRunning,
109+
})
110+
111+
return ctrl.Result{}, r.updateStatus(ctx, eviction, base)
111112
}
112113

113114
switch statusCondition.Status {
@@ -139,6 +140,7 @@ func (r *EvictionReconciler) handleRunning(ctx context.Context, eviction *kvmv1.
139140
return r.evictNext(ctx, eviction)
140141
}
141142

143+
base := eviction.DeepCopy()
142144
meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
143145
Type: kvmv1.ConditionTypeEvicting,
144146
Status: metav1.ConditionFalse,
@@ -148,10 +150,15 @@ func (r *EvictionReconciler) handleRunning(ctx context.Context, eviction *kvmv1.
148150

149151
eviction.Status.OutstandingRamMb = 0
150152
logger.FromContext(ctx).Info("succeeded")
151-
return ctrl.Result{}, r.Status().Update(ctx, eviction)
153+
return ctrl.Result{}, r.updateStatus(ctx, eviction, base)
154+
}
155+
156+
func (r *EvictionReconciler) updateStatus(ctx context.Context, eviction, base *kvmv1.Eviction) error {
157+
return r.Status().Patch(ctx, eviction, client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{}), client.FieldOwner(EvictionControllerName))
152158
}
153159

154160
func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv1.Eviction) (ctrl.Result, error) {
161+
base := eviction.DeepCopy()
155162
hypervisorName := eviction.Spec.Hypervisor
156163

157164
// Does the hypervisor even exist? Is it enabled/disabled?
@@ -176,7 +183,7 @@ func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv
176183
Message: err.Error(),
177184
Reason: kvmv1.ConditionReasonFailed,
178185
})
179-
return ctrl.Result{}, r.Status().Update(ctx, eviction)
186+
return ctrl.Result{}, r.updateStatus(ctx, eviction, base)
180187
} else {
181188
// That is (likely) an eviction for a node that never registered
182189
// so we are good to go
@@ -189,7 +196,7 @@ func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv
189196
})
190197
eviction.Status.OutstandingRamMb = 0
191198
logger.FromContext(ctx).Info(msg)
192-
return ctrl.Result{}, r.Status().Update(ctx, eviction)
199+
return ctrl.Result{}, r.updateStatus(ctx, eviction, base)
193200
}
194201
}
195202

@@ -204,7 +211,7 @@ func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv
204211
Message: err.Error(),
205212
Reason: kvmv1.ConditionReasonFailed,
206213
})
207-
return ctrl.Result{}, r.Status().Update(ctx, eviction)
214+
return ctrl.Result{}, r.updateStatus(ctx, eviction, base)
208215
}
209216

210217
if !meta.IsStatusConditionTrue(eviction.Status.Conditions, kvmv1.ConditionTypeHypervisorDisabled) {
@@ -235,15 +242,18 @@ func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv
235242
Message: "Preflight checks passed, hypervisor is disabled and ready for eviction",
236243
Reason: kvmv1.ConditionReasonSucceeded,
237244
})
238-
return ctrl.Result{}, r.Status().Update(ctx, eviction)
245+
return ctrl.Result{}, r.updateStatus(ctx, eviction, base)
239246
}
240247

241248
// Tries to handle the NotFound-error by updating the status
242-
func (r *EvictionReconciler) handleNotFound(ctx context.Context, eviction *kvmv1.Eviction, err error) error {
249+
func (r *EvictionReconciler) handleNotFound(ctx context.Context, eviction, base *kvmv1.Eviction, err error) error {
243250
if !gophercloud.ResponseCodeIs(err, http.StatusNotFound) {
244251
return err
245252
}
246253
logger.FromContext(ctx).Info("Instance is gone")
254+
if base == nil {
255+
base = eviction.DeepCopy()
256+
}
247257
instances := &eviction.Status.OutstandingInstances
248258
uuid := (*instances)[len(*instances)-1]
249259
*instances = (*instances)[:len(*instances)-1]
@@ -253,10 +263,11 @@ func (r *EvictionReconciler) handleNotFound(ctx context.Context, eviction *kvmv1
253263
Message: fmt.Sprintf("Instance %s is gone", uuid),
254264
Reason: kvmv1.ConditionReasonSucceeded,
255265
})
256-
return r.Status().Update(ctx, eviction)
266+
return r.updateStatus(ctx, eviction, base)
257267
}
258268

259269
func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Eviction) (ctrl.Result, error) {
270+
base := eviction.DeepCopy()
260271
instances := &eviction.Status.OutstandingInstances
261272
uuid := (*instances)[len(*instances)-1]
262273
log := logger.FromContext(ctx).WithName("Evict").WithValues("server", uuid)
@@ -266,7 +277,7 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic
266277
vm, err := res.Extract()
267278

268279
if err != nil {
269-
if err2 := r.handleNotFound(ctx, eviction, err); err2 != nil {
280+
if err2 := r.handleNotFound(ctx, eviction, base, err); err2 != nil {
270281
return ctrl.Result{}, err2
271282
} else {
272283
return ctrl.Result{RequeueAfter: shortRetryTime}, nil
@@ -293,7 +304,7 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic
293304
Message: fmt.Sprintf("Migration of instance %s failed: %s", vm.ID, vm.Fault.Message),
294305
Reason: kvmv1.ConditionReasonFailed,
295306
})
296-
if err := r.Status().Update(ctx, eviction); err != nil {
307+
if err := r.updateStatus(ctx, eviction, base); err != nil {
297308
return ctrl.Result{}, err
298309
}
299310

@@ -314,7 +325,7 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic
314325
// So, it is already off this one, do we need to verify it?
315326
if vm.Status == "VERIFY_RESIZE" {
316327
err := servers.ConfirmResize(ctx, r.computeClient, vm.ID).ExtractErr()
317-
if err2 := r.handleNotFound(ctx, eviction, err); err2 != nil {
328+
if err2 := r.handleNotFound(ctx, eviction, base, err); err2 != nil {
318329
// Retry confirm in next reconciliation
319330
return ctrl.Result{}, err2
320331
} else {
@@ -325,7 +336,7 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic
325336

326337
// All done
327338
*instances = (*instances)[:len(*instances)-1]
328-
return ctrl.Result{}, r.Status().Update(ctx, eviction)
339+
return ctrl.Result{}, r.updateStatus(ctx, eviction, base)
329340
}
330341

331342
if vm.TaskState == "deleting" { //nolint:gocritic
@@ -339,14 +350,14 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic
339350
Message: fmt.Sprintf("Live migration of terminating instance %s skipped", vm.ID),
340351
Reason: kvmv1.ConditionReasonFailed,
341352
})
342-
if err2 := r.Status().Update(ctx, eviction); err2 != nil {
353+
if err2 := r.updateStatus(ctx, eviction, base); err2 != nil {
343354
return ctrl.Result{}, fmt.Errorf("could update status due to %w", err2)
344355
}
345356
} else if vm.Status == "ACTIVE" || vm.PowerState == 1 {
346357
log.Info("trigger live-migration")
347358
err := r.liveMigrate(ctx, vm.ID, eviction)
348359
if err != nil {
349-
if err2 := r.handleNotFound(ctx, eviction, err); err2 != nil {
360+
if err2 := r.handleNotFound(ctx, eviction, base, err); err2 != nil {
350361
return ctrl.Result{}, err2
351362
} else {
352363
return ctrl.Result{RequeueAfter: shortRetryTime}, nil
@@ -356,7 +367,7 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic
356367
log.Info("trigger cold-migration")
357368
err := r.coldMigrate(ctx, vm.ID, eviction)
358369
if err != nil {
359-
if err2 := r.handleNotFound(ctx, eviction, err); err2 != nil {
370+
if err2 := r.handleNotFound(ctx, eviction, base, err); err2 != nil {
360371
return ctrl.Result{}, err2
361372
} else {
362373
return ctrl.Result{RequeueAfter: shortRetryTime}, nil
@@ -389,9 +400,9 @@ func (r *EvictionReconciler) handleFinalizer(ctx context.Context, eviction *kvmv
389400
}
390401
}
391402

392-
evictionBase := eviction.DeepCopy()
403+
base := eviction.DeepCopy()
393404
controllerutil.RemoveFinalizer(eviction, evictionFinalizerName)
394-
return r.Patch(ctx, eviction, client.MergeFromWithOptions(evictionBase, client.MergeFromWithOptimisticLock{}))
405+
return r.Patch(ctx, eviction, client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{}))
395406
}
396407

397408
func (r *EvictionReconciler) enableHypervisorService(ctx context.Context, eviction *kvmv1.Eviction) error {
@@ -400,18 +411,20 @@ func (r *EvictionReconciler) enableHypervisorService(ctx context.Context, evicti
400411
hypervisor, err := openstack.GetHypervisorByName(ctx, r.computeClient, eviction.Spec.Hypervisor, false)
401412
if err != nil {
402413
if errors.Is(err, openstack.ErrNoHypervisor) {
414+
base := eviction.DeepCopy()
403415
changed := meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
404416
Type: kvmv1.ConditionTypeHypervisorReEnabled,
405417
Status: metav1.ConditionTrue,
406418
Message: "Hypervisor is gone, no need to re-enable",
407419
Reason: kvmv1.ConditionReasonSucceeded,
408420
})
409421
if changed {
410-
return r.Status().Update(ctx, eviction)
422+
return r.updateStatus(ctx, eviction, base)
411423
} else {
412424
return nil
413425
}
414426
} else {
427+
base := eviction.DeepCopy()
415428
errorMessage := fmt.Sprintf("failed to get hypervisor due to %s", err)
416429
// update the condition to reflect the error, and retry the reconciliation
417430
changed := meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
@@ -422,7 +435,7 @@ func (r *EvictionReconciler) enableHypervisorService(ctx context.Context, evicti
422435
})
423436

424437
if changed {
425-
if err2 := r.Status().Update(ctx, eviction); err2 != nil {
438+
if err2 := r.updateStatus(ctx, eviction, base); err2 != nil {
426439
log.Error(err, "failed to store error message in condition", "message", errorMessage)
427440
return err2
428441
}
@@ -433,14 +446,15 @@ func (r *EvictionReconciler) enableHypervisorService(ctx context.Context, evicti
433446
}
434447

435448
if hypervisor.Service.DisabledReason != r.evictionReason(eviction) {
449+
base := eviction.DeepCopy()
436450
changed := meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
437451
Type: kvmv1.ConditionTypeHypervisorReEnabled,
438452
Status: metav1.ConditionTrue,
439453
Message: "Hypervisor already re-enabled for reason:" + hypervisor.Service.DisabledReason,
440454
Reason: kvmv1.ConditionReasonSucceeded,
441455
})
442456
if changed {
443-
return r.Status().Update(ctx, eviction)
457+
return r.updateStatus(ctx, eviction, base)
444458
} else {
445459
return nil
446460
}
@@ -451,6 +465,7 @@ func (r *EvictionReconciler) enableHypervisorService(ctx context.Context, evicti
451465
_, err = services.Update(ctx, r.computeClient, hypervisor.Service.ID, enableService).Extract()
452466

453467
if err != nil {
468+
base := eviction.DeepCopy()
454469
errorMessage := fmt.Sprintf("failed to enable hypervisor due to %s", err)
455470
changed := meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
456471
Type: kvmv1.ConditionTypeHypervisorReEnabled,
@@ -459,20 +474,21 @@ func (r *EvictionReconciler) enableHypervisorService(ctx context.Context, evicti
459474
Reason: kvmv1.ConditionReasonFailed,
460475
})
461476
if changed {
462-
if err2 := r.Status().Update(ctx, eviction); err2 != nil {
477+
if err2 := r.updateStatus(ctx, eviction, base); err2 != nil {
463478
log.Error(err, "failed to store error message in condition", "message", errorMessage)
464479
}
465480
}
466481
return ErrRetry
467482
} else {
483+
base := eviction.DeepCopy()
468484
changed := meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
469485
Type: kvmv1.ConditionTypeHypervisorReEnabled,
470486
Status: metav1.ConditionTrue,
471487
Message: "Hypervisor re-enabled successfully",
472488
Reason: kvmv1.ConditionReasonSucceeded,
473489
})
474490
if changed {
475-
return r.Status().Update(ctx, eviction)
491+
return r.updateStatus(ctx, eviction, base)
476492
} else {
477493
return nil
478494
}
@@ -486,26 +502,28 @@ func (r *EvictionReconciler) disableHypervisor(ctx context.Context, hypervisor *
486502
disabledReason := hypervisor.Service.DisabledReason
487503

488504
if disabledReason != "" && disabledReason != evictionReason {
505+
base := eviction.DeepCopy()
489506
// Disabled for another reason already
490507
meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
491508
Type: kvmv1.ConditionTypeHypervisorDisabled,
492509
Status: metav1.ConditionTrue,
493510
Message: fmt.Sprintf("Hypervisor already disabled for reason %q", disabledReason),
494511
Reason: kvmv1.ConditionReasonSucceeded,
495512
})
496-
return r.Status().Update(ctx, eviction)
513+
return r.updateStatus(ctx, eviction, base)
497514
}
498515

499516
if !controllerutil.ContainsFinalizer(eviction, evictionFinalizerName) {
500-
evictionBase := eviction.DeepCopy()
517+
base := eviction.DeepCopy()
501518
controllerutil.AddFinalizer(eviction, evictionFinalizerName)
502-
return r.Patch(ctx, eviction, client.MergeFromWithOptions(evictionBase, client.MergeFromWithOptimisticLock{}))
519+
return r.Patch(ctx, eviction, client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{}), client.FieldOwner(EvictionControllerName))
503520
}
504521

505522
disableService := services.UpdateOpts{Status: services.ServiceDisabled,
506523
DisabledReason: r.evictionReason(eviction)}
507524

508525
_, err := services.Update(ctx, r.computeClient, hypervisor.Service.ID, disableService).Extract()
526+
base := eviction.DeepCopy()
509527
if err != nil {
510528
// We expect OpenStack calls to be transient errors, so we retry for the next reconciliation
511529
meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
@@ -514,7 +532,7 @@ func (r *EvictionReconciler) disableHypervisor(ctx context.Context, hypervisor *
514532
Message: fmt.Sprintf("Failed to disable hypervisor: %v", err),
515533
Reason: kvmv1.ConditionReasonFailed,
516534
})
517-
return r.Status().Update(ctx, eviction)
535+
return r.updateStatus(ctx, eviction, base)
518536
}
519537

520538
meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
@@ -523,7 +541,7 @@ func (r *EvictionReconciler) disableHypervisor(ctx context.Context, hypervisor *
523541
Message: "Hypervisor disabled successfully",
524542
Reason: kvmv1.ConditionReasonSucceeded,
525543
})
526-
return r.Status().Update(ctx, eviction)
544+
return r.updateStatus(ctx, eviction, base)
527545
}
528546

529547
func (r *EvictionReconciler) liveMigrate(ctx context.Context, uuid string, eviction *kvmv1.Eviction) error {
@@ -568,28 +586,6 @@ func (r *EvictionReconciler) coldMigrate(ctx context.Context, uuid string, evict
568586
return nil
569587
}
570588

571-
// addCondition adds a condition to the Eviction status and updates the status
572-
func (r *EvictionReconciler) addCondition(ctx context.Context, eviction *kvmv1.Eviction,
573-
status metav1.ConditionStatus, message string, reason string) bool {
574-
575-
if !meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
576-
Type: kvmv1.ConditionTypeEvicting,
577-
Status: status,
578-
Message: message,
579-
Reason: reason,
580-
}) {
581-
return false
582-
}
583-
584-
if err := r.Status().Update(ctx, eviction); err != nil {
585-
log := logger.FromContext(ctx)
586-
log.Error(err, "Failed to update conditions")
587-
return false
588-
}
589-
590-
return true
591-
}
592-
593589
// SetupWithManager sets up the controller with the Manager.
594590
func (r *EvictionReconciler) SetupWithManager(mgr ctrl.Manager) error {
595591
ctx := context.Background()

0 commit comments

Comments
 (0)