Skip to content

fix(sub): Reset ResolutionFailed cond when error is resolved #2296

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 24, 2021
Merged

fix(sub): Reset ResolutionFailed cond when error is resolved #2296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
af3c683
fix(sub): Reset ResolutionFailed cond when error is resolved
anik120 Jul 28, 2021
3e1a16d
refractor updateStatusConditions to split calls to api server into it…
anik120 Jul 30, 2021
e419a97
Reduce number of times subscription statues are updated.
anik120 Aug 24, 2021
bdb7644
remove unnecessary continue from loop
anik120 Aug 24, 2021
17d779c
pass reference of sub to goroutine
anik120 Aug 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 43 additions & 55 deletions pkg/controller/operators/catalog/operator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -961,23 +961,30 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error {
// not-satisfiable error
if _, ok := err.(solver.NotSatisfiable); ok {
logger.WithError(err).Debug("resolution failed")
updateErr := o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "ConstraintsNotSatisfiable", err.Error(), true)
subs = o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "ConstraintsNotSatisfiable", err.Error(), true)
_, updateErr := o.updateSubscriptionStatuses(subs)
if updateErr != nil {
logger.WithError(updateErr).Debug("failed to update subs conditions")
return updateErr
}
return nil
}
updateErr := o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "ErrorPreventedResolution", err.Error(), true)
subs = o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "ErrorPreventedResolution", err.Error(), true)
_, updateErr := o.updateSubscriptionStatuses(subs)
if updateErr != nil {
logger.WithError(updateErr).Debug("failed to update subs conditions")
return updateErr
}
return err
} else {
updateErr := o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "", "", false)
}

defer func() {
subs = o.setSubsCond(subs, v1alpha1.SubscriptionResolutionFailed, "", "", false)
_, updateErr := o.updateSubscriptionStatuses(subs)
if updateErr != nil {
logger.WithError(updateErr).Debug("failed to update subs conditions")
logger.WithError(updateErr).Warn("failed to update subscription conditions")
}
}
}()

