Propose convention for storing operator bundle by tkashem · Pull Request #27 · openshift/enhancements

tkashem · 2019-09-19T16:18:38Z

This enhancement proposes standards and conventions for storing
operator manifests and metadata as conatiner images.

ashcrow · 2019-09-19T16:50:17Z

On first pass this looks pretty reasonable to me. I do notice that while OCI containers are referenced the majority of the examples rely on the docker command. I assume the custom and docker media-types are interchangeable.

In terms of the format version in use, how would one figure that out? Would there be a specific label associated with the formats version that someone would use skopeo inspect or the like to check?

deads2k · 2019-09-19T21:41:25Z

/assign @jwforres

deads2k · 2019-09-19T21:43:49Z

+    * The value `manifests+metadata` implies that this bundle contains both operator metadata and manifets.
+* The label `operators.coreos.com.bundle.mediatype` reflects the media type of the operator bundle. It could be helm charts, plain kubernetes manifests etc.
+
+The labels will also be put inside a YAML file, as shown below.


which is authoritative if they do not match?

Ideally, we would like to have these annotations defined in one place. In case of a mismatch, the annotations.yaml file is authoritative because on-cluster operator-registry ( component of olm ) that relies on these annotations has access to the yaml file only.

The potential use case for the LABELS is - an external off-cluster tool can inspect the image to check the type of bundle image without downloading the content.

I will add this info to the proposal.

@tkashem I think we should add the reasoning for why OLM doesn't just use the labels itself. As I understand it, there are certain rules around how OpenShift components interact with container images, and that precludes inspecting images directly -- does that sound correct?

deads2k · 2019-09-19T21:44:28Z

+ADD test/annotations.yaml /metadata/annotations.yaml
+```
+
+Below is the directory layout of the operator bundle insdie the image.


typo: inside

will be fixed in the next push.

deads2k · 2019-09-19T21:45:52Z

+```bash
+$ tree
+/
+├── manifests


Can you describe what you would do with this? It doesn't look like a straight apply of the directory content. It is simply free-form?

In this example, I have used the format used by operator-registry - https://github.com/operator-framework/operator-registry#manifest-format. This is the format olm uses to package operator manifests today. The directory content in this case is not directly kubectl applyable. While the operator is being installed, olm manipulates the content before applying it to the cluster.

I think, we can also package content in the operator bundle image that are straight apply.

In this example, I have used the format used by operator-registry - https://github.com/operator-framework/operator-registry#manifest-format. This is the format olm uses to package operator manifests today. The directory content in this case is not directly kubectl applyable. While the operator is being installed, olm manipulates the content before applying it to the cluster.

I think, we can also package content in the operator bundle image that are straight apply.

please include a link for reference.

The directory content in this case is not directly kubectl applyable

The content should be kubectl applyable, right?

It would be great if it supported arbitrary resources. A challenge SRE has faced developing operators for managing OSD is how to get all required resources installed. We have a limited set of resources we can include in a grpc bundle today. Resources off the top of my head that we can't bundle but need to deploy w/ the operator: Service, ServiceMonitor, PrometheusRule

deads2k · 2019-09-19T21:46:33Z

+  operators.coreos.com.bundle.mediatype: bundle
+```
+
+The package manifest YAML file that defines a set of `channel`(s) will not be included in the bundle. Operator registry will provide machineries to define `package` and `channel` that can be used while index is being built. 


can you link out to the channel and package you're describing?

This also doesn't explain what the index is - or link to something that explains the index concept. I'd be fine linking to another enhancement proposal PR for now if that is where we describe the index.

can you link out to the channel and package you're describing?

https://github.com/operator-framework/operator-lifecycle-manager/blob/master/doc/design/how-to-update-operators.md

https://github.com/operator-framework/operator-lifecycle-manager#discovery-catalogs-and-automated-upgrades

https://github.com/operator-framework/operator-lifecycle-manager/blob/master/doc/design/architecture.md#catalog-registry-design

I will add the links to the proposal also.

This also doesn't explain what the index is - or link to something that explains the index concept. I'd be fine linking to another enhancement proposal PR for now if that is where we describe the index.

Enhancement proposal that covers index - #24.

We still have some open questions whether channel and package definitions should be included inside the operator bundle image.

jwforres · 2019-09-20T02:27:24Z

+
+##### Build, Push, Pull Operator Bundle Image
+We use the following labels to annotate the operator bundle image.
+* The label `operators.coreos.com.bundle.resources` represents the bundle type:


Is this a good opportunity for us to switch to an operator framework based API domain, operators.operatorframework.io or something. Ideas?

operators.operatorframework.io sounds good to me. I am happy to update the proposal with this new domain.

At some point, we need to plan to version our APIs to be consistent across?

jwforres · 2019-09-20T02:32:49Z

+  operators.coreos.com.bundle.mediatype: bundle
+```
+
+The package manifest YAML file that defines a set of `channel`(s) will not be included in the bundle. Operator registry will provide machineries to define `package` and `channel` that can be used while index is being built. 


