Block OpenShift upgrades while incompatible operators are installed #2081

njhale · 2021-04-06T09:56:52Z

Description of the change:

Set OLM's ClusterOperator resource to Upgradeable=false when any OLM managed operator on the cluster has a maxOpenShiftVersion property less than the next minor OCP release.

Implements OLM-2074.

I decided to implement this as a new controller so we could more easily interleave new and preexisting ClusterOperator behavior.

openshift-ci-robot · 2021-04-06T09:56:53Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2021-04-06T09:56:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: njhale

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [njhale]

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

pkg/controller/operators/openshift/clusteroperator_controller.go

benluddy · 2021-04-13T18:03:29Z

pkg/controller/operators/openshift/clusteroperator_controller.go

+	*ReconcilerConfig
+
+	delayRequeue reconcile.Result
+	mutate       MutateFunc


Suggested change

mutate MutateFunc

mutate Mutator

benluddy · 2021-04-13T18:08:13Z

pkg/controller/operators/openshift/helpers.go

+type skews []skew
+
+func (s skews) String() string {
+	// TODO: Use a string builder.


(or https://golang.org/pkg/strings/#Join)

that works too.

benluddy · 2021-04-13T19:19:59Z

pkg/controller/operators/openshift/helpers.go

+		return nil, err
+	}
+
+	// Take the highest semver if there's more than one max version specified


Would it be a safer choice to go with the lowest version?

I was going at this from the perspective that folks would always make a conscious decision to add the property, but over several releases may forget to remove older ones.

benluddy · 2021-04-13T19:24:15Z

pkg/controller/operators/openshift/helpers.go

+		return nil, err
+	}
+
+	// TODO: Is there a better way to filter (server-side maybe)?


There should be an olm.copiedFrom label on copied CSVs. I've had this thought too, and I'm not sure why there are no examples of a CSV label selector referencing it.

I added a label selector.

benluddy · 2021-04-13T19:40:21Z

pkg/controller/operators/openshift/clusteroperator_controller.go

+		LastTransitionTime: r.Now(),
+	}
+
+	if r.syncTracker.SuccessfulSyncs() > 0 && reflect.DeepEqual(co.Status.Versions, r.TargetVersions) {


This seems fine, but do we need to lean on reflection? Since OperandVersion is struct{Name,Version string} it would be a tiny loop.

we can do an ElementsMatch type thing easily enough.

I went ahead and made this change.

- Refactor OpenShift ClusterOperator monitoring for olm-operator into an independent controller - Set the OLM operator's ClusterOperator to upgradeable=false whenever incompatible operators, installed by OLM, exist on cluster; effectively blocking OpenShift upgrades Signed-off-by: Nick Hale <[email protected]>

Bump controller-runtime dependency to include a fix for the Channel Source which was preventing events from being enqueued. (see kubernetes-sigs/controller-runtime#1343) Signed-off-by: Nick Hale <[email protected]>

Signed-off-by: Nick Hale <[email protected]>

exdx

one nit
/lgtm
/hold

pkg/controller/operators/openshift/helpers.go

exdx · 2021-04-15T20:52:52Z

/hold cancel

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

openshift-ci-robot requested review from anik120 and exdx April 6, 2021 09:56

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 6, 2021

njhale changed the title ~~wip: openshift compat~~ Block OpenShift upgrades while incompatible operators are installed Apr 6, 2021

njhale force-pushed the openshift/block-upgrades branch 12 times, most recently from e663cae to bc7b3f0 Compare April 12, 2021 06:46

njhale marked this pull request as ready for review April 12, 2021 06:46

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 12, 2021

njhale force-pushed the openshift/block-upgrades branch 9 times, most recently from 7fe816e to 99531be Compare April 13, 2021 13:42

njhale force-pushed the openshift/block-upgrades branch from 99531be to 2106b69 Compare April 13, 2021 13:52

exdx reviewed Apr 13, 2021

View reviewed changes

pkg/controller/operators/openshift/clusteroperator_controller.go Show resolved Hide resolved

benluddy reviewed Apr 13, 2021

View reviewed changes

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 15, 2021

njhale added 6 commits April 15, 2021 14:49

chore(modules): bump controller-runtime

f7e9584

Bump controller-runtime dependency to include a fix for the Channel Source which was preventing events from being enqueued. (see kubernetes-sigs/controller-runtime#1343) Signed-off-by: Nick Hale <[email protected]>

test(openshift): use local timezone for timestamps

484e0c7

Signed-off-by: Nick Hale <[email protected]>

refactor(openshift): don't rely on reflection as version predicate

f67451e

Signed-off-by: Nick Hale <[email protected]>

refactor(openshift): build skew message more efficiently

eb39d10

Signed-off-by: Nick Hale <[email protected]>

refactor(openshift): use mutator interface directly

54b66ca

Signed-off-by: Nick Hale <[email protected]>

njhale force-pushed the openshift/block-upgrades branch from bea36e4 to 8d0e190 Compare April 15, 2021 19:26

refactor(openshift): use a label selector to filter copied csvs

ecac3be

Signed-off-by: Nick Hale <[email protected]>

njhale force-pushed the openshift/block-upgrades branch from 8d0e190 to ecac3be Compare April 15, 2021 19:28

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 15, 2021

exdx reviewed Apr 15, 2021

View reviewed changes

pkg/controller/operators/openshift/helpers.go Show resolved Hide resolved

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 15, 2021

openshift-ci-robot assigned exdx Apr 15, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2021

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 15, 2021

openshift-merge-robot merged commit 15790a8 into operator-framework:master Apr 15, 2021

njhale mentioned this pull request Apr 16, 2021

OLM blocks minor OpenShift upgrades when incompatible optional operators are installed openshift/operator-framework-olm#57

Closed

njhale mentioned this pull request Jul 14, 2021

Bug 1979525: OLM blocks minor OpenShift upgrades when incompatible optional operators are installed openshift/operator-framework-olm#120

Merged

njhale deleted the openshift/block-upgrades branch July 23, 2021 02:45

Block OpenShift upgrades while incompatible operators are installed #2081

Block OpenShift upgrades while incompatible operators are installed #2081

Uh oh!

Conversation

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

Uh oh!

Uh oh!

openshift-ci-robot commented Apr 6, 2021

Uh oh!

openshift-ci-robot commented Apr 6, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exdx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

exdx commented Apr 15, 2021

Uh oh!

Uh oh!

njhale commented Apr 6, 2021 •

edited

Loading