-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add declarative index config enhancement #37
Add declarative index config enhancement #37
Conversation
This comment has been minimized.
This comment has been minimized.
d546187
to
aedc5cb
Compare
@benluddy oopsy. Added the .md extension. |
aedc5cb
to
8117f8f
Compare
8117f8f
to
b8d762f
Compare
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.
After reading the enhancement I am still unsure about the use case the proposed feature is aiming for. The motivation reads like it would be the user introspecting, manipulating an index representation in structured text. But the actual format and commands seem to suggest this is trying to replace hard-coded opm
-internal validating logic with CUElang based schema and validation.
While there is clearly a benefit I am unsure about what this means for OLM users. I proposed to extend the idea of text-based representation of index to actual index creation and manipulation.
@dmesser this enhancement was sort of a byproduct of the opm content management enhancement. The idea was that once we have a way to define an index, reasoning about the structure of an index will become easier, and then task like reproducing an index etc can be built on top of that. |
@anik120 I see. This wasn't something that came out of this enhancement though and without the opm content management context the index representation topic somewhat abstract. Maybe consider merging them back together? |
@dmesser Note that the opm content management PR will be updated after this enhancement has been mostly agreed upon. |
c851d4b
to
1556460
Compare
@dmesser updated the enhancement with links to the improve opm content management enhancement (in the summary and in the non-goals section). PTAL and let me know if that alleviates your concerns. |
It would be nice to capture the user stories here as well. |
1556460
to
ecdfe73
Compare
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.
Great enhancement -- Hopefully we can use a cool project like cue to define and validate an index.
ecdfe73
to
514fe28
Compare
1b0754b
to
e694c98
Compare
the examples show channels still being denoted within a bundle...have there been any thoughts about pulling channel info out of bundles and instead creating some sort of 'ugprade graph' section within the package definition? the upgrade graph section would show the end user what versions of operators belong to which channels for example. Making changes to channels or the upgrade graph would then not impact the bundle definitions. |
@jmccormick2001 were you talking about something more like this:
This current plan is to provide a way to visualize the upgrade graph of a package using opm, that'll be discussed in the opm content management enhancement. |
yes, that channel definition (or similar) is what I was hoping for! |
@anik120 Why are the |
9ed05e7
to
6efe417
Compare
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.
@anik120 This is coming together nicely, I left some specific comments throughout.
One piece that seems missing is a tool to go from bundle image
-> bundle json
. index add
would have to do that underneath, but it seems useful to get the the translation without automatically adding it to the index.
|
||
In the current state of the world, operators are introduced to operator-lifecycle-manager by representing them as [Operator Bundles](https://github.com/operator-framework/operator-registry/blob/master/docs/design/operator-bundle.md#operator-bundle). These operator bundles are then loaded onto a container using the [opm](https://github.com/operator-framework/operator-registry/blob/master/docs/design/opm-tooling.md) tool (`opm index add --bundles`) to build an index of operators. Operator bundles conceptually belong to a channel in a package, with each bundle having the ability to upgrade to a different bundle in the channel. | ||
|
||
![Alt text](assets/community-operators.png?raw=true "community-operators") |
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'm not sure that the images are totally accurate anymore. We could probably safely remove them, or we could switch to mermaid so that we can update them along with this doc.
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.
Unfortunately doesn't look like github supports mermaid yet: https://github.community/t/feature-request-support-mermaid-markdown-graph-diagrams-in-md-files/1922/94
The second image is out of date (the package representations) and I can update that, and to me it feels valuable to keep them for anyone who'll be reading the enhancement for the first time.
As a user with pull permission from the namespace an index is hosted in/as a component of operator-lifecycle-manager, I can query an index for a json/yaml representation of the packages of an index. | ||
The representation of individual packages will allow individual package owners to reason about/make changes to their individual packages in isolation. | ||
|
||
`opm index inspect --index=docker.io/my-namespace/community-operators --output=json` |
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.
Is there a registry
version of this as well?
Or more broadly, do we need to define the tooling aspects as a part of this enhancement?
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 was shooting for the tooling aspects to be a part of the opm content management enhancement
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.
Also
One piece that seems missing is a tool to go from bundle image -> bundle json. index add would have to do that underneath, but it seems useful to get the the translation without automatically adding it to the index.
Does this sound like something that can be addressed in that enhancement instead of this? Or do you see a more immediate need for this?
|
||
`opm index inspect --index=docker.io/my-namespace/community-operators --output=json` | ||
|
||
creates a new folder `community-operators` and downloads the package representations for the packages that are in the index |
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.
nit: no data is actually downloaded aside from the index image itself.
$ cat etcd.json | ||
[ | ||
{ | ||
"schema": "package.v1", |
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.
Seeing this here, I think we should use olm.package
and olm.bundle
for consistency. If schema needs to change in the future we can introduce olm.package.v2
, etc (but that may never need to happen, since the property set gives us lots of flexibility to not change the schema).
├── image_sha256.json | ||
|
||
$ cat etcd.json | ||
[ |
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.
Can we unwrap the list and make this concatenated json?
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.
Could you elaborate on why we would prefer a file with concatenated json blobs vs a list of json blobs?
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.
Concatenated json can be combined with tools that don't know how to parse json
"name": "clusterwide-alpha", | ||
"upgradeInfo": { | ||
"replaces": "etcdoperator.v0.9.0" | ||
//"skiprange": >=0.9.0 <0.9.2 // an example of skiprange if skiprange was specified for this bundle |
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.
Does this mean that the skiprange only applies to other bundles in the same channel? (that would be my assumption based on the format).
@benluddy has suggested that in the long term we will want "replaces" to essentially be an arbitrary query. I think that's an interesting direction, and it would be nice if we could simplify the graph edge representation to easily allow for current / future ideas.
(we get that for free by using properties, because we can easily add / remove properties over time, but this being in the "real" schema means that we have to consider the representation)
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.
Does this mean that the skiprange only applies to other bundles in the same channel?
Shouldn't that be the case ideally? Can we assume that when a bundle is built that mentions for skips/skipsrange for one channel, it would need to stick to those mandates for all future channels it's included in? With the config I feel like we have the opportunity to relax that rule, where one can opt to have the bundle skip/skiprange in every channel it's included in, or opt to behave in different ways in different channels.
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.
for now i expect most people to be using it to allow upgrades from one channel to another. While i can envision that someone might want the ability to only allow upgrading within the channel and actually want to ensure that the skipRange does not allow someone to jump from one channel to another, that seems somewhat less likely to me (I guess maybe it'll become common when people have preview channels and they explicilty do not want you to be able to upgrade from some version in the preview channel, to a stable version, because they insist you only install stable versions if you want to be on the stable channel).
so i guess i've talked myself in circles, but i think we should start by retaining the existing behavior, and then in the future we can consider a channel-constrained skiprange if we see a need. So let's make sure the json structure reflects that behavior (applies across channels)
|
||
#### Representing the upgrade graph in the channel json blob | ||
|
||
Currently, a bundle can be added into the index using `opm index add --bundles <list-of-bundle-paths> --mode replaces|semver|semver-skippatch --tag=<index-image-tag>` where the bundle images (like `quay.io/operatorhubio/etcd:v0.9.0`) are included in `<list-of-bundle-path>`. The bundle can also mention bundles it can be upgrade from using the `skips` or `skipsRange` fields in the bundle ClusterServiceVersion. With all of these information provided, the upgrade graph of the bundles in the package is calculated and stored in the `channel_entry` table of the sql database that is built inside the index, while the `skips`/`skiprange` information is persisted in the `operatorbundle` table. |
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.
Just wanted to note that channel_entry is always authoritative here. The operatorbundle table persists the values of skip/skiprange/replaces when the bundle was first unpacked/added to the index, and other index operations could have changed the real graph in channel_entry.
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.
Added that note
"sha256":"9e0c9c2c8e973c7e17439509d1dc1512afa3a8ed7c0b169016299a357bbc5b10" | ||
} | ||
``` | ||
When an update is attempted on an index image, the `update` command will verify that the sha information locally matches with the sha for the remote index image before overriding the content of the container image with the latest changes. This will prevent overriding of existing information that may not have been pulled locally due to asynchronous update of the config files. |
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'm torn on this idea.
I could see this being a useful feature to help prevent accidental overwrites.
On the other hand, indexes are "just" images, and "just" json files. There's nothing unique about preventing overwrites to an index image vs. any other type of image. I'm not convinced this is a task that opm needs to be concerned with (since there are plenty of other ways to solve it that would be specific to a particular workflow / consumer).
(Also, this feature doesn't do a "real" lock, and performing this check ahead of time doesn't actually guarantee that another actor hasn't swapped the tag in between when you've checked that value and when you've pushed)
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.
It just feels like a gap not to provide a mechanism to ensure data integrity of an artifact that can be modified concurrently by distributed players .
Also, this feature doesn't do a "real" lock, and performing this check ahead of time doesn't actually guarantee that another actor hasn't swapped the tag in between when you've checked that value and when you've pushed
That is true, and I don't think it was aiming to serve as a real lock but serve as an accidental override prevention mechanism.
Having said that, if we think it'll do more harm than good, I can remove it for now and maybe think about it later if we get feedback of accidental overrides being a nuisance.
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'd be in favor of removing it. I think it gives the illusion of safety without actual safety. No reason to overcommit here.
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'm +1 on removal mostly because it will make the pipeline implementation easier, i think.
community-operators | ||
├── amqstreams.json | ||
└── etcd.json | ||
$ opm index create --from=community-operators --tag=docker.io/some-namespace/community-operators |
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.
It's not explicitly stated, but I assume that this will walk a tree of files, find all json files, and slurp them up?
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.
Made it more explicit in the first sentence of this section.
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 think it's also important to also have a way to continue building the Index Image scratch. The opm registry create
is no longer useful here, but something akin to opm index create ... --generate
.
With the intermix of upstream/downstream opm
, choosing the correct version of the opm
builder image as well as multi-architecture images has been problematic.
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.
Agreed. We need to have an easy way to determine which opm
version should be used for a catalog of a certain schema version.
42c3777
to
39d544a
Compare
``` | ||
|
||
To migrate over index images built with the old configuration, a command `opm index migrate --from-index <index image> --to-index <index image>` will be introduced. This command will extrat the database from the old image, convert the data in the database into package configs, and use the configs to build an index image with the new image configuration to serve using the aggregated `config.json`. | ||
The `--from-index` flag under `opm index add` is currently used to add new bundles to an existing index. This flag will be altered to check if a database exists in the index image passed as the flag's argument, and will throw an error asking the index author to fist `opm index migrate` to migrate over the old index to the new index. If the database does not exists, the flag will output a message that contains instructions on getting an index's configs, editing a config to add a new bundle, and pushing an `update` to the index, instead of carrying logic to fetch a sqllite db from an index and insert to the database (since the database will not be a part of the image anymore). |
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.
we need to make sure the pipeline team knows this change is coming since they're going to have to be migrated before they pick up the new opm binary.....
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.
@bparees fyi updated the enhancement with an automatic migration instead of needing opm index migrate
. This will allow for non-disruptive continuation of the use of the new opm binary for the pipeline.
"image": "<operatorbundle_path>", | ||
"version": "<version>", | ||
"properties":["<list of properties of bundle that encode bundle dependencies(provided and required apis) upgrade graph info(skips/skipsRange), and bundle channel/s info>"], | ||
"relatedImages" : ["<list-of-related-images>"] |
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.
There is also relatedImages
section that gets declared in the ClusterServiceVersion(CSV), is this relatedImages
different from the one in the CSV ? If so what's the difference or how this info gets used ?
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.
Also, in one of the examples below, the relatedImages
is an object, which seems more appropriate in case we need to include additional metadata:
"relatedImages": [
{
"name": "etcdv0.6.1",
"image": "quay.io/coreos/etcd-operator@sha256:bd944a211eaf8f31da5e6d69e8541e7cada8f16a9f7a5a570b22478997819943"
}
]
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.
There is also relatedImages section that gets declared in the ClusterServiceVersion(CSV), is this relatedImages different from the one in the CSV ? If so what's the difference or how this info gets used ?
it's the same info. the CSV related images info is pulled into the catalog so that when you mirror catalogs/operators, the catalog has all the info about what images need to be mirrored w/o having to pull down each bundle and consult the csv inside. (There may be other reasons too)
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.
Just had a couple of nits, questions, etc. after spending some reading this for the first time. I'm going to give this another read at some point this week.
Also, something that I noticed is there aren't any alternatives that were proposed? It looks like this enhancement has seen a couple of different iterations based on the earlier comments. Are those previous implementations something we want to call out explicitly once we get the actual implementation details in the core enhancement thought out?
|
||
## Non-goals | ||
|
||
1. Enumerate/implement tooling to allow different operations to be performed on the package representations. Numerous tooling(eg those that allow operation on files) that already exists can be leveraged to perform various operations on package representations. |
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.
Is it reasonable to call out removing sqllite as a non-goal?
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.
@timflannagan but we do want to remove sqllite. Not sure if that classifies as a non-goal from the perspective of this enhancement.
|
||
### Story 4 | ||
|
||
Given a set of package representations, I can author a new index using just those representations. |
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.
Can we add additional context here, maybe as a bash comment like in the previous user stories? It looks like we're removing service mesh from the index and rebuilding the index, but it wasn't obvious on the first read.
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.
It looks like you also mentioned this above In the second story in that last sentence, but I still think it would be easier for first time readers if this was described here as well.
|
||
A new sub-command `inspect` will be introduced under `opm index`. | ||
|
||
When `opm index inspect` is summoned, the index image will be downloaded, and the json representations of the packages inside the index will be unfurled in a folder with the same name as the index. |
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.
Minor nit: I think this behavior, where we unpackage the configuration files within an index and create a directory that matches the index name needs to be introduced earlier in the enhancement.
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.
It is introduced in the first sentence of Story 1.
|
||
Currently, a bundle can be added into the index using `opm index add --bundles <list-of-bundle-paths> --mode replaces|semver|semver-skippatch --tag=<index-image-tag>` where the bundle images (like `quay.io/operatorhubio/etcd:v0.9.0`) are included in `<list-of-bundle-path>`. The bundle can also mention bundles it can be upgrade from using the `skips` or `skipsRange` fields in the bundle ClusterServiceVersion. With all of these information provided, the upgrade graph of the bundles in the package is calculated and stored in the `channel_entry` table of the sql database that is built inside the index, while the `skips`/`skiprange` information is persisted in the `operatorbundle` table. The `channel_entry` table is always authoritative in terms of calculating the upgrade graph in a package. The operatorbundle table persists the values of `skips`/`skipsrange`/`replaces` when the bundle was first unpacked/added to the index, and other index operations could have changed the real graph in `channel_entry`. | ||
|
||
```bash |
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.
Minor nit: you may want to just specify sql
as the language for this block.
} | ||
] | ||
} | ||
$ opm index add --bundles quay.io/operatorhubio/etcd:v0.9.0 --mode replaces --config community-operators/etcd |
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'm a bit confused on what kind of value is --config
looking for here. You mentioned above that we can provide the parent configuration to opm index add ...
but is that parent configuration the actual filename, e.g. --config community-operators/etcd.json
, or just the name of the package itself like in this current example?
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.
The value of --configs
will be the path of the directory containing the package config files in your file system.
1. The json blob capturing package information. | ||
```json | ||
{ | ||
"schema": "olm.package", |
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.
Should we include schema version identifier here as well?
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.
an older version did, but I think we can say that unadorned schema is v1 and if we need a v2 it will be "olm.package.v2"
(the fact we have an extension mechanism via properties means we may never need to bump a version)
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 had suggested adding a new user story for being able to prune indexes, I see story 4 covers the case for being able to select whole packages, so that covers part of the story.
I would like to see that extended to be able to select individual bundles from those packages (to be able to prune based on what version of an operator I want in my catalog), and for the tooling to be able to verify/satisfy dependencies.
Is this something we'd want to support in opm, or would we want tooling to work directly on the JSON instead, and have a workflow where the user extracts the json files then processes them before creating a new index?
The use case is for mirroring, where users who need to mirror an operator and its dependencies currently have quite a difficult job to do.
In looking at it, I also note that we don't have a single example of how dependency information will be store in the olm.bundle
json objects - that's something we should have.
@Jamstah I think being able to prune based on bundle versions or dependency relationship makes sense. However it's outside of the scope of this enhancement I believe. We should have all the data in the index to do that with |
34d89b1
to
cceee99
Compare
@Jamstah you can select individual bundles within those packages and prune them, although as @dmesser mentioned the tooling to support verifying dependency satisfiablity, channel upgrade graph validity etc is out of scope for this enhancement and will be considered in a separate enhancement. I've updated the Non-goals section of this enhancement to have this statement on record. |
/approve |
362fe84
to
554e275
Compare
554e275
to
a8e1319
Compare
{ | ||
"name": "olm.package", | ||
"values": { | ||
"packageName": "etcd", | ||
"version": "0.9.0" | ||
} | ||
}, |
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.
This may have already been discussed, but I'm curious if it would make sense/be possible to make the property type simpler? Trying to model what's described in this EP, we would need:
type Property struct {
Name string
Values map[string]string // Optional: only used with olm.gvk and olm.package
Type string // Optional: only used with olm.gvk
Replaces string // Optional: only used with olm.channel
Value string // Optional: only used with olm.channel
}
I would propose simplifying to:
type Property struct
Name string
Type string // Optional: only used with olm.gvk and olm.package
Values map[string]string
}
- I think
olm.package
actually needsprovided
andrequired
types as well to distinguish between required packages (fromdependencies.yaml
) and provided packages. - For
olm.channel
, I would propose movingreplaces
into thevalues
map:values: { name: "my-channel", replaces: "my-operator.v0.0.1"}
.
{ | ||
"name": "olm.package", | ||
"values": { | ||
"packageName": "etcd", | ||
"version": "0.9.2" | ||
} | ||
}, |
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.
This seems like duplicative information, since we already capture package name and version in the root of the olm.bundle
schema. Is it necessary for this property to be explicitly included?
Also related. I didn't see any mention of dependencies.yaml
in this EP. Do package dependencies need to be accounted for? If so, is there an example for how that looks?
}, | ||
"channels": ["alpha", "singlenamespace-alpha", "clusterwide-alpha"], | ||
"description": "A message about etcd operator, a description of channels" | ||
}, |
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.
Is this file expected to be a JSON list of maps (i.e [ {}, {}, {} ]
) or a just concatenated JSON documents (i.e. {}{}{}{}
)?
It seems like the commas between documents without the surrounding square brackets would make this file format cumbersome to parse.
No description provided.