Skip to content

Commit 859291e

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #176 from joelanford/bz-1994648
Bug 1994648: fix(sub): Reset ResolutionFailed cond when error is resolved
2 parents d4a3aa1 + 52aa59c commit 859291e

File tree

4 files changed

+166
-115
lines changed

4 files changed

+166
-115
lines changed

‎staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go

Lines changed: 43 additions & 55 deletions
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 {

‎staging/operator-lifecycle-manager/pkg/controller/operators/catalog/subscriptions_test.go

Lines changed: 48 additions & 0 deletions
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
},

‎staging/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go

Lines changed: 32 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2113,8 +2113,11 @@ var _ = Describe("Subscription", func() {
21132113
teardown func()
21142114
cleanup func()
21152115
packages []registry.PackageManifest
2116-
subName = genName("test-subscription")
2117-
catSrcName = genName("test-catalog")
2116+
crd = newCRD(genName("foo-"))
2117+
csvA operatorsv1alpha1.ClusterServiceVersion
2118+
csvB operatorsv1alpha1.ClusterServiceVersion
2119+
subName = genName("test-subscription-")
2120+
catSrcName = genName("test-catalog-")
21182121
)
21192122

21202123
BeforeEach(func() {
@@ -2130,10 +2133,9 @@ var _ = Describe("Subscription", func() {
21302133
DefaultChannelName: "alpha",
21312134
},
21322135
}
2133-
crd := newCRD(genName("foo"))
2134-
csv := newCSV("csvA", testNamespace, "", semver.MustParse("1.0.0"), nil, []apiextensions.CustomResourceDefinition{crd}, nil)
2136+
csvA = newCSV("csvA", testNamespace, "", semver.MustParse("1.0.0"), nil, []apiextensions.CustomResourceDefinition{crd}, nil)
21352137

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

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

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

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

0 commit comments

Comments
 (0)