rollouts: add condition when external sync is up to date #3845
Conversation
| @@ -125,6 +126,8 @@ func (r *RemoteRootSyncReconciler) Reconcile(ctx context.Context, req ctrl.Reque | |||
| // Delete the external sync resource | |||
| err := r.deleteExternalResources(ctx, &remoterootsync) | |||
| if err != nil && !apierrors.IsNotFound(err) { | |||
| r.updateStatus(ctx, &remoterootsync, "", err) | |||
There was a problem hiding this comment.
Maybe log the error here if this fails. Not much else we can do at this point, but it would at least give some hint somewhere that something failed.
There was a problem hiding this comment.
Can do. Will push a commit.
| if syncError == nil { | ||
| rrs.Status.SyncStatus = syncStatus | ||
|
|
||
| meta.SetStatusCondition(&rrs.Status.Conditions, metav1.Condition{Type: externalSyncCreatedConditionType, Status: metav1.ConditionTrue, Reason: "SyncCreated"}) |
There was a problem hiding this comment.
Do these functions automatically handle the LastUpdatedTimestamp?
There was a problem hiding this comment.
Confirming that these functions do handle the LastTransitionTime field. Reference, https://github.com/kubernetes/apimachinery/blob/master/pkg/api/meta/conditions.go.
| @@ -63,7 +63,8 @@ var ( | |||
| ) | |||
|
|
|||
| const ( | |||
| externalSyncCreatedConditionType = "ExternalSyncCreated" | |||
| externalSyncCreatedConditionType = "ExternalSyncCreated" | |||
There was a problem hiding this comment.
What is the difference between these? It would assume that it being up-to-date would imply that it is also created?
Also, have we thought if we can adopt some of the more common condition types as described in https://github.com/kubernetes-sigs/cli-utils/tree/master/pkg/kstatus. This might also be relevant: kubernetes-sigs/kubebuilder-declarative-pattern#308
There was a problem hiding this comment.
What is the difference between these? It would assume that it being up-to-date would imply that it is also created?
ExternalSyncCreated indicates the sync was created on the target cluster and ExternalSyncUpToDate indicates the sync is up to date with the latest configuration of the remote root sync. This does similar, but the difference is after you create a rollout resource to rollout v1 of a package and the rollout completes, the rollout resource is updated to rollout the new v2 version of the package, and if, for whatever reason, the reconciler is unable to update the sync on the target cluster (say the cluster is on-premise and there are temporary connectivity issues), the ExternalSyncCreated will remain true since the external sync was created and ExternalSyncUpToDate would be set to a non-True value since the connection cannot be made to the external sync to update it from v1 of the package to v2.
Also, have we thought if we can adopt some of the more common condition types as described in https://github.com/kubernetes-sigs/cli-utils/tree/master/pkg/kstatus. This might also be relevant: kubernetes-sigs/kubebuilder-declarative-pattern#308
Looking into this. My initial thought is to avoid the Ready condition since we don't have the correct information yet to determine if all sub-resources (including any resources that the sync creates) is in ready status. Similarly with Reconciling. The Stalled condition I am thinking can be a good candidate. I'll likely reach out to discuss more.
|
Closing this pull request in favor of #3849. |
This pull request adds a new "ExternalSyncUpToDate" condition the remote root sync resource. This condition is set to true when the external sync is up to date with its latest spec as defined by the remote root sync resource. If the external sync is not up to date (reasons can include not being able to connect to the target cluster or not having the neccessary access on the cluster), the condition is updated with the error message.