This also doesn't explain what the index is - or link to something that explains the index concept. I'd be fine linking to another enhancement proposal PR for now if that is where we describe the index.

jwforres · 2019-09-20T02:39:16Z

+```bash
+cat <<EOF | kubectl apply -f -
+apiVersion: operators.coreos.com/v2alpha1
+kind: Operator


What component would watch for this Operator resource and translate this into a running operator.

This example shows how a user can run the operator using olm. This is being covered here - #28.

@tkashem I think it would also be useful to write out manual application by pulling the bundle image locally, unpacking it, and applying to the cluster.

@tkashem I think we can scope this section to just what @ecordell said as "install w/ bundle image using Operator" is covered by #28

jwforres · 2019-09-20T02:41:13Z

+$ cd test
+
+# the following command generates the necessary scaffolding.
+$ operator-framework bundle init --type=operator-registry --bundle-folder=0.1.0


Do we have existing values for this --type flag today? If so what are they? I'm not convinced that operator-registry is the right thing to put here, but I'm missing some context to say what might be better.

https://github.com/operator-framework/operator-registry#manifest-format - operator registry bundle format is what olm uses today to pack/unpack operators. We wanted to take an incremental approach:

Quick win: package operator-registry into the bundle image, this gets us off of app-registry with relatively less efforts and sooner.

Long term: We can focus our efforts on a different format where operator metadata and manifests are separately defined and stored. csv is decomposed into plain kube manifests. We can also focus on adding support for other packaging format like helm.

The values for --type can be - operator-registry, plain ( straight apply kube manifets ), helm.

tkashem · 2019-09-20T17:04:38Z

In terms of the format version in use, how would one figure that out? Would there be a specific label associated with the formats version that someone would use skopeo inspect or the like to check?

Good point, we can use operators.coreos.com.bundle.mediatype=bundle+v1 or a new label operators.coreos.com.bundle.version.

ecordell · 2019-09-20T17:54:30Z

+$ cd test
+
+# the following command generates the necessary scaffolding.
+$ operator-framework bundle init --type=operator-registry --bundle-folder=0.1.0


If we merge this proposal are we committing to the operator-framework cli name?

No, it's just an example. :) It's an open question whether we have a new cli, or use operator-sdk or operator-registry.

estroz · 2019-09-27T06:14:23Z

+We use the following labels to annotate the operator bundle image.
+* The label `operators.operatorframework.io.bundle.resources` represents the bundle type:
+    * The value `manifests` implies that this bundle contains operator manifests only.
+    * The value `metadada` implies that this bundle has operator metadata only.


Suggested change

* The value `metadada` implies that this bundle has operator metadata only.

* The value `metadata` implies that this bundle has operator metadata only.

estroz · 2019-09-27T06:14:54Z

+* The label `operators.operatorframework.io.bundle.resources` represents the bundle type:
+    * The value `manifests` implies that this bundle contains operator manifests only.
+    * The value `metadada` implies that this bundle has operator metadata only.
+    * The value `manifests+metadata` implies that this bundle contains both operator metadata and manifets.


Suggested change

* The value `manifests+metadata` implies that this bundle contains both operator metadata and manifets.

* The value `manifests+metadata` implies that this bundle contains both operator metadata and manifests.

ralphbean · 2019-09-27T15:27:55Z

+By standardizing on a container format for this data, we get many other features for free, such as: identity, distribution, replication, deduplication, signing, and ingress.
+
+### Goals
+* Define a convention for storing operator manifetss and metadata with container image.


Suggested change

* Define a convention for storing operator manifetss and metadata with container image.

* Define a convention for storing operator manifests and metadata with container image.

mhrivnak · 2019-09-30T20:32:17Z

+* The `/manifests` folder is expected to contain resources that can be applied to the cluster using standard tooling like `kubectl`.
+* The `/metadata` folder is expected to contain resources that are not directly appliable. It can be used to store supporting metadata associated with the operator.
+* The image is not runnable, it is built from `scratch`.
+* The first image layer is expected to contain all of `/manifests`


What's the reason for this requirement?

I removed it, so unpacking the bundle content will now involve going through each image layer and extracting it.

mhrivnak · 2019-09-30T20:34:42Z

+$ cd test
+
+# the following command generates the necessary scaffolding.
+$ operator-framework bundle init --type=registry --bundle-folder=0.1.0


Why not add this to operator-sdk?

It will be - we're building the libraries and tooling so that operator-sdk can just wrap them. I believe this is just a strawman, but we'll likely have an entry point outside of sdk as well.

mhrivnak · 2019-09-30T20:36:48Z

+Once the scaffolding is generated the user can do a `docker build` to create an operator bundle image.
+
+#### Validate an Operator Bundle
+As an operator author I want to validate an operator bundle image so that I can ensure it runs as expected on a cluster.


As stated above, these images do not and cannot run, so consider rewording this. You could just leave it at "As an operator author I want to validate an operator bundle image."

it's a typo, it should be I can ensure my operator runs as expected on a cluster.

mhrivnak · 2019-09-30T20:37:19Z

+```
+
+The validate command will do the following:
+* Make sure the image `label` and `annotations.yaml` are appropriately configured. If there is any mismatch, the tool shoud generate apropriate error message. 


