Skip to content

Commit af3c683

Browse files
committed
fix(sub): Reset ResolutionFailed cond when error is resolved
In operator-framework#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 <anikbhattacharya93@gmail.com>
1 parent 35deef2 commit af3c683

File tree

3 files changed

+108
-16
lines changed

3 files changed

+108
-16
lines changed

pkg/controller/operators/catalog/operator.go

Lines changed: 28 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -961,19 +961,31 @@ 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+
_, updateErr := o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "ConstraintsNotSatisfiable", err.Error(), true)
965965
if updateErr != nil {
966966
logger.WithError(updateErr).Debug("failed to update subs conditions")
967967
}
968968
return nil
969969
}
970-
updateErr := o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "ErrorPreventedResolution", err.Error(), true)
970+
_, updateErr := o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "ErrorPreventedResolution", err.Error(), true)
971971
if updateErr != nil {
972972
logger.WithError(updateErr).Debug("failed to update subs conditions")
973973
}
974974
return err
975975
} else {
976-
updateErr := o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "", "", false)
976+
subs, updateErr := o.setSubsCond(updatedSubs, v1alpha1.SubscriptionResolutionFailed, "", "", false)
977+
if updateErr != nil {
978+
logger.WithError(updateErr).Debug("failed to update subs conditions")
979+
}
980+
updatedSubs = subs
981+
982+
// Also clear the condition from the subscriptions there were not included in the list of updatedSubs
983+
allSubs, err := o.listSubscriptions(namespace)
984+
if err != nil {
985+
logger.WithError(err).Debug("couldn't list subscriptions")
986+
return err
987+
}
988+
_, updateErr = o.setSubsCond(allSubs, v1alpha1.SubscriptionResolutionFailed, "", "", false)
977989
if updateErr != nil {
978990
logger.WithError(updateErr).Debug("failed to update subs conditions")
979991
}
@@ -1254,7 +1266,7 @@ func (o *Operator) createInstallPlan(namespace string, gen int, subs []*v1alpha1
12541266
return reference.GetReference(res)
12551267
}
12561268

1257-
func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.SubscriptionConditionType, reason, message string, setTrue bool) error {
1269+
func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.SubscriptionConditionType, reason, message string, setTrue bool) ([]*v1alpha1.Subscription, error) {
12581270
var (
12591271
errs []error
12601272
mu sync.Mutex
@@ -1269,26 +1281,31 @@ func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.
12691281
cond.Reason = reason
12701282
cond.Message = message
12711283
if setTrue {
1284+
if cond.Status == corev1.ConditionTrue {
1285+
continue
1286+
}
12721287
cond.Status = corev1.ConditionTrue
12731288
} else {
1289+
if cond.Status == corev1.ConditionFalse {
1290+
continue
1291+
}
12741292
cond.Status = corev1.ConditionFalse
12751293
}
12761294
sub.Status.SetCondition(cond)
12771295

12781296
wg.Add(1)
1279-
go func(s v1alpha1.Subscription) {
1297+
go func(sub v1alpha1.Subscription) {
12801298
defer wg.Done()
12811299

12821300
update := func() error {
12831301
// Update the status of the latest revision
1284-
latest, err := o.client.OperatorsV1alpha1().Subscriptions(s.GetNamespace()).Get(context.TODO(), s.GetName(), getOpts)
1302+
latest, err := o.client.OperatorsV1alpha1().Subscriptions(sub.GetNamespace()).Get(context.TODO(), sub.GetName(), getOpts)
12851303
if err != nil {
12861304
return err
12871305
}
1288-
1289-
latest.Status = s.Status
1290-
_, err = o.client.OperatorsV1alpha1().Subscriptions(s.Namespace).UpdateStatus(context.TODO(), latest, updateOpts)
1291-
1306+
latest.Status = sub.Status
1307+
sub = *latest
1308+
_, err = o.client.OperatorsV1alpha1().Subscriptions(sub.Namespace).UpdateStatus(context.TODO(), latest, updateOpts)
12921309
return err
12931310
}
12941311
if err := retry.RetryOnConflict(retry.DefaultRetry, update); err != nil {
@@ -1300,7 +1317,7 @@ func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.
13001317
}
13011318
wg.Wait()
13021319

1303-
return utilerrors.NewAggregate(errs)
1320+
return subs, utilerrors.NewAggregate(errs)
13041321
}
13051322

13061323
type UnpackedBundleReference struct {

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
},

test/e2e/subscription_e2e_test.go

Lines changed: 32 additions & 5 deletions
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)