Allow users to toggle off csv copying for AllNamespace mode operators #2466

awgreene · 2021-11-19T18:14:55Z

No description provided.

openshift-ci · 2021-11-19T18:14:56Z

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

pkg/controller/operators/olm/config.go

tylerslaton

Looks great! Just a couple of nits.

pkg/controller/operators/olm/operator.go

cmd/olm/main.go

pkg/controller/operators/olm/operator.go

awgreene · 2021-11-29T21:43:53Z

Relies on operator-framework/api#198

pkg/controller/operators/olm/operator.go

awgreene · 2021-11-30T21:50:37Z

pkg/controller/operators/olm/operator.go

+		condition.Message = "At least one CSV had an unexpected number of copied CSVs"
+		condition.Status = metav1.ConditionFalse
+		defer func() {
+			a.olmConfigQueue.AddAfter(olmConfig, time.Second*5)


The AddRateLimited option was not correctly incrementing the number of failures and would spam the API server. Planning to look at this tomorrow.

Could also be done by triggering a watch on copied csvs.

We need a watch/sync on copied CSVs if, for some reason, a copied csv gets created after the feature has been disabled; an edge case that might happen on upgrade or due to a bad actor (e.g. me).

or due to a bad actor (e.g. me).

I don't know why your stage performance should influence this PR, but I agree that this controller should watch/sync on copied csvs ;)

pkg/controller/operators/olm/operator.go

go.mod

njhale · 2021-12-08T21:07:05Z

deploy/chart/crds/0000_50_olm_00-olmconfigs.crd.yaml

+                conditions:
+                  type: array
+                  items:
+                    description: "Condition contains details for one aspect of the current state of this API Resource. --- This struct is intended for direct use as an array at the field path .status.conditions.  For example, type FooStatus struct{     // Represents the observations of a foo's current state.     // Known .status.conditions.type are: \"Available\", \"Progressing\", and \"Degraded\"     // +patchMergeKey=type     // +patchStrategy=merge     // +listType=map     // +listMapKey=type     Conditions []metav1.Condition `json:\"conditions,omitempty\" patchStrategy:\"merge\" patchMergeKey:\"type\" protobuf:\"bytes,1,rep,name=conditions\"` \n     // other fields }"


Looks like the boilerplate description made it through code review. Let's override the Foo example when we get the chance.

Is there a way to override the description of a structure not owned by your package?

go.mod

pkg/controller/operators/olm/operator.go

njhale · 2021-12-08T21:58:41Z

pkg/controller/operators/olm/operator.go

+		condition.Message = "At least one CSV had an unexpected number of copied CSVs"
+		condition.Status = metav1.ConditionFalse
+		defer func() {
+			a.olmConfigQueue.AddAfter(olmConfig, time.Second*5)


We need a watch/sync on copied CSVs if, for some reason, a copied csv gets created after the feature has been disabled; an edge case that might happen on upgrade or due to a bad actor (e.g. me).

pkg/controller/operators/olm/operator.go

njhale

/lgtm

njhale · 2021-12-14T18:55:01Z

test/e2e/csv_e2e_test.go

+
+				olmConfig.Spec = operatorsv1.OLMConfigSpec{
+					Features: &operatorsv1.Features{
+						DisableCopiedCSVs: getPointer(true),


If the number of occurrences is few, you could also invoke an anonymous function:

DisableCopiedCSVs: func() *bool { b := true return &b }(),

openshift-ci · 2021-12-14T18:55:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, 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~~ [awgreene,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/olm/operator.go

test/e2e/csv_e2e_test.go

tylerslaton · 2021-12-14T20:06:33Z

/lgtm

test/e2e/csv_e2e_test.go

tylerslaton · 2021-12-14T20:29:53Z

/lgtm

This commit introduces a controller for the olmConfig CRD. The olmConfig CRD will be used to configure olm's behavior on cluster. As of today, this CRD introduces the ability for customer to disable copied csvs for operators installed in allNamespace mode. When copied csv are disabled, an event will be created in the operators namespace signaling that it has no copied csvs and that users on the cluster may have difficulty identifying which operators are available in a given namespace. Signed-off-by: Alexander Greene <[email protected]>

tylerslaton · 2021-12-14T21:38:57Z

/lgtm

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 19, 2021

openshift-ci bot requested review from ankitathomas and kevinrizza November 19, 2021 18:15

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 19, 2021

perdasilva reviewed Nov 23, 2021

View reviewed changes

pkg/controller/operators/olm/config.go Outdated Show resolved Hide resolved

perdasilva reviewed Nov 23, 2021

View reviewed changes

pkg/controller/operators/olm/config.go Outdated Show resolved Hide resolved

tylerslaton reviewed Nov 23, 2021

View reviewed changes

awgreene force-pushed the copied-csv-toggle branch from 50c5709 to d7f6a83 Compare November 24, 2021 00:31

timflannagan reviewed Nov 29, 2021

View reviewed changes

cmd/olm/main.go Outdated Show resolved Hide resolved

timflannagan reviewed Nov 29, 2021

View reviewed changes

pkg/controller/operators/olm/operator.go Outdated Show resolved Hide resolved

timflannagan reviewed Nov 29, 2021

View reviewed changes

pkg/controller/operators/olm/operator.go Outdated Show resolved Hide resolved

timflannagan reviewed Nov 29, 2021

View reviewed changes

pkg/controller/operators/olm/operator.go Outdated Show resolved Hide resolved

timflannagan changed the title ~~WIP: Allow users to toggel off csv copying for cluster scoped operators~~ WIP: Allow users to toggle off csv copying for cluster scoped operators Nov 29, 2021

awgreene commented Nov 29, 2021

View reviewed changes

pkg/controller/operators/olm/operator.go Outdated Show resolved Hide resolved

awgreene mentioned this pull request Nov 29, 2021

Toggle Copied CSVs operator-framework/enhancements#92

Merged

awgreene force-pushed the copied-csv-toggle branch from ba40d6d to 888b9f6 Compare November 30, 2021 16:41

awgreene commented Nov 30, 2021

View reviewed changes

pkg/controller/operators/olm/operator.go Outdated Show resolved Hide resolved

timflannagan reviewed Nov 30, 2021

View reviewed changes

pkg/controller/operators/olm/operator.go Outdated Show resolved Hide resolved

timflannagan reviewed Nov 30, 2021

View reviewed changes

pkg/controller/operators/olm/operator.go Outdated Show resolved Hide resolved

timflannagan reviewed Nov 30, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

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

awgreene force-pushed the copied-csv-toggle branch from 994fa1d to 8887105 Compare December 1, 2021 18:32

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

awgreene force-pushed the copied-csv-toggle branch from cf0126a to 98a066b Compare December 2, 2021 18:54

awgreene mentioned this pull request Dec 8, 2021

WIP: Introduce olm config (Revert CR Bump in API Repo) #2513

Closed

njhale requested changes Dec 8, 2021

View reviewed changes

awgreene force-pushed the copied-csv-toggle branch from 34850b6 to b9b3170 Compare December 10, 2021 15:15

awgreene force-pushed the copied-csv-toggle branch 2 times, most recently from 2fa650f to 6192ee0 Compare December 14, 2021 18:17

awgreene changed the title ~~Allow users to toggle off csv copying for cluster scoped operators~~ Allow users to toggle off csv copying for AllNamespace mode operators Dec 14, 2021

njhale approved these changes Dec 14, 2021

View reviewed changes

openshift-ci bot assigned njhale Dec 14, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021

awgreene force-pushed the copied-csv-toggle branch from 6192ee0 to 3b10eee Compare December 14, 2021 19:26

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021

awgreene force-pushed the copied-csv-toggle branch 3 times, most recently from 1cf4355 to f7876cf Compare December 14, 2021 19:29

tylerslaton reviewed Dec 14, 2021

View reviewed changes

pkg/controller/operators/olm/operator.go Show resolved Hide resolved

tylerslaton reviewed Dec 14, 2021

View reviewed changes

test/e2e/csv_e2e_test.go Show resolved Hide resolved

tylerslaton reviewed Dec 14, 2021

View reviewed changes

test/e2e/csv_e2e_test.go Show resolved Hide resolved

openshift-ci bot assigned tylerslaton Dec 14, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021

tylerslaton reviewed Dec 14, 2021

View reviewed changes

test/e2e/csv_e2e_test.go Show resolved Hide resolved

awgreene force-pushed the copied-csv-toggle branch from f7876cf to 332b6cc Compare December 14, 2021 20:28

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021

awgreene force-pushed the copied-csv-toggle branch from 332b6cc to 7dd1541 Compare December 14, 2021 20:43

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021

awgreene force-pushed the copied-csv-toggle branch from 7dd1541 to c0b6e95 Compare December 14, 2021 20:47

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021

openshift-merge-robot merged commit 52f368d into operator-framework:master Dec 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users to toggle off csv copying for AllNamespace mode operators #2466

Allow users to toggle off csv copying for AllNamespace mode operators #2466

awgreene commented Nov 19, 2021

openshift-ci bot commented Nov 19, 2021

tylerslaton left a comment

awgreene commented Nov 29, 2021

awgreene Nov 30, 2021 •

edited

Loading

awgreene Nov 30, 2021

njhale Dec 8, 2021

awgreene Dec 13, 2021 •

edited

Loading

njhale Dec 8, 2021

awgreene Dec 9, 2021

njhale Dec 8, 2021

njhale left a comment

njhale Dec 14, 2021

openshift-ci bot commented Dec 14, 2021

tylerslaton commented Dec 14, 2021

tylerslaton commented Dec 14, 2021

tylerslaton commented Dec 14, 2021

Allow users to toggle off csv copying for AllNamespace mode operators #2466

Allow users to toggle off csv copying for AllNamespace mode operators #2466

Conversation

awgreene commented Nov 19, 2021

openshift-ci bot commented Nov 19, 2021

tylerslaton left a comment

Choose a reason for hiding this comment

awgreene commented Nov 29, 2021

awgreene Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

awgreene Nov 30, 2021

Choose a reason for hiding this comment

njhale Dec 8, 2021

Choose a reason for hiding this comment

awgreene Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

njhale Dec 8, 2021

Choose a reason for hiding this comment

awgreene Dec 9, 2021

Choose a reason for hiding this comment

njhale Dec 8, 2021

Choose a reason for hiding this comment

njhale left a comment

Choose a reason for hiding this comment

njhale Dec 14, 2021

Choose a reason for hiding this comment

openshift-ci bot commented Dec 14, 2021

tylerslaton commented Dec 14, 2021

tylerslaton commented Dec 14, 2021

tylerslaton commented Dec 14, 2021

awgreene Nov 30, 2021 •

edited

Loading

awgreene Dec 13, 2021 •

edited

Loading