From 8bb1efda9dd9918a3a698acb317a851ae64bed05 Mon Sep 17 00:00:00 2001 From: Fiachra Corcoran Date: Thu, 22 Aug 2024 18:40:46 +0100 Subject: [PATCH] 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](https://github.com/nephio-project/nephio/issues/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. --- .../pkg/reconcilers/approval/reconciler.go | 13 ++++++++----- controllers/pkg/reconcilers/config/config.go | 15 ++++++++------- operators/nephio-controller-manager/main.go | 3 +++ 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/controllers/pkg/reconcilers/approval/reconciler.go b/controllers/pkg/reconcilers/approval/reconciler.go index ec8a1440..0a7100c9 100644 --- a/controllers/pkg/reconcilers/approval/reconciler.go +++ b/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,10 +81,12 @@ 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) { @@ -91,7 +94,7 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu 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 diff --git a/controllers/pkg/reconcilers/config/config.go b/controllers/pkg/reconcilers/config/config.go index d1037f84..83456c43 100644 --- a/controllers/pkg/reconcilers/config/config.go +++ b/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 } diff --git a/operators/nephio-controller-manager/main.go b/operators/nephio-controller-manager/main.go index d572d81c..9060cd64 100644 --- a/operators/nephio-controller-manager/main.go +++ b/operators/nephio-controller-manager/main.go @@ -65,6 +65,7 @@ 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.") @@ -72,6 +73,7 @@ func main() { "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)