Skip to content

Commit dedbbed

Browse files
anik120anik120
authored
fix(sub): Reset ResolutionFailed cond when error is resolved (#2296)
* fix(sub): Reset ResolutionFailed cond when error is resolved In #2269, a new condition was introduced for Subscription to indicate any dependency resolution error in it's message. However, when the case of the error was resolved, the condition status (true) and the message was sticking around. This PR fixes the issue, and makes sure that the condition status is set to false, and the message and reason of the condition are cleared off. Signed-off-by: Anik Bhattacharjee <[email protected]> * refractor updateStatusConditions to split calls to api server into it's own task Signed-off-by: Anik Bhattacharjee <[email protected]> * Reduce number of times subscription statues are updated. The statuses of the subscriptions are updated multiple number of times while syncing the resolving namespace. This commit switches to preserving the state of the subscription statues instead, and only updating the statuses on cluster only when it's neccessary. Signed-off-by: Anik Bhattacharjee <[email protected]> * remove unnecessary continue from loop Signed-off-by: Anik Bhattacharjee <[email protected]> * pass reference of sub to goroutine Signed-off-by: Anik Bhattacharjee <[email protected]>
1 parent 35deef2 commit dedbbed

File tree

3 files changed

+123
-60
lines changed

3 files changed

+123
-60
lines changed

‎pkg/controller/operators/catalog/operator.go

+43-55
Original file line numberDiff line numberDiff line change
@@ -961,23 +961,30 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error {
961961
// not-satisfiable error
962962
if _, ok := err.(solver.NotSatisfiable); ok {
963963
logger.WithError(err).Debug("resolution failed")
964-
updateErr := o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "ConstraintsNotSatisfiable", err.Error(), true)
964+
subs = o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "ConstraintsNotSatisfiable", err.Error(), true)
965+
_, updateErr := o.updateSubscriptionStatuses(subs)
965966
if updateErr != nil {
966967
logger.WithError(updateErr).Debug("failed to update subs conditions")
968+
return updateErr
967969
}
968970
return nil
969971
}
970-
updateErr := o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "ErrorPreventedResolution", err.Error(), true)
972+
subs = o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "ErrorPreventedResolution", err.Error(), true)
973+
_, updateErr := o.updateSubscriptionStatuses(subs)
971974
if updateErr != nil {
972975
logger.WithError(updateErr).Debug("failed to update subs conditions")
976+
return updateErr
973977
}
974978
return err
975-
} else {
976-
updateErr := o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "", "", false)
979+
}
980+
981+
defer func() {
982+
subs = o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "", "", false)
983+
_, updateErr := o.updateSubscriptionStatuses(subs)
977984
if updateErr != nil {
978-
logger.WithError(updateErr).Debug("failed to update subs conditions")
985+
logger.WithError(updateErr).Warn("failed to update subscription conditions")
979986
}
980-
}
987+
}()
981988

