From a6e06f3fb7cf3955b4c375f7462cd898ac9a3c80 Mon Sep 17 00:00:00 2001 From: John Belamaric Date: Wed, 20 Dec 2023 12:53:16 -0800 Subject: [PATCH] Requeue in approval and generic specializer (#452) In the most recent Porch builds, I have cleaned up a bunch of noisy watch notifications. This is a good thing, except that our approval controller and specializers were relying on the verbose watch events being thrown out. This means that our e2e test times will go up a lot if we bump Porch version. This PR resolves that, by requeueing when it makes sense to. There is more we could do (for example watching PackageVariants so we find out immediately when they flip to Ready state), but this is a good start. Here are the timings I have: | Test | R1 | Current Nephio
New Porch| This PR Nephio
New Porch | | ---- | ---- | ----------------------- | -------------------------| | 001.sh | 235 secs | 540 secs | 235 secs | | 002.sh | 730 secs | 1307 secs | 584 secs | | 003.sh | 112 secs | 53 secs | 69 secs | | 004.sh | 152 secs | 432 secs | 91 secs | | 005.sh | 358 secs | 388 secs | 222 secs | | 006.sh | 307 secs | 413 secs | 176 secs | | 007.sh | 371 secs | 484 secs | 186 secs | | 008.sh | 291 secs | 289 secs |67 secs | | 009.sh | 26 secs | 30 secs | 31 secs | I also added events in the approval controller for when it proposes and approves the PackageRevision. --- .../pkg/reconcilers/approval/reconciler.go | 17 +++++++++++++---- .../generic-specializer/reconciler.go | 9 +++++++-- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/controllers/pkg/reconcilers/approval/reconciler.go b/controllers/pkg/reconcilers/approval/reconciler.go index 6f78ec51..b8279d56 100644 --- a/controllers/pkg/reconcilers/approval/reconciler.go +++ b/controllers/pkg/reconcilers/approval/reconciler.go @@ -47,6 +47,7 @@ const ( DelayAnnotationName = "approval.nephio.org/delay" PolicyAnnotationName = "approval.nephio.org/policy" InitialPolicyAnnotationValue = "initial" + RequeueDuration = 10 * time.Second ) func init() { @@ -122,7 +123,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{}, nil + return ctrl.Result{RequeueAfter: RequeueDuration}, nil } // All policies require readiness gates to be met, so if they @@ -131,7 +132,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{}, nil + return ctrl.Result{RequeueAfter: RequeueDuration}, nil } // Readiness is met, so check our other policies @@ -157,7 +158,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{}, nil + return ctrl.Result{RequeueAfter: RequeueDuration}, nil } // Delay if needed, and let the user know via an event @@ -185,8 +186,13 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return ctrl.Result{RequeueAfter: requeue}, nil } + action := "approving" + reason := "Approved" + // All policies met if pr.Spec.Lifecycle == porchv1alpha1.PackageRevisionLifecycleDraft { + action = "proposing" + reason = "Proposed" pr.Spec.Lifecycle = porchv1alpha1.PackageRevisionLifecycleProposed err = r.Update(ctx, pr) } else { @@ -198,7 +204,10 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu if err != nil { r.recorder.Eventf(pr, corev1.EventTypeWarning, - "Error", "error approving: %s", err.Error()) + "Error", "error %s: %s", action, err.Error()) + } else { + r.recorder.Eventf(pr, corev1.EventTypeNormal, + reason, "all approval policies met") } return ctrl.Result{}, err diff --git a/controllers/pkg/reconcilers/generic-specializer/reconciler.go b/controllers/pkg/reconcilers/generic-specializer/reconciler.go index 6519d364..88b255cd 100644 --- a/controllers/pkg/reconcilers/generic-specializer/reconciler.go +++ b/controllers/pkg/reconcilers/generic-specializer/reconciler.go @@ -21,6 +21,7 @@ import ( "fmt" "reflect" "strings" + "time" "github.com/GoogleContainerTools/kpt-functions-sdk/go/fn" kptv1 "github.com/GoogleContainerTools/kpt/pkg/api/kptfile/v1" @@ -51,6 +52,10 @@ import ( "sigs.k8s.io/kustomize/kyaml/kio/kioutil" ) +const ( + RequeueDuration = 10 * time.Second +) + func init() { reconcilerinterface.Register("genericspecializer", &reconciler{}) } @@ -111,14 +116,14 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu r.recorder.Event(pr, corev1.EventTypeWarning, "Error", fmt.Sprintf("could not get owning PackageVariant: %s", err.Error())) - return ctrl.Result{}, nil + return ctrl.Result{}, err } if !pvReady { r.recorder.Event(pr, corev1.EventTypeNormal, "Waiting", "owning PackageVariant not Ready") - return ctrl.Result{}, nil + return ctrl.Result{RequeueAfter: RequeueDuration}, nil } ipamf := ipamfn.New(r.ipamClientProxy)