Merge pull request #322 from justinsb/test_reconciler_sleep_after_apply

testreconciler: pause after reconciliation

Author: k8s-ci-robot

pkg/patterns/declarative/hooks.go

@@ -48,3 +48,19 @@ type AfterApply interface {
 type BeforeApply interface {
 	BeforeApply(ctx context.Context, op *ApplyOperation) error
 }
+
+// UpdateStatusOperation contains the details of an Apply operation
+type UpdateStatusOperation struct {
+	// Subject is the object we are reconciling
+	Subject DeclarativeObject
+}
+
+// AfterUpdateStatus is implemented by hooks that want to be called after the update-status phase
+type AfterUpdateStatus interface {
+	AfterUpdateStatus(ctx context.Context, op *UpdateStatusOperation) error
+}
+
+// BeforeUpdateStatus is implemented by hooks that want to be called before the update-status phase
+type BeforeUpdateStatus interface {
+	BeforeUpdateStatus(ctx context.Context, op *UpdateStatusOperation) error
+}

pkg/patterns/declarative/reconciler.go

@@ -198,25 +198,56 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 		}
 	}
 
+	if err := r.updateStatus(ctx, original, instance); err != nil {
+		return result, err
+	}
+
+	return result, nil
+}
+
+func (r *Reconciler) updateStatus(ctx context.Context, original DeclarativeObject, instance DeclarativeObject) error {
+	log := log.FromContext(ctx)
+
 	// Write the status if it has changed
 	oldStatus, err := utils.GetCommonStatus(original)
 	if err != nil {
 		log.Error(err, "error getting status")
-		return result, err
+		return err
 	}
 	newStatus, err := utils.GetCommonStatus(instance)
 	if err != nil {
 		log.Error(err, "error getting status")
-		return result, err
+		return err
+	}
+
+	statusOperation := &UpdateStatusOperation{}
+
+	for _, hook := range r.options.hooks {
+		if beforeUpdateStatus, ok := hook.(BeforeUpdateStatus); ok {
+			if err := beforeUpdateStatus.BeforeUpdateStatus(ctx, statusOperation); err != nil {
+				log.Error(err, "calling BeforeUpdateStatus hook")
+				return fmt.Errorf("error calling BeforeUpdateStatus hook: %w", err)
+			}
+		}
 	}
+
 	if !reflect.DeepEqual(oldStatus, newStatus) {
 		if err := r.client.Status().Update(ctx, instance); err != nil {
 			log.Error(err, "error updating status")
-			return result, err
+			return err
 		}
 	}
 
-	return result, err
+	for _, hook := range r.options.hooks {
+		if afterUpdateStatus, ok := hook.(AfterUpdateStatus); ok {
+			if err := afterUpdateStatus.AfterUpdateStatus(ctx, statusOperation); err != nil {
+				log.Error(err, "calling AfterUpdateStatus hook")
+				return fmt.Errorf("error calling AfterUpdateStatus hook: %w", err)
+			}
+		}
+	}
+
+	return nil
 }
 
 func (r *Reconciler) reconcileExists(ctx context.Context, name types.NamespacedName, instance DeclarativeObject) (*StatusInfo, error) {

pkg/test/testreconciler/simpletest/controller.go

@@ -17,8 +17,12 @@ limitations under the License.
 package simpletest
 
 import (
+	"context"
+	"time"
+
 	"github.com/go-logr/logr"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/klog/v2"
 
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -76,6 +80,8 @@ func (r *SimpleTestReconciler) setupReconciler(mgr ctrl.Manager) error {
 
 		declarative.WithManifestController(r.manifestController),
 		declarative.WithApplier(r.applier),
+
+		declarative.WithHook(&sleepAfterUpdateStatusHook{}),
 	)
 }
 
@@ -103,3 +109,19 @@ func (r *SimpleTestReconciler) SetupWithManager(mgr ctrl.Manager) error {
 
 	return nil
 }
+
+// We sleep briefly after updating the status.
+// This is a hack to ensure that we see the watch event, because otherwise we can start a re-reconcile based on our derived objects,
+// and then reconcile again based on our own status update.  This is racy behaviour and causes non-determinism.
+// We do this only in this test controller, but maybe we should have similar logic to "debounce" in all controllers.
+type sleepAfterUpdateStatusHook struct {
+}
+
+var _ declarative.AfterUpdateStatus = &sleepAfterUpdateStatusHook{}
+
+func (h *sleepAfterUpdateStatusHook) AfterUpdateStatus(ctx context.Context, op *declarative.UpdateStatusOperation) error {
+	sleepFor := 100 * time.Millisecond
+	klog.Infof("sleeping after apply for %v", sleepFor)
+	time.Sleep(sleepFor)
+	return nil
+}