Skip to content

Commit

Permalink
fix(sub): Only update subscriptions that have changes in status
Browse files Browse the repository at this point in the history 
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 <vudinh@outlook.com>
  • Loading branch information
@dinhxuanvu
dinhxuanvu committed Oct 20, 2021
1 parent adf9a83 commit 6af6678
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 85 deletions.
107 changes: 73 additions & 34 deletions pkg/controller/operators/catalog/operator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -962,31 +962,34 @@ 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
}
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")
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
48 changes: 0 additions & 48 deletions pkg/controller/operators/catalog/subscriptions_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,6 @@ func TestSyncSubscriptions(t *testing.T) {
},
LastUpdated: now,
InstallPlanGeneration: 1,
Conditions: []v1alpha1.SubscriptionCondition{
{
Type: "ResolutionFailed",
Status: corev1.ConditionFalse,
Reason: "",
Message: "",
},
},
},
},
},
Expand Down Expand Up @@ -306,14 +298,6 @@ func TestSyncSubscriptions(t *testing.T) {
},
LastUpdated: now,
InstallPlanGeneration: 1,
Conditions: []v1alpha1.SubscriptionCondition{
{
Type: "ResolutionFailed",
Status: corev1.ConditionFalse,
Reason: "",
Message: "",
},
},
},
},
},
Expand Down Expand Up @@ -462,14 +446,6 @@ func TestSyncSubscriptions(t *testing.T) {
},
InstallPlanGeneration: 1,
LastUpdated: now,
Conditions: []v1alpha1.SubscriptionCondition{
{
Type: "ResolutionFailed",
Status: corev1.ConditionFalse,
Reason: "",
Message: "",
},
},
},
},
},
Expand Down Expand Up @@ -623,14 +599,6 @@ func TestSyncSubscriptions(t *testing.T) {
},
LastUpdated: now,
InstallPlanGeneration: 1,
Conditions: []v1alpha1.SubscriptionCondition{
{
Type: "ResolutionFailed",
Status: corev1.ConditionFalse,
Reason: "",
Message: "",
},
},
},
},
},
Expand Down Expand Up @@ -807,14 +775,6 @@ func TestSyncSubscriptions(t *testing.T) {
},
LastUpdated: now,
InstallPlanGeneration: 1,
Conditions: []v1alpha1.SubscriptionCondition{
{
Type: "ResolutionFailed",
Status: corev1.ConditionFalse,
Reason: "",
Message: "",
},
},
},
},
},
Expand Down Expand Up @@ -998,14 +958,6 @@ func TestSyncSubscriptions(t *testing.T) {
},
LastUpdated: now,
InstallPlanGeneration: 2,
Conditions: []v1alpha1.SubscriptionCondition{
{
Type: "ResolutionFailed",
Status: corev1.ConditionFalse,
Reason: "",
Message: "",
},
},
},
},
},
Expand Down
6 changes: 3 additions & 3 deletions test/e2e/subscription_e2e_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2188,14 +2188,14 @@ var _ = Describe("Subscription", func() {

updateInternalCatalog(GinkgoT(), c, crc, catSrcName, testNamespace, []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() {
FIt("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(testNamespace).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))
})
})
})
Expand Down

0 comments on commit 6af6678

Please sign in to comment.