982989
// create installplan if anything updated
983990
if len(updatedSubs) > 0 {
@@ -1008,9 +1015,13 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error {
10081015
logger.WithError(err).Debug("error ensuring installplan")
10091016
return err
10101017
}
1011-
if err := o.updateSubscriptionStatus(namespace, maxGeneration+1, updatedSubs, installPlanReference); err != nil {
1012-
logger.WithError(err).Debug("error ensuring subscription installplan state")
1013-
return err
1018+
updatedSubs = o.setIPReference(updatedSubs, maxGeneration+1, installPlanReference)
1019+
for _, updatedSub := range updatedSubs {
1020+
for i, sub := range subs {
1021+
if sub.Name == updatedSub.Name && sub.Namespace == updatedSub.Namespace {
1022+
subs[i] = updatedSub
1023+
}
1024+
}
10141025
}
10151026
} else {
10161027
logger.Debugf("no subscriptions were updated")
@@ -1128,12 +1139,8 @@ func (o *Operator) ensureSubscriptionCSVState(logger *logrus.Entry, sub *v1alpha
11281139
return updatedSub, true, nil
11291140
}
11301141

1131-
func (o *Operator) updateSubscriptionStatus(namespace string, gen int, subs []*v1alpha1.Subscription, installPlanRef *corev1.ObjectReference) error {
1142+
func (o *Operator) setIPReference(subs []*v1alpha1.Subscription, gen int, installPlanRef *corev1.ObjectReference) []*v1alpha1.Subscription {
11321143
var (
1133-
errs []error
1134-
mu sync.Mutex
1135-
wg sync.WaitGroup
1136-
getOpts = metav1.GetOptions{}
11371144
lastUpdated = o.now()
11381145
)
11391146
for _, sub := range subs {
@@ -1144,33 +1151,8 @@ func (o *Operator) updateSubscriptionStatus(namespace string, gen int, subs []*v
11441151
sub.Status.State = v1alpha1.SubscriptionStateUpgradePending
11451152
sub.Status.InstallPlanGeneration = gen
11461153
}
1147-
1148-
wg.Add(1)
1149-
go func(s v1alpha1.Subscription) {
1150-
defer wg.Done()
1151-
1152-
update := func() error {
1153-
// Update the status of the latest revision
1154-
latest, err := o.client.OperatorsV1alpha1().Subscriptions(s.GetNamespace()).Get(context.TODO(), s.GetName(), getOpts)
1155-
if err != nil {
1156-
return err
1157-
}
1158-
1159-
latest.Status = s.Status
1160-
_, err = o.client.OperatorsV1alpha1().Subscriptions(namespace).UpdateStatus(context.TODO(), latest, metav1.UpdateOptions{})
1161-
1162-
return err
1163-
}
1164-
if err := retry.RetryOnConflict(retry.DefaultRetry, update); err != nil {
1165-
mu.Lock()
1166-
defer mu.Unlock()
1167-
errs = append(errs, err)
1168-
}
1169-
}(*sub)
11701154
}
1171-
wg.Wait()
1172-
1173-
return utilerrors.NewAggregate(errs)
1155+
return subs
11741156
}
11751157

11761158
func (o *Operator) ensureInstallPlan(logger *logrus.Entry, namespace string, gen int, subs []*v1alpha1.Subscription, installPlanApproval v1alpha1.Approval, steps []*v1alpha1.Step, bundleLookups []v1alpha1.BundleLookup) (*corev1.ObjectReference, error) {
@@ -1254,13 +1236,8 @@ func (o *Operator) createInstallPlan(namespace string, gen int, subs []*v1alpha1
12541236
return reference.GetReference(res)
12551237
}
12561238

1257-
func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.SubscriptionConditionType, reason, message string, setTrue bool) error {
1239+
func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.SubscriptionConditionType, reason, message string, setTrue bool) []*v1alpha1.Subscription {
12581240
var (
1259-
errs []error
1260-
mu sync.Mutex
1261-
wg sync.WaitGroup
1262-
getOpts = metav1.GetOptions{}
1263-
updateOpts = metav1.UpdateOptions{}
12641241
lastUpdated = o.now()
12651242
)
12661243
for _, sub := range subs {
@@ -1274,33 +1251,44 @@ func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.
12741251
cond.Status = corev1.ConditionFalse
12751252
}
12761253
sub.Status.SetCondition(cond)
1254+
}
1255+
return subs
1256+
}
1257+
1258+
func (o *Operator) updateSubscriptionStatuses(subs []*v1alpha1.Subscription) ([]*v1alpha1.Subscription, error) {
1259+
var (
1260+
errs []error
1261+
mu sync.Mutex
1262+
wg sync.WaitGroup
1263+
getOpts = metav1.GetOptions{}
1264+
updateOpts = metav1.UpdateOptions{}
1265+
)
12771266

1267+
for _, sub := range subs {
12781268
wg.Add(1)
1279-
go func(s v1alpha1.Subscription) {
1269+
go func(sub *v1alpha1.Subscription) {
12801270
defer wg.Done()
12811271

12821272
update := func() error {
12831273
// Update the status of the latest revision
1284-
latest, err := o.client.OperatorsV1alpha1().Subscriptions(s.GetNamespace()).Get(context.TODO(), s.GetName(), getOpts)
1274+
latest, err := o.client.OperatorsV1alpha1().Subscriptions(sub.GetNamespace()).Get(context.TODO(), sub.GetName(), getOpts)
12851275
if err != nil {
12861276
return err
12871277
}
1288-
1289-
latest.Status = s.Status
1290-
_, err = o.client.OperatorsV1alpha1().Subscriptions(s.Namespace).UpdateStatus(context.TODO(), latest, updateOpts)
1291-
1278+
latest.Status = sub.Status
1279+
*sub = *latest
1280+
_, err = o.client.OperatorsV1alpha1().Subscriptions(sub.Namespace).UpdateStatus(context.TODO(), latest, updateOpts)
12921281
return err
12931282
}
12941283
if err := retry.RetryOnConflict(retry.DefaultRetry, update); err != nil {
12951284
mu.Lock()
12961285
defer mu.Unlock()
12971286
errs = append(errs, err)
12981287
}
1299-
}(*sub)
1288+
}(sub)
13001289
}
13011290
wg.Wait()
1302-
1303-
return utilerrors.NewAggregate(errs)
1291+
return subs, utilerrors.NewAggregate(errs)
13041292
}
13051293

13061294
type UnpackedBundleReference struct {

‎pkg/controller/operators/catalog/subscriptions_test.go

+48
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,14 @@ func TestSyncSubscriptions(t *testing.T) {
155155
},
156156
LastUpdated: now,
157157
InstallPlanGeneration: 1,
158+
Conditions: []v1alpha1.SubscriptionCondition{
159+
{
160+
Type: "ResolutionFailed",
161+
Status: corev1.ConditionFalse,
162+
Reason: "",
163+
Message: "",
164+
},
165+
},
158166
},
159167
},
160168
},
@@ -298,6 +306,14 @@ func TestSyncSubscriptions(t *testing.T) {
298306
},
299307
LastUpdated: now,
300308
InstallPlanGeneration: 1,
309+
Conditions: []v1alpha1.SubscriptionCondition{
310+
{
311+
Type: "ResolutionFailed",
312+
Status: corev1.ConditionFalse,
313+
Reason: "",
314+
Message: "",
315+
},
316+
},
301317
},
302318
},
303319
},
@@ -446,6 +462,14 @@ func TestSyncSubscriptions(t *testing.T) {
446462
},
447463
InstallPlanGeneration: 1,
448464
LastUpdated: now,
465+
Conditions: []v1alpha1.SubscriptionCondition{
466+
{
467+
Type: "ResolutionFailed",
468+
Status: corev1.ConditionFalse,
469+
Reason: "",
470+
Message: "",
471+
},
472+
},
449473
},
450474
},
451475
},
@@ -599,6 +623,14 @@ func TestSyncSubscriptions(t *testing.T) {
599623
},
600624
LastUpdated: now,
601625
InstallPlanGeneration: 1,
626+
Conditions: []v1alpha1.SubscriptionCondition{
627+
{
628+
Type: "ResolutionFailed",
629+
Status: corev1.ConditionFalse,
630+
Reason: "",
631+
Message: "",
632+
},
633+
},
602634
},
603635
},
604636
},
@@ -775,6 +807,14 @@ func TestSyncSubscriptions(t *testing.T) {
775807
},
776808
LastUpdated: now,
777809
InstallPlanGeneration: 1,
810+
Conditions: []v1alpha1.SubscriptionCondition{
811+
{
812+
Type: "ResolutionFailed",
813+
Status: corev1.ConditionFalse,
814+
Reason: "",
815+
Message: "",
816+
},
817+
},
778818
},
779819
},
780820
},
@@ -958,6 +998,14 @@ func TestSyncSubscriptions(t *testing.T) {
958998
},
959999
LastUpdated: now,
9601000
InstallPlanGeneration: 2,
1001+
Conditions: []v1alpha1.SubscriptionCondition{
1002+
{
1003+
Type: "ResolutionFailed",
1004+
Status: corev1.ConditionFalse,
1005+
Reason: "",
1006+
Message: "",
1007+
},
1008+
},
9611009
},
9621010
},
9631011
},

