From 8b6fe3d7477d7af3f518c169bdd94eb5fc0dbf2e Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Thu, 8 Dec 2022 12:11:03 +0100 Subject: [PATCH] Refactor our defer calls Our defer call at the end of Reconcile does two things: 1) update the Ready condition of the instance if all the other conditions are in True state 2) persist the metadata and the status changes of the object into etcd. The #2 is complicated but common for each of our controllers so it is pulled out and the impl moved to ReconcileBase. --- controllers/common.go | 36 +++++++++++++++++++++++++ controllers/nova_controller.go | 34 ++--------------------- controllers/novaapi_controller.go | 34 ++--------------------- controllers/novacell_controller.go | 20 ++------------ controllers/novaconductor_controller.go | 20 ++------------ 5 files changed, 44 insertions(+), 100 deletions(-) diff --git a/controllers/common.go b/controllers/common.go index b26fda008..4930269b5 100644 --- a/controllers/common.go +++ b/controllers/common.go @@ -274,3 +274,39 @@ func (r *ReconcilerBase) GenerateConfigs( // to enable unit testing at some point. return configmap.EnsureConfigMaps(ctx, h, instance, cms, envVars) } + +func (r *ReconcilerBase) patchInstance(ctx context.Context, h *helper.Helper, instance client.Object) error { + var err error + + if err = h.SetAfter(instance); err != nil { + util.LogErrorForObject(h, err, "Set after and calc patch/diff", instance) + return err + } + + changes := h.GetChanges() + patch := client.MergeFrom(h.GetBeforeObject()) + + if changes["metadata"] { + err = r.Client.Patch(ctx, instance, patch) + if k8s_errors.IsConflict(err) { + util.LogForObject(h, "Metadata update conflict", instance) + return err + } else if err != nil && !k8s_errors.IsNotFound(err) { + util.LogErrorForObject(h, err, "Metadate update failed", instance) + return err + } + } + + if changes["status"] { + err = r.Client.Status().Patch(ctx, instance, patch) + if k8s_errors.IsConflict(err) { + util.LogForObject(h, "Status update conflict", instance) + return err + + } else if err != nil && !k8s_errors.IsNotFound(err) { + util.LogErrorForObject(h, err, "Status update failed", instance) + return err + } + } + return nil +} diff --git a/controllers/nova_controller.go b/controllers/nova_controller.go index 99f94e206..736b76fbd 100644 --- a/controllers/nova_controller.go +++ b/controllers/nova_controller.go @@ -25,7 +25,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" @@ -112,40 +111,11 @@ func (r *NovaReconciler) Reconcile(ctx context.Context, req ctrl.Request) (resul instance.Status.Conditions.MarkTrue( condition.ReadyCondition, condition.ReadyMessage) } - if err := h.SetAfter(instance); err != nil { - util.LogErrorForObject(h, err, "Set after and calc patch/diff", instance) + err := r.patchInstance(ctx, h, instance) + if err != nil { _err = err return } - - changes := h.GetChanges() - patch := client.MergeFrom(h.GetBeforeObject()) - - if changes["metadata"] { - err = r.Client.Patch(ctx, instance, patch) - if k8s_errors.IsConflict(err) { - util.LogForObject(h, "Metadata update conflict", instance) - _err = err - return - } else if err != nil && !k8s_errors.IsNotFound(err) { - util.LogErrorForObject(h, err, "Metadate update failed", instance) - _err = err - return - } - } - - if changes["status"] { - err = r.Client.Status().Patch(ctx, instance, patch) - if k8s_errors.IsConflict(err) { - util.LogForObject(h, "Status update conflict", instance) - _err = err - return - } else if err != nil && !k8s_errors.IsNotFound(err) { - util.LogErrorForObject(h, err, "Status update failed", instance) - _err = err - return - } - } }() if !instance.DeletionTimestamp.IsZero() { diff --git a/controllers/novaapi_controller.go b/controllers/novaapi_controller.go index 9532c7d9e..01f64a4a7 100644 --- a/controllers/novaapi_controller.go +++ b/controllers/novaapi_controller.go @@ -23,7 +23,6 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" @@ -116,40 +115,11 @@ func (r *NovaAPIReconciler) Reconcile(ctx context.Context, req ctrl.Request) (re instance.Status.Conditions.MarkTrue( condition.ReadyCondition, condition.ReadyMessage) } - if err := h.SetAfter(instance); err != nil { - util.LogErrorForObject(h, err, "Set after and calc patch/diff", instance) + err := r.patchInstance(ctx, h, instance) + if err != nil { _err = err return } - - changes := h.GetChanges() - patch := client.MergeFrom(h.GetBeforeObject()) - - if changes["metadata"] { - err = r.Client.Patch(ctx, instance, patch) - if k8s_errors.IsConflict(err) { - util.LogForObject(h, "Metadata update conflict", instance) - _err = err - return - } else if err != nil && !k8s_errors.IsNotFound(err) { - util.LogErrorForObject(h, err, "Metadate update failed", instance) - _err = err - return - } - } - - if changes["status"] { - err = r.Client.Status().Patch(ctx, instance, patch) - if k8s_errors.IsConflict(err) { - util.LogForObject(h, "Status update conflict", instance) - _err = err - return - } else if err != nil && !k8s_errors.IsNotFound(err) { - util.LogErrorForObject(h, err, "Status update failed", instance) - _err = err - return - } - } }() if !instance.DeletionTimestamp.IsZero() { diff --git a/controllers/novacell_controller.go b/controllers/novacell_controller.go index 76254ce82..ad8e07d83 100644 --- a/controllers/novacell_controller.go +++ b/controllers/novacell_controller.go @@ -23,7 +23,6 @@ import ( k8s_errors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" @@ -97,26 +96,11 @@ func (r *NovaCellReconciler) Reconcile(ctx context.Context, req ctrl.Request) (r instance.Status.Conditions.MarkTrue( condition.ReadyCondition, condition.ReadyMessage) } - if err := h.SetAfter(instance); err != nil { - util.LogErrorForObject(h, err, "Set after and calc patch/diff", instance) + err := r.patchInstance(ctx, h, instance) + if err != nil { _err = err return } - - if changed := h.GetChanges()["status"]; changed { - patch := client.MergeFrom(h.GetBeforeObject()) - - err = r.Client.Status().Patch(ctx, instance, patch) - if k8s_errors.IsConflict(err) { - util.LogForObject(h, "Status update conflict", instance) - _err = err - return - } else if err != nil && !k8s_errors.IsNotFound(err) { - util.LogErrorForObject(h, err, "Status update failed", instance) - _err = err - return - } - } }() result, err = r.ensureConductor(ctx, h, instance) diff --git a/controllers/novaconductor_controller.go b/controllers/novaconductor_controller.go index a507a67c6..d3017fcb1 100644 --- a/controllers/novaconductor_controller.go +++ b/controllers/novaconductor_controller.go @@ -24,7 +24,6 @@ import ( k8s_errors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" common "github.com/openstack-k8s-operators/lib-common/modules/common" @@ -107,26 +106,11 @@ func (r *NovaConductorReconciler) Reconcile(ctx context.Context, req ctrl.Reques instance.Status.Conditions.MarkTrue( condition.ReadyCondition, condition.ReadyMessage) } - if err := h.SetAfter(instance); err != nil { - util.LogErrorForObject(h, err, "Set after and calc patch/diff", instance) + err := r.patchInstance(ctx, h, instance) + if err != nil { _err = err return } - - if changed := h.GetChanges()["status"]; changed { - patch := client.MergeFrom(h.GetBeforeObject()) - - err = r.Client.Status().Patch(ctx, instance, patch) - if k8s_errors.IsConflict(err) { - util.LogForObject(h, "Status update conflict", instance) - _err = err - return - } else if err != nil && !k8s_errors.IsNotFound(err) { - util.LogErrorForObject(h, err, "Status update failed", instance) - _err = err - return - } - } }() // TODO(gibi): Can we use a simple map[string][string] for hashes?