From 0688f63e3f157961853253c1e5c48ebe4332fe6c Mon Sep 17 00:00:00 2001 From: Chris Fry Date: Thu, 23 Feb 2023 17:46:02 +0000 Subject: [PATCH 1/4] rollouts: add status conditions to remote root sync --- .../controllers/remoterootsync_controller.go | 90 ++++++++++++++----- 1 file changed, 69 insertions(+), 21 deletions(-) diff --git a/rollouts/controllers/remoterootsync_controller.go b/rollouts/controllers/remoterootsync_controller.go index eb90a84941..14311db676 100644 --- a/rollouts/controllers/remoterootsync_controller.go +++ b/rollouts/controllers/remoterootsync_controller.go @@ -63,7 +63,11 @@ var ( ) const ( - externalSyncCreatedConditionType = "ExternalSyncCreated" + conditionReady = "Ready" + conditionReconciling = "Reconciling" + conditionStalled = "Stalled" + + reasonSyncNotCreated = "SyncNotCreated" ) // RemoteRootSyncReconciler reconciles a RemoteRootSync object @@ -121,10 +125,16 @@ func (r *RemoteRootSyncReconciler) Reconcile(ctx context.Context, req ctrl.Reque // The object is being deleted if controllerutil.ContainsFinalizer(&remoterootsync, myFinalizerName) { // our finalizer is present, so lets handle any external dependency - if meta.IsStatusConditionTrue(remoterootsync.Status.Conditions, externalSyncCreatedConditionType) { + if r.isExternalSyncCreated(&remoterootsync) { // Delete the external sync resource err := r.deleteExternalResources(ctx, &remoterootsync) if err != nil && !apierrors.IsNotFound(err) { + statusError := r.updateStatus(ctx, &remoterootsync, "", err) + + if statusError != nil { + logger.Error(statusError, "Failed to update status") + } + // if fail to delete the external dependency here, return with error // so that it can be retried return ctrl.Result{}, fmt.Errorf("have problem to delete external resource: %w", err) @@ -145,42 +155,70 @@ func (r *RemoteRootSyncReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, nil } - clusterRef := &remoterootsync.Spec.ClusterRef - dynCl, err := r.getDynamicClientForCluster(ctx, clusterRef) - if err != nil { - return ctrl.Result{}, err - } + syncStatus, syncError := r.syncExternalSync(ctx, &remoterootsync) - if err := r.patchRootSync(ctx, dynCl, req.Name, &remoterootsync); err != nil { + if err := r.updateStatus(ctx, &remoterootsync, syncStatus, syncError); err != nil { + logger.Error(err, "Failed to update status") return ctrl.Result{}, err } - r.setupWatches(ctx, remoterootsync.Name, remoterootsync.Namespace, remoterootsync.Spec.ClusterRef) + return ctrl.Result{}, syncError +} + +func (r *RemoteRootSyncReconciler) syncExternalSync(ctx context.Context, rrs *gitopsv1alpha1.RemoteRootSync) (string, error) { + syncName := rrs.Name + clusterRef := &rrs.Spec.ClusterRef - syncStatus, err := checkSyncStatus(ctx, dynCl, req.Name) + dynCl, err := r.getDynamicClientForCluster(ctx, clusterRef) if err != nil { - return ctrl.Result{}, err + return "", fmt.Errorf("failed to create client: %w", err) } - if err := r.updateStatus(ctx, &remoterootsync, syncStatus); err != nil { - logger.Error(err, "Failed to update status") - return ctrl.Result{}, err + if err := r.patchRootSync(ctx, dynCl, syncName, rrs); err != nil { + return "", fmt.Errorf("failed to create/update sync: %w", err) + } + + r.setupWatches(ctx, rrs.Name, rrs.Namespace, rrs.Spec.ClusterRef) + + syncStatus, err := checkSyncStatus(ctx, dynCl, syncName) + if err != nil { + return "", fmt.Errorf("faild to check status: %w", err) } - return ctrl.Result{}, nil + return syncStatus, nil } -func (r *RemoteRootSyncReconciler) updateStatus(ctx context.Context, rrs *gitopsv1alpha1.RemoteRootSync, syncStatus string) error { +func (r *RemoteRootSyncReconciler) updateStatus(ctx context.Context, rrs *gitopsv1alpha1.RemoteRootSync, syncStatus string, syncError error) error { logger := klog.FromContext(ctx) - // Don't update if there are no changes. - rrsPrior := rrs.DeepCopy() + conditions := &rrs.Status.Conditions - rrs.Status.SyncStatus = syncStatus - rrs.Status.ObservedGeneration = rrs.Generation + if syncError == nil { + rrs.Status.SyncStatus = syncStatus - meta.SetStatusCondition(&rrs.Status.Conditions, metav1.Condition{Type: externalSyncCreatedConditionType, Status: metav1.ConditionTrue, Reason: "SyncCreated"}) + meta.SetStatusCondition(conditions, metav1.Condition{Type: conditionReady, Status: metav1.ConditionTrue, Reason: "Ready"}) + meta.RemoveStatusCondition(conditions, conditionReconciling) + meta.RemoveStatusCondition(conditions, conditionStalled) + } else { + readyReason := "PendingReconcilation" + readyStatus := metav1.ConditionUnknown + + rrs.Status.SyncStatus = "Unknown" + + if r.isExternalSyncCreated(rrs) { + } else { + rrs.Status.SyncStatus = "" + readyReason = reasonSyncNotCreated + readyStatus = metav1.ConditionFalse + } + + meta.SetStatusCondition(conditions, metav1.Condition{Type: conditionReady, Status: readyStatus, Reason: readyReason}) + meta.SetStatusCondition(conditions, metav1.Condition{Type: conditionReconciling, Status: metav1.ConditionTrue, Reason: "Reconciling"}) + meta.SetStatusCondition(conditions, metav1.Condition{Type: conditionStalled, Status: metav1.ConditionTrue, Reason: "Error", Message: syncError.Error()}) + } + + rrs.Status.ObservedGeneration = rrs.Generation if reflect.DeepEqual(rrs.Status, rrsPrior.Status) { return nil @@ -333,6 +371,16 @@ func (r *RemoteRootSyncReconciler) getDynamicClientForCluster(ctx context.Contex return dynamicClient, nil } +func (r *RemoteRootSyncReconciler) isExternalSyncCreated(rrs *gitopsv1alpha1.RemoteRootSync) bool { + readyCondition := meta.FindStatusCondition(rrs.Status.Conditions, conditionReady) + + if readyCondition == nil || (readyCondition.Status != metav1.ConditionTrue && readyCondition.Reason == "SyncNotCreated") { + return false + } + + return true +} + // SetupWithManager sets up the controller with the Manager. func (r *RemoteRootSyncReconciler) SetupWithManager(mgr ctrl.Manager) error { r.channel = make(chan event.GenericEvent, 10) From 2f20dbaa3ce452abefa11c4f65e82ddd716ae6a8 Mon Sep 17 00:00:00 2001 From: Chris Fry Date: Fri, 24 Feb 2023 19:00:24 +0000 Subject: [PATCH 2/4] use constants for condition reasons --- .../controllers/remoterootsync_controller.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/rollouts/controllers/remoterootsync_controller.go b/rollouts/controllers/remoterootsync_controller.go index 14311db676..f9e289bafc 100644 --- a/rollouts/controllers/remoterootsync_controller.go +++ b/rollouts/controllers/remoterootsync_controller.go @@ -67,7 +67,11 @@ const ( conditionReconciling = "Reconciling" conditionStalled = "Stalled" - reasonSyncNotCreated = "SyncNotCreated" + reasonSyncNotCreated = "SyncNotCreated" + reasonPendingReconcilation = "PendingReconcilation" + reasonReady = "Ready" + reasonReconciling = "Reconciling" + reasonError = "Error" ) // RemoteRootSyncReconciler reconciles a RemoteRootSync object @@ -197,11 +201,11 @@ func (r *RemoteRootSyncReconciler) updateStatus(ctx context.Context, rrs *gitops if syncError == nil { rrs.Status.SyncStatus = syncStatus - meta.SetStatusCondition(conditions, metav1.Condition{Type: conditionReady, Status: metav1.ConditionTrue, Reason: "Ready"}) + meta.SetStatusCondition(conditions, metav1.Condition{Type: conditionReady, Status: metav1.ConditionTrue, Reason: reasonReady}) meta.RemoveStatusCondition(conditions, conditionReconciling) meta.RemoveStatusCondition(conditions, conditionStalled) } else { - readyReason := "PendingReconcilation" + readyReason := reasonPendingReconcilation readyStatus := metav1.ConditionUnknown rrs.Status.SyncStatus = "Unknown" @@ -214,8 +218,8 @@ func (r *RemoteRootSyncReconciler) updateStatus(ctx context.Context, rrs *gitops } meta.SetStatusCondition(conditions, metav1.Condition{Type: conditionReady, Status: readyStatus, Reason: readyReason}) - meta.SetStatusCondition(conditions, metav1.Condition{Type: conditionReconciling, Status: metav1.ConditionTrue, Reason: "Reconciling"}) - meta.SetStatusCondition(conditions, metav1.Condition{Type: conditionStalled, Status: metav1.ConditionTrue, Reason: "Error", Message: syncError.Error()}) + meta.SetStatusCondition(conditions, metav1.Condition{Type: conditionReconciling, Status: metav1.ConditionTrue, Reason: reasonReconciling}) + meta.SetStatusCondition(conditions, metav1.Condition{Type: conditionStalled, Status: metav1.ConditionTrue, Reason: reasonError, Message: syncError.Error()}) } rrs.Status.ObservedGeneration = rrs.Generation @@ -374,7 +378,7 @@ func (r *RemoteRootSyncReconciler) getDynamicClientForCluster(ctx context.Contex func (r *RemoteRootSyncReconciler) isExternalSyncCreated(rrs *gitopsv1alpha1.RemoteRootSync) bool { readyCondition := meta.FindStatusCondition(rrs.Status.Conditions, conditionReady) - if readyCondition == nil || (readyCondition.Status != metav1.ConditionTrue && readyCondition.Reason == "SyncNotCreated") { + if readyCondition == nil || (readyCondition.Status != metav1.ConditionTrue && readyCondition.Reason == reasonSyncNotCreated) { return false } From a0038b9bc19970305cf678147d5554bcca469caf Mon Sep 17 00:00:00 2001 From: Chris Fry Date: Fri, 24 Feb 2023 23:00:26 +0000 Subject: [PATCH 3/4] remove redundant ready condition --- .../controllers/remoterootsync_controller.go | 26 +++++++------------ 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/rollouts/controllers/remoterootsync_controller.go b/rollouts/controllers/remoterootsync_controller.go index f9e289bafc..32358c6295 100644 --- a/rollouts/controllers/remoterootsync_controller.go +++ b/rollouts/controllers/remoterootsync_controller.go @@ -63,15 +63,12 @@ var ( ) const ( - conditionReady = "Ready" conditionReconciling = "Reconciling" conditionStalled = "Stalled" - reasonSyncNotCreated = "SyncNotCreated" - reasonPendingReconcilation = "PendingReconcilation" - reasonReady = "Ready" - reasonReconciling = "Reconciling" - reasonError = "Error" + reasonCreateSync = "CreateSync" + reasonUpdateSync = "UpdateSync" + reasonError = "Error" ) // RemoteRootSyncReconciler reconciles a RemoteRootSync object @@ -201,24 +198,19 @@ func (r *RemoteRootSyncReconciler) updateStatus(ctx context.Context, rrs *gitops if syncError == nil { rrs.Status.SyncStatus = syncStatus - meta.SetStatusCondition(conditions, metav1.Condition{Type: conditionReady, Status: metav1.ConditionTrue, Reason: reasonReady}) meta.RemoveStatusCondition(conditions, conditionReconciling) meta.RemoveStatusCondition(conditions, conditionStalled) } else { - readyReason := reasonPendingReconcilation - readyStatus := metav1.ConditionUnknown + reconcileReason := reasonUpdateSync rrs.Status.SyncStatus = "Unknown" - if r.isExternalSyncCreated(rrs) { - } else { + if !r.isExternalSyncCreated(rrs) { rrs.Status.SyncStatus = "" - readyReason = reasonSyncNotCreated - readyStatus = metav1.ConditionFalse + reconcileReason = reasonCreateSync } - meta.SetStatusCondition(conditions, metav1.Condition{Type: conditionReady, Status: readyStatus, Reason: readyReason}) - meta.SetStatusCondition(conditions, metav1.Condition{Type: conditionReconciling, Status: metav1.ConditionTrue, Reason: reasonReconciling}) + meta.SetStatusCondition(conditions, metav1.Condition{Type: conditionReconciling, Status: metav1.ConditionTrue, Reason: reconcileReason}) meta.SetStatusCondition(conditions, metav1.Condition{Type: conditionStalled, Status: metav1.ConditionTrue, Reason: reasonError, Message: syncError.Error()}) } @@ -376,9 +368,9 @@ func (r *RemoteRootSyncReconciler) getDynamicClientForCluster(ctx context.Contex } func (r *RemoteRootSyncReconciler) isExternalSyncCreated(rrs *gitopsv1alpha1.RemoteRootSync) bool { - readyCondition := meta.FindStatusCondition(rrs.Status.Conditions, conditionReady) + reconcilingCondition := meta.FindStatusCondition(rrs.Status.Conditions, conditionReconciling) - if readyCondition == nil || (readyCondition.Status != metav1.ConditionTrue && readyCondition.Reason == reasonSyncNotCreated) { + if rrs.Status.ObservedGeneration == 0 || (reconcilingCondition != nil && reconcilingCondition.Reason == reasonCreateSync) { return false } From 5ce39965bd1b198966a00234e3144f25f704eb9f Mon Sep 17 00:00:00 2001 From: Chris Fry Date: Tue, 28 Feb 2023 17:45:06 +0000 Subject: [PATCH 4/4] add sync created status field --- rollouts/api/v1alpha1/remoterootsync_types.go | 4 ++++ .../crd/bases/gitops.kpt.dev_remoterootsyncs.yaml | 7 +++++++ rollouts/controllers/remoterootsync_controller.go | 15 +++------------ 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/rollouts/api/v1alpha1/remoterootsync_types.go b/rollouts/api/v1alpha1/remoterootsync_types.go index a4cee11e6c..54f229577c 100644 --- a/rollouts/api/v1alpha1/remoterootsync_types.go +++ b/rollouts/api/v1alpha1/remoterootsync_types.go @@ -70,7 +70,11 @@ type RemoteRootSyncStatus struct { // Conditions describes the reconciliation state of the object. Conditions []metav1.Condition `json:"conditions,omitempty"` + // SyncStatus describes the observed state of external sync. SyncStatus string `json:"syncStatus,omitempty"` + + // Internal only. SyncCreated describes if the external sync has been created. + SyncCreated bool `json:"syncCreated"` } //+kubebuilder:object:root=true diff --git a/rollouts/config/crd/bases/gitops.kpt.dev_remoterootsyncs.yaml b/rollouts/config/crd/bases/gitops.kpt.dev_remoterootsyncs.yaml index 6a6e096794..eaeb68f6fd 100644 --- a/rollouts/config/crd/bases/gitops.kpt.dev_remoterootsyncs.yaml +++ b/rollouts/config/crd/bases/gitops.kpt.dev_remoterootsyncs.yaml @@ -191,8 +191,15 @@ spec: this file' format: int64 type: integer + syncCreated: + description: Internal only. SyncCreated describes if the external + sync has been created. + type: boolean syncStatus: + description: SyncStatus describes the observed state of external sync. type: string + required: + - syncCreated type: object type: object served: true diff --git a/rollouts/controllers/remoterootsync_controller.go b/rollouts/controllers/remoterootsync_controller.go index 32358c6295..db08c19bcb 100644 --- a/rollouts/controllers/remoterootsync_controller.go +++ b/rollouts/controllers/remoterootsync_controller.go @@ -126,7 +126,7 @@ func (r *RemoteRootSyncReconciler) Reconcile(ctx context.Context, req ctrl.Reque // The object is being deleted if controllerutil.ContainsFinalizer(&remoterootsync, myFinalizerName) { // our finalizer is present, so lets handle any external dependency - if r.isExternalSyncCreated(&remoterootsync) { + if remoterootsync.Status.SyncCreated { // Delete the external sync resource err := r.deleteExternalResources(ctx, &remoterootsync) if err != nil && !apierrors.IsNotFound(err) { @@ -197,6 +197,7 @@ func (r *RemoteRootSyncReconciler) updateStatus(ctx context.Context, rrs *gitops if syncError == nil { rrs.Status.SyncStatus = syncStatus + rrs.Status.SyncCreated = true meta.RemoveStatusCondition(conditions, conditionReconciling) meta.RemoveStatusCondition(conditions, conditionStalled) @@ -205,7 +206,7 @@ func (r *RemoteRootSyncReconciler) updateStatus(ctx context.Context, rrs *gitops rrs.Status.SyncStatus = "Unknown" - if !r.isExternalSyncCreated(rrs) { + if !rrs.Status.SyncCreated { rrs.Status.SyncStatus = "" reconcileReason = reasonCreateSync } @@ -367,16 +368,6 @@ func (r *RemoteRootSyncReconciler) getDynamicClientForCluster(ctx context.Contex return dynamicClient, nil } -func (r *RemoteRootSyncReconciler) isExternalSyncCreated(rrs *gitopsv1alpha1.RemoteRootSync) bool { - reconcilingCondition := meta.FindStatusCondition(rrs.Status.Conditions, conditionReconciling) - - if rrs.Status.ObservedGeneration == 0 || (reconcilingCondition != nil && reconcilingCondition.Reason == reasonCreateSync) { - return false - } - - return true -} - // SetupWithManager sets up the controller with the Manager. func (r *RemoteRootSyncReconciler) SetupWithManager(mgr ctrl.Manager) error { r.channel = make(chan event.GenericEvent, 10)