fix(sub): Only update subscriptions that have changes in status #2399
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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") | ||
| _, 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 | ||
| } | ||
|
|
||
| _, 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 | ||
| } | ||
|
|
||
| // 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) | ||
| } else { | ||
| logger.Debugf("no subscriptions were updated") | ||
| } | ||
|
|
||
| // Remove resolutionfailed condition from subscriptions | ||
| subs = o.removeSubsCond(subs, v1alpha1.SubscriptionResolutionFailed) | ||
|
There was a problem hiding this comment. Just a quick note: it feels like this function may be a candidate for refactoring in the future. I don't think we need to tackle that kind of change in this PR but I wanted to get any thoughts on whether you had that same though when poking at this locally? There was a problem hiding this comment. I'm open for refactoring but what do you have in mind in term of changes that can be made to this function? It is pretty minimal as it is so I'm not sure what else we can do simplify it further. There was a problem hiding this comment. Just copy-pasting the comment chain on the other PR: #2429 (comment). There was a problem hiding this comment. I'd like to hold off on refactoring and keeping the scope of these changes small as they're affecting upstream/downstream e2e runs, and combing down hot looping behavior in some edge cases. |
||
| 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() | ||
|
|
||
| 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) | ||
| } | ||
|
|
||
| // 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 { | ||
|
There was a problem hiding this comment. This is more of a general question/comment than anything blocking: at a glance, this method currently resembles a Setter but we're passing the reference to the list of subscriptions, and outputting a list of subscriptions. Any idea on whether we're actually double dipping here - modifying the input and outputting a new subscription list? I guess that might not matter a whole ton as it's contingent on how the call site is handling this logic, and it's possible my brain power is limited going into EOD. There was a problem hiding this comment. This is just a "lazy" solution in term of not wanting to deal with removing element(s) from existing list by slicing the array. I can change this to a solution that will do the slicing so that the existing modified list will be returned instead of a new one. |
||
| var ( | ||
| lastUpdated = o.now() | ||
| subList []*v1alpha1.Subscription | ||
| ) | ||
|
|
||
| for _, sub := range subs { | ||
| subCond := sub.Status.GetCondition(cond.Type) | ||
| if subCond.Equals(cond) { | ||
| continue | ||
| } | ||
|
Comment on lines
+1256
to
+1258
There was a problem hiding this comment. It looks like this Equals method does account for the edge case where the existing and new status conditions are equal, and avoids bumping the subCond.LastTransitionTime unnecessarily, avoiding another potential hotlooping scenario. Any value in adding a debug logging here, or is that just overkill? There was a problem hiding this comment. We can add a log here to indicate the condition already exists. I think it can be helpful for debugging reason. |
||
| 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) | ||
| // if status is ConditionUnknown, the condition doesn't exist. Just skip | ||
| if cond.Status == corev1.ConditionUnknown { | ||
| continue | ||
| } | ||
| sub.Status.LastUpdated = lastUpdated | ||
| sub.Status.RemoveConditions(condType) | ||
| subList = append(subList, sub) | ||
| } | ||
| return subList | ||
| } | ||
|
|
||
| func (o *Operator) updateSubscriptionStatuses(subs []*v1alpha1.Subscription) ([]*v1alpha1.Subscription, error) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to removing this defer case. I think this reads a lot more clearer, and being more explicit in this code path likely helps limit side effects.