s/shoud/should/

s/apropriate/appropriate/

mhrivnak · 2019-09-30T20:48:21Z

+├── 0.1.0
+│   ├── testbackup.crd.yaml
+│   ├── testcluster.crd.yaml
+│   ├── testoperator.v0.1.0.clusterserviceversion.yaml


Do CSVs still include the "placeholder" namespace? This seems to assume that's not a problem in this example.

We have a separate effort to enable "directly applying CSVs" that this section assumes is already in place.

But I think the example probably should include a namespace flag?

mhrivnak · 2019-09-30T20:55:11Z

+* The label `operators.operatorframework.io.bundle.resources` represents the bundle type:
+    * The value `manifests` implies that this bundle contains operator manifests only.
+    * The value `metadada` implies that this bundle has operator metadata only.
+    * The value `manifests+metadata` implies that this bundle contains both operator metadata and manifets.


What's the use case for having a bundle with only one or the other? It seems like not a lot of data; why not "bundle" it together all the time and not worry about this difference?

The spec keeps room for separation, on the other hand our implementation which is already in progress "bundles" all of manifests/metadata together.

This enhancement proposes standards and conventions for storing operator manifests and metadata as conatiner images.

mhrivnak · 2019-10-02T19:18:00Z

+* The label `operators.operatorframework.io.bundle.resources` represents the bundle type:
+    * The value `manifests` implies that this bundle contains operator manifests only.
+    * The value `metadata` implies that this bundle has operator metadata only.
+    * The value `manifests+metadata` implies that this bundle contains both operator metadata and manifests.


If there's not a direct need for this now, I suggest leaving it out of the initial implementation. I'm sure you will version the format you're creating, and if this turns out to be a valuable distinction, you can add it in a later version.

Otherwise, this adds a fair amount of complexity that may not be worthwhile.

I think this falls under the inspect portion of the Build, Push, Pull Operator Bundle story. It's necessary if a user wants to identify a bundle image before pulling its layers.

I agree with Michael, this is not something that we need to be focusing on.

njhale · 2019-10-02T20:36:43Z

+
+We would like to be able to talk about a set of metadata and manifests, outside the context of a cluster, as representing a particular application or service (in this case, an operator).
+
+By standardizing on a container format for this data, we get many other features for free, such as: identity, distribution, replication, deduplication, signing, and ingress.


... on a container format ...

Would "on an OCI-compliant format" be more clear?

njhale · 2019-10-02T20:37:46Z

+By standardizing on a container format for this data, we get many other features for free, such as: identity, distribution, replication, deduplication, signing, and ingress.
+
+### Goals
+* Define a convention for storing operator manifests and metadata with container image.


Suggested change

* Define a convention for storing operator manifests and metadata with container image.

* Define a convention for storing operator manifests and metadata with a container image.

njhale · 2019-10-02T20:56:19Z

+* Build on-cluster tooling to interact with bundles
+
+## Proposal
+We delineate the operator metadata from the operator manifests. The operator manifests refers to a set of kubernetes manifest(s) the defines the deployment and RBAC model of the operator. The operator metadata on the other hand are, but not limited to:


Suggested change

