Enable delayed retry when preflight fails

[tomasaschan]

What would you like to be added:
When using a status provider with a Preflight method defined, it would be nice to have a way to control the requeue behavior of the reconcile.Result returned here. Since the err is currently both used a signal whether the pre-flight check was successful and for whether to requeue, there's no way to fail the precheck but requeue in a more controlled manner.

Why is this needed:
For example, we have a resource that depends on some stuff existing in an external system; we can create a Preflight check that ensures this thing is available (and exposes a key for it on the status, making it available to read in object transforms in the reconciliation method), but when it is not there is no way for us to signal "please don't reconcile now, but retry in 10 minutes" or similar.

[tomasaschan]

Here's one way to solve for my need. It's not super elegant (maybe it would be better to encapsulate the tupled return values in a struct of some sort) but it works:

From bf7e57efe4b404eae875a0b1468e328cfa860e68 Mon Sep 17 00:00:00 2001
From: Tomas Aschan <[email protected]>
Date: Mon, 22 Nov 2021 16:48:57 +0100
Subject: [PATCH] Support more complex preflight checks

---
 pkg/patterns/declarative/reconciler.go |  5 ++++-
 pkg/patterns/declarative/status.go     | 20 +++++++++++++++-----
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/pkg/patterns/declarative/reconciler.go b/pkg/patterns/declarative/reconciler.go
index 1224b4b..6564051 100644
--- a/pkg/patterns/declarative/reconciler.go
+++ b/pkg/patterns/declarative/reconciler.go
@@ -135,9 +135,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 	}
 
 	if r.options.status != nil {
-		if err := r.options.status.Preflight(ctx, instance); err != nil {
+		if shouldReconcile, result, err := r.options.status.Preflight(ctx, instance); err != nil {
 			log.Error(err, "preflight check failed, not reconciling")
 			return reconcile.Result{}, err
+		} else if !shouldReconcile {
+			log.Info("preflight check indicated reconciliation should be skipped")
+			return result, nil
 		}
 	}
 
diff --git a/pkg/patterns/declarative/status.go b/pkg/patterns/declarative/status.go
index 6b43085..bf94060 100644
--- a/pkg/patterns/declarative/status.go
+++ b/pkg/patterns/declarative/status.go
@@ -19,6 +19,7 @@ package declarative
 import (
 	"context"
 
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/declarative/pkg/manifest"
 )
 
@@ -38,9 +39,18 @@ type Reconciled interface {
 
 type Preflight interface {
 	// Preflight validates if the current state of the world is ready for reconciling.
-	// Returning a non-nil error on this object will prevent Reconcile from running.
-	// The caller is encouraged to surface the error status on the DeclarativeObject.
-	Preflight(context.Context, DeclarativeObject) error
+	//
+	// The return values should be interpreted as follows:
+	// - a bool indicating whether reconciliation should happen
+	// - a reconcile.Result indicating if and when to requeue reconciliation; only relevant if the first value is false
+	// - any error that might have occurred during preflight checks
+	//
+	// If an error is returned, Reoncile will not run, and a retry will be immediately requeued.
+	// If no error is returned, and the boolean value is false, the reconcile.Result will be returned by the controller
+	// If no error is returned, and the boolean value is true, Reconcile will run and the reconcile.Result returned here will be ignored.
+	//
+	// The caller is encouraged to surface the any errors or other reasons the preflights failed on the DeclarativeObject.
+	Preflight(context.Context, DeclarativeObject) (bool, reconcile.Result, error)
 }
 
 type VersionCheck interface {
@@ -64,11 +74,11 @@ func (s *StatusBuilder) Reconciled(ctx context.Context, src DeclarativeObject, o
 	return nil
 }
 
-func (s *StatusBuilder) Preflight(ctx context.Context, src DeclarativeObject) error {
+func (s *StatusBuilder) Preflight(ctx context.Context, src DeclarativeObject) (bool, reconcile.Result, error) {
 	if s.PreflightImpl != nil {
 		return s.PreflightImpl.Preflight(ctx, src)
 	}
-	return nil
+	return true, reconcile.Result{}, nil
 }
 
 func (s *StatusBuilder) VersionCheck(ctx context.Context, src DeclarativeObject, objs *manifest.Objects) (bool, error) {

With this in place, I can create a status object with a preflight request that returns e.g. false, reconcile.Result{RequeueAfter: 10*time.Minute}, nil to say "don't run the reconciliation now, but please try again in 10 minutes". This is an improvement on the current way things work, as I can today only say either "please run the reconciliation now" (by returning a nil error) or "please retry the preflight immediately" (by returning a non-nil error, which will be returned also by the reconciliation function itself).

[fsommar]

Are there more instances where we'd possibly like to set a different requeue timing? Could it be solved in a more general manner by having a handler for errors for whether they should be instantly retried or not? Alternatively, would it make sense to return a nil error but set the requeue timing? It's not semantically covering what's happening, but effectively it would do the same/a similar thing?

[justinsb]

This is certainly a valid use-case, and I imagine we'll discover more of these things over time.

One pattern that allows for evolution is to pass an object in, e.g. Preflight(operation PreflightOperation), and then we can extend PreflightOperation without breaking clients.

Looking at the existing arguments, we have to decide whether to embed them in PreflightOperation. I'd say we should still keep the src DeclarativeObject because it acts to document when Preflight gets called. ctx context.Context is trickier. The go style guide says we shouldn't embed context.Context, but I think this is mostly about long-lived objects; here a PreflightOperation would have a similar lifetime to context.Context.

My personal opinion is that we should end up with something like Preflight(ctx PreflightOperationContext, src DeclarativeObject), and that PreflightOperationContext should implement context.Context - this is as simple as embedding context.Context. I'll try to follow up with some go-gurus at google to see what they think, but we can also make our own decision :-)

[tomasaschan]

@justinsb Coming back to this now that vacations are over :)

I think what you're saying makes sense in terms of how to enable this API to evolve, but I'm also not sure if it relates strictly to what I'm trying to accomplish. Note that the changes in my hacky diff are in the return values, not in the arguments... But the approach you suggest - wrapping everything in a struct that is specific to Preflight - makes sense in that context too. I suppose we'd end up with something like

 type Preflight interface {
-	Preflight(context.Context, DeclarativeObject) error
+	Preflight(PreflightOperationContext, DeclarativeObject) (PreflightOperationResult, error)
 }
 
+ type PreflightOperationContext struct {
+ 	context.Context
+ }
 
+ type PreflightOperationResult struct {
+ 	Reconcile bool        // whether to proceed with reconciliation
+ 	Result    ctrl.Result // if not proceeding, the result to return from the reconciler, to allow e.g. requeues
+ }

And then we can extend the PreflightOperationContext and PreflightOperationResult types with more stuff later, whenever need arises - as long as we can handle their default values, that won't be breaking.

Is this the design you have in mind?

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[tomasaschan]

/remove-lifecycle rotten

[fsommar]

This issue is fixed by #199. The PR introduced the ability to return *declarative.ErrorResult from Preflight, giving Preflight the agency to affect the returned reconciliation result (and error). To delay the retry:

return &declarative.ErrorResult{
  Result: ctrl.Result{RequeueAfter: wait.Jitter(20*time.Minute, 0.5)},
  Err: nil,
}

This results in an early exit of the full reconciliation, and a returned ctrl.Result{RequeueAfter: wait.Jitter(20*time.Minute, 0.5)}, nil pair in controller-runtime terms.