From 088aaa7a54277ebac5d5f17f250a339224bbdb72 Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Tue, 22 Aug 2023 11:01:20 -0400 Subject: [PATCH] fix: check for unsat resolutions in Reconcile Signed-off-by: Joe Lanford --- internal/controllers/operator_controller.go | 39 +++++++++++++++++++++ test/e2e/install_test.go | 14 ++++---- 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/internal/controllers/operator_controller.go b/internal/controllers/operator_controller.go index a842b8d20..94e25d906 100644 --- a/internal/controllers/operator_controller.go +++ b/internal/controllers/operator_controller.go @@ -18,10 +18,14 @@ package controllers import ( "context" + "errors" "fmt" + "sort" + "strings" "github.com/go-logr/logr" catalogd "github.com/operator-framework/catalogd/api/core/v1alpha1" + "github.com/operator-framework/deppy/pkg/deppy" "github.com/operator-framework/deppy/pkg/deppy/solver" rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1" "k8s.io/apimachinery/pkg/api/equality" @@ -136,6 +140,21 @@ func (r *OperatorReconciler) reconcile(ctx context.Context, op *operatorsv1alpha return ctrl.Result{}, err } + // TODO: Checking for unsat is awkward using the current version of deppy. + // This awkwardness has been fixed in an unreleased version of deppy. + // When there is a new minor release of deppy, we can revisit this and + // simplify this to a normal error check. + // See https://github.com/operator-framework/deppy/issues/139. + unsat := deppy.NotSatisfiable{} + if ok := errors.As(solution.Error(), &unsat); ok && len(unsat) > 0 { + op.Status.InstalledBundleResource = "" + setInstalledStatusConditionUnknown(&op.Status.Conditions, "installation has not been attempted as resolution is unsatisfiable", op.GetGeneration()) + op.Status.ResolvedBundleResource = "" + msg := prettyUnsatMessage(unsat) + setResolvedStatusConditionFailed(&op.Status.Conditions, msg, op.GetGeneration()) + return ctrl.Result{}, unsat + } + // lookup the bundle entity in the solution that corresponds to the // Operator's desired package name. bundleEntity, err := r.getBundleEntityFromSolution(solution, op.Spec.PackageName) @@ -459,3 +478,23 @@ func operatorRequestsForCatalog(ctx context.Context, c client.Reader, logger log return requests } } + +// TODO: This can be removed when operator controller bumps to a +// version of deppy that contains a fix for this issue: +// https://github.com/operator-framework/deppy/issues/142 + +// prettyUnsatMessage ensures that the unsat message is deterministic and +// human-readable. It sorts the individual constraint strings lexicographically +// and joins them with a semicolon (rather than a comma, which the unsat.Error() +// function does). This function also has the side effect of sorting the items +// in the unsat slice. +func prettyUnsatMessage(unsat deppy.NotSatisfiable) string { + sort.Slice(unsat, func(i, j int) bool { + return unsat[i].String() < unsat[j].String() + }) + msgs := make([]string, 0, len(unsat)) + for _, c := range unsat { + msgs = append(msgs, c.String()) + } + return fmt.Sprintf("constraints not satisfiable: %s", strings.Join(msgs, "; ")) +} diff --git a/test/e2e/install_test.go b/test/e2e/install_test.go index a598cc2f8..d135eb7e3 100644 --- a/test/e2e/install_test.go +++ b/test/e2e/install_test.go @@ -6,16 +6,14 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - + catalogd "github.com/operator-framework/catalogd/api/core/v1alpha1" + rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1" "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/rand" - catalogd "github.com/operator-framework/catalogd/api/core/v1alpha1" - rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1" - operatorv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" ) @@ -185,16 +183,16 @@ var _ = Describe("Operator Install", func() { g.Expect(operator.Status.ResolvedBundleResource).ToNot(BeEmpty()) }).Should(Succeed()) - By("updating the Operator resource to an invalid version") - operator.Spec.Version = "0.65.1" // the correct one should be 0.47.0 + By("updating the Operator resource to a non-successor version") + operator.Spec.Version = "0.65.1" // current (0.37.0) and successor (0.47.0) are the only values that would be SAT. Expect(c.Update(ctx, operator)).To(Succeed()) - By("eventually reporting a failed resolution") + By("eventually reporting an unsatisfiable resolution") Eventually(func(g Gomega) { g.Expect(c.Get(ctx, types.NamespacedName{Name: operator.Name}, operator)).To(Succeed()) cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorv1alpha1.TypeResolved) g.Expect(cond).ToNot(BeNil()) g.Expect(cond.Reason).To(Equal(operatorv1alpha1.ReasonResolutionFailed)) - g.Expect(cond.Message).To(ContainSubstring("entity for package \"prometheus\" not found in solutio")) + g.Expect(cond.Message).To(MatchRegexp(`^constraints not satisfiable:.*; installed package prometheus requires at least one of.*0.47.0[^,]*,[^,]*0.37.0[^;]*;.*`)) g.Expect(operator.Status.ResolvedBundleResource).To(BeEmpty()) }).Should(Succeed())