docs(operatorgroups): add more extensive docs #721

njhale · 2019-02-20T22:04:28Z

Document OperatorGroup purpose and behavior in high fidelity.

Documentation/design/operatorgroups.md

ecordell

This is great! There were a couple of small suggestions, but I love how detailed this is. We should write more docs like this.

ecordell · 2019-02-21T13:16:12Z

Documentation/design/operatorgroups.md

+  * Contains the namespace of the `OperatorGroup`
+* `olm.targetNamespaces=<target-namespaces>`
+  * Contains a comma-delimited string listing the `OperatorGroup`'s target namespace selection. This annotation is projected onto the pod template of a CSV's deployments and can be consumed by a pod instance via [The Downward API](https://kubernetes.io/docs/tasks/inject-data-application/downward-api-volume-expose-pod-information/#the-downward-api)
+* If the CSV does not [have a Copied status reason](#copied-csvs)


I think this is missing the end of the sentence?

ecordell · 2019-02-21T13:17:26Z

Documentation/design/operatorgroups.md

+* `OwnNamespace`: If supported, the set of namespaces targetted by an `OperatorGroup` must contain the namespace the Operator is to be installed in.
+* `SingleNamespace`: If supported, the set of namespaces targetted by an `OperatorGroup` can be of length 1.
+* `MultiNamespace`: If supported, the set of namespaces targetted by an `OperatorGroup` can be of length >= 1. Any Operator supporting `MultiNamespace` implicitly supports `SingleNamespace` as well.
+* `AllNamespaces`: If supported, the Operator can support an `OperatorGroup` that selects all namespaces with a targetted set = [""]. Any Operator supporting `AllNamespaces` implicity supports `OwnNamespace` as well. 


I don’t think we would install an operator with ‘AllNamespaces’ into a group watching one namespaces, so I think that this extra comment makes sense

This just means that any CSV supporting AllNamespaces must also support OwnNamespace - For reference OwnNamespace doesn't imply SingleNamespace.

But can't I write an operator that starts up and watches all namespaces, but can't be deployed in a configuration that watches only its own namespace?

In the current implementation, OwnNamespace is an additional restriction on an OperatorGroup’s namespace set. More specifically, it means that a CSV can be a member of an OperatorGroup whose target namespace set includes the CSV’s namespace. This rule applies to Single, Multi, and All, rather than just applying to Single (see https://github.com/operator-framework/operator-lifecycle-manager/blob/master/pkg/api/apis/operators/v1alpha1/clusterserviceversion.go#L99).

We can change this logic to match your definition (It makes sense to) - but I think a change like that warrants a follow up PR.

ecordell · 2019-02-21T13:18:41Z

Documentation/design/operatorgroups.md

+  namespaces:
+  - local
+```
+


I think we should introduce the Copied status and the static provided apis around here; they’re referenced frequently in the intersection section.

It's been updated to reflect your comments - forgot to push this morning.

njhale · 2019-02-22T01:08:19Z

/retest

njhale · 2019-02-22T13:56:37Z

/retest

njhale · 2019-02-23T12:30:45Z

/retest

njhale · 2019-02-24T06:28:09Z

/retest

Documentation/design/developing.md

alecmerdler

Awesome docs, Nick. I finally put my Technical Writing 101 skills to use in this review.

Documentation/design/operatorgroups.md

alecmerdler · 2019-02-25T17:55:24Z

Documentation/design/operatorgroups.md

+
+* An `OperatorGroup` selects a set of target namespaces in which to generate required RBAC access for its member operators.
+* The set of target namespaces is provided via a comma-delimited string stored in the `olm.targetNamespaces` annotation. This annotation is applied to member operator's deployments and is accessible through [The Downward API](https://kubernetes.io/docs/tasks/inject-data-application/downward-api-volume-expose-pod-information/#the-downward-api)
+* An operator is said to be a [member of an `OperatorGroup`](#operatorgroup-membership) if its CSV exists in the same namespace as the `OperatorGroup` and its CSV's [`InstallModes` support the set of namespaces targetted by the `OperatorGroup`](#installmodes-and-supported-operatorgroups)


Could be better to split out InstallModes to a separate bullet (or remove from "OperatorGroup Overview") to simplify what it means to belong to an OperatorGroup.

Besides being in the same namespace, InstallModes are the primary factor in determining membership. I think it makes sense for it to be in the overview. InstallModes are tightly coupled to membership so I feel that putting it in a separate bullet will dilute that association.
The link serves as a pointer to the definition since I don't want to leave people wondering or ctrl-fing.

Documentation/design/operatorgroups.md

njhale · 2019-02-27T02:49:53Z

/test e2e-aws

SamiSousa

Overall this looks great! Glad to see more docs on OperatorGroups and InstallModes

SamiSousa · 2019-02-20T22:45:44Z

Documentation/design/operatorgroups.md

+metadata:
+  name: my-group
+  namespace: my-namespace
+  targetNamespaces:


spec: targetNamespaces:

SamiSousa · 2019-02-27T19:45:07Z

Documentation/design/operatorgroups.md

+* `OwnNamespace`: If supported, the set of namespaces targetted by an `OperatorGroup` must contain the namespace the Operator is to be installed in.
+* `SingleNamespace`: If supported, the set of namespaces targetted by an `OperatorGroup` can be of length 1.
+* `MultiNamespace`: If supported, the set of namespaces targetted by an `OperatorGroup` can be of length >= 1. Any Operator supporting `MultiNamespace` implicitly supports `SingleNamespace` as well.
+* `AllNamespaces`: If supported, the set of namespaces targetted by an `OperatorGroup` can be = [""] (which tells operators to watch all namespaces). Any Operator supporting `AllNamespaces` implicity supports `OwnNamespace` as well. 


Does this mean that OwnNamespace must be set to true if AllNamespaces is supported?

That is the current behavior, but not the desired. I'm updating the docs to reflect our desired behavior, in which case OwnNamespace does not need to be set to true if AllNamespaces is supported.

A follow up PR that changes the behavior in OLM is on the way.

Comments addressed

ecordell · 2019-02-27T20:31:14Z

/lgtm

ecordell · 2019-02-27T20:39:15Z

/lgtm

openshift-ci-robot · 2019-02-27T20:39:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell, 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~~ [ecordell,njhale]

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

njhale · 2019-02-27T22:36:58Z

/retest

docs(operatorgroups): add more extensive docs

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 Feb 20, 2019

openshift-ci-robot requested review from alecmerdler and jpeeler February 20, 2019 22:04

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 20, 2019

dinhxuanvu reviewed Feb 20, 2019

View reviewed changes

Documentation/design/operatorgroups.md Outdated Show resolved Hide resolved

Documentation/design/operatorgroups.md Outdated Show resolved Hide resolved

njhale force-pushed the operatorgroup-docs branch from 1d134f6 to 38edf47 Compare February 21, 2019 12:15

njhale changed the title ~~[WIP] docs(operatorgroups): add more extensive docs~~ docs(operatorgroups): add more extensive docs Feb 21, 2019

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 Feb 21, 2019

ecordell reviewed Feb 21, 2019

View reviewed changes

njhale force-pushed the operatorgroup-docs branch from 38edf47 to 8d63163 Compare February 21, 2019 19:09

alecmerdler reviewed Feb 25, 2019

View reviewed changes

Documentation/design/developing.md Outdated Show resolved Hide resolved

alecmerdler previously requested changes Feb 25, 2019

View reviewed changes

njhale force-pushed the operatorgroup-docs branch from 8d63163 to b4b4a72 Compare February 26, 2019 16:03

SamiSousa suggested changes Feb 27, 2019

View reviewed changes

njhale force-pushed the operatorgroup-docs branch from b4b4a72 to 8641c63 Compare February 27, 2019 20:21

openshift-ci-robot assigned ecordell Feb 27, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 27, 2019

docs(operatorgroups): add more extensive docs

a6cd32f

njhale force-pushed the operatorgroup-docs branch from 8641c63 to a6cd32f Compare February 27, 2019 20:35

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 27, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 27, 2019

openshift-merge-robot merged commit 4cd7c2e into operator-framework:master Feb 28, 2019

bergerhoffer mentioned this pull request Mar 1, 2019

OSDOCS-316: Updated OperatorGroup docs openshift/openshift-docs#13907

Merged

ecordell pushed a commit to ecordell/operator-lifecycle-manager that referenced this pull request Mar 8, 2019

Merge pull request operator-framework#721 from njhale/operatorgroup-docs

918f917

docs(operatorgroups): add more extensive docs

njhale added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 19, 2019

njhale deleted the operatorgroup-docs branch September 30, 2019 21:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs(operatorgroups): add more extensive docs #721

docs(operatorgroups): add more extensive docs #721

njhale commented Feb 20, 2019

ecordell left a comment

ecordell Feb 21, 2019

ecordell Feb 21, 2019

njhale Feb 21, 2019

ecordell Feb 22, 2019

njhale Feb 24, 2019

ecordell Feb 21, 2019

njhale Feb 21, 2019

njhale commented Feb 22, 2019

njhale commented Feb 22, 2019

njhale commented Feb 23, 2019

njhale commented Feb 24, 2019

alecmerdler left a comment

alecmerdler Feb 25, 2019

njhale Feb 26, 2019

njhale commented Feb 27, 2019

SamiSousa left a comment

SamiSousa Feb 20, 2019

SamiSousa Feb 27, 2019

njhale Feb 27, 2019 •

edited

Loading

ecordell commented Feb 27, 2019

ecordell commented Feb 27, 2019

openshift-ci-robot commented Feb 27, 2019

njhale commented Feb 27, 2019

docs(operatorgroups): add more extensive docs #721

docs(operatorgroups): add more extensive docs #721

Conversation

njhale commented Feb 20, 2019

ecordell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njhale commented Feb 22, 2019

njhale commented Feb 22, 2019

njhale commented Feb 23, 2019

njhale commented Feb 24, 2019

alecmerdler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njhale commented Feb 27, 2019

SamiSousa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njhale Feb 27, 2019 • edited Loading

Choose a reason for hiding this comment

ecordell commented Feb 27, 2019

ecordell commented Feb 27, 2019

openshift-ci-robot commented Feb 27, 2019

njhale commented Feb 27, 2019

njhale Feb 27, 2019 •

edited

Loading