Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement for opm content management features #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anik120
Copy link
Contributor

@anik120 anik120 commented Jun 9, 2020

No description provided.

Copy link
Member

@exdx exdx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job anik, just left some nits and thoughts


This enhancement is a proposal for extending the existing capabilites of the `opm` command line utility, to provide more fine grained control over the management of catalog indexes.

`opm` will have significant improvement to UX for editing an index (removing individual bundles from a package, removing channels from a package). Indexes can also be queried directly for inspection, and update graph visualization for individual packages will be added. Bundle validation without a deamon will is also proposed in this enhancement.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`opm` will have significant improvement to UX for editing an index (removing individual bundles from a package, removing channels from a package). Indexes can also be queried directly for inspection, and update graph visualization for individual packages will be added. Bundle validation without a deamon will is also proposed in this enhancement.
`opm` will significantly improve the UX around working with indexes by enabling removing individual bundles from a package and removing channels from a package. Indexes will now also be able to be queried directly for inspection, and update graph visualization for individual packages will be added. Bundle validation without a deamon is also proposed in this enhancement.


This enhancement is a proposal for extending the existing capabilites of the `opm` command line utility, to provide more fine grained control over the management of catalog indexes.

`opm` will have significant improvement to UX for editing an index (removing individual bundles from a package, removing channels from a package). Indexes can also be queried directly for inspection, and update graph visualization for individual packages will be added. Bundle validation without a deamon will is also proposed in this enhancement.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the daemon-less validation for docker users only? would it be worth mentioning that explicitly?

enhancements/opm-content-management.md Show resolved Hide resolved

A new sub-command `graph` will be added under `opm alpha bundle` to allow operator authors or cluster admins to understand the update graph for a given Operator in a catalog, so that the update graph can be reasoned about for correctness and available versions.

The `graph` sub-command will have a `type` flag, to denote the type of graph visualization, one of `ascii` (for ASCII representation of the graph), `graphical` (for a graphical visualization of the graph, possibly using (go-graphviz)[https://github.com/goccy/go-graphviz]), or `json` (for a human readable data format of the graph). The default option will be `ascii`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is really interesting. Maybe we could also have a png flag to simply export the graph to a file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@exdx that's a great idea! I like the idea of exporting the graphical visualization to a file.


### Support for deamonless bundle validation

Currently, `opm` requires either the docker or the podman deamon for bundle validation (mostly for unpacking container image). This dependency on a deamon will be removed by using the imported (api library)[https://github.com/operator-framework/api] which now
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe podman requires a daemon IIRC

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. podman is daemonless.

a) From the leaf end of the update graph for a package
b) From the beginning of the update graph for a package
c) From the middle of an update graph for a package

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A simple example here of the 3 scenarios would be helpful

2) Build and serve the registry database to expose the grpc api
3) Query the database

The `inspect` sub-command will be introduced under the `opm index` command to facilitate single step inspection of an index.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this command going to use grpcurl under the hood?


### Support for deamonless bundle validation

