[WIP] Add cluster runtime constraint handling #2498

perdasilva · 2021-11-25T17:15:13Z

Signed-off-by: Per G. da Silva [email protected]

Description of the change:
This PR introduces a backdoor way adding cluster runtime constraints to the resolver. The mechanism will look for a yaml file containing olm.constraints that will be parsed into the cache.Predicates and considered during the resolution.

Current Resolution Integration Approach
I've come up with a couple of strategies:

Apply a global filter to the cache (I've added code for this but it's commented out)
Set any installable violating any runtime constraint to prohibited in the addInvariants method.

Option 2. seems the most reasonable since 1. adds a bit of hidden behaviour to the cache and it mightn't always be clear what is being applied and when.

Motivation for the change:
OLM will needs to support cluster runtime contraints. We are currently working on a fully fledged solution but it won't be ready in time to unblock our users. This approach gives downstreams a way to side-load runtime constraints in the mean time.

Reviewer Checklist

Implementation matches the proposed design, or proposal is updated to match implementation
Sufficient unit test coverage
Sufficient end-to-end test coverage
Docs updated or added to /doc
Commit messages sensible and descriptive

openshift-ci · 2021-11-25T17:15:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: perdasilva
To complete the pull request process, please assign dinhxuanvu after the PR has been reviewed.
You can assign the PR to them by writing /assign @dinhxuanvu in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dinhxuanvu

Looking promising.
I have a few comments. Plus, there is a bundles.json file and not sure what is it for?

pkg/controller/registry/reconciler/reconciler.go

pkg/controller/registry/resolver/resolver.go

dinhxuanvu · 2021-11-30T04:07:33Z

pkg/controller/registry/resolver/runtime_constraints/provider.go

+	}
+
+	// Parse yaml
+	var propertiesFile = &registry.PropertiesFile{}


We need to decide if this property or a dependency so we can use either PropertiesFile or DependenciesFile. Honestly, it is just wording but it is a decision to be made for syntax sake.

pkg/controller/registry/resolver/runtime_constraints/provider.go

dinhxuanvu · 2021-11-30T04:28:33Z

pkg/controller/registry/resolver/runtime_constraints/provider.go

