-
Notifications
You must be signed in to change notification settings - Fork 261
Initial proposal for mutable operator registry #95
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| # Ability to Modify Operator Registry | ||
|
|
||
| ## Motivation | ||
|
|
||
| An index image is defined as a set of operator bundle images that are released independently and optimized in a way that can be productized by operator authors and pipeline developers. The intent of the index image is to aggregate these images that are out of scope of this enhancement. As an operator author, I want to be able to modify an existing index so I don't have to build it from scratch each time. | ||
|
|
||
| ## Proposal | ||
|
|
||
| This implementation makes the assumption that a separate process is built that generates container images that contain individual operator bundles for each version of an operator. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can omit this |
||
|
|
||
| To start, let's define what the operator-registry does today. It: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. being nitpicky again - there are multiple tools involved here, it might be worth calling that out. |
||
|
|
||
| 1. Takes a set of manifest files and outputs a database file. | ||
| 2. Takes a database file and serves a grpc api that serves content from that db | ||
| 3. Takes a configmap reference, builds a db and serves a grpc api that serves content from that db | ||
| 4. Takes an app-registry path/namespace reference, builds a db and serves a grpc api that serves content from that db | ||
|
|
||
| And what we would like it to: | ||
|
|
||
| 1. Use a reference to a container image (also referred to here as the operator index) rather than app-registry namespaces to expose installable operators on a cluster | ||
| 2. Build an operator-registry database that serves the data required by OLM to drive install and UI workflows | ||
| 3. Have a way to optimize the build workflow of operator-registry databases (which currently drive OLM's workflows) from previous versions of the database plus new content (ex. new operator at new version) so that the database need not be built from scratch every time they are created. | ||
|
|
||
| ### Updating the Operator Registry to insert incremental updates | ||
|
|
||
| We want to add create db, delete, and batch insert APIs to the model layer of operator-registry and a new set of operator registry commands to utilize those new APIs: | ||
|
|
||
| `operator-registry create` | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These aren't the actual commands we are going to implement though right? |
||
|
|
||
| - inputs: none | ||
|
|
||
| - outputs: empty operator registry database | ||
|
|
||
| `operator-registry add` | ||
|
|
||
| - inputs: $operatorBundleImagePath, $dbFile | ||
|
|
||
| - outputs: updated database file | ||
|
|
||
| ex: `operator-registry add quay.io/community-operators/foo:sha256@abcd123 example.db` | ||
|
|
||
| `operator-registry add --batch` | ||
|
|
||
| - inputs: $operatorBundleImagePath, $dbFile | ||
|
|
||
| - outputs: updated database file | ||
|
|
||
| ex: `operator-registry add "quay.io/community-operators/foo:sha256@abcd123,quay.io/community-operators/bar:sha256@defg456" example.db` | ||
|
|
||
| `operator-registry delete` | ||
|
|
||
| - inputs: $operatorName, $dbFile | ||
|
|
||
| - outputs: updated database file without $operatorName included | ||
|
|
||
| ex: `operator-registry delete bar example.db` | ||
|
|
||
| `operator-registry delete-latest` | ||
|
|
||
| - inputs: $operatorName, $dbFile | ||
|
|
||
| - outputs: updated database file without latest version of $operatorName included | ||
|
|
||
| ex: `operator-registry delete-latest foo example.db` | ||
|
|
||
| ### Implementation details | ||
|
|
||
| #### Add | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this section is making some assumptions about what the workflow is that may not be obvious to the reader. Can you explicitly define what happens internally when you call
|
||
|
|
||
| To achieve this we need to pull down images from a registry using podman, extract the bundle files from the container's filesystem and insert the bundle into the database. For each channel defined in the bundle's `package.yaml`: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably support more than just
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Along these lines, it's reasonable to just use containerd. Then users don't require additional tooling. (we still may want options for shelling out for certain environments, but I think containerd gives the best ux out of the door)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if we want to have a common user experience to the If the sdk is going to call these commands as functions, should we attempt to also have the same experience there? We could even just default to containerd if we wanted to, but from the perspective of a user trying to wrangle all these tools together does it make more sense that they all have the same defaults?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This reminds me a lot of the Interface Segregation Principle. We don't require everything that SDK does, so why take on the burdens of requiring podman? I don't think the UX of this tool will differ since we're already abstracting over the operation. |
||
|
|
||
| 1. If the channel points to a csv that is not in this bundle. | ||
|
|
||
| In this case, IGNORE this channel. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you be more specific about input and output here? What do you mean by ignore the channel? As in, ignore the update path for the new bundle in this channel? Can you outline the general strategy around how the |
||
|
|
||
| 2. The bundle is attached to an operator that is not in the database. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This wording could be a bit clearer. Are you saying "the image reference (quay.io/...), contains a |
||
|
|
||
| In this case, we can simply add this bundle to the database. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did I miss the section that describes the schema changes that allow adding bundle references to the db? |
||
|
|
||
| 3. The bundle is attached to an operator in the database. If the channel points to a csv that is in this bundle: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't this be "for each channel in the package.yaml in the bundle, if the channel points to ..."? |
||
|
|
||
| 1. This channel is not in the database. Insert the bundle for this new channel into the database. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And insert the channel as well? |
||
|
|
||
| 2. This channel is in the database. SELECT all csv's of that channel from the database. By looking at the replaces and skips fields of the bundle's csv, we can know where this new csv fits in the update graph. If: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually think this whole section can be reduced to the loading steps similar to what we already do:
This is nice because:
|
||
|
|
||
| - That csv is the latest version for that channel. Insert the bundle for that channel. | ||
|
|
||
| - That csv defines a replacement that another csv is already replacing. Suppose we have we are trying to insert A which replaces C. But B also replaces C. We need the operator owner to either specify in A that it skips/replaces B or to delete B from the database and update B such that it replaces/skips A. Without these conditions we cannot insert. | ||
|
|
||
| - The csv specifies a replacement that cannot be found. Cannot insert. | ||
|
|
||
| - The csv is already in the database. Cannot insert. | ||
|
|
||
| 4. The bundle is an updated version of an operator that is in the database. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this imply that |
||
|
|
||
| Not allowed. | ||
|
|
||
| #### Add --batch | ||
|
|
||
| This is similar to `add`. We have two options: | ||
|
|
||
| 1. We can naively, sequentially insert from the list of images | ||
|
|
||
| Pros: simple to implement | ||
|
|
||
| Cons: ordering of images may affect the state of the db? | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not a question - this is true given the above description of channel handling |
||
|
|
||
| 2. We can preprocess the images to minimize collisions or optimize for a given objective | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you go into more detail here?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideas for heuristics:
But all of these have a problem, which is that they all require unpacking all of the bundles locally at once for processing. I lean towards doing (1) and addressing the other concerns with the separate "graph metadata" we discussed.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like we can do all of (1) (2) and (3) by updating the database schema and storing those values as well. Or is that exactly what you are describing with your last sentence? |
||
|
|
||
| Pros: versatile | ||
|
|
||
| Cons: Not clear what objective we would need to optimze for | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo: optimize also, the objective is to build the graph the user intends. this is really a discussion of how a user can understand how to build that graph. |
||
|
|
||
| #### Delete | ||
|
|
||
| Delete will remove all channels and versions of a given operator from the database. | ||
|
|
||
| We need to warn the user before deleting an operator whose APIs are required by another operator in that same database. | ||
|
|
||
| #### Delete-latest | ||
|
|
||
| Delete-latest will remove the latest version added to an operator's channel. The latest added version to the database is defined as the version that is at the tip of the update graph for a given channel that has the latest insert time (or index key) among all channels. | ||
|
|
||
| We should warn the user about APIs that are required by another operator that are not in previous versions of this operator. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being nitpicky: an index image is really defined as container image the serves the grpc api. I'm concerned someone reading this will thing that "list of operator bundle images" == "index image"
Can we just say
to aggregate these [bundle images](/link/to/bundle/enhancement)?I think this should either be rewritten to fit in the paragraph, or be separated and called out as a user story.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that this section doesn't adequately explain why these artifacts need to be mutable, or explain that, in todays world, they're immutable. Could you add something to that effect?