From a971ed6ff771873a701f73ccc33ab4148c8243fc Mon Sep 17 00:00:00 2001 From: Francesco Pantano Date: Tue, 2 Apr 2024 23:50:34 +0200 Subject: [PATCH] Re-init conditions each reconcile This patch follows the same pattern applied to the other operators, where we re-init the condition at each reconcile loop. Conditions are re-evaluated and updated, keeping the LastTransitionTime for those that haven't changed (it avoids the transition from True to Unknown to True again). In addition, the "observedGeneration" field is introduced, and it is used by the openstack-operator to check the IsReady() function for a particular CR in case a minor update is triggered. All the conditions are evaluated during the main Reconcile loop ( or the reconcileNormal function in some circumstances), hence the main ReadyCondition is updated within the same flow. The defer function still updates the Resource and Mirror the condition to the top-level CR. Signed-off-by: Francesco Pantano --- ....openstack.org_openstackbaremetalsets.yaml | 8 ++ api/v1beta1/openstackbaremetalset_types.go | 5 + ....openstack.org_openstackbaremetalsets.yaml | 8 ++ .../openstackbaremetalset_controller.go | 59 ++++---- .../openstackprovisionserver_controller.go | 127 +++++++++--------- go.mod | 2 +- go.sum | 4 +- 7 files changed, 125 insertions(+), 88 deletions(-) diff --git a/api/bases/baremetal.openstack.org_openstackbaremetalsets.yaml b/api/bases/baremetal.openstack.org_openstackbaremetalsets.yaml index caedfcd..3ab8bd0 100644 --- a/api/bases/baremetal.openstack.org_openstackbaremetalsets.yaml +++ b/api/bases/baremetal.openstack.org_openstackbaremetalsets.yaml @@ -399,6 +399,14 @@ spec: type: string description: Map of hashes to track e.g. job status type: object + observedGeneration: + description: ObservedGeneration - the most recent generation observed + for this service. If the observed generation is less than the spec + generation, then the controller has not processed the latest changes + injected by the opentack-operator in the top-level CR (e.g. the + ContainerImage) + format: int64 + type: integer type: object type: object served: true diff --git a/api/v1beta1/openstackbaremetalset_types.go b/api/v1beta1/openstackbaremetalset_types.go index 56e8df5..0f97a14 100644 --- a/api/v1beta1/openstackbaremetalset_types.go +++ b/api/v1beta1/openstackbaremetalset_types.go @@ -138,6 +138,11 @@ type OpenStackBaremetalSetStatus struct { Hash map[string]string `json:"hash,omitempty"` // BaremetalHosts that are being processed or have been processed for this OpenStackBaremetalSet BaremetalHosts map[string]HostStatus `json:"baremetalHosts,omitempty" optional:"true"` + // ObservedGeneration - the most recent generation observed for this + // service. If the observed generation is less than the spec generation, + // then the controller has not processed the latest changes injected by + // the opentack-operator in the top-level CR (e.g. the ContainerImage) + ObservedGeneration int64 `json:"observedGeneration,omitempty"` } // +kubebuilder:object:root=true diff --git a/config/crd/bases/baremetal.openstack.org_openstackbaremetalsets.yaml b/config/crd/bases/baremetal.openstack.org_openstackbaremetalsets.yaml index caedfcd..3ab8bd0 100644 --- a/config/crd/bases/baremetal.openstack.org_openstackbaremetalsets.yaml +++ b/config/crd/bases/baremetal.openstack.org_openstackbaremetalsets.yaml @@ -399,6 +399,14 @@ spec: type: string description: Map of hashes to track e.g. job status type: object + observedGeneration: + description: ObservedGeneration - the most recent generation observed + for this service. If the observed generation is less than the spec + generation, then the controller has not processed the latest changes + injected by the opentack-operator in the top-level CR (e.g. the + ContainerImage) + format: int64 + type: integer type: object type: object served: true diff --git a/controllers/openstackbaremetalset_controller.go b/controllers/openstackbaremetalset_controller.go index b58c3e3..bec09a1 100644 --- a/controllers/openstackbaremetalset_controller.go +++ b/controllers/openstackbaremetalset_controller.go @@ -95,17 +95,23 @@ func (r *OpenStackBaremetalSetReconciler) Reconcile(ctx context.Context, req ctr return ctrl.Result{}, err } - // Always patch the instance status when exiting this function so we can persist any changes. + // initialize status if Conditions is nil, but do not reset if it already + // exists + isNewInstance := instance.Status.Conditions == nil + if isNewInstance { + instance.Status.Conditions = condition.Conditions{} + } + + // Save a copy of the condtions so that we can restore the LastTransitionTime + // when a condition's state doesn't change. + savedConditions := instance.Status.Conditions.DeepCopy() + + // Always patch the instance status when exiting this function so we can + // persist any changes. defer func() { - // update the Ready condition based on the sub conditions - if instance.Status.Conditions.AllSubConditionIsTrue() { - instance.Status.Conditions.MarkTrue( - condition.ReadyCondition, condition.ReadyMessage) - } else { - // something is not ready so reset the Ready condition - instance.Status.Conditions.MarkUnknown( - condition.ReadyCondition, condition.InitReason, condition.ReadyInitMessage) - // and recalculate it based on the state of the rest of the conditions + condition.RestoreLastTransitionTimes( + &instance.Status.Conditions, savedConditions) + if instance.Status.Conditions.IsUnknown(condition.ReadyCondition) { instance.Status.Conditions.Set( instance.Status.Conditions.Mirror(condition.ReadyCondition)) } @@ -116,28 +122,25 @@ func (r *OpenStackBaremetalSetReconciler) Reconcile(ctx context.Context, req ctr } }() - // If we're not deleting this and the service object doesn't have our finalizer, add it. - if instance.DeletionTimestamp.IsZero() && controllerutil.AddFinalizer(instance, helper.GetFinalizer()) { - return ctrl.Result{}, nil - } - // // initialize status // - if instance.Status.Conditions == nil { - instance.Status.Conditions = condition.Conditions{} - // initialize conditions used later as Status=Unknown - cl := condition.CreateList( - condition.UnknownCondition(condition.InputReadyCondition, condition.InitReason, condition.InputReadyInitMessage), - condition.UnknownCondition(baremetalv1.OpenStackBaremetalSetProvServerReadyCondition, condition.InitReason, baremetalv1.OpenStackBaremetalSetProvServerReadyInitMessage), - condition.UnknownCondition(baremetalv1.OpenStackBaremetalSetBmhProvisioningReadyCondition, condition.InitReason, baremetalv1.OpenStackBaremetalSetBmhProvisioningReadyInitMessage), - ) + // initialize conditions used later as Status=Unknown + cl := condition.CreateList( + condition.UnknownCondition(condition.ReadyCondition, condition.InitReason, condition.ReadyInitMessage), + condition.UnknownCondition(condition.InputReadyCondition, condition.InitReason, condition.InputReadyInitMessage), + condition.UnknownCondition(baremetalv1.OpenStackBaremetalSetProvServerReadyCondition, condition.InitReason, baremetalv1.OpenStackBaremetalSetProvServerReadyInitMessage), + condition.UnknownCondition(baremetalv1.OpenStackBaremetalSetBmhProvisioningReadyCondition, condition.InitReason, baremetalv1.OpenStackBaremetalSetBmhProvisioningReadyInitMessage), + ) - instance.Status.Conditions.Init(&cl) + instance.Status.Conditions.Init(&cl) + instance.Status.ObservedGeneration = instance.Generation - // Register overall status immediately to have an early feedback e.g. in the cli + // If we're not deleting this and the service object doesn't have our finalizer, add it. + if instance.DeletionTimestamp.IsZero() && controllerutil.AddFinalizer(instance, helper.GetFinalizer()) || isNewInstance { return ctrl.Result{}, nil } + if instance.Status.Hash == nil { instance.Status.Hash = map[string]string{} } @@ -446,6 +449,12 @@ func (r *OpenStackBaremetalSetReconciler) reconcileNormal(ctx context.Context, i instance.Status.Conditions.MarkTrue(baremetalv1.OpenStackBaremetalSetBmhProvisioningReadyCondition, baremetalv1.OpenStackBaremetalSetBmhProvisioningReadyMessage) // provision BMHs - end + // We reached the end of the Reconcile, update the Ready condition based on + // the sub conditions + if instance.Status.Conditions.AllSubConditionIsTrue() { + instance.Status.Conditions.MarkTrue( + condition.ReadyCondition, condition.ReadyMessage) + } r.Log.Info(fmt.Sprintf("Reconciled OpenStackBaremetalSet '%s' successfully", instance.Name)) return ctrl.Result{}, nil } diff --git a/controllers/openstackprovisionserver_controller.go b/controllers/openstackprovisionserver_controller.go index 48087c7..119d84d 100644 --- a/controllers/openstackprovisionserver_controller.go +++ b/controllers/openstackprovisionserver_controller.go @@ -119,17 +119,23 @@ func (r *OpenStackProvisionServerReconciler) Reconcile(ctx context.Context, req return ctrl.Result{}, err } - // Always patch the instance status when exiting this function so we can persist any changes. + // initialize status if Conditions is nil, but do not reset if it already + // exists + isNewInstance := instance.Status.Conditions == nil + if isNewInstance { + instance.Status.Conditions = condition.Conditions{} + } + + // Save a copy of the condtions so that we can restore the LastTransitionTime + // when a condition's state doesn't change. + savedConditions := instance.Status.Conditions.DeepCopy() + + // Always patch the instance status when exiting this function so we can + // persist any changes. defer func() { - // update the Ready condition based on the sub conditions - if instance.Status.Conditions.AllSubConditionIsTrue() { - instance.Status.Conditions.MarkTrue( - condition.ReadyCondition, condition.ReadyMessage) - } else { - // something is not ready so reset the Ready condition - instance.Status.Conditions.MarkUnknown( - condition.ReadyCondition, condition.InitReason, condition.ReadyInitMessage) - // and recalculate it based on the state of the rest of the conditions + condition.RestoreLastTransitionTimes( + &instance.Status.Conditions, savedConditions) + if instance.Status.Conditions.IsUnknown(condition.ReadyCondition) { instance.Status.Conditions.Set( instance.Status.Conditions.Mirror(condition.ReadyCondition)) } @@ -140,61 +146,56 @@ func (r *OpenStackProvisionServerReconciler) Reconcile(ctx context.Context, req } }() - // If we're not deleting this and the service object doesn't have our finalizer, add it. - if instance.DeletionTimestamp.IsZero() && controllerutil.AddFinalizer(instance, helper.GetFinalizer()) { - return ctrl.Result{}, nil - } - // // initialize status // - if instance.Status.Conditions == nil { - instance.Status.Conditions = condition.Conditions{} - // initialize conditions used later as Status=Unknown - cl := condition.CreateList( - condition.UnknownCondition( - condition.DeploymentReadyCondition, - condition.InitReason, - condition.DeploymentReadyInitMessage, - ), - condition.UnknownCondition( - condition.ServiceConfigReadyCondition, - condition.InitReason, - condition.ServiceConfigReadyInitMessage, - ), - condition.UnknownCondition( - baremetalv1.OpenStackProvisionServerProvIntfReadyCondition, - condition.InitReason, - baremetalv1.OpenStackProvisionServerProvIntfReadyInitMessage, - ), - condition.UnknownCondition( - baremetalv1.OpenStackProvisionServerLocalImageURLReadyCondition, - condition.InitReason, - baremetalv1.OpenStackProvisionServerLocalImageURLReadyInitMessage, - ), - - // service account, role, rolebinding conditions - condition.UnknownCondition( - condition.ServiceAccountReadyCondition, - condition.InitReason, - condition.ServiceAccountReadyInitMessage, - ), - condition.UnknownCondition( - condition.RoleReadyCondition, - condition.InitReason, - condition.RoleReadyInitMessage, - ), - condition.UnknownCondition( - condition.RoleBindingReadyCondition, - condition.InitReason, - condition.RoleBindingReadyInitMessage, - ), - ) - instance.Status.Conditions.Init(&cl) - - // Register overall status immediately to have an early feedback e.g. in the cli + // initialize conditions used later as Status=Unknown + cl := condition.CreateList( + condition.UnknownCondition( + condition.DeploymentReadyCondition, + condition.InitReason, + condition.DeploymentReadyInitMessage, + ), + condition.UnknownCondition( + condition.ServiceConfigReadyCondition, + condition.InitReason, + condition.ServiceConfigReadyInitMessage, + ), + condition.UnknownCondition( + baremetalv1.OpenStackProvisionServerProvIntfReadyCondition, + condition.InitReason, + baremetalv1.OpenStackProvisionServerProvIntfReadyInitMessage, + ), + condition.UnknownCondition( + baremetalv1.OpenStackProvisionServerLocalImageURLReadyCondition, + condition.InitReason, + baremetalv1.OpenStackProvisionServerLocalImageURLReadyInitMessage, + ), + + // service account, role, rolebinding conditions + condition.UnknownCondition( + condition.ServiceAccountReadyCondition, + condition.InitReason, + condition.ServiceAccountReadyInitMessage, + ), + condition.UnknownCondition( + condition.RoleReadyCondition, + condition.InitReason, + condition.RoleReadyInitMessage, + ), + condition.UnknownCondition( + condition.RoleBindingReadyCondition, + condition.InitReason, + condition.RoleBindingReadyInitMessage, + ), + ) + instance.Status.Conditions.Init(&cl) + + // If we're not deleting this and the service object doesn't have our finalizer, add it. + if instance.DeletionTimestamp.IsZero() && controllerutil.AddFinalizer(instance, helper.GetFinalizer()) || isNewInstance { return ctrl.Result{}, nil } + if instance.Status.Hash == nil { instance.Status.Hash = map[string]string{} } @@ -455,6 +456,12 @@ func (r *OpenStackProvisionServerReconciler) reconcileNormal(ctx context.Context } // check ProvisionIp/LocalImageURL - end + // We reached the end of the Reconcile, update the Ready condition based on + // the sub conditions + if instance.Status.Conditions.AllSubConditionIsTrue() { + instance.Status.Conditions.MarkTrue( + condition.ReadyCondition, condition.ReadyMessage) + } r.Log.Info(fmt.Sprintf("Reconciled OpenStackProvisionServer '%s' successfully", instance.Name)) return ctrl.Result{}, nil } diff --git a/go.mod b/go.mod index 9994deb..4a6f60b 100644 --- a/go.mod +++ b/go.mod @@ -9,7 +9,7 @@ require ( github.com/metal3-io/baremetal-operator/apis v0.5.1 github.com/onsi/ginkgo/v2 v2.17.1 github.com/onsi/gomega v1.32.0 - github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240214144842-5dcac51e5b36 + github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240412091425-bb628ded5eb8 github.com/openstack-k8s-operators/openstack-baremetal-operator/api v0.0.0-00010101000000-000000000000 github.com/spf13/cobra v1.8.0 k8s.io/api v0.28.8 diff --git a/go.sum b/go.sum index 7da16a0..45340ad 100644 --- a/go.sum +++ b/go.sum @@ -94,8 +94,8 @@ github.com/onsi/ginkgo/v2 v2.17.1 h1:V++EzdbhI4ZV4ev0UTIj0PzhzOcReJFyJaLjtSF55M8 github.com/onsi/ginkgo/v2 v2.17.1/go.mod h1:llBI3WDLL9Z6taip6f33H76YcWtJv+7R3HigUjbIBOs= github.com/onsi/gomega v1.32.0 h1:JRYU78fJ1LPxlckP6Txi/EYqJvjtMrDC04/MM5XRHPk= github.com/onsi/gomega v1.32.0/go.mod h1:a4x4gW6Pz2yK1MAmvluYme5lvYTn61afQ2ETw/8n4Lg= -github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240214144842-5dcac51e5b36 h1:ZSVQYuevQyYZ+bD/x3NLzZx/oVcrsT2tG5agqFzs8fQ= -github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240214144842-5dcac51e5b36/go.mod h1:bQwzyQtWCR9F0+IvWZ30J9d1lB6tcX3CNJ0Ten1smDw= +github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240412091425-bb628ded5eb8 h1:5ywsORdn4y7vNO732ODwYlmfRXnEphfoqB6PCYWt9r8= +github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240412091425-bb628ded5eb8/go.mod h1:gqByVGUdKQB/NkhKV4eD+8NWYkHq961nC96rTCB3ywE= github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=