Improve approval reconciler timings (#797)

Change approval controller PR Get to hit the api directly instead of
reading from local cache.
Adjust the reque duration to prevent race condition.

During debugging the approval delay issue reported
[here](#462) it became
apparent that the packagerev being fetched was a cached version which
didn't get updated for quite some time.
To circumvent this, we are retrieving the PR using the apiReader
interface which bypasses the local cache and hits the k8s api directly.

controllers/pkg/reconcilers/approval/reconciler.go

@@ -47,7 +47,6 @@ const (
 	DelayAnnotationName          = "approval.nephio.org/delay"
 	PolicyAnnotationName         = "approval.nephio.org/policy"
 	InitialPolicyAnnotationValue = "initial"
-	RequeueDuration              = 10 * time.Second
 )
 
 func init() {
@@ -67,10 +66,12 @@ func (r *reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, c i
 		return nil, fmt.Errorf("cannot initialize, expecting controllerConfig, got: %s", reflect.TypeOf(c).Name())
 	}
 
+	r.apiReader = mgr.GetAPIReader()
 	r.baseClient = mgr.GetClient()
 	r.porchClient = cfg.PorchClient
 	r.porchRESTClient = cfg.PorchRESTClient
 	r.recorder = mgr.GetEventRecorderFor("approval-controller")
+	r.requeueDuration = time.Duration(cfg.ApprovalRequeueDuration) * time.Second
 
 	return nil, ctrl.NewControllerManagedBy(mgr).
 		Named("ApprovalController").
@@ -80,18 +81,20 @@ func (r *reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, c i
 
 // reconciler reconciles a NetworkInstance object
 type reconciler struct {
+	apiReader       client.Reader
 	baseClient      client.Client
 	porchClient     client.Client
 	porchRESTClient rest.Interface
 	recorder        record.EventRecorder
+	requeueDuration time.Duration
 }
 
 func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	log := log.FromContext(ctx).WithValues("req", req)
 	log.Info("reconcile approval")
 
 	pr := &porchv1alpha1.PackageRevision{}
-	if err := r.baseClient.Get(ctx, req.NamespacedName, pr); err != nil {
+	if err := r.apiReader.Get(ctx, req.NamespacedName, pr); err != nil {
 		// There's no need to requeue if we no longer exist. Otherwise we'll be
 		// requeued implicitly because we return an error.
 		if resource.IgnoreNotFound(err) != nil {
@@ -123,7 +126,7 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		r.recorder.Event(pr, corev1.EventTypeNormal,
 			"NotApproved", "owning PackageVariant not Ready")
 
-		return ctrl.Result{RequeueAfter: RequeueDuration}, nil
+		return ctrl.Result{RequeueAfter: r.requeueDuration}, nil
 	}
 
 	// All policies require readiness gates to be met, so if they
@@ -132,7 +135,7 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		r.recorder.Event(pr, corev1.EventTypeNormal,
 			"NotApproved", "readiness gates not met")
 
-		return ctrl.Result{RequeueAfter: RequeueDuration}, nil
+		return ctrl.Result{RequeueAfter: r.requeueDuration}, nil
 	}
 
 	// Readiness is met, so check our other policies
@@ -158,7 +161,7 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		r.recorder.Eventf(pr, corev1.EventTypeNormal,
 			"NotApproved", "approval policy %q not met", policy)
 
-		return ctrl.Result{RequeueAfter: RequeueDuration}, nil
+		return ctrl.Result{RequeueAfter: r.requeueDuration}, nil
 	}
 
 	// Delay if needed, and let the user know via an event

controllers/pkg/reconcilers/config/config.go

@@ -28,11 +28,12 @@ import (
 )
 
 type ControllerConfig struct {
-	PorchClient     client.Client
-	PorchRESTClient rest.Interface
-	Poll            time.Duration
-	Copts           controller.Options
-	Address         string // backend server address
-	IpamClientProxy clientproxy.Proxy[*ipamv1alpha1.NetworkInstance, *ipamv1alpha1.IPClaim]
-	VlanClientProxy clientproxy.Proxy[*vlanv1alpha1.VLANIndex, *vlanv1alpha1.VLANClaim]
+	PorchClient             client.Client
+	PorchRESTClient         rest.Interface
+	Poll                    time.Duration
+	Copts                   controller.Options
+	Address                 string // backend server address
+	IpamClientProxy         clientproxy.Proxy[*ipamv1alpha1.NetworkInstance, *ipamv1alpha1.IPClaim]
+	VlanClientProxy         clientproxy.Proxy[*vlanv1alpha1.VLANIndex, *vlanv1alpha1.VLANClaim]
+	ApprovalRequeueDuration int64
 }

operators/nephio-controller-manager/main.go

@@ -65,13 +65,15 @@ func main() {
 	var enableLeaderElection bool
 	var probeAddr string
 	var enabledReconcilersString string
+	var approvalRequeueDuration int64
 
 	flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.")
 	flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
 	flag.BoolVar(&enableLeaderElection, "leader-elect", false,
 		"Enable leader election for controller manager. "+
 			"Enabling this will ensure there is only one active controller manager.")
 	flag.StringVar(&enabledReconcilersString, "reconcilers", "", "reconcilers that should be enabled; use * to mean 'enable all'")
+	flag.Int64Var(&approvalRequeueDuration, "approval-requeue-duration", 15, "Interval to allow before requeue of the approval controller reconcile key")
 
 	opts := zap.Options{
 		Development: true,
@@ -139,6 +141,7 @@ func main() {
 		VlanClientProxy: vlan.New(ctx, clientproxy.Config{
 			Address: backendAddress,
 		}),
+		ApprovalRequeueDuration: approvalRequeueDuration,
 	}
 
 	enabledReconcilers := parseReconcilers(enabledReconcilersString)