‎test/e2e/subscription_e2e_test.go

+32-5
Original file line numberDiff line numberDiff line change
@@ -2112,8 +2112,11 @@ var _ = Describe("Subscription", func() {
21122112
teardown func()
21132113
cleanup func()
21142114
packages []registry.PackageManifest
2115-
subName = genName("test-subscription")
2116-
catSrcName = genName("test-catalog")
2115+
crd = newCRD(genName("foo-"))
2116+
csvA operatorsv1alpha1.ClusterServiceVersion
2117+
csvB operatorsv1alpha1.ClusterServiceVersion
2118+
subName = genName("test-subscription-")
2119+
catSrcName = genName("test-catalog-")
21172120
)
21182121

21192122
BeforeEach(func() {
@@ -2129,10 +2132,9 @@ var _ = Describe("Subscription", func() {
21292132
DefaultChannelName: "alpha",
21302133
},
21312134
}
2132-
crd := newCRD(genName("foo"))
2133-
csv := newCSV("csvA", testNamespace, "", semver.MustParse("1.0.0"), nil, []apiextensions.CustomResourceDefinition{crd}, nil)
2135+
csvA = newCSV("csvA", testNamespace, "", semver.MustParse("1.0.0"), nil, []apiextensions.CustomResourceDefinition{crd}, nil)
21342136

2135-
_, teardown = createInternalCatalogSource(c, ctx.Ctx().OperatorClient(), catSrcName, testNamespace, packages, nil, []operatorsv1alpha1.ClusterServiceVersion{csv})
2137+
_, teardown = createInternalCatalogSource(c, ctx.Ctx().OperatorClient(), catSrcName, testNamespace, packages, nil, []operatorsv1alpha1.ClusterServiceVersion{csvA})
21362138

21372139
// Ensure that the catalog source is resolved before we create a subscription.
21382140
_, err := fetchCatalogSourceOnStatus(crc, catSrcName, testNamespace, catalogSourceRegistryPodSynced)
@@ -2156,6 +2158,31 @@ var _ = Describe("Subscription", func() {
21562158
}).Should(Equal(corev1.ConditionTrue))
21572159
})
21582160

2161+
When("the required API is made available", func() {
2162+
BeforeEach(func() {
2163+
newPkg := registry.PackageManifest{
2164+
PackageName: "PackageB",
2165+
Channels: []registry.PackageChannel{
2166+
{Name: "alpha", CurrentCSVName: "csvB"},
2167+
},
2168+
DefaultChannelName: "alpha",
2169+
}
2170+
packages = append(packages, newPkg)
2171+
2172+
csvB = newCSV("csvB", testNamespace, "", semver.MustParse("1.0.0"), []apiextensions.CustomResourceDefinition{crd}, nil, nil)
2173+
2174+
updateInternalCatalog(GinkgoT(), c, crc, catSrcName, testNamespace, []apiextensions.CustomResourceDefinition{crd}, []operatorsv1alpha1.ClusterServiceVersion{csvA, csvB}, packages)
2175+
})
2176+
It("the ResolutionFailed condition previously set in it's status that indicated the resolution error is cleared off", func() {
2177+
Eventually(func() (corev1.ConditionStatus, error) {
2178+
sub, err := crc.OperatorsV1alpha1().Subscriptions(testNamespace).Get(context.Background(), subName, metav1.GetOptions{})
2179+
if err != nil {
2180+
return corev1.ConditionUnknown, err
2181+
}
2182+
return sub.Status.GetCondition(operatorsv1alpha1.SubscriptionResolutionFailed).Status, nil
2183+
}).Should(Equal(corev1.ConditionFalse))
2184+
})
2185+
})
21592186
})
21602187

21612188
When("an unannotated ClusterServiceVersion exists with an associated Subscription", func() {

0 commit comments

Comments
 (0)