Initial proposal for mutable operator registry by gallettilance · Pull Request #95 · operator-framework/operator-registry

gallettilance · 2019-10-03T14:59:14Z

Description of the change:

Initial proposal for mutable operator registry. As an operator author, I want to modify operator registry so that I don't have to rebuild the database from scratch during iterative development.

Motivation for the change:

Reviewer Checklist

Docs updated or added to /docs
Commit messages sensible and descriptive

openshift-ci-robot · 2019-10-03T14:59:18Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gallettilance
To complete the pull request process, please assign ecordell
You can assign the PR to them by writing /assign @ecordell in a comment when ready.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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 · 2019-10-03T15:07:03Z

@gallettilance: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/unit	`6e4c681`	link	`/test unit`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

kevinrizza · 2019-10-03T16:48:59Z

+
+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`


These aren't the actual commands we are going to implement though right? operator-registry does not exist as a top level command based on #89

kevinrizza · 2019-10-03T16:50:05Z

+
+#### Add
+
+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`:


We should probably support more than just podman here, probably through an environment variable (docker)? We can default to podman though.

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)

I'm wondering if we want to have a common user experience to the operator-sdk in this case. I do know that the sdk uses environment variables and wraps their build commands (for example) but under the hood they are using podman/buildah/docker cli: https://github.com/operator-framework/operator-sdk/blob/master/cmd/operator-sdk/build/cmd.go#L62

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?

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.

kevinrizza · 2019-10-03T16:56:50Z

+
+### Implementation details
+
+#### Add


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 add? For example, today when we call initialize it reads the yaml files into memory, reads them into a set of data structures, and then validates that the set is internally consistent. Now we have some other constraints that involve more than just reading that stuff into memory, validating, and then performing an insert:

We need to download the images that were specified

We need to do something about checking whether adding the new bundle is internally consistent with the rest of the data. Does that happen via an insert and failing due to changes in the schema itself? Do we pull the bundles that are relevant to building the graph at runtime and perform the same checks that already exist by adding the new bundle to that map? How does this work?

What schema changes (if any) are required for this?

kevinrizza · 2019-10-03T16:58:34Z

+
+1. If the channel points to a csv that is not in this bundle.
+
+In this case, IGNORE this channel.


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 package.yaml file will be used to handle updates before going into each edge case?

kevinrizza · 2019-10-03T16:59:52Z

+
+        - The csv is already in the database. Cannot insert.
+
+4. The bundle is an updated version of an operator that is in the database.


Does this imply that version + package name == operator bundle? Can you call that out explicitly?

kevinrizza · 2019-10-03T17:00:21Z

+
+Cons: ordering of images may affect the state of the db?
+
+2. We can preprocess the images to minimize collisions or optimize for a given objective


Can you go into more detail here?

Ideas for heuristics:

order based on semver versions of bundles

