Update status even if reconciliation fails #16
Conversation
| } | ||
|
|
||
| func (s *stubProvider) ListClusters(ctx context.Context) ([]*providers.ProviderCluster, error) { | ||
| return s.response, nil | ||
| return s.response, s.responseErr |
| @@ -83,6 +83,9 @@ func (r *AutomatedClusterDiscoveryReconciler) Reconcile(ctx context.Context, req | |||
| // Set the value of the reconciliation request in status. | |||
| if v, ok := meta.ReconcileAnnotationValue(clusterDiscovery.GetAnnotations()); ok { | |||
| clusterDiscovery.Status.LastHandledReconcileAt = v | |||
| if err := r.Status().Update(ctx, clusterDiscovery); err != nil { | |||
| return ctrl.Result{}, err | |||
| } | |||
There was a problem hiding this comment.
This looks okay to my eye.. I'm not sure if there are implications in potentially writing the status twice per reconcilation. cc @bigkevmcd
Ideally we want probably only want to write the status once. Which we could do by refactoring the code a bit here. Kevin you also mentioned you might be re-working the code here too, so in that case we could keep the code change in this PR small like this.
We also have r.patchStatus() we could use below, which refetches the ACD before updating. Not completely if we should do that here too.
There was a problem hiding this comment.
We should use patchStatus, because as you say, it will reload the ACD before updating, otherwise you can get into conflict errors.
There was a problem hiding this comment.
@sarataha Can you use patchStatus for this?
|
Nice! Lets also add a PR description!
|
| @@ -565,13 +566,98 @@ func TestReconcilingWithAnnotationChange(t *testing.T) { | |||
| assert.Equal(t, aksCluster.Status.LastHandledReconcileAt, "testing") | |||
| } | |||
|
|
|||
| func TestReconcilingWithAnnotationChangeWithError(t *testing.T) { | |||
| ctx := context.TODO() | |||
There was a problem hiding this comment.
Is there a reason to run this outside the TestAutomatedClusterDiscoveryReconciler sub-tests?
Starting the test env will take several seconds to start and stop, so if possible, we should reuse it?
There was a problem hiding this comment.
Makes sense to me! I added it to one of the subtasks already created :)
sarataha
force-pushed
the
sync-error-flow
branch
from
4e77f8e
to
97a0de1
Compare
| @@ -149,6 +149,22 @@ func TestAutomatedClusterDiscoveryReconciler(t *testing.T) { | |||
| } | |||
| assertHasOwnerReference(t, gitopsCluster, clusterRef) | |||
| assertHasOwnerReference(t, secret, clusterRef) | |||
|
|
|||
There was a problem hiding this comment.
This isn't quite what I meant, can you split out this into an explicit test for failing to reconcile recording the status?
Rather than tacking it on to the end of the test here, it can be a sub-test, take your previous test, and create a new sub-test.
sarataha
force-pushed
the
sync-error-flow
branch
from
97a0de1
to
e0a8d19
Compare
PR to ensure status is updated even when the reconciliation fails. This is needed because users in the UI might be clicking on the sync button to try and "fix" broken resources, so we should ensure that the
lastHandledReconcileAtis updated correctly.A test case was added for when
stubProvider's methodListClustersreturns an error which causes the reconciliation to fail.