From a0d9ab12a01d222d8f95047d418b23b92579bb37 Mon Sep 17 00:00:00 2001 From: Vu Dinh Date: Tue, 5 Oct 2021 18:37:09 -0400 Subject: [PATCH] fix(sub): Only update subscriptions that have changes in status Currently, when resolution happens, every subscription will get updated regardless if there are any changes applied or not. This resolves into unnecessary API update calls. This commit will filter out subscriptions that don't get changed and only changed ones get updated. Note: Remove an additional update API call from one of subscription sync methods as well. Signed-off-by: Vu Dinh --- pkg/controller/operators/catalog/operator.go | 107 ++++++++++++------ .../operators/catalog/subscriptions_test.go | 48 -------- test/e2e/subscription_e2e_test.go | 6 +- 3 files changed, 76 insertions(+), 85 deletions(-) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index a66712d878..d152b399a8 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -962,16 +962,27 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error { // not-satisfiable error if _, ok := err.(solver.NotSatisfiable); ok { logger.WithError(err).Debug("resolution failed") - subs = o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "ConstraintsNotSatisfiable", err.Error(), true) - _, updateErr := o.updateSubscriptionStatuses(subs) + _, updateErr := o.updateSubscriptionStatuses( + o.setSubsCond(subs, v1alpha1.SubscriptionCondition{ + Type: v1alpha1.SubscriptionResolutionFailed, + Reason: "ConstraintsNotSatisfiable", + Message: err.Error(), + Status: corev1.ConditionTrue, + })) if updateErr != nil { logger.WithError(updateErr).Debug("failed to update subs conditions") return updateErr } return nil } - subs = o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "ErrorPreventedResolution", err.Error(), true) - _, updateErr := o.updateSubscriptionStatuses(subs) + + _, updateErr := o.updateSubscriptionStatuses( + o.setSubsCond(subs, v1alpha1.SubscriptionCondition{ + Type: v1alpha1.SubscriptionResolutionFailed, + Reason: "ErrorPreventedResolution", + Message: err.Error(), + Status: corev1.ConditionTrue, + })) if updateErr != nil { logger.WithError(updateErr).Debug("failed to update subs conditions") return updateErr @@ -979,14 +990,6 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error { return err } - defer func() { - subs = o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "", "", false) - _, updateErr := o.updateSubscriptionStatuses(subs) - if updateErr != nil { - logger.WithError(updateErr).Warn("failed to update subscription conditions") - } - }() - // create installplan if anything updated if len(updatedSubs) > 0 { logger.Debug("resolution caused subscription changes, creating installplan") @@ -1017,17 +1020,36 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error { return err } updatedSubs = o.setIPReference(updatedSubs, maxGeneration+1, installPlanReference) - for _, updatedSub := range updatedSubs { - for i, sub := range subs { - if sub.Name == updatedSub.Name && sub.Namespace == updatedSub.Namespace { - subs[i] = updatedSub - } - } - } } else { logger.Debugf("no subscriptions were updated") } + // Remove resolutionfailed condition from subscriptions + subs = o.removeSubsCond(subs, v1alpha1.SubscriptionResolutionFailed) + newSub := true + for _, updatedSub := range updatedSubs { + updatedSub.Status.RemoveConditions(v1alpha1.SubscriptionResolutionFailed) + for i, sub := range subs { + if sub.Name == updatedSub.Name && sub.Namespace == updatedSub.Namespace { + subs[i] = updatedSub + newSub = false + break + } + } + if newSub { + subs = append(subs, updatedSub) + continue + } + newSub = true + } + + // Update subscriptions with all changes so far + _, updateErr := o.updateSubscriptionStatuses(subs) + if updateErr != nil { + logger.WithError(updateErr).Warn("failed to update subscription conditions") + return updateErr + } + return nil } @@ -1084,12 +1106,7 @@ func (o *Operator) ensureSubscriptionInstallPlanState(logger *logrus.Entry, sub out.Status.CurrentCSV = out.Spec.StartingCSV out.Status.LastUpdated = o.now() - updated, err := o.client.OperatorsV1alpha1().Subscriptions(sub.GetNamespace()).UpdateStatus(context.TODO(), out, metav1.UpdateOptions{}) - if err != nil { - return nil, false, err - } - - return updated, true, nil + return out, true, nil } func (o *Operator) ensureSubscriptionCSVState(logger *logrus.Entry, sub *v1alpha1.Subscription, querier SourceQuerier) (*v1alpha1.Subscription, bool, error) { @@ -1225,23 +1242,45 @@ func (o *Operator) createInstallPlan(namespace string, gen int, subs []*v1alpha1 return reference.GetReference(res) } -func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.SubscriptionConditionType, reason, message string, setTrue bool) []*v1alpha1.Subscription { +// setSubsCond will set the condition to the subscription if it doesn't already +// exist or if it is different +// Only return the list of updated subscriptions +func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, cond v1alpha1.SubscriptionCondition) []*v1alpha1.Subscription { var ( lastUpdated = o.now() + subList []*v1alpha1.Subscription ) + for _, sub := range subs { + subCond := sub.Status.GetCondition(cond.Type) + if subCond.Equals(cond) { + continue + } sub.Status.LastUpdated = lastUpdated + sub.Status.SetCondition(cond) + subList = append(subList, sub) + } + return subList +} + +// removeSubsCond will remove the condition to the subscription if it exists +// Only return the list of updated subscriptions +func (o *Operator) removeSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.SubscriptionConditionType) []*v1alpha1.Subscription { + var ( + lastUpdated = o.now() + ) + var subList []*v1alpha1.Subscription + for _, sub := range subs { cond := sub.Status.GetCondition(condType) - cond.Reason = reason - cond.Message = message - if setTrue { - cond.Status = corev1.ConditionTrue - } else { - cond.Status = corev1.ConditionFalse + // if status is ConditionUnknown, the condition doesn't exist. Just skip + if cond.Status == corev1.ConditionUnknown { + continue } - sub.Status.SetCondition(cond) + sub.Status.LastUpdated = lastUpdated + sub.Status.RemoveConditions(condType) + subList = append(subList, sub) } - return subs + return subList } func (o *Operator) updateSubscriptionStatuses(subs []*v1alpha1.Subscription) ([]*v1alpha1.Subscription, error) { diff --git a/pkg/controller/operators/catalog/subscriptions_test.go b/pkg/controller/operators/catalog/subscriptions_test.go index 4d35a80394..15e44269fb 100644 --- a/pkg/controller/operators/catalog/subscriptions_test.go +++ b/pkg/controller/operators/catalog/subscriptions_test.go @@ -155,14 +155,6 @@ func TestSyncSubscriptions(t *testing.T) { }, LastUpdated: now, InstallPlanGeneration: 1, - Conditions: []v1alpha1.SubscriptionCondition{ - { - Type: "ResolutionFailed", - Status: corev1.ConditionFalse, - Reason: "", - Message: "", - }, - }, }, }, }, @@ -306,14 +298,6 @@ func TestSyncSubscriptions(t *testing.T) { }, LastUpdated: now, InstallPlanGeneration: 1, - Conditions: []v1alpha1.SubscriptionCondition{ - { - Type: "ResolutionFailed", - Status: corev1.ConditionFalse, - Reason: "", - Message: "", - }, - }, }, }, }, @@ -462,14 +446,6 @@ func TestSyncSubscriptions(t *testing.T) { }, InstallPlanGeneration: 1, LastUpdated: now, - Conditions: []v1alpha1.SubscriptionCondition{ - { - Type: "ResolutionFailed", - Status: corev1.ConditionFalse, - Reason: "", - Message: "", - }, - }, }, }, }, @@ -623,14 +599,6 @@ func TestSyncSubscriptions(t *testing.T) { }, LastUpdated: now, InstallPlanGeneration: 1, - Conditions: []v1alpha1.SubscriptionCondition{ - { - Type: "ResolutionFailed", - Status: corev1.ConditionFalse, - Reason: "", - Message: "", - }, - }, }, }, }, @@ -807,14 +775,6 @@ func TestSyncSubscriptions(t *testing.T) { }, LastUpdated: now, InstallPlanGeneration: 1, - Conditions: []v1alpha1.SubscriptionCondition{ - { - Type: "ResolutionFailed", - Status: corev1.ConditionFalse, - Reason: "", - Message: "", - }, - }, }, }, }, @@ -998,14 +958,6 @@ func TestSyncSubscriptions(t *testing.T) { }, LastUpdated: now, InstallPlanGeneration: 2, - Conditions: []v1alpha1.SubscriptionCondition{ - { - Type: "ResolutionFailed", - Status: corev1.ConditionFalse, - Reason: "", - Message: "", - }, - }, }, }, }, diff --git a/test/e2e/subscription_e2e_test.go b/test/e2e/subscription_e2e_test.go index 978fe2b578..3d4f16f58f 100644 --- a/test/e2e/subscription_e2e_test.go +++ b/test/e2e/subscription_e2e_test.go @@ -2169,14 +2169,14 @@ var _ = Describe("Subscription", func() { updateInternalCatalog(GinkgoT(), c, crc, catSrcName, generatedNamespace.GetName(), []apiextensions.CustomResourceDefinition{crd}, []operatorsv1alpha1.ClusterServiceVersion{csvA, csvB}, packages) }) - It("the ResolutionFailed condition previously set in it's status that indicated the resolution error is cleared off", func() { + It("the ResolutionFailed condition previously set in its status that indicated the resolution error is cleared off", func() { Eventually(func() (corev1.ConditionStatus, error) { sub, err := crc.OperatorsV1alpha1().Subscriptions(generatedNamespace.GetName()).Get(context.Background(), subName, metav1.GetOptions{}) if err != nil { - return corev1.ConditionUnknown, err + return corev1.ConditionFalse, err } return sub.Status.GetCondition(operatorsv1alpha1.SubscriptionResolutionFailed).Status, nil - }).Should(Equal(corev1.ConditionFalse)) + }).Should(Equal(corev1.ConditionUnknown)) }) }) })