// create installplan if anything updated
if len(updatedSubs) > 0 {
Expand Down Expand Up @@ -1008,9 +1015,13 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error {
logger.WithError(err).Debug("error ensuring installplan")
return err
}
if err := o.updateSubscriptionStatus(namespace, maxGeneration+1, updatedSubs, installPlanReference); err != nil {
logger.WithError(err).Debug("error ensuring subscription installplan state")
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")
Expand Down Expand Up @@ -1128,12 +1139,8 @@ func (o *Operator) ensureSubscriptionCSVState(logger *logrus.Entry, sub *v1alpha
return updatedSub, true, nil
}

func (o *Operator) updateSubscriptionStatus(namespace string, gen int, subs []*v1alpha1.Subscription, installPlanRef *corev1.ObjectReference) error {
func (o *Operator) setIPReference(subs []*v1alpha1.Subscription, gen int, installPlanRef *corev1.ObjectReference) []*v1alpha1.Subscription {
var (
errs []error
mu sync.Mutex
wg sync.WaitGroup
getOpts = metav1.GetOptions{}
lastUpdated = o.now()
)
for _, sub := range subs {
Expand All @@ -1144,33 +1151,8 @@ func (o *Operator) updateSubscriptionStatus(namespace string, gen int, subs []*v
sub.Status.State = v1alpha1.SubscriptionStateUpgradePending
sub.Status.InstallPlanGeneration = gen
}

wg.Add(1)
go func(s v1alpha1.Subscription) {
defer wg.Done()

update := func() error {
// Update the status of the latest revision
latest, err := o.client.OperatorsV1alpha1().Subscriptions(s.GetNamespace()).Get(context.TODO(), s.GetName(), getOpts)
if err != nil {
return err
}

latest.Status = s.Status
_, err = o.client.OperatorsV1alpha1().Subscriptions(namespace).UpdateStatus(context.TODO(), latest, metav1.UpdateOptions{})

return err
}
if err := retry.RetryOnConflict(retry.DefaultRetry, update); err != nil {
mu.Lock()
defer mu.Unlock()
errs = append(errs, err)
}
}(*sub)
}
wg.Wait()

return utilerrors.NewAggregate(errs)
return subs
}

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) {
Expand Down Expand Up @@ -1254,13 +1236,8 @@ 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) error {
func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.SubscriptionConditionType, reason, message string, setTrue bool) []*v1alpha1.Subscription {
var (
errs []error
mu sync.Mutex
wg sync.WaitGroup
getOpts = metav1.GetOptions{}
updateOpts = metav1.UpdateOptions{}
lastUpdated = o.now()
)
for _, sub := range subs {
Expand All @@ -1274,33 +1251,44 @@ func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.
cond.Status = corev1.ConditionFalse
}
sub.Status.SetCondition(cond)
}
return subs
}

func (o *Operator) updateSubscriptionStatuses(subs []*v1alpha1.Subscription) ([]*v1alpha1.Subscription, error) {
var (
errs []error
mu sync.Mutex
wg sync.WaitGroup
getOpts = metav1.GetOptions{}
updateOpts = metav1.UpdateOptions{}
)

for _, sub := range subs {
wg.Add(1)
go func(s v1alpha1.Subscription) {
go func(sub *v1alpha1.Subscription) {
defer wg.Done()

update := func() error {
// Update the status of the latest revision
latest, err := o.client.OperatorsV1alpha1().Subscriptions(s.GetNamespace()).Get(context.TODO(), s.GetName(), getOpts)
latest, err := o.client.OperatorsV1alpha1().Subscriptions(sub.GetNamespace()).Get(context.TODO(), sub.GetName(), getOpts)
if err != nil {
return err
}

latest.Status = s.Status
_, err = o.client.OperatorsV1alpha1().Subscriptions(s.Namespace).UpdateStatus(context.TODO(), latest, updateOpts)

latest.Status = sub.Status
*sub = *latest
_, err = o.client.OperatorsV1alpha1().Subscriptions(sub.Namespace).UpdateStatus(context.TODO(), latest, updateOpts)
return err
}
if err := retry.RetryOnConflict(retry.DefaultRetry, update); err != nil {
mu.Lock()
defer mu.Unlock()
errs = append(errs, err)
}
}(*sub)
}(sub)
}
wg.Wait()

return utilerrors.NewAggregate(errs)
return subs, utilerrors.NewAggregate(errs)
}

type UnpackedBundleReference struct {
Expand Down
48 changes: 48 additions & 0 deletions pkg/controller/operators/catalog/subscriptions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,14 @@ func TestSyncSubscriptions(t *testing.T) {
},
LastUpdated: now,
InstallPlanGeneration: 1,
Conditions: []v1alpha1.SubscriptionCondition{
{
Type: "ResolutionFailed",
Status: corev1.ConditionFalse,
Reason: "",
Message: "",
},
},
},
},
},
Expand Down Expand Up @@ -298,6 +306,14 @@ func TestSyncSubscriptions(t *testing.T) {
},
LastUpdated: now,
InstallPlanGeneration: 1,
Conditions: []v1alpha1.SubscriptionCondition{
{
Type: "ResolutionFailed",
Status: corev1.ConditionFalse,
Reason: "",
Message: "",
},
},
},
},
},
Expand Down Expand Up @@ -446,6 +462,14 @@ func TestSyncSubscriptions(t *testing.T) {
},
InstallPlanGeneration: 1,
LastUpdated: now,
Conditions: []v1alpha1.SubscriptionCondition{
{
Type: "ResolutionFailed",
Status: corev1.ConditionFalse,
Reason: "",
Message: "",
},
},
},
},
},
Expand Down Expand Up @@ -599,6 +623,14 @@ func TestSyncSubscriptions(t *testing.T) {
},
LastUpdated: now,
InstallPlanGeneration: 1,
Conditions: []v1alpha1.SubscriptionCondition{
{
Type: "ResolutionFailed",
Status: corev1.ConditionFalse,
Reason: "",
Message: "",
},
},
},
},
},
Expand Down Expand Up @@ -775,6 +807,14 @@ func TestSyncSubscriptions(t *testing.T) {
},
LastUpdated: now,
InstallPlanGeneration: 1,
Conditions: []v1alpha1.SubscriptionCondition{
{
Type: "ResolutionFailed",
Status: corev1.ConditionFalse,
Reason: "",
Message: "",
},
},
},
},
},
Expand Down Expand Up @@ -958,6 +998,14 @@ func TestSyncSubscriptions(t *testing.T) {
},
LastUpdated: now,
InstallPlanGeneration: 2,
Conditions: []v1alpha1.SubscriptionCondition{
{
Type: "ResolutionFailed",
Status: corev1.ConditionFalse,
Reason: "",
Message: "",
},
},
},
},
},
Expand Down
37 changes: 32 additions & 5 deletions test/e2e/subscription_e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2112,8 +2112,11 @@ var _ = Describe("Subscription", func() {
teardown func()
cleanup func()
packages []registry.PackageManifest
subName = genName("test-subscription")
catSrcName = genName("test-catalog")
crd = newCRD(genName("foo-"))
csvA operatorsv1alpha1.ClusterServiceVersion
csvB operatorsv1alpha1.ClusterServiceVersion
subName = genName("test-subscription-")
catSrcName = genName("test-catalog-")
)

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

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

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

When("the required API is made available", func() {
BeforeEach(func() {
newPkg := registry.PackageManifest{
PackageName: "PackageB",
Channels: []registry.PackageChannel{
{Name: "alpha", CurrentCSVName: "csvB"},
},
DefaultChannelName: "alpha",
}
packages = append(packages, newPkg)

csvB = newCSV("csvB", testNamespace, "", semver.MustParse("1.0.0"), []apiextensions.CustomResourceDefinition{crd}, nil, nil)

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() {
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 sub.Status.GetCondition(operatorsv1alpha1.SubscriptionResolutionFailed).Status, nil
}).Should(Equal(corev1.ConditionFalse))
})
})
})

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