Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rollouts: add status conditions to remote root sync #3849

Merged
merged 4 commits into from
Feb 28, 2023
Merged

rollouts: add status conditions to remote root sync #3849

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0688f63
rollouts: add status conditions to remote root sync
ChristopherFry Feb 23, 2023
2f20dba
use constants for condition reasons
ChristopherFry Feb 24, 2023
a0038b9
remove redundant ready condition
ChristopherFry Feb 24, 2023
5ce3996
add sync created status field
ChristopherFry Feb 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 69 additions & 21 deletions rollouts/controllers/remoterootsync_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,11 @@ var (
)

const (
externalSyncCreatedConditionType = "ExternalSyncCreated"
conditionReady = "Ready"
conditionReconciling = "Reconciling"
ChristopherFry marked this conversation as resolved.
Show resolved Hide resolved
conditionStalled = "Stalled"

reasonSyncNotCreated = "SyncNotCreated"
)

// RemoteRootSyncReconciler reconciles a RemoteRootSync object
Expand Down Expand Up @@ -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) {
ChristopherFry marked this conversation as resolved.
Show resolved Hide resolved
// 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)
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a TODO here:
Reconciling or Stalled condition should be updated (or externalSync conditions should be added) by examining syncStatus. SyncStatus could have values Stalled, Reconciling, Error, Pending even when syncError is nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want actually do this with these being kstatus conditions. From my understanding, the Reconciling and Stalled conditions (in the case of the remote root sync) should only consider if the root sync is created and up to date with the latest spec as defined by the remote root sync, regardless of the sync status of the root sync. When the Reconciling condition is set to true it implies that the Remote Root Sync still needs to reconcile the root sync with the lates spec from the remote root sync. @mortent can you confirm?

Copy link
Contributor

@droot droot Feb 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the root sync is created and up to date with the latest spec as defined by the remote root sync

You are right and that's where my comment come from. If you look at checkSyncStatus, it can return Pending status without any error when generation != observedGeneration (meaning rootsync is not up to date with the latest spec of rootsync). I think for other values Stalled, Reconciling, Error, we will have to create an extra condition to track externalSyncStatus or something.

Never mind. I misunderstood what you meant up there :). I will capture out yesterday's discussion in another issue, I think that would be better.

meta.RemoveStatusCondition(conditions, conditionStalled)
} else {
readyReason := "PendingReconcilation"
ChristopherFry marked this conversation as resolved.
Show resolved Hide resolved
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
ChristopherFry marked this conversation as resolved.
Show resolved Hide resolved

if reflect.DeepEqual(rrs.Status, rrsPrior.Status) {
return nil
Expand Down Expand Up @@ -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") {
ChristopherFry marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand Down