+
+func readRuntimeConstraintsYaml(yamlPath string) (*registry.PropertiesFile, error) {
+	// Read file
+	yamlFile, err := ioutil.ReadFile(yamlPath)


Did you test how this file-reading code will work in real life? How is this file gonna be "injected" into the process?

I've tested it manually. I still need to add unit tests for this. I'm thinking the way to side-load it should be we have an environment variable that points to the path of the constraints file. Downstream, the file will be baked in to the olm operator image and we'll update the deployment to include the env var. This way, if people want to turn it off, it's doable. I'm not sure how this strategy fits with support, etc. E.g. if they turn it off would that mean the customer is in an unmanaged state?

if they turn it off would that mean the customer is in an unmanaged state?

it does

pkg/controller/registry/resolver/cache/predicates.go

dinhxuanvu · 2021-11-30T04:32:25Z

pkg/controller/registry/resolver/runtime_constraints/provider.go

+)
+
+const (
+	maxRuntimeConstraints = 10


Reasonable restriction to avoid abusing. Make sure this number is known and noticed upon the completion of this work.

perdasilva · 2021-11-30T19:07:56Z

Looking promising. I have a few comments. Plus, there is a bundles.json file and not sure what is it for?

Thank you ^^ I've removed the bundles.json. That was a mistake.

njhale

So this is very similar to an injection method we rejected a while back: https://github.com/operator-framework/operator-lifecycle-manager/pull/2360/files

OLM will needs to support cluster runtime contraints. We are currently working on a fully fledged solution but it won't be ready in time to unblock our users. This approach gives downstreams a way to side-load runtime constraints in the mean time.

Which users are blocked without a method to side load global constraints? If we're talking about OpenShift users and the MaxOpenShiftVersion property, then I'd like to avoid adding new upstream APIs for them to be immediately superseded. We can carry a patch downstream that injects such specific constraints. In fact, I have some prior art there: openshift/operator-framework-olm#194

pkg/controller/registry/resolver/cache/cache.go

pkg/controller/registry/resolver/resolver.go

njhale · 2021-12-03T17:55:01Z

pkg/controller/registry/resolver/resolver.go

@@ -20,19 +22,39 @@ import (
 	opregistry "github.com/operator-framework/operator-registry/pkg/registry"
 )

+const (
+	runtimeConstraintsFilePath = "runtime_constraints.json"


Seems like something that could be part of the OLMConfig CR instead of a file.

I think this could be much nicer. I just don't know what it means for the longer term vision of deppy and olm, etc. If this is still the way we'll want to handle these types of constraints. My thinking right now is to keep the "under-the-covers" way of doing it (baking this file in the downstream operator images) because it's super easy to roll-back and it buys us time to be more deliberate about how we want to handle these constraints in a first-class way. @joelanford @kevinrizza wdyt?

njhale · 2021-12-03T17:57:22Z

pkg/controller/registry/resolver/resolver.go

 		log:   logger,
+		// #2: apply constraints in addInvariants
+		runtimeConstraintsProvider: runtimeConstraintProvider, // uncomment when using option #2


it might require more refactoring, but we probably want to inject these dependencies.

njhale · 2021-12-03T18:00:52Z

pkg/controller/registry/resolver/runtime_constraints/provider.go

+
+func readRuntimeConstraintsYaml(yamlPath string) (*registry.PropertiesFile, error) {
+	// Read file
+	yamlFile, err := ioutil.ReadFile(yamlPath)


if they turn it off would that mean the customer is in an unmanaged state?

it does

pkg/controller/registry/resolver/resolver_test.go

pkg/controller/registry/resolver/resolver.go

perdasilva · 2021-12-03T21:37:29Z

OLM will needs to support cluster runtime contraints. We are currently working on a fully fledged solution but it won't be ready in time to unblock our users. This approach gives downstreams a way to side-load runtime constraints in the mean time.

Which users are blocked without a method to side load global constraints? If we're talking about OpenShift users and the MaxOpenShiftVersion property, then I'd like to avoid adding new upstream APIs for them to be immediately superseded. We can carry a patch downstream that injects such specific constraints. In fact, I have some prior art there: openshift/operator-framework-olm#194

Yeah, the main targets at the moment are OpenShift and IBM. I don't think we will be targetting anything other than OCP version. But, personally, I don't think we are making too many throw away changes. I think being able to inject these constraints (somehow) and have them understood by the resolver already adds value to the upstream, even if we don't offer a nice interface (yet) to define those. It also puts it in the hand of the community to test, evaluate, use our way of side-loading, come up with their own, etc. So, I think I'd push back a bit on the idea of a downstream (code) patch atm.

Signed-off-by: Per G. da Silva <[email protected]>

perdasilva · 2021-12-13T02:56:34Z

closing in favor of #2523

openshift-ci bot requested review from ecordell and kevinrizza November 25, 2021 17:15

perdasilva force-pushed the cluster-constraints branch 4 times, most recently from 6a7747e to 3eaa726 Compare November 26, 2021 20:03

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 29, 2021

dinhxuanvu reviewed Nov 30, 2021

View reviewed changes

perdasilva force-pushed the cluster-constraints branch from 3eaa726 to 39c27e5 Compare November 30, 2021 19:07

perdasilva force-pushed the cluster-constraints branch from 39c27e5 to 3109383 Compare November 30, 2021 19:10

perdasilva mentioned this pull request Dec 2, 2021

[WIP] Add cluster runtime constraints handling openshift/operator-framework-olm#220

Closed

perdasilva force-pushed the cluster-constraints branch 2 times, most recently from ec632fc to 4ababe0 Compare December 2, 2021 23:08

njhale reviewed Dec 3, 2021

View reviewed changes

perdasilva force-pushed the cluster-constraints branch 2 times, most recently from f19c3a2 to 67745af Compare December 7, 2021 00:08

perdasilva added 11 commits December 6, 2021 16:09

Add cluster constraints handling

bb47c5f

Signed-off-by: Per G. da Silva <[email protected]>

Add second implementation option

6b876a0

Signed-off-by: Per G. da Silva <[email protected]>

Add e2e test

73a56dc

Signed-off-by: Per G. da Silva <[email protected]>

Add e2e test setup

de8afda

Signed-off-by: Per G. da Silva <[email protected]>

Add resolver creation unit tests

49ff255

Signed-off-by: Per G. da Silva <[email protected]>

Fix lint issues

ce65ef2

Signed-off-by: Per G. da Silva <[email protected]>

Add missing resource file

c9cd2a7

Signed-off-by: Per G. da Silva <[email protected]>

Fix resource file names

6e0465e

Signed-off-by: Per G. da Silva <[email protected]>

Fix e2e test harness

3576e84

Signed-off-by: Per G. da Silva <[email protected]>

Add provider unit tests and small refactor

ac67d37

Signed-off-by: Per G. da Silva <[email protected]>

Removes global filters option

6324417

perdasilva added 2 commits December 6, 2021 16:09

Refactor unit tests and remove not predicate

13d3679

Signed-off-by: Per G. da Silva <[email protected]>

Refactor SatResolver constructor

8bf42c5

Signed-off-by: Per G. da Silva <[email protected]>

perdasilva force-pushed the cluster-constraints branch from 67745af to 8bf42c5 Compare December 7, 2021 00:09

perdasilva closed this Dec 13, 2021

[WIP] Add cluster runtime constraint handling #2498

[WIP] Add cluster runtime constraint handling #2498

Uh oh!

Conversation

perdasilva commented Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Nov 25, 2021

Uh oh!

dinhxuanvu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dinhxuanvu Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dinhxuanvu Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

perdasilva Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njhale Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dinhxuanvu Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

perdasilva commented Nov 30, 2021

Uh oh!

njhale left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

njhale Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

perdasilva Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

njhale Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

njhale Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

perdasilva commented Dec 3, 2021

Uh oh!

perdasilva commented Dec 13, 2021

Uh oh!

Uh oh!

perdasilva commented Nov 25, 2021 •

edited

Loading

perdasilva Nov 30, 2021 •

edited

Loading

njhale Dec 3, 2021 •

edited

Loading

njhale Dec 3, 2021 •

edited

Loading