From eb6ad21a64458c4455240f8c73ceb233331dd630 Mon Sep 17 00:00:00 2001 From: Francesco Pantano Date: Tue, 9 Apr 2024 10:15:26 +0200 Subject: [PATCH] Add ObservedGeneration to the sub Resources This patch does a few things: 1. it adds observedGeneration to the sub custom resources 2. it proposes to bump the observedGeneration at the beginning of the reconciliation loop (knative/serving#4937) 3. it checks, at the top level, if the ObservedGeneration matches with the metadata.generation assigned to the subCR(s), and if it's the last version it mirrors the Conditions 4. before marking the DeploymentReadyCondition as True, the ObservedGeneration is compared with the Generation of the Deployment/DaemonSets Signed-off-by: Francesco Pantano --- ...enstack.org_octaviaamphoracontrollers.yaml | 8 + .../octavia.openstack.org_octaviaapis.yaml | 8 + api/v1beta1/amphoracontroller_types.go | 6 + api/v1beta1/octaviaapi_types.go | 6 + ...enstack.org_octaviaamphoracontrollers.yaml | 8 + .../octavia.openstack.org_octaviaapis.yaml | 8 + controllers/amphoracontroller_controller.go | 63 +++---- controllers/octavia_controller.go | 162 +++++++++++++----- controllers/octaviaapi_controller.go | 67 ++++---- 9 files changed, 233 insertions(+), 103 deletions(-) diff --git a/api/bases/octavia.openstack.org_octaviaamphoracontrollers.yaml b/api/bases/octavia.openstack.org_octaviaamphoracontrollers.yaml index 403b8ecd..b210f58d 100644 --- a/api/bases/octavia.openstack.org_octaviaamphoracontrollers.yaml +++ b/api/bases/octavia.openstack.org_octaviaamphoracontrollers.yaml @@ -304,6 +304,14 @@ spec: type: array description: NetworkAttachment status of the deployment pods 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 readyCount: description: ReadyCount of Octavia Amphora Controllers format: int32 diff --git a/api/bases/octavia.openstack.org_octaviaapis.yaml b/api/bases/octavia.openstack.org_octaviaapis.yaml index 2743a14b..8fee8a5a 100644 --- a/api/bases/octavia.openstack.org_octaviaapis.yaml +++ b/api/bases/octavia.openstack.org_octaviaapis.yaml @@ -459,6 +459,14 @@ spec: type: array description: NetworkAttachment status of the deployment pods 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 readyCount: description: ReadyCount of octavia API instances format: int32 diff --git a/api/v1beta1/amphoracontroller_types.go b/api/v1beta1/amphoracontroller_types.go index 367948b5..af8c8b8a 100644 --- a/api/v1beta1/amphoracontroller_types.go +++ b/api/v1beta1/amphoracontroller_types.go @@ -161,6 +161,12 @@ type OctaviaAmphoraControllerStatus struct { // NetworkAttachment 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 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/api/v1beta1/octaviaapi_types.go b/api/v1beta1/octaviaapi_types.go index c0ef3879..b5a5520e 100644 --- a/api/v1beta1/octaviaapi_types.go +++ b/api/v1beta1/octaviaapi_types.go @@ -163,6 +163,12 @@ type OctaviaAPIStatus struct { // NetworkAttachment 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 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/octavia.openstack.org_octaviaamphoracontrollers.yaml b/config/crd/bases/octavia.openstack.org_octaviaamphoracontrollers.yaml index 403b8ecd..b210f58d 100644 --- a/config/crd/bases/octavia.openstack.org_octaviaamphoracontrollers.yaml +++ b/config/crd/bases/octavia.openstack.org_octaviaamphoracontrollers.yaml @@ -304,6 +304,14 @@ spec: type: array description: NetworkAttachment status of the deployment pods 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 readyCount: description: ReadyCount of Octavia Amphora Controllers format: int32 diff --git a/config/crd/bases/octavia.openstack.org_octaviaapis.yaml b/config/crd/bases/octavia.openstack.org_octaviaapis.yaml index 2743a14b..8fee8a5a 100644 --- a/config/crd/bases/octavia.openstack.org_octaviaapis.yaml +++ b/config/crd/bases/octavia.openstack.org_octaviaapis.yaml @@ -459,6 +459,14 @@ spec: type: array description: NetworkAttachment status of the deployment pods 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 readyCount: description: ReadyCount of octavia API instances format: int32 diff --git a/controllers/amphoracontroller_controller.go b/controllers/amphoracontroller_controller.go index 2fe402fc..f1f2d10f 100644 --- a/controllers/amphoracontroller_controller.go +++ b/controllers/amphoracontroller_controller.go @@ -162,6 +162,7 @@ func (r *OctaviaAmphoraControllerReconciler) Reconcile(ctx context.Context, req ) instance.Status.Conditions.Init(&cl) + instance.Status.ObservedGeneration = instance.Generation if isNewInstance { if err := r.Status().Update(ctx, instance); err != nil { @@ -463,40 +464,42 @@ func (r *OctaviaAmphoraControllerReconciler) reconcileNormal(ctx context.Context return ctrlResult, nil } - // verify if network attachment matches expectations - networkReady, networkAttachmentStatus, err := nad.VerifyNetworkStatusFromAnnotation( - ctx, - helper, - instance.Spec.NetworkAttachments, - serviceLabels, - instance.Status.ReadyCount, - ) - if err != nil { - return ctrl.Result{}, err - } + if dset.GetDaemonSet().Generation == dset.GetDaemonSet().Status.ObservedGeneration { + instance.Status.DesiredNumberScheduled = dset.GetDaemonSet().Status.DesiredNumberScheduled + // TODO(gthiemonge) change for NumberReady? + instance.Status.ReadyCount = dset.GetDaemonSet().Status.NumberReady - instance.Status.NetworkAttachments = networkAttachmentStatus - if networkReady { - instance.Status.Conditions.MarkTrue(condition.NetworkAttachmentsReadyCondition, condition.NetworkAttachmentsReadyMessage) - } else { - err := fmt.Errorf("not all pods have interfaces with ips as configured in NetworkAttachments: %s", instance.Spec.NetworkAttachments) - instance.Status.Conditions.Set(condition.FalseCondition( - condition.NetworkAttachmentsReadyCondition, - condition.ErrorReason, - condition.SeverityWarning, - condition.NetworkAttachmentsReadyErrorMessage, - err.Error())) + // verify if network attachment matches expectations + networkReady, networkAttachmentStatus, err := nad.VerifyNetworkStatusFromAnnotation( + ctx, + helper, + instance.Spec.NetworkAttachments, + serviceLabels, + instance.Status.ReadyCount, + ) + if err != nil { + return ctrl.Result{}, err + } - return ctrl.Result{RequeueAfter: time.Duration(1) * time.Second}, nil - } + instance.Status.NetworkAttachments = networkAttachmentStatus + if networkReady { + instance.Status.Conditions.MarkTrue(condition.NetworkAttachmentsReadyCondition, condition.NetworkAttachmentsReadyMessage) + } else { + err := fmt.Errorf("not all pods have interfaces with ips as configured in NetworkAttachments: %s", instance.Spec.NetworkAttachments) + instance.Status.Conditions.Set(condition.FalseCondition( + condition.NetworkAttachmentsReadyCondition, + condition.ErrorReason, + condition.SeverityWarning, + condition.NetworkAttachmentsReadyErrorMessage, + err.Error())) - instance.Status.DesiredNumberScheduled = dset.GetDaemonSet().Status.DesiredNumberScheduled - // TODO(gthiemonge) change for NumberReady? - instance.Status.ReadyCount = dset.GetDaemonSet().Status.NumberReady - if instance.Status.ReadyCount == instance.Status.DesiredNumberScheduled { - instance.Status.Conditions.MarkTrue(condition.DeploymentReadyCondition, condition.DeploymentReadyMessage) - } + return ctrl.Result{RequeueAfter: time.Duration(1) * time.Second}, nil + } + if instance.Status.ReadyCount == instance.Status.DesiredNumberScheduled { + instance.Status.Conditions.MarkTrue(condition.DeploymentReadyCondition, condition.DeploymentReadyMessage) + } + } // create DaemonSet - end // We reached the end of the Reconcile, update the Ready condition based on diff --git a/controllers/octavia_controller.go b/controllers/octavia_controller.go index d2b44bf1..83ade46a 100644 --- a/controllers/octavia_controller.go +++ b/controllers/octavia_controller.go @@ -196,6 +196,7 @@ func (r *OctaviaReconciler) Reconcile(ctx context.Context, req ctrl.Request) (re ) instance.Status.Conditions.Init(&cl) + 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 { @@ -609,19 +610,34 @@ func (r *OctaviaReconciler) reconcileNormal(ctx context.Context, instance *octav err.Error())) return ctrl.Result{}, err } - if op != controllerutil.OperationResultNone { - Log.Info(fmt.Sprintf("Deployment %s successfully reconciled - operation: %s", instance.Name, string(op))) + // Check the underlying OctaviaAPI condition according to the + // ObservedGeneration + apiObsGen, err := r.checkOctaviaAPIGeneration(instance) + if err != nil { + instance.Status.Conditions.Set(condition.FalseCondition( + octaviav1.OctaviaAPIReadyCondition, + condition.ErrorReason, + condition.SeverityWarning, + octaviav1.OctaviaAPIReadyErrorMessage, + err.Error())) + return ctrlResult, nil } - - // Mirror OctaviaAPI status' ReadyCount to this parent CR - // TODO(beagles): We need to have a way to aggregate conditions from the other services into this - // - instance.Status.OctaviaAPIReadyCount = octaviaAPI.Status.ReadyCount - conditionStatus := octaviaAPI.Status.Conditions.Mirror(octaviav1.OctaviaAPIReadyCondition) - if conditionStatus != nil { - instance.Status.Conditions.Set(conditionStatus) + if !apiObsGen { + instance.Status.Conditions.Set(condition.UnknownCondition( + octaviav1.OctaviaAPIReadyCondition, + condition.InitReason, + octaviav1.OctaviaAPIReadyInitMessage, + )) } else { - instance.Status.Conditions.MarkTrue(octaviav1.OctaviaAPIReadyCondition, condition.DeploymentReadyMessage) + // Mirror OctaviaAPI status' ReadyCount to this parent CR + instance.Status.OctaviaAPIReadyCount = octaviaAPI.Status.ReadyCount + conditionStatus := octaviaAPI.Status.Conditions.Mirror(octaviav1.OctaviaAPIReadyCondition) + if conditionStatus != nil { + instance.Status.Conditions.Set(conditionStatus) + } + } + if op != controllerutil.OperationResultNone && apiObsGen { + Log.Info(fmt.Sprintf("Deployment %s successfully reconciled - operation: %s", instance.Name, string(op))) } // ------------------------------------------------------------------------------------------------------------ @@ -654,17 +670,34 @@ func (r *OctaviaReconciler) reconcileNormal(ctx context.Context, instance *octav err.Error())) return ctrl.Result{}, err } - - if op != controllerutil.OperationResultNone { - Log.Info(fmt.Sprintf("Deployment of OctaviaHealthManager for %s successfully reconciled - operation: %s", instance.Name, string(op))) + // Even if we trigger three deployments, the Amphora subCR is only one, no + // need to call this functions three times in the same reconciliation loop + ampObsGen, err := r.checkAmphoraGeneration(instance) + if err != nil { + instance.Status.Conditions.Set(condition.FalseCondition( + amphoraControllerReadyCondition(octaviav1.HealthManager), + condition.ErrorReason, + condition.SeverityWarning, + amphoraControllerErrorMessage(octaviav1.HealthManager), + err.Error())) + return ctrlResult, nil } - - instance.Status.OctaviaHealthManagerReadyCount = octaviaHealthManager.Status.ReadyCount - conditionStatus = octaviaHealthManager.Status.Conditions.Mirror(amphoraControllerReadyCondition(octaviav1.HealthManager)) - if conditionStatus != nil { - instance.Status.Conditions.Set(conditionStatus) + if !ampObsGen { + instance.Status.Conditions.Set(condition.UnknownCondition( + amphoraControllerReadyCondition(octaviav1.HealthManager), + condition.InitReason, + amphoraControllerErrorMessage(octaviav1.HealthManager), + )) } else { - instance.Status.Conditions.MarkTrue(amphoraControllerReadyCondition(octaviav1.HealthManager), condition.DeploymentReadyMessage) + instance.Status.OctaviaHealthManagerReadyCount = octaviaHealthManager.Status.ReadyCount + conditionStatus := octaviaHealthManager.Status.Conditions.Mirror(amphoraControllerReadyCondition(octaviav1.HealthManager)) + if conditionStatus != nil { + instance.Status.Conditions.Set(conditionStatus) + } + } + + if op != controllerutil.OperationResultNone && ampObsGen { + Log.Info(fmt.Sprintf("Deployment of OctaviaHealthManager for %s successfully reconciled - operation: %s", instance.Name, string(op))) } // @@ -687,17 +720,22 @@ func (r *OctaviaReconciler) reconcileNormal(ctx context.Context, instance *octav err.Error())) return ctrl.Result{}, err } - - if op != controllerutil.OperationResultNone { - Log.Info(fmt.Sprintf("Deployment of OctaviaHousekeeping for %s successfully reconciled - operation: %s", instance.Name, string(op))) + if !ampObsGen { + instance.Status.Conditions.Set(condition.UnknownCondition( + amphoraControllerReadyCondition(octaviav1.Housekeeping), + condition.InitReason, + amphoraControllerErrorMessage(octaviav1.Housekeeping), + )) + } else { + instance.Status.OctaviaHousekeepingReadyCount = octaviaHousekeeping.Status.ReadyCount + conditionStatus := octaviaHousekeeping.Status.Conditions.Mirror(amphoraControllerReadyCondition(octaviav1.Housekeeping)) + if conditionStatus != nil { + instance.Status.Conditions.Set(conditionStatus) + } } - instance.Status.OctaviaHousekeepingReadyCount = octaviaHousekeeping.Status.ReadyCount - conditionStatus = octaviaHousekeeping.Status.Conditions.Mirror(amphoraControllerReadyCondition(octaviav1.Housekeeping)) - if conditionStatus != nil { - instance.Status.Conditions.Set(conditionStatus) - } else { - instance.Status.Conditions.MarkTrue(amphoraControllerReadyCondition(octaviav1.Housekeeping), condition.DeploymentReadyMessage) + if op != controllerutil.OperationResultNone && ampObsGen { + Log.Info(fmt.Sprintf("Deployment of OctaviaHousekeeping for %s successfully reconciled - operation: %s", instance.Name, string(op))) } octaviaWorker, op, err := r.amphoraControllerDaemonSetCreateOrUpdate(instance, networkInfo, @@ -711,17 +749,21 @@ func (r *OctaviaReconciler) reconcileNormal(ctx context.Context, instance *octav err.Error())) return ctrl.Result{}, err } - - if op != controllerutil.OperationResultNone { - Log.Info(fmt.Sprintf("Deployment of OctaviaWorker for %s successfully reconciled - operation: %s", instance.Name, string(op))) - } - - instance.Status.OctaviaWorkerReadyCount = octaviaWorker.Status.ReadyCount - conditionStatus = octaviaWorker.Status.Conditions.Mirror(amphoraControllerReadyCondition(octaviav1.Worker)) - if conditionStatus != nil { - instance.Status.Conditions.Set(conditionStatus) + if !ampObsGen { + instance.Status.Conditions.Set(condition.UnknownCondition( + amphoraControllerReadyCondition(octaviav1.Worker), + condition.InitReason, + amphoraControllerErrorMessage(octaviav1.Worker), + )) } else { - instance.Status.Conditions.MarkTrue(amphoraControllerReadyCondition(octaviav1.Worker), condition.DeploymentReadyMessage) + instance.Status.OctaviaWorkerReadyCount = octaviaWorker.Status.ReadyCount + conditionStatus := octaviaWorker.Status.Conditions.Mirror(amphoraControllerReadyCondition(octaviav1.Worker)) + if conditionStatus != nil { + instance.Status.Conditions.Set(conditionStatus) + } + } + if op != controllerutil.OperationResultNone && ampObsGen { + Log.Info(fmt.Sprintf("Deployment of OctaviaWorker for %s successfully reconciled - operation: %s", instance.Name, string(op))) } // remove finalizers from unused MariaDBAccount records @@ -751,8 +793,6 @@ func (r *OctaviaReconciler) reconcileNormal(ctx context.Context, instance *octav // create Deployment - end - // Update the lastObserved generation before evaluating conditions - instance.Status.ObservedGeneration = instance.Generation // We reached the end of the Reconcile, update the Ready condition based on // the sub conditions if instance.Status.Conditions.AllSubConditionIsTrue() { @@ -1355,3 +1395,43 @@ func amphoraControllerErrorMessage(role string) string { } return condMap[role] } + +// checkOctaviaAPIGeneration - +func (r *OctaviaReconciler) checkOctaviaAPIGeneration( + instance *octaviav1.Octavia, +) (bool, error) { + api := &octaviav1.OctaviaAPIList{} + listOpts := []client.ListOption{ + client.InNamespace(instance.Namespace), + } + if err := r.Client.List(context.Background(), api, listOpts...); err != nil { + r.Log.Error(err, "Unable to retrieve OctaviaAPI %w") + return false, err + } + for _, item := range api.Items { + if item.Generation != item.Status.ObservedGeneration { + return false, nil + } + } + return true, nil +} + +// checkAmphoraGeneration - +func (r *OctaviaReconciler) checkAmphoraGeneration( + instance *octaviav1.Octavia, +) (bool, error) { + amph := &octaviav1.OctaviaAmphoraControllerList{} + listOpts := []client.ListOption{ + client.InNamespace(instance.Namespace), + } + if err := r.Client.List(context.Background(), amph, listOpts...); err != nil { + r.Log.Error(err, "Unable to retrieve OctaviaAPI %w") + return false, err + } + for _, item := range amph.Items { + if item.Generation != item.Status.ObservedGeneration { + return false, nil + } + } + return true, nil +} diff --git a/controllers/octaviaapi_controller.go b/controllers/octaviaapi_controller.go index 14b114a0..52ac1670 100644 --- a/controllers/octaviaapi_controller.go +++ b/controllers/octaviaapi_controller.go @@ -175,6 +175,7 @@ func (r *OctaviaAPIReconciler) Reconcile(ctx context.Context, req ctrl.Request) ) instance.Status.Conditions.Init(&cl) + 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 { @@ -778,41 +779,43 @@ func (r *OctaviaAPIReconciler) reconcileNormal(ctx context.Context, instance *oc return ctrlResult, nil } - // verify if network attachment matches expectations - networkReady := false - networkAttachmentStatus := map[string][]string{} - if *instance.Spec.Replicas > 0 { - networkReady, networkAttachmentStatus, err = nad.VerifyNetworkStatusFromAnnotation( - ctx, - helper, - instance.Spec.NetworkAttachments, - serviceLabels, - instance.Status.ReadyCount, - ) - if err != nil { - return ctrl.Result{}, err + if depl.GetDeployment().Generation == depl.GetDeployment().Status.ObservedGeneration { + instance.Status.ReadyCount = depl.GetDeployment().Status.ReadyReplicas + // verify if network attachment matches expectations + networkReady := false + networkAttachmentStatus := map[string][]string{} + if *instance.Spec.Replicas > 0 { + networkReady, networkAttachmentStatus, err = nad.VerifyNetworkStatusFromAnnotation( + ctx, + helper, + instance.Spec.NetworkAttachments, + serviceLabels, + instance.Status.ReadyCount, + ) + if err != nil { + return ctrl.Result{}, err + } + } else { + networkReady = true } - } else { - networkReady = true - } - instance.Status.NetworkAttachments = networkAttachmentStatus - if networkReady { - instance.Status.Conditions.MarkTrue(condition.NetworkAttachmentsReadyCondition, condition.NetworkAttachmentsReadyMessage) - } else { - err := fmt.Errorf("not all pods have interfaces with ips as configured in NetworkAttachments: %s", instance.Spec.NetworkAttachments) - instance.Status.Conditions.Set(condition.FalseCondition( - condition.NetworkAttachmentsReadyCondition, - condition.ErrorReason, - condition.SeverityWarning, - condition.NetworkAttachmentsReadyErrorMessage, - err.Error())) + instance.Status.NetworkAttachments = networkAttachmentStatus + if networkReady { + instance.Status.Conditions.MarkTrue(condition.NetworkAttachmentsReadyCondition, condition.NetworkAttachmentsReadyMessage) + } else { + err := fmt.Errorf("not all pods have interfaces with ips as configured in NetworkAttachments: %s", instance.Spec.NetworkAttachments) + instance.Status.Conditions.Set(condition.FalseCondition( + condition.NetworkAttachmentsReadyCondition, + condition.ErrorReason, + condition.SeverityWarning, + condition.NetworkAttachmentsReadyErrorMessage, + err.Error())) - return ctrl.Result{}, err - } - instance.Status.ReadyCount = depl.GetDeployment().Status.ReadyReplicas - if instance.Status.ReadyCount > 0 { - instance.Status.Conditions.MarkTrue(condition.DeploymentReadyCondition, condition.DeploymentReadyMessage) + return ctrl.Result{}, err + } + if instance.Status.ReadyCount == *instance.Spec.Replicas { + instance.Status.Conditions.MarkTrue(condition.DeploymentReadyCondition, condition.DeploymentReadyMessage) + } } // create Deployment - end