From 0b3d293a0ccd10778a702b2803a88001fc78b690 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Thu, 11 Apr 2024 18:54:53 +0200 Subject: [PATCH 1/4] Default readyCount to 0 This patch defaults `readyCount` for cinderapi, cinderbackups, cinderschedulers, and cindervolumes CRDs to 0. It also defaults to 0 their counterparts in the cinder CRD, except the cindervolumes that is a map with no entries. --- api/bases/cinder.openstack.org_cinderapis.yaml | 4 ++++ api/bases/cinder.openstack.org_cinderbackups.yaml | 4 ++++ api/bases/cinder.openstack.org_cinders.yaml | 10 ++++++++++ api/bases/cinder.openstack.org_cinderschedulers.yaml | 4 ++++ api/bases/cinder.openstack.org_cindervolumes.yaml | 4 ++++ api/v1beta1/cinder_types.go | 12 +++++++++--- api/v1beta1/cinderapi_types.go | 4 +++- api/v1beta1/cinderbackup_types.go | 4 +++- api/v1beta1/cinderscheduler_types.go | 4 +++- api/v1beta1/cindervolume_types.go | 4 +++- .../crd/bases/cinder.openstack.org_cinderapis.yaml | 4 ++++ .../bases/cinder.openstack.org_cinderbackups.yaml | 4 ++++ config/crd/bases/cinder.openstack.org_cinders.yaml | 10 ++++++++++ .../bases/cinder.openstack.org_cinderschedulers.yaml | 4 ++++ .../bases/cinder.openstack.org_cindervolumes.yaml | 4 ++++ 15 files changed, 73 insertions(+), 7 deletions(-) diff --git a/api/bases/cinder.openstack.org_cinderapis.yaml b/api/bases/cinder.openstack.org_cinderapis.yaml index 95abd8b9..75c02ec9 100644 --- a/api/bases/cinder.openstack.org_cinderapis.yaml +++ b/api/bases/cinder.openstack.org_cinderapis.yaml @@ -987,12 +987,16 @@ spec: type: array type: object readyCount: + default: 0 format: int32 + minimum: 0 type: integer serviceIDs: additionalProperties: type: string type: object + required: + - readyCount type: object type: object served: true diff --git a/api/bases/cinder.openstack.org_cinderbackups.yaml b/api/bases/cinder.openstack.org_cinderbackups.yaml index 64ca610b..072f4504 100644 --- a/api/bases/cinder.openstack.org_cinderbackups.yaml +++ b/api/bases/cinder.openstack.org_cinderbackups.yaml @@ -917,8 +917,12 @@ spec: type: array type: object readyCount: + default: 0 format: int32 + minimum: 0 type: integer + required: + - readyCount type: object type: object served: true diff --git a/api/bases/cinder.openstack.org_cinders.yaml b/api/bases/cinder.openstack.org_cinders.yaml index e32e076d..4c53941b 100644 --- a/api/bases/cinder.openstack.org_cinders.yaml +++ b/api/bases/cinder.openstack.org_cinders.yaml @@ -1152,13 +1152,19 @@ spec: type: object type: object cinderAPIReadyCount: + default: 0 format: int32 + minimum: 0 type: integer cinderBackupReadyCount: + default: 0 format: int32 + minimum: 0 type: integer cinderSchedulerReadyCount: + default: 0 format: int32 + minimum: 0 type: integer cinderVolumesReadyCounts: additionalProperties: @@ -1202,6 +1208,10 @@ spec: type: object transportURLSecret: type: string + required: + - cinderAPIReadyCount + - cinderBackupReadyCount + - cinderSchedulerReadyCount type: object type: object served: true diff --git a/api/bases/cinder.openstack.org_cinderschedulers.yaml b/api/bases/cinder.openstack.org_cinderschedulers.yaml index 1819a783..a2372c57 100644 --- a/api/bases/cinder.openstack.org_cinderschedulers.yaml +++ b/api/bases/cinder.openstack.org_cinderschedulers.yaml @@ -917,8 +917,12 @@ spec: type: array type: object readyCount: + default: 0 format: int32 + minimum: 0 type: integer + required: + - readyCount type: object type: object served: true diff --git a/api/bases/cinder.openstack.org_cindervolumes.yaml b/api/bases/cinder.openstack.org_cindervolumes.yaml index 34547f2d..7e423dda 100644 --- a/api/bases/cinder.openstack.org_cindervolumes.yaml +++ b/api/bases/cinder.openstack.org_cindervolumes.yaml @@ -918,8 +918,12 @@ spec: type: array type: object readyCount: + default: 0 format: int32 + minimum: 0 type: integer + required: + - readyCount type: object type: object served: true diff --git a/api/v1beta1/cinder_types.go b/api/v1beta1/cinder_types.go index 4b395766..0483d8ce 100644 --- a/api/v1beta1/cinder_types.go +++ b/api/v1beta1/cinder_types.go @@ -160,13 +160,19 @@ type CinderStatus struct { ServiceIDs map[string]string `json:"serviceIDs,omitempty"` // ReadyCount of Cinder API instance - CinderAPIReadyCount int32 `json:"cinderAPIReadyCount,omitempty"` + // +kubebuilder:validation:Minimum=0 + // +kubebuilder:default=0 + CinderAPIReadyCount int32 `json:"cinderAPIReadyCount"` // ReadyCount of Cinder Backup instance - CinderBackupReadyCount int32 `json:"cinderBackupReadyCount,omitempty"` + // +kubebuilder:validation:Minimum=0 + // +kubebuilder:default=0 + CinderBackupReadyCount int32 `json:"cinderBackupReadyCount"` // ReadyCount of Cinder Scheduler instance - CinderSchedulerReadyCount int32 `json:"cinderSchedulerReadyCount,omitempty"` + // +kubebuilder:validation:Minimum=0 + // +kubebuilder:default=0 + CinderSchedulerReadyCount int32 `json:"cinderSchedulerReadyCount"` // ReadyCounts of Cinder Volume instances CinderVolumesReadyCounts map[string]int32 `json:"cinderVolumesReadyCounts,omitempty"` diff --git a/api/v1beta1/cinderapi_types.go b/api/v1beta1/cinderapi_types.go index b57316b1..6d403572 100644 --- a/api/v1beta1/cinderapi_types.go +++ b/api/v1beta1/cinderapi_types.go @@ -97,7 +97,9 @@ type CinderAPIStatus struct { Conditions condition.Conditions `json:"conditions,omitempty" optional:"true"` // ReadyCount of Cinder API instances - ReadyCount int32 `json:"readyCount,omitempty"` + // +kubebuilder:validation:Minimum=0 + // +kubebuilder:default=0 + ReadyCount int32 `json:"readyCount"` // ServiceIDs ServiceIDs map[string]string `json:"serviceIDs,omitempty"` diff --git a/api/v1beta1/cinderbackup_types.go b/api/v1beta1/cinderbackup_types.go index ec223b95..5be153aa 100644 --- a/api/v1beta1/cinderbackup_types.go +++ b/api/v1beta1/cinderbackup_types.go @@ -82,7 +82,9 @@ type CinderBackupStatus struct { Conditions condition.Conditions `json:"conditions,omitempty" optional:"true"` // ReadyCount of Cinder Backup instances - ReadyCount int32 `json:"readyCount,omitempty"` + // +kubebuilder:validation:Minimum=0 + // +kubebuilder:default=0 + ReadyCount int32 `json:"readyCount"` // NetworkAttachments status of the deployment pods NetworkAttachments map[string][]string `json:"networkAttachments,omitempty"` diff --git a/api/v1beta1/cinderscheduler_types.go b/api/v1beta1/cinderscheduler_types.go index 32f09fcf..114fc6a6 100644 --- a/api/v1beta1/cinderscheduler_types.go +++ b/api/v1beta1/cinderscheduler_types.go @@ -82,7 +82,9 @@ type CinderSchedulerStatus struct { Conditions condition.Conditions `json:"conditions,omitempty" optional:"true"` // ReadyCount of Cinder Scheduler instances - ReadyCount int32 `json:"readyCount,omitempty"` + // +kubebuilder:validation:Minimum=0 + // +kubebuilder:default=0 + ReadyCount int32 `json:"readyCount"` // NetworkAttachments status of the deployment pods NetworkAttachments map[string][]string `json:"networkAttachments,omitempty"` diff --git a/api/v1beta1/cindervolume_types.go b/api/v1beta1/cindervolume_types.go index e7694122..fb61bb86 100644 --- a/api/v1beta1/cindervolume_types.go +++ b/api/v1beta1/cindervolume_types.go @@ -83,7 +83,9 @@ type CinderVolumeStatus struct { Conditions condition.Conditions `json:"conditions,omitempty" optional:"true"` // ReadyCount of Cinder Volume instances - ReadyCount int32 `json:"readyCount,omitempty"` + // +kubebuilder:validation:Minimum=0 + // +kubebuilder:default=0 + ReadyCount int32 `json:"readyCount"` // NetworkAttachments status of the deployment pods NetworkAttachments map[string][]string `json:"networkAttachments,omitempty"` diff --git a/config/crd/bases/cinder.openstack.org_cinderapis.yaml b/config/crd/bases/cinder.openstack.org_cinderapis.yaml index 95abd8b9..75c02ec9 100644 --- a/config/crd/bases/cinder.openstack.org_cinderapis.yaml +++ b/config/crd/bases/cinder.openstack.org_cinderapis.yaml @@ -987,12 +987,16 @@ spec: type: array type: object readyCount: + default: 0 format: int32 + minimum: 0 type: integer serviceIDs: additionalProperties: type: string type: object + required: + - readyCount type: object type: object served: true diff --git a/config/crd/bases/cinder.openstack.org_cinderbackups.yaml b/config/crd/bases/cinder.openstack.org_cinderbackups.yaml index 64ca610b..072f4504 100644 --- a/config/crd/bases/cinder.openstack.org_cinderbackups.yaml +++ b/config/crd/bases/cinder.openstack.org_cinderbackups.yaml @@ -917,8 +917,12 @@ spec: type: array type: object readyCount: + default: 0 format: int32 + minimum: 0 type: integer + required: + - readyCount type: object type: object served: true diff --git a/config/crd/bases/cinder.openstack.org_cinders.yaml b/config/crd/bases/cinder.openstack.org_cinders.yaml index e32e076d..4c53941b 100644 --- a/config/crd/bases/cinder.openstack.org_cinders.yaml +++ b/config/crd/bases/cinder.openstack.org_cinders.yaml @@ -1152,13 +1152,19 @@ spec: type: object type: object cinderAPIReadyCount: + default: 0 format: int32 + minimum: 0 type: integer cinderBackupReadyCount: + default: 0 format: int32 + minimum: 0 type: integer cinderSchedulerReadyCount: + default: 0 format: int32 + minimum: 0 type: integer cinderVolumesReadyCounts: additionalProperties: @@ -1202,6 +1208,10 @@ spec: type: object transportURLSecret: type: string + required: + - cinderAPIReadyCount + - cinderBackupReadyCount + - cinderSchedulerReadyCount type: object type: object served: true diff --git a/config/crd/bases/cinder.openstack.org_cinderschedulers.yaml b/config/crd/bases/cinder.openstack.org_cinderschedulers.yaml index 1819a783..a2372c57 100644 --- a/config/crd/bases/cinder.openstack.org_cinderschedulers.yaml +++ b/config/crd/bases/cinder.openstack.org_cinderschedulers.yaml @@ -917,8 +917,12 @@ spec: type: array type: object readyCount: + default: 0 format: int32 + minimum: 0 type: integer + required: + - readyCount type: object type: object served: true diff --git a/config/crd/bases/cinder.openstack.org_cindervolumes.yaml b/config/crd/bases/cinder.openstack.org_cindervolumes.yaml index 34547f2d..7e423dda 100644 --- a/config/crd/bases/cinder.openstack.org_cindervolumes.yaml +++ b/config/crd/bases/cinder.openstack.org_cindervolumes.yaml @@ -918,8 +918,12 @@ spec: type: array type: object readyCount: + default: 0 format: int32 + minimum: 0 type: integer + required: + - readyCount type: object type: object served: true From 8538636276f37331ffe962ddd1d59aaaf1ef4b81 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Fri, 12 Apr 2024 11:53:34 +0200 Subject: [PATCH 2/4] Fix ObservedGeneration assigment ObservedGeneration needs to be assigned as soon as the reconciler starts processing the CRD and its conditions have been reset correctly, because at that point the controller has "observed" the changes and set the conditions accordingly. This patch moves the assignment from the end of the reconcile loop to almost the beginning. --- api/v1beta1/cinder_types.go | 5 ++++- controllers/cinder_controller.go | 3 ++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/api/v1beta1/cinder_types.go b/api/v1beta1/cinder_types.go index 0483d8ce..a7d018ff 100644 --- a/api/v1beta1/cinder_types.go +++ b/api/v1beta1/cinder_types.go @@ -177,7 +177,10 @@ type CinderStatus struct { // ReadyCounts of Cinder Volume instances CinderVolumesReadyCounts map[string]int32 `json:"cinderVolumesReadyCounts,omitempty"` - //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. + // ObservedGeneration - the most recent generation observed for this service. + // If the observed generation is different than the spec generation, then thei + // controller has not started processing the latest changes, and the status + // and its conditions are likely stale. ObservedGeneration int64 `json:"observedGeneration,omitempty"` } diff --git a/controllers/cinder_controller.go b/controllers/cinder_controller.go index c0bf5de9..12badab3 100644 --- a/controllers/cinder_controller.go +++ b/controllers/cinder_controller.go @@ -196,6 +196,8 @@ func (r *CinderReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res condition.UnknownCondition(condition.RoleBindingReadyCondition, condition.InitReason, condition.RoleBindingReadyInitMessage), ) instance.Status.Conditions.Init(&cl) + // Always mark the Generation as observed early on + instance.Status.ObservedGeneration = instance.Generation // 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 { @@ -841,7 +843,6 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder } Log.Info(fmt.Sprintf("Reconciled Service '%s' successfully", instance.Name)) - instance.Status.ObservedGeneration = instance.Generation // update the overall status condition if service is ready if instance.IsReady() { instance.Status.Conditions.MarkTrue(condition.ReadyCondition, condition.ReadyMessage) From 8a67a43dfd2852362c2618e16734f3623fc953fa Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Fri, 12 Apr 2024 12:31:46 +0200 Subject: [PATCH 3/4] Add ObservedGeneration to sub resources To properly manage the top cinder CRD's ObservedGeneration we need ObservedGeneration in the sub CRDs: cinderapi, cinderbackup, cinderschedulers, and cindervolumes. This patch adds them to the CRDs and marks them as watched as soon as the reconciliation starts, just like we are doing in the top level CRD. --- api/bases/cinder.openstack.org_cinderapis.yaml | 3 +++ api/bases/cinder.openstack.org_cinderbackups.yaml | 3 +++ api/bases/cinder.openstack.org_cinderschedulers.yaml | 3 +++ api/bases/cinder.openstack.org_cindervolumes.yaml | 3 +++ api/v1beta1/cinder_types.go | 2 +- api/v1beta1/cinderapi_types.go | 6 ++++++ api/v1beta1/cinderbackup_types.go | 6 ++++++ api/v1beta1/cinderscheduler_types.go | 6 ++++++ api/v1beta1/cindervolume_types.go | 6 ++++++ config/crd/bases/cinder.openstack.org_cinderapis.yaml | 3 +++ config/crd/bases/cinder.openstack.org_cinderbackups.yaml | 3 +++ config/crd/bases/cinder.openstack.org_cinderschedulers.yaml | 3 +++ config/crd/bases/cinder.openstack.org_cindervolumes.yaml | 3 +++ controllers/cinderapi_controller.go | 2 ++ controllers/cinderbackup_controller.go | 2 ++ controllers/cinderscheduler_controller.go | 2 ++ controllers/cindervolume_controller.go | 2 ++ 17 files changed, 57 insertions(+), 1 deletion(-) diff --git a/api/bases/cinder.openstack.org_cinderapis.yaml b/api/bases/cinder.openstack.org_cinderapis.yaml index 75c02ec9..d8767d37 100644 --- a/api/bases/cinder.openstack.org_cinderapis.yaml +++ b/api/bases/cinder.openstack.org_cinderapis.yaml @@ -986,6 +986,9 @@ spec: type: string type: array type: object + observedGeneration: + format: int64 + type: integer readyCount: default: 0 format: int32 diff --git a/api/bases/cinder.openstack.org_cinderbackups.yaml b/api/bases/cinder.openstack.org_cinderbackups.yaml index 072f4504..9a15c7c8 100644 --- a/api/bases/cinder.openstack.org_cinderbackups.yaml +++ b/api/bases/cinder.openstack.org_cinderbackups.yaml @@ -916,6 +916,9 @@ spec: type: string type: array type: object + observedGeneration: + format: int64 + type: integer readyCount: default: 0 format: int32 diff --git a/api/bases/cinder.openstack.org_cinderschedulers.yaml b/api/bases/cinder.openstack.org_cinderschedulers.yaml index a2372c57..351efb7f 100644 --- a/api/bases/cinder.openstack.org_cinderschedulers.yaml +++ b/api/bases/cinder.openstack.org_cinderschedulers.yaml @@ -916,6 +916,9 @@ spec: type: string type: array type: object + observedGeneration: + format: int64 + type: integer readyCount: default: 0 format: int32 diff --git a/api/bases/cinder.openstack.org_cindervolumes.yaml b/api/bases/cinder.openstack.org_cindervolumes.yaml index 7e423dda..505bf642 100644 --- a/api/bases/cinder.openstack.org_cindervolumes.yaml +++ b/api/bases/cinder.openstack.org_cindervolumes.yaml @@ -917,6 +917,9 @@ spec: type: string type: array type: object + observedGeneration: + format: int64 + type: integer readyCount: default: 0 format: int32 diff --git a/api/v1beta1/cinder_types.go b/api/v1beta1/cinder_types.go index a7d018ff..de335de1 100644 --- a/api/v1beta1/cinder_types.go +++ b/api/v1beta1/cinder_types.go @@ -178,7 +178,7 @@ type CinderStatus struct { CinderVolumesReadyCounts map[string]int32 `json:"cinderVolumesReadyCounts,omitempty"` // ObservedGeneration - the most recent generation observed for this service. - // If the observed generation is different than the spec generation, then thei + // If the observed generation is different than the spec generation, then the // controller has not started processing the latest changes, and the status // and its conditions are likely stale. ObservedGeneration int64 `json:"observedGeneration,omitempty"` diff --git a/api/v1beta1/cinderapi_types.go b/api/v1beta1/cinderapi_types.go index 6d403572..7ea1132b 100644 --- a/api/v1beta1/cinderapi_types.go +++ b/api/v1beta1/cinderapi_types.go @@ -106,6 +106,12 @@ type CinderAPIStatus struct { // NetworkAttachments status of the deployment pods NetworkAttachments map[string][]string `json:"networkAttachments,omitempty"` + + // ObservedGeneration - the most recent generation observed for this service. + // If the observed generation is different than the spec generation, then the + // controller has not started processing the latest changes, and the status + // and its conditions are likely stale. + ObservedGeneration int64 `json:"observedGeneration,omitempty"` } //+kubebuilder:object:root=true diff --git a/api/v1beta1/cinderbackup_types.go b/api/v1beta1/cinderbackup_types.go index 5be153aa..a40ffdd2 100644 --- a/api/v1beta1/cinderbackup_types.go +++ b/api/v1beta1/cinderbackup_types.go @@ -88,6 +88,12 @@ type CinderBackupStatus struct { // NetworkAttachments status of the deployment pods NetworkAttachments map[string][]string `json:"networkAttachments,omitempty"` + + // ObservedGeneration - the most recent generation observed for this service. + // If the observed generation is different than the spec generation, then the + // controller has not started processing the latest changes, and the status + // and its conditions are likely stale. + ObservedGeneration int64 `json:"observedGeneration,omitempty"` } //+kubebuilder:object:root=true diff --git a/api/v1beta1/cinderscheduler_types.go b/api/v1beta1/cinderscheduler_types.go index 114fc6a6..845e532a 100644 --- a/api/v1beta1/cinderscheduler_types.go +++ b/api/v1beta1/cinderscheduler_types.go @@ -88,6 +88,12 @@ type CinderSchedulerStatus struct { // NetworkAttachments status of the deployment pods NetworkAttachments map[string][]string `json:"networkAttachments,omitempty"` + + // ObservedGeneration - the most recent generation observed for this service. + // If the observed generation is different than the spec generation, then the + // controller has not started processing the latest changes, and the status + // and its conditions are likely stale. + ObservedGeneration int64 `json:"observedGeneration,omitempty"` } //+kubebuilder:object:root=true diff --git a/api/v1beta1/cindervolume_types.go b/api/v1beta1/cindervolume_types.go index fb61bb86..5b814e92 100644 --- a/api/v1beta1/cindervolume_types.go +++ b/api/v1beta1/cindervolume_types.go @@ -89,6 +89,12 @@ type CinderVolumeStatus struct { // NetworkAttachments status of the deployment pods NetworkAttachments map[string][]string `json:"networkAttachments,omitempty"` + + // ObservedGeneration - the most recent generation observed for this service. + // If the observed generation is different than the spec generation, then the + // controller has not started processing the latest changes, and the status + // and its conditions are likely stale. + ObservedGeneration int64 `json:"observedGeneration,omitempty"` } //+kubebuilder:object:root=true diff --git a/config/crd/bases/cinder.openstack.org_cinderapis.yaml b/config/crd/bases/cinder.openstack.org_cinderapis.yaml index 75c02ec9..d8767d37 100644 --- a/config/crd/bases/cinder.openstack.org_cinderapis.yaml +++ b/config/crd/bases/cinder.openstack.org_cinderapis.yaml @@ -986,6 +986,9 @@ spec: type: string type: array type: object + observedGeneration: + format: int64 + type: integer readyCount: default: 0 format: int32 diff --git a/config/crd/bases/cinder.openstack.org_cinderbackups.yaml b/config/crd/bases/cinder.openstack.org_cinderbackups.yaml index 072f4504..9a15c7c8 100644 --- a/config/crd/bases/cinder.openstack.org_cinderbackups.yaml +++ b/config/crd/bases/cinder.openstack.org_cinderbackups.yaml @@ -916,6 +916,9 @@ spec: type: string type: array type: object + observedGeneration: + format: int64 + type: integer readyCount: default: 0 format: int32 diff --git a/config/crd/bases/cinder.openstack.org_cinderschedulers.yaml b/config/crd/bases/cinder.openstack.org_cinderschedulers.yaml index a2372c57..351efb7f 100644 --- a/config/crd/bases/cinder.openstack.org_cinderschedulers.yaml +++ b/config/crd/bases/cinder.openstack.org_cinderschedulers.yaml @@ -916,6 +916,9 @@ spec: type: string type: array type: object + observedGeneration: + format: int64 + type: integer readyCount: default: 0 format: int32 diff --git a/config/crd/bases/cinder.openstack.org_cindervolumes.yaml b/config/crd/bases/cinder.openstack.org_cindervolumes.yaml index 7e423dda..505bf642 100644 --- a/config/crd/bases/cinder.openstack.org_cindervolumes.yaml +++ b/config/crd/bases/cinder.openstack.org_cindervolumes.yaml @@ -917,6 +917,9 @@ spec: type: string type: array type: object + observedGeneration: + format: int64 + type: integer readyCount: default: 0 format: int32 diff --git a/controllers/cinderapi_controller.go b/controllers/cinderapi_controller.go index 4c1f7b57..0074fed9 100644 --- a/controllers/cinderapi_controller.go +++ b/controllers/cinderapi_controller.go @@ -173,6 +173,8 @@ func (r *CinderAPIReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( condition.UnknownCondition(condition.TLSInputReadyCondition, condition.InitReason, condition.InputReadyInitMessage), ) instance.Status.Conditions.Init(&cl) + // Always mark the Generation as observed early on + instance.Status.ObservedGeneration = instance.Generation // 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 { diff --git a/controllers/cinderbackup_controller.go b/controllers/cinderbackup_controller.go index 1ce09827..2e12746e 100644 --- a/controllers/cinderbackup_controller.go +++ b/controllers/cinderbackup_controller.go @@ -153,6 +153,8 @@ func (r *CinderBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request condition.UnknownCondition(condition.TLSInputReadyCondition, condition.InitReason, condition.InputReadyInitMessage), ) instance.Status.Conditions.Init(&cl) + // Always mark the Generation as observed early on + instance.Status.ObservedGeneration = instance.Generation // 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 { diff --git a/controllers/cinderscheduler_controller.go b/controllers/cinderscheduler_controller.go index 9ec1ddf3..93c9c4aa 100644 --- a/controllers/cinderscheduler_controller.go +++ b/controllers/cinderscheduler_controller.go @@ -153,6 +153,8 @@ func (r *CinderSchedulerReconciler) Reconcile(ctx context.Context, req ctrl.Requ condition.UnknownCondition(condition.TLSInputReadyCondition, condition.InitReason, condition.InputReadyInitMessage), ) instance.Status.Conditions.Init(&cl) + // Always mark the Generation as observed early on + instance.Status.ObservedGeneration = instance.Generation // 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 { diff --git a/controllers/cindervolume_controller.go b/controllers/cindervolume_controller.go index 1739e077..caf4bb45 100644 --- a/controllers/cindervolume_controller.go +++ b/controllers/cindervolume_controller.go @@ -155,6 +155,8 @@ func (r *CinderVolumeReconciler) Reconcile(ctx context.Context, req ctrl.Request condition.UnknownCondition(condition.TLSInputReadyCondition, condition.InitReason, condition.InputReadyInitMessage), ) instance.Status.Conditions.Init(&cl) + // Always mark the Generation as observed early on + instance.Status.ObservedGeneration = instance.Generation // 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 { From 7aba2b7de5cbbfbd665367b5948790aa6143296a Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Tue, 23 Apr 2024 14:32:24 +0200 Subject: [PATCH 4/4] Fix status conditions This patch leverages the ObservedGeneration to fix the status conditions. Top level cinder CRD cannot use the information of the sub CRDs content unless the Generation and ObservedGeneration match, and it will only be ready if it's generation is right and all the subCRDs are ready. For the sub CRDs to be ready they need to: - Have observed and reacted to the latest Generation - Have the right number of replicas - Be deployment ready or have requested 0 replicas and not be deployment ready --- api/v1beta1/cinder_types.go | 3 +- api/v1beta1/cinderapi_types.go | 5 +- api/v1beta1/cinderbackup_types.go | 5 +- api/v1beta1/cinderscheduler_types.go | 5 +- api/v1beta1/cindervolume_types.go | 5 +- controllers/cinder_controller.go | 91 +++++++++++++---------- controllers/cinderapi_controller.go | 25 ++++++- controllers/cinderbackup_controller.go | 25 ++++++- controllers/cinderscheduler_controller.go | 25 ++++++- controllers/cindervolume_controller.go | 25 ++++++- 10 files changed, 158 insertions(+), 56 deletions(-) diff --git a/api/v1beta1/cinder_types.go b/api/v1beta1/cinder_types.go index de335de1..633c8d40 100644 --- a/api/v1beta1/cinder_types.go +++ b/api/v1beta1/cinder_types.go @@ -226,7 +226,8 @@ func init() { // IsReady - returns true if all subresources Ready condition is true func (instance Cinder) IsReady() bool { - return instance.Status.Conditions.IsTrue(CinderAPIReadyCondition) && + return instance.Generation == instance.Status.ObservedGeneration && + instance.Status.Conditions.IsTrue(CinderAPIReadyCondition) && instance.Status.Conditions.IsTrue(CinderBackupReadyCondition) && instance.Status.Conditions.IsTrue(CinderSchedulerReadyCondition) && instance.Status.Conditions.IsTrue(CinderVolumeReadyCondition) diff --git a/api/v1beta1/cinderapi_types.go b/api/v1beta1/cinderapi_types.go index 7ea1132b..df5ab150 100644 --- a/api/v1beta1/cinderapi_types.go +++ b/api/v1beta1/cinderapi_types.go @@ -144,5 +144,8 @@ func init() { // IsReady - returns true if service is ready to serve requests func (instance CinderAPI) IsReady() bool { - return instance.Status.ReadyCount == *instance.Spec.Replicas + return instance.Generation == instance.Status.ObservedGeneration && + instance.Status.ReadyCount == *instance.Spec.Replicas && + (instance.Status.Conditions.IsTrue(condition.DeploymentReadyCondition) || + (instance.Status.Conditions.IsFalse(condition.DeploymentReadyCondition) && *instance.Spec.Replicas == 0)) } diff --git a/api/v1beta1/cinderbackup_types.go b/api/v1beta1/cinderbackup_types.go index a40ffdd2..edcafc94 100644 --- a/api/v1beta1/cinderbackup_types.go +++ b/api/v1beta1/cinderbackup_types.go @@ -126,5 +126,8 @@ func init() { // IsReady - returns true if service is ready to serve requests func (instance CinderBackup) IsReady() bool { - return instance.Status.ReadyCount == *instance.Spec.Replicas + return instance.Generation == instance.Status.ObservedGeneration && + instance.Status.ReadyCount == *instance.Spec.Replicas && + (instance.Status.Conditions.IsTrue(condition.DeploymentReadyCondition) || + (instance.Status.Conditions.IsFalse(condition.DeploymentReadyCondition) && *instance.Spec.Replicas == 0)) } diff --git a/api/v1beta1/cinderscheduler_types.go b/api/v1beta1/cinderscheduler_types.go index 845e532a..616c5d1e 100644 --- a/api/v1beta1/cinderscheduler_types.go +++ b/api/v1beta1/cinderscheduler_types.go @@ -126,5 +126,8 @@ func init() { // IsReady - returns true if service is ready to serve requests func (instance CinderScheduler) IsReady() bool { - return instance.Status.ReadyCount == *instance.Spec.Replicas + return instance.Generation == instance.Status.ObservedGeneration && + instance.Status.ReadyCount == *instance.Spec.Replicas && + (instance.Status.Conditions.IsTrue(condition.DeploymentReadyCondition) || + (instance.Status.Conditions.IsFalse(condition.DeploymentReadyCondition) && *instance.Spec.Replicas == 0)) } diff --git a/api/v1beta1/cindervolume_types.go b/api/v1beta1/cindervolume_types.go index 5b814e92..6960a6ee 100644 --- a/api/v1beta1/cindervolume_types.go +++ b/api/v1beta1/cindervolume_types.go @@ -127,5 +127,8 @@ func init() { // IsReady - returns true if service is ready to serve requests func (instance CinderVolume) IsReady() bool { - return instance.Status.ReadyCount == *instance.Spec.Replicas + return instance.Generation == instance.Status.ObservedGeneration && + instance.Status.ReadyCount == *instance.Spec.Replicas && + (instance.Status.Conditions.IsTrue(condition.DeploymentReadyCondition) || + (instance.Status.Conditions.IsFalse(condition.DeploymentReadyCondition) && *instance.Spec.Replicas == 0)) } diff --git a/controllers/cinder_controller.go b/controllers/cinder_controller.go index 12badab3..cc754da5 100644 --- a/controllers/cinder_controller.go +++ b/controllers/cinder_controller.go @@ -688,15 +688,18 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder Log.Info(fmt.Sprintf("Deployment %s successfully reconciled - operation: %s", instance.Name, string(op))) } - // Mirror CinderAPI status' APIEndpoints and ReadyCount to this parent CR - instance.Status.APIEndpoints = cinderAPI.Status.APIEndpoints - instance.Status.ServiceIDs = cinderAPI.Status.ServiceIDs - instance.Status.CinderAPIReadyCount = cinderAPI.Status.ReadyCount - - // Mirror CinderAPI's condition status - c := cinderAPI.Status.Conditions.Mirror(cinderv1beta1.CinderAPIReadyCondition) - if c != nil { - instance.Status.Conditions.Set(c) + // Mirror values when the data in the StatefulSet is for the current generation + if cinderAPI.Generation == cinderAPI.Status.ObservedGeneration { + // Mirror CinderAPI status' APIEndpoints and ReadyCount to this parent CR + instance.Status.APIEndpoints = cinderAPI.Status.APIEndpoints + instance.Status.ServiceIDs = cinderAPI.Status.ServiceIDs + instance.Status.CinderAPIReadyCount = cinderAPI.Status.ReadyCount + + // Mirror CinderAPI's condition status + c := cinderAPI.Status.Conditions.Mirror(cinderv1beta1.CinderAPIReadyCondition) + if c != nil { + instance.Status.Conditions.Set(c) + } } // deploy cinder-scheduler @@ -714,13 +717,16 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder Log.Info(fmt.Sprintf("Deployment %s successfully reconciled - operation: %s", instance.Name, string(op))) } - // Mirror CinderScheduler status' ReadyCount to this parent CR - instance.Status.CinderSchedulerReadyCount = cinderScheduler.Status.ReadyCount + // Mirror values when the data in the StatefulSet is for the current generation + if cinderScheduler.Generation == cinderScheduler.Status.ObservedGeneration { + // Mirror CinderScheduler status' ReadyCount to this parent CR + instance.Status.CinderSchedulerReadyCount = cinderScheduler.Status.ReadyCount - // Mirror CinderScheduler's condition status - c = cinderScheduler.Status.Conditions.Mirror(cinderv1beta1.CinderSchedulerReadyCondition) - if c != nil { - instance.Status.Conditions.Set(c) + // Mirror CinderScheduler's condition status + c := cinderScheduler.Status.Conditions.Mirror(cinderv1beta1.CinderSchedulerReadyCondition) + if c != nil { + instance.Status.Conditions.Set(c) + } } // deploy cinder-backup, but only if necessary @@ -743,24 +749,25 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder Log.Info(fmt.Sprintf("Deployment %s successfully reconciled - operation: %s", instance.Name, string(op))) } - // Mirror CinderBackup status' ReadyCount to this parent CR - instance.Status.CinderBackupReadyCount = cinderBackup.Status.ReadyCount + // Mirror values when the data in the StatefulSet is for the current generation + if cinderBackup.Generation == cinderBackup.Status.ObservedGeneration { + // Mirror CinderBackup status' ReadyCount to this parent CR + instance.Status.CinderBackupReadyCount = cinderBackup.Status.ReadyCount - // Mirror CinderBackup's condition status - backupCondition = cinderBackup.Status.Conditions.Mirror(cinderv1beta1.CinderBackupReadyCondition) + // Mirror CinderBackup's condition status + backupCondition = cinderBackup.Status.Conditions.Mirror(cinderv1beta1.CinderBackupReadyCondition) + instance.Status.Conditions.Set(backupCondition) + } } else { // Clean up cinder-backup if there are no replicas err = r.backupCleanupDeployment(ctx, instance) if err != nil { + // Should we set the condition to False? return ctrl.Result{}, err } - } + // TODO: Wait for the deployment to actually disappear before setting the condition - if backupCondition != nil { - // If there's a backupCondition then set that as the CinderBackupReadyCondition - instance.Status.Conditions.Set(backupCondition) - } else { // The CinderBackup is ready, even if the service wasn't deployed. // Using "condition.DeploymentReadyMessage" here because that is what gets mirrored // as the message for the other Cinder children when they are successfully-deployed @@ -769,6 +776,7 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder // deploy cinder-volumes var volumeCondition *condition.Condition + waitingGenerationMatch := false for name, volume := range instance.Spec.CinderVolumes { cinderVolume, op, err := r.volumeDeploymentCreateOrUpdate(ctx, instance, name, volume) if err != nil { @@ -784,27 +792,32 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder Log.Info(fmt.Sprintf("Deployment %s successfully reconciled - operation: %s", instance.Name, string(op))) } - // Mirror CinderVolume status' ReadyCount to this parent CR - // TODO: Somehow this status map can be nil here despite being initialized - // in the Reconcile function above - if instance.Status.CinderVolumesReadyCounts == nil { - instance.Status.CinderVolumesReadyCounts = map[string]int32{} - } - instance.Status.CinderVolumesReadyCounts[name] = cinderVolume.Status.ReadyCount - - // If this cinderVolume is not IsReady, mirror the condition to get the latest step it is in. - // Could also check the overall ReadyCondition of the cinderVolume. - if !cinderVolume.IsReady() { - c = cinderVolume.Status.Conditions.Mirror(cinderv1beta1.CinderVolumeReadyCondition) - // Get the condition with higher priority for volumeCondition. - volumeCondition = condition.GetHigherPrioCondition(c, volumeCondition).DeepCopy() + // Mirror values when the data in the StatefulSet is for the current generation + if cinderVolume.Generation != cinderVolume.Status.ObservedGeneration { + waitingGenerationMatch = true + } else { + // Mirror CinderVolume status' ReadyCount to this parent CR + // TODO: Somehow this status map can be nil here despite being initialized + // in the Reconcile function above + if instance.Status.CinderVolumesReadyCounts == nil { + instance.Status.CinderVolumesReadyCounts = map[string]int32{} + } + instance.Status.CinderVolumesReadyCounts[name] = cinderVolume.Status.ReadyCount + + // If this cinderVolume is not IsReady, mirror the condition to get the latest step it is in. + // Could also check the overall ReadyCondition of the cinderVolume. + if !cinderVolume.IsReady() { + c := cinderVolume.Status.Conditions.Mirror(cinderv1beta1.CinderVolumeReadyCondition) + // Get the condition with higher priority for volumeCondition. + volumeCondition = condition.GetHigherPrioCondition(c, volumeCondition).DeepCopy() + } } } if volumeCondition != nil { // If there was a Status=False condition, set that as the CinderVolumeReadyCondition instance.Status.Conditions.Set(volumeCondition) - } else { + } else if !waitingGenerationMatch { // The CinderVolumes are ready. // Using "condition.DeploymentReadyMessage" here because that is what gets mirrored // as the message for the other Cinder children when they are successfully-deployed diff --git a/controllers/cinderapi_controller.go b/controllers/cinderapi_controller.go index 0074fed9..bfab3e79 100644 --- a/controllers/cinderapi_controller.go +++ b/controllers/cinderapi_controller.go @@ -873,6 +873,7 @@ func (r *CinderAPIReconciler) reconcileNormal(ctx context.Context, instance *cin time.Duration(5)*time.Second, ) + var ssData appsv1.StatefulSet ctrlResult, err = ss.CreateOrPatch(ctx, helper) if err != nil { instance.Status.Conditions.Set(condition.FalseCondition( @@ -882,15 +883,32 @@ func (r *CinderAPIReconciler) reconcileNormal(ctx context.Context, instance *cin condition.DeploymentReadyErrorMessage, err.Error())) return ctrlResult, err - } else if (ctrlResult != ctrl.Result{}) { + + } else if (ctrlResult == ctrl.Result{}) { + // Wait until the data in the StatefulSet is for the current generation + ssData = ss.GetStatefulSet() + if ssData.Generation != ssData.Status.ObservedGeneration { + ctrlResult = ctrl.Result{RequeueAfter: time.Duration(10) * time.Second} + err = fmt.Errorf("waiting for Statefulset %s to start reconciling", ssData.Name) + } + } + + if (ctrlResult != ctrl.Result{}) { instance.Status.Conditions.Set(condition.FalseCondition( condition.DeploymentReadyCondition, condition.RequestedReason, condition.SeverityInfo, condition.DeploymentReadyRunningMessage)) - return ctrlResult, nil + // If the deployment is not ready, then neither are the NADs + instance.Status.Conditions.Set(condition.FalseCondition( + condition.NetworkAttachmentsReadyCondition, + condition.RequestedReason, + condition.SeverityInfo, + condition.NetworkAttachmentsReadyInitMessage)) + return ctrlResult, err } - instance.Status.ReadyCount = ss.GetStatefulSet().Status.ReadyReplicas + + instance.Status.ReadyCount = ssData.Status.ReadyReplicas // verify if network attachment matches expectations networkReady := false @@ -956,6 +974,7 @@ func (r *CinderAPIReconciler) reconcileNormal(ctx context.Context, instance *cin if instance.IsReady() { instance.Status.Conditions.MarkTrue(condition.ReadyCondition, condition.ReadyMessage) } + // For non ready we'll let the main defer func handle the status update using the Mirror function return ctrl.Result{}, nil } diff --git a/controllers/cinderbackup_controller.go b/controllers/cinderbackup_controller.go index 2e12746e..abeaa774 100644 --- a/controllers/cinderbackup_controller.go +++ b/controllers/cinderbackup_controller.go @@ -543,6 +543,7 @@ func (r *CinderBackupReconciler) reconcileNormal(ctx context.Context, instance * time.Duration(5)*time.Second, ) + var ssData appsv1.StatefulSet ctrlResult, err = ss.CreateOrPatch(ctx, helper) if err != nil { instance.Status.Conditions.Set(condition.FalseCondition( @@ -552,15 +553,32 @@ func (r *CinderBackupReconciler) reconcileNormal(ctx context.Context, instance * condition.DeploymentReadyErrorMessage, err.Error())) return ctrlResult, err - } else if (ctrlResult != ctrl.Result{}) { + + } else if (ctrlResult == ctrl.Result{}) { + // Wait until the data in the StatefulSet is for the current generation + ssData = ss.GetStatefulSet() + if ssData.Generation != ssData.Status.ObservedGeneration { + ctrlResult = ctrl.Result{RequeueAfter: time.Duration(10) * time.Second} + err = fmt.Errorf("waiting for Statefulset %s to start reconciling", ssData.Name) + } + } + + if (ctrlResult != ctrl.Result{}) { instance.Status.Conditions.Set(condition.FalseCondition( condition.DeploymentReadyCondition, condition.RequestedReason, condition.SeverityInfo, condition.DeploymentReadyRunningMessage)) - return ctrlResult, nil + // If the deployment is not ready, then neither are the NADs + instance.Status.Conditions.Set(condition.FalseCondition( + condition.NetworkAttachmentsReadyCondition, + condition.RequestedReason, + condition.SeverityInfo, + condition.NetworkAttachmentsReadyInitMessage)) + return ctrlResult, err } - instance.Status.ReadyCount = ss.GetStatefulSet().Status.ReadyReplicas + + instance.Status.ReadyCount = ssData.Status.ReadyReplicas // verify if network attachment matches expectations networkReady := false @@ -625,6 +643,7 @@ func (r *CinderBackupReconciler) reconcileNormal(ctx context.Context, instance * if instance.IsReady() { instance.Status.Conditions.MarkTrue(condition.ReadyCondition, condition.ReadyMessage) } + // For non ready we'll let the main defer func handle the status update using the Mirror function return ctrl.Result{}, nil } diff --git a/controllers/cinderscheduler_controller.go b/controllers/cinderscheduler_controller.go index 93c9c4aa..5f43b8d9 100644 --- a/controllers/cinderscheduler_controller.go +++ b/controllers/cinderscheduler_controller.go @@ -542,6 +542,7 @@ func (r *CinderSchedulerReconciler) reconcileNormal(ctx context.Context, instanc time.Duration(5)*time.Second, ) + var ssData appsv1.StatefulSet ctrlResult, err = ss.CreateOrPatch(ctx, helper) if err != nil { instance.Status.Conditions.Set(condition.FalseCondition( @@ -551,15 +552,32 @@ func (r *CinderSchedulerReconciler) reconcileNormal(ctx context.Context, instanc condition.DeploymentReadyErrorMessage, err.Error())) return ctrlResult, err - } else if (ctrlResult != ctrl.Result{}) { + + } else if (ctrlResult == ctrl.Result{}) { + // Wait until the data in the StatefulSet is for the current generation + ssData = ss.GetStatefulSet() + if ssData.Generation != ssData.Status.ObservedGeneration { + ctrlResult = ctrl.Result{RequeueAfter: time.Duration(10) * time.Second} + err = fmt.Errorf("waiting for Statefulset %s to start reconciling", ssData.Name) + } + } + + if (ctrlResult != ctrl.Result{}) { instance.Status.Conditions.Set(condition.FalseCondition( condition.DeploymentReadyCondition, condition.RequestedReason, condition.SeverityInfo, condition.DeploymentReadyRunningMessage)) - return ctrlResult, nil + // If the deployment is not ready, then neither are the NADs + instance.Status.Conditions.Set(condition.FalseCondition( + condition.NetworkAttachmentsReadyCondition, + condition.RequestedReason, + condition.SeverityInfo, + condition.NetworkAttachmentsReadyInitMessage)) + return ctrlResult, err } - instance.Status.ReadyCount = ss.GetStatefulSet().Status.ReadyReplicas + + instance.Status.ReadyCount = ssData.Status.ReadyReplicas // verify if network attachment matches expectations networkReady := false @@ -624,6 +642,7 @@ func (r *CinderSchedulerReconciler) reconcileNormal(ctx context.Context, instanc if instance.IsReady() { instance.Status.Conditions.MarkTrue(condition.ReadyCondition, condition.ReadyMessage) } + // For non ready we'll let the main defer func handle the status update using the Mirror function return ctrl.Result{}, nil } diff --git a/controllers/cindervolume_controller.go b/controllers/cindervolume_controller.go index caf4bb45..b2592d08 100644 --- a/controllers/cindervolume_controller.go +++ b/controllers/cindervolume_controller.go @@ -545,6 +545,7 @@ func (r *CinderVolumeReconciler) reconcileNormal(ctx context.Context, instance * time.Duration(5)*time.Second, ) + var ssData appsv1.StatefulSet ctrlResult, err = ss.CreateOrPatch(ctx, helper) if err != nil { instance.Status.Conditions.Set(condition.FalseCondition( @@ -554,15 +555,32 @@ func (r *CinderVolumeReconciler) reconcileNormal(ctx context.Context, instance * condition.DeploymentReadyErrorMessage, err.Error())) return ctrlResult, err - } else if (ctrlResult != ctrl.Result{}) { + + } else if (ctrlResult == ctrl.Result{}) { + // Wait until the data in the StatefulSet is for the current generation + ssData = ss.GetStatefulSet() + if ssData.Generation != ssData.Status.ObservedGeneration { + ctrlResult = ctrl.Result{RequeueAfter: time.Duration(10) * time.Second} + err = fmt.Errorf("waiting for Statefulset %s to start reconciling", ssData.Name) + } + } + + if (ctrlResult != ctrl.Result{}) { instance.Status.Conditions.Set(condition.FalseCondition( condition.DeploymentReadyCondition, condition.RequestedReason, condition.SeverityInfo, condition.DeploymentReadyRunningMessage)) - return ctrlResult, nil + // If the deployment is not ready, then neither are the NADs + instance.Status.Conditions.Set(condition.FalseCondition( + condition.NetworkAttachmentsReadyCondition, + condition.RequestedReason, + condition.SeverityInfo, + condition.NetworkAttachmentsReadyInitMessage)) + return ctrlResult, err } - instance.Status.ReadyCount = ss.GetStatefulSet().Status.ReadyReplicas + + instance.Status.ReadyCount = ssData.Status.ReadyReplicas // verify if network attachment matches expectations networkReady := false @@ -627,6 +645,7 @@ func (r *CinderVolumeReconciler) reconcileNormal(ctx context.Context, instance * if instance.IsReady() { instance.Status.Conditions.MarkTrue(condition.ReadyCondition, condition.ReadyMessage) } + // For non ready we'll let the main defer func handle the status update using the Mirror function return ctrl.Result{}, nil }