Currently, `opm` requires either the docker or the podman deamon for bundle validation (mostly for unpacking container image). This dependency on a deamon will be removed by using the imported (api library)[https://github.com/operator-framework/api] which now
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the second sentence is incomplete.

### Goals

- Allow `opm` users to perform bundle validations in environments without a Docker daemon. Bundle validation also includes validation of bundle image labels against `metadata/annotations.yaml`
- Allow `opm` users to safely edit indexes, which includes removing bundle version from a package in an index, and removing a channel from a bundle.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC we also want to edit the channels

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

example: promotion from techpreview to stable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gallettilance do you mean change the default channel of a package?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any channel

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to handle deprecation (say the 4.1 channel should be removed) and promotion (say the techpreview becomes stable)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gallettilance this section tackles the deprecation of a channel: https://github.com/operator-framework/enhancements/pull/26/files#diff-e5605260b53989d798d8558d3bf4c1f0R93

However, promotion of techpreview to stable seems like creating a discrepancy between what the operator author had communicated (via CSVs) and what the index author is attempting to do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

promotion of techpreview to stable seems like creating a discrepancy between what the operator author had communicated (via CSVs) and what the index author is attempting to do.

I think we need to treat what the CSV says as the initial state, not the final state. The workflow for channel promotion becomes hairy (and you lose provenance - the bundle digest will change after promotion) if we don't allow the index to be authoritative on this

- Allow `opm` users to perform bundle validations in environments without a Docker daemon. Bundle validation also includes validation of bundle image labels against `metadata/annotations.yaml`
- Allow `opm` users to safely edit indexes, which includes removing bundle version from a package in an index, and removing a channel from a bundle.
- Allow `opm` users to inspect catalog content and return the update graph of Operators in machine-readable format.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC we also want to define the source of truth for an index so that it can be rebuilt. IMO the transactions that are applied to the DB that don't come from bundle labels / annotations should be logged or output somewhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 understanding provenance and reproducibility of distributed artifacts is key.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gallettilance @mhrivnak the state of the index is already recorded in a database, which is also edited when content in the index is edited. The information on the database is sufficient to rebuild the index with, unless there's other reasoning/s to support logging of the transactions.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anik120 I suggest to plug here your parallel work on declarative index configs, to form the base of reproducible catalog/index creation. The goal being:

  • Allow opm users to create a catalog / index reproducible from scratch using an on-disk index representation in human-friendly and machine-readable text format


Currently, `opm alpha bundle validate` validates only the content of `metadata/annotations.yaml` file, but not the image labels of the DockerFile. The image labels are used when the bundle is added to an index image. As a result, if there are mismatches between the content of the bundle image (i.e `metadata/annotations.yaml`) and the bundle image labels, the bundle image is invalid and could potentially cause issues when it is added to an index image.

The `opm alpha bundle validate` command will be enhanced with an extra step that'll check to see if the contents of `metadata/annotations.yaml` matches with the labels of the bundle image.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The `opm alpha bundle validate` command will be enhanced with an extra step that'll check to see if the contents of `metadata/annotations.yaml` matches with the labels of the bundle image.
The `opm alpha bundle validate` command will be enhanced with an extra step that will check to see if the contents of `metadata/annotations.yaml` matches with the labels of the bundle image.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does it mean to match? Is it ok to have labels that are not in the annotations? annotations that are not in the labels?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gallettilance the idea is to have the exact number of annotations as the number of labels. I'll be more specific about this in this para.


### Explain and visualize the update graph of an operator in an index

A new sub-command `graph` will be added under `opm alpha bundle` to allow operator authors or cluster admins to understand the update graph for a given Operator in a catalog, so that the update graph can be reasoned about for correctness and available versions.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be a command on the index subcommand since it has to do with an index?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gallettilance that is a good point. Editing this section to have the graph sub-command under the index command.


`opm index rm --package=example-operator quay.io/example-namespace/example-index:v0.0.1` will remove a package from an index

`opm index rm --bundle=example-operatorv1.0.0 quay.io/example-namespace/example-index:v0.0.1` will remove the bundle from a package in an index.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have it be the bundlepath instead of the csv name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure that works too.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some cleanup based on changes since this PR was created:

opm <index | registry> rm --packages <packagenames> was added in PR operator-framework/operator-registry#243

Being able to deprecate bundles was added in PR operator-framework/operator-registry#397


#### Open question

Once a bundle is removed from a pacakge in an index, is the onus on `opm` to handle the upgrade of bundles that have already been installed in a cluster, but have been removed from the index?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this is up to the product team or operator owner to decide / be aware of

@anik120 anik120 force-pushed the opm-content-management-enhancement branch 2 times, most recently from da58969 to ee96928 Compare June 16, 2020 19:52
@anik120
Copy link
Contributor Author

anik120 commented Jun 16, 2020

@exdx @gallettilance @mhrivnak updated the enhancement based on your feedbacks. Thanks!


A new sub-command `graph` will be added under `opm index` to allow operator authors or cluster admins to understand the update graph for a given Operator in a catalog, so that the update graph can be reasoned about for correctness and available versions.

The `graph` sub-command will have a `type` flag, to denote the type of graph visualization, one of `ascii` (for ASCII representation of the graph), `graphical` (for a graphical visualization of the graph, possibly using (go-graphviz)[https://github.com/goccy/go-graphviz] that will ultimately be exported to a png file in the working directory), or `json` (for a human readable data format of the graph). The default option will be `ascii`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: text is more common for other ecosystem tools

Copy link
Member

@ecordell ecordell Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make more sense change graphical to png to allow more options in the future.

We might also want type=dot in addition, so that users can modify the graph before rendering.

--out is more common than --type


`opm index rm --package=<package-name> quay.io/example-namespace/example-index:v0.0.1` will remove a package from an index

`opm index rm --bundle=<bundle-path> quay.io/example-namespace/example-index:v0.0.1` will remove the bundle from a package in an index.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this section could use a little bit more detail.

one of the things we want rm to do is to "skip" the removed operator, so an outline of what changes will happen to the graph would be good.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is an enhancement, we should write out why we want to skip instead of delete (so that existing clusters that have installed the "bad" release can get updated the newer version).


#### Open question

Once a bundle is removed from a pacakge in an index, is the onus on `opm` to handle the upgrade of bundles that have already been installed in a cluster, but have been removed from the index?
Copy link
Member

@ecordell ecordell Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do want opm to manage the skip list for surrounding operator updates, so that clusters that have seen the "bad" operator can update properly. See: https://github.com/operator-framework/operator-lifecycle-manager/blob/master/doc/design/how-to-update-operators.md#skipping-updates

Once a bundle is removed from a pacakge in an index, is the onus on `opm` to handle the upgrade of bundles that have already been installed in a cluster, but have been removed from the index?


### Package inspection using opm
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will this differ from the graph json output? Can we just rely on the graph json, or additional "types" of graph output?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ecordell if you're referring to the json output (as one of json, text etc) then that was for visualizing the update graph of a particular bundle.
This section talks about a machine-readable representation of all the packages present inside an index.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Understanding the update graph of a particular package or bundle version is a different use case to understanding the content of the catalog as whole.


Currently, `opm alpha bundle validate` validates only the content of `metadata/annotations.yaml` file, but not the image labels of the DockerFile. The image labels are used when the bundle is added to an index image. As a result, if there are mismatches between the content of the bundle image (i.e `metadata/annotations.yaml`) and the bundle image labels, the bundle image is invalid and could potentially cause issues when it is added to an index image.

The `opm alpha bundle validate` command will be enhanced with an extra step that will check to see if the contents of `metadata/annotations.yaml` matches with the labels of the bundle image. If the `metadata/annotations.yaml` contain fields that are not there in the labels of the image or vice versa, the validation will fail.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we saying we want to fail if the set of labels in the annotations.yaml don't exactly match the labels on the generated image? I believe what we want is the image labels to be a superset of the the annotation.yaml labels, so they can be customized and used in an ad-hoc way (they are just images).

I think this is what you're saying here, but maybe just clarify a bit because it is a bit confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@exdx aren't the labels in the images supposed to be exactly the annotations in annotations.yaml?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to fail if there are labels in annotations.yaml that are not on the image. I think other tooling/registries should remain able to apply additional labels on the image without opm bundle validate complaining.


Currently, `opm` requires the docker deamon for bundle validation (mostly for unpacking container image) by default. This dependency on a deamon will be removed by using containerd or podman as default container tools to pull and unpack container images for validating the bundle.

As an operator author, I want to be able to validate bundle image without a daemon or source out to a binary/CLI tool in local host. Ideally, bundle validation tool is a self-container tool without requiring any external tool being installed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
As an operator author, I want to be able to validate bundle image without a daemon or source out to a binary/CLI tool in local host. Ideally, bundle validation tool is a self-container tool without requiring any external tool being installed.
As an operator author, I want to be able to validate a bundle image without requiring a daemon or shelling out to a local binary. Ideally, bundle validation is self-contained without requiring any external tool being installed.

@anik120 anik120 force-pushed the opm-content-management-enhancement branch 2 times, most recently from d064ab3 to a41ec3c Compare June 18, 2020 21:22
@anik120 anik120 force-pushed the opm-content-management-enhancement branch from a41ec3c to 80852d5 Compare June 19, 2020 16:52
Copy link

@dmesser dmesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a solid proposal to me. I definitely recommend bringing the declarative index config work in here or split out the reproducible index creation use case into that separate enhancement.

- Allow `opm` users to perform bundle validations in environments without a Docker daemon. Bundle validation also includes validation of bundle image labels against `metadata/annotations.yaml`
- Allow `opm` users to safely edit indexes, which includes removing bundle version from a package in an index, and removing a channel from a bundle.
- Allow `opm` users to inspect catalog content and return the update graph of Operators in machine-readable format.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anik120 I suggest to plug here your parallel work on declarative index configs, to form the base of reproducible catalog/index creation. The goal being:

  • Allow opm users to create a catalog / index reproducible from scratch using an on-disk index representation in human-friendly and machine-readable text format


Currently, `opm alpha bundle validate` validates only the content of `metadata/annotations.yaml` file, but not the image labels of the DockerFile. The image labels are used when the bundle is added to an index image. As a result, if there are mismatches between the content of the bundle image (i.e `metadata/annotations.yaml`) and the bundle image labels, the bundle image is invalid and could potentially cause issues when it is added to an index image.

The `opm alpha bundle validate` command will be enhanced with an extra step that will check to see if the contents of `metadata/annotations.yaml` matches with the labels of the bundle image. If the `metadata/annotations.yaml` contain fields that are not there in the labels of the image or vice versa, the validation will fail.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to fail if there are labels in annotations.yaml that are not on the image. I think other tooling/registries should remain able to apply additional labels on the image without opm bundle validate complaining.

Once a bundle is removed from a pacakge in an index, is the onus on `opm` to handle the upgrade of bundles that have already been installed in a cluster, but have been removed from the index?


### Package inspection using opm
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Understanding the update graph of a particular package or bundle version is a different use case to understanding the content of the catalog as whole.

@cdjohnson
Copy link

@anik120
There are some great features defined here, and some of them have been written in other PR's such as

opm <image | registry> rm --packages
and
opm image deprecatetruncate

Can we update with the current state and review in the OLM Dev call?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants