Resolution error message is not deterministic

[m1kola]

I noticed that we iterate over maps here:

operator-controller/internal/resolution/variablesources/crd_constraints.go

Lines 66 to 73 in e001a0f

	for packageName, bundleIDMap := range pkgToBundleMap {
	var bundleIDs []deppy.Identifier
	for bundleID := range bundleIDMap {
	bundleIDs = append(bundleIDs, bundleID)
	}
	varID := deppy.IdentifierFromString(fmt.Sprintf("%s package uniqueness", packageName))
	variables = append(variables, olmvariables.NewBundleUniquenessVariable(varID, bundleIDs...))
	}

I believe this results in randomised order of variables.

Tests do not flag this because they use ConsistOf which ignores order:

operator-controller/internal/resolution/variablesources/crd_constraints_test.go

Lines 285 to 294 in e001a0f

	// Note: As above, the 5 GVKs would appear here as GVK uniqueness constraints
	// if GVK Uniqueness were being accounted for.
	Expect(crdConstraintVariables).To(WithTransform(CollectGlobalConstraintVariableIDs, ConsistOf([]string{
	"another-package package uniqueness",
	"bar-package package uniqueness",
	"test-package-2 package uniqueness",
	"test-package package uniqueness",
	"some-package package uniqueness",
	"some-other-package package uniqueness",
	})))

It likely contributes to what was reported in operator-framework/deppy#142:

It turns out the assertion is flaky because the content of the message changes from run to run, only in terms of the order in which the individual constraint messages appear. This is problematic when this error message is written to a kubernetes status object because it results in a reconcile -> update status -> reconcile -> update status loop until we get "lucky" and deppy returns the same message two reconciles in a row.

[m1kola]

#461 gets rid of randomness in package uniqueness ordering.

I would still like to keep this issue open so we can get rid of this function:

operator-controller/internal/controllers/operator_controller.go

Lines 467 to 485 in e001a0f

	// 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, "; "))
	}

This will require:

• Change to error formating on deppy side (replace comma with semicolon)
• Additional tests for OperatorReconciler to make sure that ordering is stable

[m1kola]

This will require:

• Change to error formating on deppy side (replace comma with semicolon)
• Additional tests for OperatorReconciler to make sure that ordering is stable

We updated separators in operator-framework/deppy#158. There is operator-framework/deppy#159 now open to deal with non deterministic output from Deppy solver.

Once we have a new release of Deppy which contains operator-framework/deppy#159, remaining steps on operator-controller side will be:

1. Update deppy
2. Remove prettyUnsatMessage function