lexicographic order (may work for bundles that put versions in the name but don't keep the version field aligned)

timestamp labels

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.

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?

ecordell · 2019-10-03T19:10:56Z

+
+## 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.


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"

The intent of the index image is to aggregate these images that are out of scope of this enhancement.

Can we just say to aggregate these [bundle images](/link/to/bundle/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.

I think this should either be rewritten to fit in the paragraph, or be separated and called out as a user story.

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?

ecordell · 2019-10-03T19:13:11Z

+
+## 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.


I think you can omit this

ecordell · 2019-10-03T19:13:35Z

+
+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.
+
+To start, let's define what the operator-registry does today. It:


being nitpicky again - there are multiple tools involved here, it might be worth calling that out.

ecordell · 2019-10-03T20:09:54Z

+
+#### Add
+
+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`:


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)

ecordell · 2019-10-03T20:11:05Z

+
+Pros: simple to implement
+
+Cons: ordering of images may affect the state of the db?


not a question - this is true given the above description of channel handling

ecordell · 2019-10-03T20:20:52Z

+
+In this case, IGNORE this channel.
+
+2. The bundle is attached to an operator that is not in the database.


This wording could be a bit clearer.

Are you saying "the image reference (quay.io/...), contains a csv such that csv.metadata.name is not a key in the bundle table"? (We don't need to be that specific here, but we should call out what the uniqueness is.)

ecordell · 2019-10-03T20:21:32Z

+
+2. The bundle is attached to an operator that is not in the database.
+
+In this case, we can simply add this bundle to the database.


Did I miss the section that describes the schema changes that allow adding bundle references to the db?

ecordell · 2019-10-03T20:22:52Z

+
+In this case, we can simply add this bundle to the database.
+
+3. The bundle is attached to an operator in the database. If the channel points to a csv that is in this bundle:


shouldn't this be "for each channel in the package.yaml in the bundle, if the channel points to ..."?

ecordell · 2019-10-03T20:23:04Z

+
+3. The bundle is attached to an operator in the database. If the channel points to a csv that is in this bundle:
+
+    1. This channel is not in the database. Insert the bundle for this new channel into the database.


And insert the channel as well?

ecordell · 2019-10-03T20:33:36Z

+
+    1. This channel is not in the database. Insert the bundle for this new channel into the database.
+
+    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:


I actually think this whole section can be reduced to the loading steps similar to what we already do:

Add the bundle to the database

Add the package data:

If the package is not there, create it.

For each channel

if the channel is not there, create it

if the channel is there

if the new head "replaces" the current head

update the channel pointer and add the new head, replacing the old head.

else, delete the whole channel. Walk through the entire channel and delete the channel entries.

add the new head of the channel as a channel entry as specified in package.yaml

add all of the channel entries by walking back through the "replaces"

This is nice because:

we already have functions defined that do most of these steps, they might just need some refactring

we don't have to worry about "correctly" perturbing the channel in the right way

this generalized to batch processing easily, because you can add all of the bundles and then take the latest package data as a final step to shake out the channels

we have the option of optimizing more in the future

stale · 2020-02-26T17:04:08Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2020-04-26T17:52:13Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

njhale · 2020-06-26T15:27:29Z

IIRC, parts of this ended up being absorbed into other enhancements.

Initial proposal for mutable operator registry

6e4c681

openshift-ci-robot requested review from dinhxuanvu and jpeeler October 3, 2019 14:59

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

kevinrizza requested changes Oct 3, 2019

View reviewed changes

ecordell reviewed Oct 3, 2019

View reviewed changes

stale Bot added the wontfix label Feb 26, 2020

openshift-ci-robot added triage/unresolved Indicates an issue that can not or will not be resolved. and removed wontfix labels Feb 27, 2020

stale Bot added the wontfix label Apr 26, 2020

openshift-ci-robot removed the wontfix label May 1, 2020

njhale closed this Jun 26, 2020


		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`


		#### Add

		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`:


		1. If the channel points to a csv that is not in this bundle.

		In this case, IGNORE this channel.


		- The csv is already in the database. Cannot insert.

		4. The bundle is an updated version of an operator that is in the database.


		Cons: ordering of images may affect the state of the db?

		2. We can preprocess the images to minimize collisions or optimize for a given objective


		## 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.


		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.

		To start, let's define what the operator-registry does today. It:


		Pros: simple to implement

		Cons: ordering of images may affect the state of the db?


		In this case, IGNORE this channel.

		2. The bundle is attached to an operator that is not in the database.


		2. The bundle is attached to an operator that is not in the database.

		In this case, we can simply add this bundle to the database.


		In this case, we can simply add this bundle to the database.

		3. The bundle is attached to an operator in the database. If the channel points to a csv that is in this bundle:


		3. The bundle is attached to an operator in the database. If the channel points to a csv that is in this bundle:

		1. This channel is not in the database. Insert the bundle for this new channel into the database.


		1. This channel is not in the database. Insert the bundle for this new channel into the database.

		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:

Conversation

gallettilance commented Oct 3, 2019

Uh oh!

openshift-ci-robot commented Oct 3, 2019

Uh oh!

openshift-ci-robot commented Oct 3, 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

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!

stale Bot commented Feb 26, 2020

Uh oh!

stale Bot commented Apr 26, 2020

Uh oh!

njhale commented Jun 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants