Operator defined install namespace #3862

jhadvig · 2020-01-03T17:52:55Z

Story: https://issues.redhat.com/browse/CONSOLE-1670

If the operator has defined a recommended namespace where it should be installed, it should be shown to the user upon the install process, so user can either honor the operator recommendation or pick an available namespace based on the install mode.

In case the operator is recommending an install namespace, there are also two cases:

AllNamespace install mode, where we default to the recommended namespace

or user can pick openshift-operators namespace

OwnNamespace install mode, where user can either honor the recommended namespace

or pick any available namespace

In case the operator is not recommending an install namespace, there are also two cases:

AllNamespace install mode, where only the openshift-operators namespace will be available in the disabled namespace dropdown

OwnNamespace install mode, where user can pick any available namespace

Worked the design out with @itsptk

Still working on the PR.

/assign @spadgett

@benjaminapetersen fyi

spadgett · 2020-01-03T18:35:47Z

cc @shawn-hurley @bparees

spadgett

I know this is still WIP, but wanted to get you feedback.

spadgett · 2020-01-03T18:42:12Z

...d/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsx

operatorframework.io/suggested-namespace

https://github.com/openshift/enhancements/blob/master/enhancements/olm/olm-managed-operator-metrics.md#fulfilling-namespace-and-rbac-requirements

We should change the var name here since it's not really forced.

spadgett · 2020-01-03T18:45:46Z

...d/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsx

Suggested change

const isOpenshiftNamespace = (ns: string): boolean => {

const isOpenShiftNamespace = (ns: string): boolean => {

spadgett · 2020-01-03T18:49:45Z

...d/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsx

avoid inline styles

I'd call this enableMonitoringCheckbox

spadgett · 2020-01-03T18:49:49Z

...d/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsx

avoid inline styles

spadgett · 2020-01-03T18:52:01Z

...d/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsx

Suggested change

const newNamespace = {

const newNamespace: K8sResourceCommon = {

spadgett · 2020-01-03T18:59:54Z

...d/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsx

I think we need an else here, otherwise we'll just use whatever the previous value was.

spadgett · 2020-01-03T19:02:16Z

...d/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsx

Better to use catch

Suggested change

(err) => setError(err.message),

.catch(({ message = 'Could not create namespace.' }) => setError(message),

spadgett · 2020-01-03T19:02:40Z

...d/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsx

Better to use .then(...).catch(...)

spadgett · 2020-01-03T19:04:11Z

...d/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsx

We should consider converting this to async/await, which would simplify things

...d/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsx

spadgett · 2020-01-03T19:20:59Z

Based on https://github.com/openshift/enhancements/blob/master/enhancements/olm/olm-managed-operator-metrics.md#fulfilling-namespace-and-rbac-requirements, it doesn't sound like we need the checkbox.

The operatorframework.io/cluster-monitoring=true annotation. When this annotation is set to true, the OpenShift Console will update the namespace that the operator is being deployed to with the openshift.io/cluster-monitoring=true label. When this annotation is present, the UI will update the OpenShift Monitoring Prometheus Operator ServiceAccount with the appropriate RBAC privileges for the given namespace as well, allowing operators to be scraped by the OpenShift Monitoring Prometheus Operator.

I'm not sure it should be the console's responsibility to update RBAC for the service account. @shawn-hurley @bparees ?

spadgett · 2020-01-03T20:05:33Z

it doesn't sound like we need the checkbox.

Disregard, I see this is discussed here in openshift/openshift-origin-design#307 (comment)

bparees · 2020-01-03T22:07:51Z

the "own namespace" cases where you are using an existing namespace do not offer the "enable monitoring" checkbox.

I think we should be offering that checkbox for any operator that requests monitoring, regardless of whether you are installing to its chosen namespace, or another one. Though it is a bit tricky in the case that you are picking a namespace that can't be monitored (isn't openshift-*) (you can still label it for monitoring but nothing will happen).

i'd also like @s-urbaniak and @lilic to review the "warning text" that goes with the enable monitoring checkbox. I'm not sure it's sufficiently cautionary in its current form.

itsptk · 2020-01-03T22:31:47Z

Just wanted to mention that the UX design always has a separate radio option for selecting the targetNamespace, regardless if it is being created at this point or not: openshift/openshift-origin-design#307

Just noticed that wasn't what was being conveyed in the initial comment's screenshots, though Jakub and I were working out this design basically simultaneously when this PR was opened.

lilic

Left suggestion for the note.

Just a side note, we should not merge the PR until the e2e tests that were agreed with the OLM team are in place in origin. Not sure there is a PR or work on that for that already @shawn-hurley @awgreene?

lilic · 2020-01-06T12:48:27Z

...d/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsx

As mentioned in the other PR openshift/openshift-origin-design#307 (comment) we really want to warn the user that if they install a non redhat operator into an openshift namespace with enabled monitoring, that this voids user support. We should be clear on that, as that is the potential consequence of this action.

I underline the severity of enabling monitoring in this case too. It voids user support as @lilic mentioned.

I underline the severity of enabling monitoring in this case too. It voids user support

@lilic @s-urbaniak @bparees in that case are we sure we want to give the user and way how to fiddle with the monitoring? Seeing all the comments (here and in the design PR) I wonder if we won't shoot ourselves into the leg with this single checkbox, since the variety of all the use-cases is just and potential damages is just too big.
Thinking if it wont be for the best to leave it out for now and just aim for the suggested-namespace.

Thoughts ?

@lilic @s-urbaniak @bparees in that case are we sure we want to give the user and way how to fiddle with the monitoring?

yes because we want to make the install story for redhat operators better than it currently is. that's the primary purpose of this feature.

benjaminapetersen

/lgtm

openshift-ci-robot · 2020-01-24T02:50:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benjaminapetersen, jhadvig

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/packages/operator-lifecycle-manager/OWNERS~~ [benjaminapetersen,jhadvig]

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

spadgett · 2020-01-24T02:52:38Z

/refresh

openshift-ci-robot assigned spadgett Jan 3, 2020

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 3, 2020

openshift-ci-robot requested review from jeff-phillips-18 and spadgett January 3, 2020 17:53

openshift-ci-robot added component/olm Related to OLM approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 3, 2020

spadgett reviewed Jan 3, 2020

View reviewed changes

spadgett added this to the v4.4 milestone Jan 3, 2020

lilic reviewed Jan 6, 2020

View reviewed changes

tiraboschi mentioned this pull request Jan 22, 2020

change HCO OperatorGroup membership to select only one namespace kubevirt/hyperconverged-cluster-operator#408

Closed

Operator defined install namespace

7f76eac

spadgett force-pushed the 1670-1-ns branch 8 times, most recently from 46e067d to 4d4a0a5 Compare January 24, 2020 01:11

OLM: suggested namespace updates

0e8e683

spadgett force-pushed the 1670-1-ns branch from 4d4a0a5 to 0e8e683 Compare January 24, 2020 01:16

spadgett changed the title ~~[WIP] Operator defined install namespace~~ Operator defined install namespace Jan 24, 2020

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 Jan 24, 2020

benjaminapetersen approved these changes Jan 24, 2020

View reviewed changes

openshift-ci-robot assigned benjaminapetersen Jan 24, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2020

openshift-merge-robot merged commit 21c360b into openshift:master Jan 24, 2020

spadgett mentioned this pull request Jan 24, 2020

OLM: fix for single namespace monitoring checkbox #4061

Merged

	const isOpenshiftNamespace = (ns: string): boolean => {
	const isOpenShiftNamespace = (ns: string): boolean => {

	const newNamespace = {
	const newNamespace: K8sResourceCommon = {

	(err) => setError(err.message),
	.catch(({ message = 'Could not create namespace.' }) => setError(message),

Operator defined install namespace #3862

Operator defined install namespace #3862

Uh oh!

Conversation

jhadvig commented Jan 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spadgett commented Jan 3, 2020

Uh oh!

spadgett left a comment

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!

Uh oh!

spadgett commented Jan 3, 2020

Uh oh!

spadgett commented Jan 3, 2020

Uh oh!

bparees commented Jan 3, 2020

Uh oh!

itsptk commented Jan 3, 2020

Uh oh!

lilic left a comment

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!

benjaminapetersen left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Jan 24, 2020

Uh oh!

spadgett commented Jan 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jhadvig commented Jan 3, 2020 •

edited

Loading