We delineate the operator metadata from the operator manifests. The operator manifests refers to a set of kubernetes manifest(s) the defines the deployment and RBAC model of the operator. The operator metadata on the other hand are, but not limited to:

We delineate operator metadata from operator manifests. We define _operator manifests_ as a set of kubernetes manifests that compose an operator's deployment (`Deployment`, RBAC, etc), and _operator metadata_ as including:

njhale · 2019-10-02T21:00:36Z

+
+*Constraints*:
+* An operator bundle (including both manifests and metadata) should be identifiable using a single versioned identifier. 
+* For an operator The metadata can be downloaded independently of the manifest.


Suggested change

* For an operator The metadata can be downloaded independently of the manifest.

* An operator's metadata can be downloaded independently of its manifests.

njhale · 2019-10-02T21:12:19Z

+* The label `operators.operatorframework.io.bundle.resources` represents the bundle type:
+    * The value `manifests` implies that this bundle contains operator manifests only.
+    * The value `metadata` implies that this bundle has operator metadata only.
+    * The value `manifests+metadata` implies that this bundle contains both operator metadata and manifests.


I think this falls under the inspect portion of the Build, Push, Pull Operator Bundle story. It's necessary if a user wants to identify a bundle image before pulling its layers.

njhale · 2019-10-02T21:30:45Z

+    * The value `manifests` implies that this bundle contains operator manifests only.
+    * The value `metadata` implies that this bundle has operator metadata only.
+    * The value `manifests+metadata` implies that this bundle contains both operator metadata and manifests.
+* The label `operators.operatorframework.io.bundle.mediatype` reflects the media type or format of the operator bundle. It could be helm charts, plain kubernetes manifests etc.


Are we sure that manifests and metadata will always be of the same mediatype?

njhale · 2019-10-02T21:47:26Z

