From be615d8cd406bc75825d88cb4f4c939a284025c8 Mon Sep 17 00:00:00 2001 From: Jan Chaloupka Date: Mon, 8 Jan 2024 22:00:41 +0100 Subject: [PATCH] sync(library-go): revision_controller: update last revision only when a revision is completely rendered --- go.mod | 2 +- go.sum | 4 +- .../revisioncontroller/revision_controller.go | 89 ++++++++++++------- vendor/modules.txt | 2 +- 4 files changed, 63 insertions(+), 34 deletions(-) diff --git a/go.mod b/go.mod index 6d79d70420..d4cc11331a 100644 --- a/go.mod +++ b/go.mod @@ -15,7 +15,7 @@ require ( github.com/openshift/api v0.0.0-20231219140051-ddc590a81acb github.com/openshift/build-machinery-go v0.0.0-20230228230858-4cd708338479 github.com/openshift/client-go v0.0.0-20231218155125-ff7d9f9bf415 - github.com/openshift/library-go v0.0.0-20231219145842-6c315b5f5029 + github.com/openshift/library-go v0.0.0-20240108202620-5674ec6ced1c github.com/pkg/profile v1.5.0 // indirect github.com/prometheus/client_golang v1.16.0 github.com/spf13/cobra v1.7.0 diff --git a/go.sum b/go.sum index d2a20016f1..ac4b2b372e 100644 --- a/go.sum +++ b/go.sum @@ -163,8 +163,8 @@ github.com/openshift/build-machinery-go v0.0.0-20230228230858-4cd708338479 h1:IU github.com/openshift/build-machinery-go v0.0.0-20230228230858-4cd708338479/go.mod h1:b1BuldmJlbA/xYtdZvKi+7j5YGB44qJUJDZ9zwiNCfE= github.com/openshift/client-go v0.0.0-20231218155125-ff7d9f9bf415 h1:wfnn3E0Z62bB3wYM5eO1AZ9EYZpFd7M1p4PclcIyVv0= github.com/openshift/client-go v0.0.0-20231218155125-ff7d9f9bf415/go.mod h1:5W+xoimHjRdZ0dI/yeQR0ANRNLK9mPmXMzUWPAIPADo= -github.com/openshift/library-go v0.0.0-20231219145842-6c315b5f5029 h1:+pX04GmRvwBqivuQqqURYFZBHEQukdakz+kBnVYQ9x8= -github.com/openshift/library-go v0.0.0-20231219145842-6c315b5f5029/go.mod h1:82B0gt8XawdXWRtKMrm3jSMTeRsiOSYKCi4F0fvPjG0= +github.com/openshift/library-go v0.0.0-20240108202620-5674ec6ced1c h1:zGVuYVRf/tflaFHbpyce/12QSQ5K0OElDhVdnEPtHB8= +github.com/openshift/library-go v0.0.0-20240108202620-5674ec6ced1c/go.mod h1:82B0gt8XawdXWRtKMrm3jSMTeRsiOSYKCi4F0fvPjG0= github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= diff --git a/vendor/github.com/openshift/library-go/pkg/operator/revisioncontroller/revision_controller.go b/vendor/github.com/openshift/library-go/pkg/operator/revisioncontroller/revision_controller.go index 2afeeb0472..afb2e5ca94 100644 --- a/vendor/github.com/openshift/library-go/pkg/operator/revisioncontroller/revision_controller.go +++ b/vendor/github.com/openshift/library-go/pkg/operator/revisioncontroller/revision_controller.go @@ -89,12 +89,14 @@ func (c RevisionController) createRevisionIfNeeded(ctx context.Context, recorder // check to make sure that the latestRevision has the exact content we expect. No mutation here, so we start creating the next Revision only when it is required if isLatestRevisionCurrent { + klog.V(4).Infof("Returning early, %d triggered and up to date", latestAvailableRevision) return false, nil } nextRevision := latestAvailableRevision + 1 - recorder.Eventf("RevisionTriggered", "new revision %d triggered by %q", nextRevision, reason) - if err := c.createNewRevision(ctx, recorder, nextRevision, reason); err != nil { + recorder.Eventf("StartingNewRevision", "new revision %d triggered by %q", nextRevision, reason) + createdNewRevision, err := c.createNewRevision(ctx, recorder, nextRevision, reason) + if err != nil { cond := operatorv1.OperatorCondition{ Type: "RevisionControllerDegraded", Status: operatorv1.ConditionTrue, @@ -108,6 +110,12 @@ func (c RevisionController) createRevisionIfNeeded(ctx context.Context, recorder return true, nil } + if !createdNewRevision { + klog.V(4).Infof("Revision %v not created", nextRevision) + return false, nil + } + recorder.Eventf("RevisionTriggered", "new revision %d triggered by %q", nextRevision, reason) + cond := operatorv1.OperatorCondition{ Type: "RevisionControllerDegraded", Status: operatorv1.ConditionFalse, @@ -208,55 +216,80 @@ func (c RevisionController) isLatestRevisionCurrent(ctx context.Context, revisio return true, "" } -func (c RevisionController) createNewRevision(ctx context.Context, recorder events.Recorder, revision int32, reason string) error { +// returns true if we created a revision +func (c RevisionController) createNewRevision(ctx context.Context, recorder events.Recorder, revision int32, reason string) (bool, error) { // Create a new InProgress status configmap - statusConfigMap := &corev1.ConfigMap{ + desiredStatusConfigMap := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: c.targetNamespace, Name: nameFor("revision-status", revision), + Annotations: map[string]string{ + "operator.openshift.io/revision-ready": "false", + }, }, Data: map[string]string{ "revision": fmt.Sprintf("%d", revision), "reason": reason, }, } - statusConfigMap, _, err := resourceapply.ApplyConfigMap(ctx, c.configMapGetter, recorder, statusConfigMap) - if err != nil { - return err + createdStatus, err := c.configMapGetter.ConfigMaps(desiredStatusConfigMap.Namespace).Create(ctx, desiredStatusConfigMap, metav1.CreateOptions{}) + switch { + case apierrors.IsAlreadyExists(err): + if createdStatus == nil || len(createdStatus.UID) == 0 { + createdStatus, err = c.configMapGetter.ConfigMaps(desiredStatusConfigMap.Namespace).Get(ctx, desiredStatusConfigMap.Name, metav1.GetOptions{}) + if err != nil { + return false, err + } + } + // take a live GET here to get current status to check the annotation + if createdStatus.Annotations["operator.openshift.io/revision-ready"] == "true" { + // no work to do because our cache is out of date and when we're updated, we will be able to see the result + klog.Infof("down the branch indicating that our cache was out of date and we're trying to recreate a revision.") + return false, nil + } + // update the sync and continue + case err != nil: + return false, err } ownerRefs := []metav1.OwnerReference{{ APIVersion: "v1", Kind: "ConfigMap", - Name: statusConfigMap.Name, - UID: statusConfigMap.UID, + Name: createdStatus.Name, + UID: createdStatus.UID, }} for _, cm := range c.configMaps { obj, _, err := resourceapply.SyncConfigMap(ctx, c.configMapGetter, recorder, c.targetNamespace, cm.Name, c.targetNamespace, nameFor(cm.Name, revision), ownerRefs) if err != nil { - return err + return false, err } if obj == nil && !cm.Optional { - return apierrors.NewNotFound(corev1.Resource("configmaps"), cm.Name) + return false, apierrors.NewNotFound(corev1.Resource("configmaps"), cm.Name) } } for _, s := range c.secrets { obj, _, err := resourceapply.SyncSecret(ctx, c.secretGetter, recorder, c.targetNamespace, s.Name, c.targetNamespace, nameFor(s.Name, revision), ownerRefs) if err != nil { - return err + return false, err } if obj == nil && !s.Optional { - return apierrors.NewNotFound(corev1.Resource("secrets"), s.Name) + return false, apierrors.NewNotFound(corev1.Resource("secrets"), s.Name) } } - return nil + createdStatus.Annotations["operator.openshift.io/revision-ready"] = "true" + if _, err := c.configMapGetter.ConfigMaps(createdStatus.Namespace).Update(ctx, createdStatus, metav1.UpdateOptions{}); err != nil { + return false, err + } + + return true, nil } // getLatestAvailableRevision returns the latest known revision to the operator // This is determined by checking revision status configmaps. func (c RevisionController) getLatestAvailableRevision(ctx context.Context) (int32, error) { + // this appears to use a cached getter. I conceded that past-David should have explicitly used Listers configMaps, err := c.configMapGetter.ConfigMaps(c.targetNamespace).List(ctx, metav1.ListOptions{}) if err != nil { return 0, err @@ -281,7 +314,7 @@ func (c RevisionController) getLatestAvailableRevision(ctx context.Context) (int } func (c RevisionController) sync(ctx context.Context, syncCtx factory.SyncContext) error { - operatorSpec, _, latestAvailableRevision, resourceVersion, err := c.operatorClient.GetLatestRevisionState() + operatorSpec, _, latestAvailableRevisionSeenByOperator, resourceVersion, err := c.operatorClient.GetLatestRevisionState() if err != nil { return err } @@ -290,26 +323,22 @@ func (c RevisionController) sync(ctx context.Context, syncCtx factory.SyncContex return nil } - // If the operator status has 0 as its latest available revision, this is either the first revision - // or possibly the operator resource was deleted and reset back to 0, which is not what we want so check configmaps - if latestAvailableRevision == 0 { - // Check to see if current revision is accurate and if not, search through configmaps for latest revision - latestRevision, err := c.getLatestAvailableRevision(ctx) + // If the operator status's latest available revision is not the same as the observed latest revision, update the operator. + latestObservedRevision, err := c.getLatestAvailableRevision(ctx) + if err != nil { + return err + } + if latestObservedRevision != 0 && latestAvailableRevisionSeenByOperator != latestObservedRevision { + // Then make sure that revision number is what's in the operator status + _, _, err := c.operatorClient.UpdateLatestRevisionOperatorStatus(ctx, latestObservedRevision) if err != nil { return err } - if latestRevision != 0 { - // Then make sure that revision number is what's in the operator status - _, _, err := c.operatorClient.UpdateLatestRevisionOperatorStatus(ctx, latestRevision) - if err != nil { - return err - } - // regardless of whether we made a change, requeue to rerun the sync with updated status - return factory.SyntheticRequeueError - } + // regardless of whether we made a change, requeue to rerun the sync with updated status + return factory.SyntheticRequeueError } - requeue, syncErr := c.createRevisionIfNeeded(ctx, syncCtx.Recorder(), latestAvailableRevision, resourceVersion) + requeue, syncErr := c.createRevisionIfNeeded(ctx, syncCtx.Recorder(), latestAvailableRevisionSeenByOperator, resourceVersion) if requeue && syncErr == nil { return factory.SyntheticRequeueError } diff --git a/vendor/modules.txt b/vendor/modules.txt index 2cf6b1a26c..93339e2bf9 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -334,7 +334,7 @@ github.com/openshift/client-go/operatorcontrolplane/informers/externalversions/i github.com/openshift/client-go/operatorcontrolplane/informers/externalversions/operatorcontrolplane github.com/openshift/client-go/operatorcontrolplane/informers/externalversions/operatorcontrolplane/v1alpha1 github.com/openshift/client-go/operatorcontrolplane/listers/operatorcontrolplane/v1alpha1 -# github.com/openshift/library-go v0.0.0-20231219145842-6c315b5f5029 +# github.com/openshift/library-go v0.0.0-20240108202620-5674ec6ced1c ## explicit; go 1.21 github.com/openshift/library-go/pkg/assets github.com/openshift/library-go/pkg/authorization/hardcodedauthorizer