diff --git a/apis/common/v1/condition.go b/apis/common/v1/condition.go index 3ee7dc5b9..699442d90 100644 --- a/apis/common/v1/condition.go +++ b/apis/common/v1/condition.go @@ -44,6 +44,7 @@ const ( ReasonAvailable ConditionReason = "Available" ReasonUnavailable ConditionReason = "Unavailable" ReasonCreating ConditionReason = "Creating" + ReasonUpdating ConditionReason = "Updating" ReasonDeleting ConditionReason = "Deleting" ) @@ -194,6 +195,17 @@ func Creating() Condition { } } +// Updating returns a condition that indicates the resource is currently +// being updated. +func Updating() Condition { + return Condition{ + Type: TypeReady, + Status: corev1.ConditionFalse, + LastTransitionTime: metav1.Now(), + Reason: ReasonUpdating, + } +} + // Deleting returns a condition that indicates the resource is currently // being deleted. func Deleting() Condition { diff --git a/pkg/reconciler/managed/reconciler.go b/pkg/reconciler/managed/reconciler.go index 37dc9507c..e79da3d1d 100644 --- a/pkg/reconciler/managed/reconciler.go +++ b/pkg/reconciler/managed/reconciler.go @@ -1142,15 +1142,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu } } - if observation.ResourceUpToDate { - // We did not need to create, update, or delete our external resource. - // Per the below issue nothing will notify us if and when the external - // resource we manage changes, so we requeue a speculative reconcile - // after the specified poll interval in order to observe it and react - // accordingly. - // https://github.com/crossplane/crossplane/issues/289 + // skip the update if the management policy is set to ignore updates + if !policy.ShouldUpdate() { reconcileAfter := r.pollIntervalHook(managed, r.pollInterval) - log.Debug("External resource is up to date", "requeue-after", time.Now().Add(reconcileAfter)) + log.Debug("Skipping update due to managementPolicies. Reconciliation succeeded", "requeue-after", time.Now().Add(reconcileAfter)) managed.SetConditions(xpv1.ReconcileSuccess()) return reconcile.Result{RequeueAfter: reconcileAfter}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } @@ -1159,45 +1154,48 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu log.Debug("External resource differs from desired state", "diff", observation.Diff) } - // skip the update if the management policy is set to ignore updates - if !policy.ShouldUpdate() { - reconcileAfter := r.pollIntervalHook(managed, r.pollInterval) - log.Debug("Skipping update due to managementPolicies. Reconciliation succeeded", "requeue-after", time.Now().Add(reconcileAfter)) - managed.SetConditions(xpv1.ReconcileSuccess()) - return reconcile.Result{RequeueAfter: reconcileAfter}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) - } + if !observation.ResourceUpToDate && policy.ShouldUpdate() { + update, err := external.Update(externalCtx, managed) + if err != nil { + // We'll hit this condition if we can't update our external resource, + // for example if our provider credentials don't have access to update + // it. If this is the first time we encounter this issue we'll be + // requeued implicitly when we update our status with the new error + // condition. If not, we requeue explicitly, which will trigger backoff. + log.Debug("Cannot update external resource") + record.Event(managed, event.Warning(reasonCannotUpdate, err)) + managed.SetConditions(xpv1.Updating(), xpv1.ReconcileError(errors.Wrap(err, errReconcileUpdate))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + } - update, err := external.Update(externalCtx, managed) - if err != nil { - // We'll hit this condition if we can't update our external resource, - // for example if our provider credentials don't have access to update - // it. If this is the first time we encounter this issue we'll be - // requeued implicitly when we update our status with the new error - // condition. If not, we requeue explicitly, which will trigger backoff. - log.Debug("Cannot update external resource") - record.Event(managed, event.Warning(reasonCannotUpdate, err)) - managed.SetConditions(xpv1.ReconcileError(errors.Wrap(err, errReconcileUpdate))) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) - } + if _, err := r.managed.PublishConnection(ctx, managed, update.ConnectionDetails); err != nil { + // If this is the first time we encounter this issue we'll be requeued + // implicitly when we update our status with the new error condition. If + // not, we requeue explicitly, which will trigger backoff. + log.Debug("Cannot publish connection details", "error", err) + record.Event(managed, event.Warning(reasonCannotPublish, err)) + managed.SetConditions(xpv1.Updating(), xpv1.ReconcileError(err)) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + } - if _, err := r.managed.PublishConnection(ctx, managed, update.ConnectionDetails); err != nil { - // If this is the first time we encounter this issue we'll be requeued - // implicitly when we update our status with the new error condition. If - // not, we requeue explicitly, which will trigger backoff. - log.Debug("Cannot publish connection details", "error", err) - record.Event(managed, event.Warning(reasonCannotPublish, err)) - managed.SetConditions(xpv1.ReconcileError(err)) + // We've successfully updated our external resource. In many cases the + // updating process takes a little time to finish. We requeue explicitly + // order to observe the external resource to determine whether it's + // ready for use. + log.Debug("Successfully requested update of external resource") + record.Event(managed, event.Normal(reasonUpdated, "Successfully requested update of external resource")) + managed.SetConditions(xpv1.Updating(), xpv1.ReconcileSuccess()) return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } - // We've successfully updated our external resource. Per the below issue - // nothing will notify us if and when the external resource we manage - // changes, so we requeue a speculative reconcile after the specified poll - // interval in order to observe it and react accordingly. + // We did not need to create, update, or delete our external resource. + // Per the below issue nothing will notify us if and when the external + // resource we manage changes, so we requeue a speculative reconcile + // after the specified poll interval in order to observe it and react + // accordingly. // https://github.com/crossplane/crossplane/issues/289 reconcileAfter := r.pollIntervalHook(managed, r.pollInterval) - log.Debug("Successfully requested update of external resource", "requeue-after", time.Now().Add(reconcileAfter)) - record.Event(managed, event.Normal(reasonUpdated, "Successfully requested update of external resource")) + log.Debug("External resource is up to date", "requeue-after", time.Now().Add(reconcileAfter)) managed.SetConditions(xpv1.ReconcileSuccess()) return reconcile.Result{RequeueAfter: reconcileAfter}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } diff --git a/pkg/reconciler/managed/reconciler_test.go b/pkg/reconciler/managed/reconciler_test.go index 137ce3044..acdc57c64 100644 --- a/pkg/reconciler/managed/reconciler_test.go +++ b/pkg/reconciler/managed/reconciler_test.go @@ -1075,6 +1075,7 @@ func TestReconciler(t *testing.T) { MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := &fake.Managed{} want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errReconcileUpdate))) + want.SetConditions(xpv1.Updating()) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := "Errors while updating an external resource should be reported as a conditioned status." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) @@ -1114,6 +1115,7 @@ func TestReconciler(t *testing.T) { MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := &fake.Managed{} want.SetConditions(xpv1.ReconcileError(errBoom)) + want.SetConditions(xpv1.Updating()) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := "Errors publishing connection details after an update should be reported as a conditioned status." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) @@ -1156,7 +1158,7 @@ func TestReconciler(t *testing.T) { want: want{result: reconcile.Result{Requeue: true}}, }, "UpdateSuccessful": { - reason: "A successful managed resource update should trigger a requeue after a long wait.", + reason: "A successful managed resource update should trigger a requeue after a short wait", args: args{ m: &fake.Manager{ Client: &test.MockClient{ @@ -1164,6 +1166,7 @@ func TestReconciler(t *testing.T) { MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := &fake.Managed{} want.SetConditions(xpv1.ReconcileSuccess()) + want.SetConditions(xpv1.Updating()) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := "A successful managed resource update should be reported as a conditioned status." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) @@ -1192,7 +1195,7 @@ func TestReconciler(t *testing.T) { WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), }, }, - want: want{result: reconcile.Result{RequeueAfter: defaultPollInterval}}, + want: want{result: reconcile.Result{Requeue: true}}, }, "ReconciliationPausedSuccessful": { reason: `If a managed resource has the pause annotation with value "true", there should be no further requeue requests.`, @@ -1662,7 +1665,7 @@ func TestReconciler(t *testing.T) { want: want{result: reconcile.Result{RequeueAfter: defaultPollInterval}}, }, "ManagementPolicyAllUpdateSuccessful": { - reason: "A successful managed resource update using management policies should trigger a requeue after a long wait.", + reason: "A successful managed resource update using management policies should trigger a requeue after a short wait.", args: args{ m: &fake.Manager{ Client: &test.MockClient{ @@ -1675,6 +1678,7 @@ func TestReconciler(t *testing.T) { want := &fake.Managed{} want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionAll}) want.SetConditions(xpv1.ReconcileSuccess()) + want.SetConditions(xpv1.Updating()) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := "A successful managed resource update should be reported as a conditioned status." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) @@ -1704,10 +1708,10 @@ func TestReconciler(t *testing.T) { WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), }, }, - want: want{result: reconcile.Result{RequeueAfter: defaultPollInterval}}, + want: want{result: reconcile.Result{Requeue: true}}, }, "ManagementPolicyUpdateUpdateSuccessful": { - reason: "A successful managed resource update using management policies should trigger a requeue after a long wait.", + reason: "A successful managed resource update using management policies should trigger a requeue after a short wait.", args: args{ m: &fake.Manager{ Client: &test.MockClient{ @@ -1720,6 +1724,7 @@ func TestReconciler(t *testing.T) { want := &fake.Managed{} want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionAll}) want.SetConditions(xpv1.ReconcileSuccess()) + want.SetConditions(xpv1.Updating()) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := "A successful managed resource update should be reported as a conditioned status." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) @@ -1749,7 +1754,7 @@ func TestReconciler(t *testing.T) { WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), }, }, - want: want{result: reconcile.Result{RequeueAfter: defaultPollInterval}}, + want: want{result: reconcile.Result{Requeue: true}}, }, "ManagementPolicySkipLateInitialize": { reason: "Should skip updating a managed resource to persist late initialized fields and should trigger a requeue after a long wait.",