+* `registry`: Format used by [Operator Registry](https://github.com/operator-framework/operator-registry#manifest-format) to package an operator.
+* `helm`: Can be used to package a helm chart inside an operator bundle.
+* `plain`: Can be used to package plain k8s manifests inside an operator bundle.


Can we call out kustomize too?

njhale · 2019-10-02T23:29:48Z

+$ cd test
+
+# the following command generates the necessary scaffolding.
+$ operator-framework bundle init --type=registry --bundle-folder=0.1.0


What registry version would --type=registry use, the latest the tool is privy to? Should we always require the proposed version suffix (+<v>)?

njhale · 2019-10-02T23:40:29Z

+* Verify that the format of the bundle is valid. If the bundle is of `registry` format, we should verify that the bundle conforms to operator-registry standards.
+
+#### Run the Operator from the Bundle Image
+As an operator author I want to run my operator directly from the bundle image. Once an operator is packaged into a bundle image, we want to give the author ability to run it using `olm` directly from the bundle image.


Suggested change

As an operator author I want to run my operator directly from the bundle image. Once an operator is packaged into a bundle image, we want to give the author ability to run it using `olm` directly from the bundle image.

As an operator author I want to install my operator directly using only its bundle image.

njhale · 2019-10-02T23:43:12Z

+```bash
+cat <<EOF | kubectl apply -f -
+apiVersion: operators.coreos.com/v2alpha1
+kind: Operator


@tkashem I think we can scope this section to just what @ecordell said as "install w/ bundle image using Operator" is covered by #28

mhrivnak · 2019-10-04T18:28:47Z

Did you consider putting the metadata into a LABEL on the image? That would simplify retrieval of metadata from a bundle. That approach was used successfully with APBs.

shawn-hurley · 2019-10-07T18:06:41Z

+
+`Dockerfile` for operator bundle
+```
+FROM scratch


Have we considered that this is included in the operator image in a well-defined place?

Seems to me the only issue would be the SHA that is used in the deployment manifest but could OLM/Index Image replace that?

As discussed off-line - this is a feature we could build as part of the index in the future, but it would make it harder to audit images that are running on-cluster. We should be able to layer it on to the current proposal easily, but we need to weigh building that vs. the UX of understanding what's actually running on your cluster.

ecordell · 2019-10-07T21:41:16Z

@mhrivnak Labels would be ideal, but we need the metadata on-cluster, and we won't have access to the actual (docker v2-2) manifest on-cluster.

There's nothing stopping us from replicating the metadata into labels, much like we're doing for the media types here, as need arises.

shawn-hurley · 2019-10-09T01:40:41Z

+* The potential use case for the `LABELS` is - an external off-cluster tool can inspect the image to check the type of a given bundle image without downloading the content.
+
+###### Format
+We can use the following values for `mediatype`: 


Does this indicate that we will have to write the ability to deploy with these types? Can we say that these are potential types and not implement them until later? I think that we just focus on registry for now.

ecordell · 2019-10-10T18:54:44Z

/approve
/lgtm

The remaining issues will be addressed in a followup PR.

shawn-hurley · 2019-10-10T18:54:45Z

/lgtm

openshift-ci-robot · 2019-10-10T18:54:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell, shawn-hurley, tkashem

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:

~~OWNERS~~ [ecordell,shawn-hurley]

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

openshift-ci-robot requested review from ashcrow and sdodson September 19, 2019 16:18

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 19, 2019

openshift-ci-robot assigned jwforres Sep 19, 2019

deads2k reviewed Sep 19, 2019

View reviewed changes

jwforres reviewed Sep 20, 2019

View reviewed changes

ecordell reviewed Sep 20, 2019

View reviewed changes

tkashem force-pushed the operator-bundle branch 5 times, most recently from 2c0c623 to dec2726 Compare September 23, 2019 22:04

kevinrizza mentioned this pull request Sep 24, 2019

Enhancement for operator-registry #24

Merged

tkashem mentioned this pull request Sep 25, 2019

WIP Add operators.coreos.com/v2alpha1 operator-framework/api#4

Closed

njhale mentioned this pull request Sep 25, 2019

validation library operator-framework/api#3

Merged

estroz reviewed Sep 27, 2019

View reviewed changes

ralphbean reviewed Sep 27, 2019

View reviewed changes

mhrivnak reviewed Sep 30, 2019

View reviewed changes

Propose convention for storing operator bundle

909628c

This enhancement proposes standards and conventions for storing operator manifests and metadata as conatiner images.

tkashem force-pushed the operator-bundle branch from dec2726 to 909628c Compare October 1, 2019 19:14

mhrivnak reviewed Oct 2, 2019

View reviewed changes

njhale reviewed Oct 3, 2019

View reviewed changes

shawn-hurley reviewed Oct 7, 2019

View reviewed changes

shawn-hurley reviewed Oct 9, 2019

View reviewed changes

openshift-ci-robot assigned ecordell and shawn-hurley Oct 10, 2019

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

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2019

openshift-merge-robot merged commit aa0d20f into openshift:master Oct 10, 2019

dmesser mentioned this pull request Oct 14, 2019

doc/proposals/sdk-integration-with-olm.md: SDK/OLM integration… operator-framework/operator-sdk#1913

Merged

ecordell mentioned this pull request Oct 18, 2019

Add bundle library to build bundle manifest image operator-framework/operator-registry#97

Merged

5 tasks

	* The value `metadada` implies that this bundle has operator metadata only.
	* The value `metadata` implies that this bundle has operator metadata only.

	* The value `manifests+metadata` implies that this bundle contains both operator metadata and manifets.
	* The value `manifests+metadata` implies that this bundle contains both operator metadata and manifests.

	* Define a convention for storing operator manifetss and metadata with container image.
	* Define a convention for storing operator manifests and metadata with container image.


		We would like to be able to talk about a set of metadata and manifests, outside the context of a cluster, as representing a particular application or service (in this case, an operator).

		By standardizing on a container format for this data, we get many other features for free, such as: identity, distribution, replication, deduplication, signing, and ingress.

	We delineate the operator metadata from the operator manifests. The operator manifests refers to a set of kubernetes manifest(s) the defines the deployment and RBAC model of the operator. The operator metadata on the other hand are, but not limited to:
	We delineate operator metadata from operator manifests. We define _operator manifests_ as a set of kubernetes manifests that compose an operator's deployment (`Deployment`, RBAC, etc), and _operator metadata_ as including:

	* For an operator The metadata can be downloaded independently of the manifest.
	* An operator's metadata can be downloaded independently of its manifests.

	As an operator author I want to run my operator directly from the bundle image. Once an operator is packaged into a bundle image, we want to give the author ability to run it using `olm` directly from the bundle image.
	As an operator author I want to install my operator directly using only its bundle image.

Conversation

tkashem commented Sep 19, 2019

Uh oh!

ashcrow commented Sep 19, 2019

Uh oh!

deads2k commented Sep 19, 2019

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!

tkashem Sep 20, 2019 • 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!

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!

tkashem Sep 20, 2019 • 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!

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!

tkashem commented Sep 20, 2019

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

tkashem Sep 20, 2019 •

edited

Loading

tkashem Sep 20, 2019 •

edited

Loading