operator-framework / enhancements

Apache License 2.0
9 stars 40 forks source link

Add declarative index config enhancement #37

Closed anik120 closed 3 years ago

benluddy commented 4 years ago

Could you add a .md extension to the new file? GitHub isn't giving me the option to view it as rendered markdown.

anik120 commented 4 years ago

@benluddy oopsy. Added the .md extension.

anik120 commented 4 years ago

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.

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

dmesser commented 4 years ago

@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?

anik120 commented 4 years ago

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 I like the idea of them being two separate enhancements, so that they're self contained features with the declarative index config being the building block for opm content management. I also like the idea of mentioning that this enhancement is the building block for the opm content managment enhancement and vice versa, so linking the two from each other seems like a good idea.

Note that the opm content management PR will be updated after this enhancement has been mostly agreed upon.

anik120 commented 4 years ago

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

njhale commented 4 years ago

It would be nice to capture the user stories here as well.

anik120 commented 4 years ago

@njhale @dmesser @benluddy updated the enhancement to lead with user stories instead of implementation details. PTAL, thanks!

jmccormick2001 commented 4 years ago

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.

anik120 commented 4 years ago

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?

@jmccormick2001 were you talking about something more like this:

$ cat etcd.json
"name": "etcd",
        "defaultChannel": "stable",
        "icon": "asdfasdfoipuasdfvasdfqer",
        "channels": [
            {
                "name": "alpha",
                "bundles": [
                    {
                        "version": "v0.0.1",
                        "bundlePath": "quay.io/random/etcdv0.0.1",
                        "upgradeMode": "semver",
                        "ownedAPIs": [
                            {
                                "group": "etcd.database.coreos.com",
                                "version": "v1alpha1",
                                "kind": "etcd"
                            }
                        ],
                        "requiredAPIs": []
                    },
                ]
            },
            {
                "name": "stable",
                "bundles": [
                    {
                        "version": "v0.0.2",
                        "path": "quay.io/somethingelse/etcdv0.0.2",
                        "upgradeMode": "semver",
                        "ownedAPIs": [
                            {
                                "group": "etcd.database.coreos.com",
                                "version": "v1alpha1",
                                "kind": "etcd"
                            }
                        ],
                        "requiredAPIs": []
                    }
                ]
            }
        ]
    }

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.

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. Once this enhancement is finalized, the opm content management enhancement will discuss ways to leverage the declarative index config to provide a way to do tasks like visualizing the upgrade graph.

anik120 commented 4 years ago

@dmesser @bparees @Jamstah the enhancement has been rewritten PTAL when you get a chance.

jmccormick2001 commented 4 years ago

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?

@jmccormick2001 were you talking about something more like this:

$ cat etcd.json
"name": "etcd",
        "defaultChannel": "stable",
        "icon": "asdfasdfoipuasdfvasdfqer",
        "channels": [
            {
                "name": "alpha",
                "bundles": [
                    {
                        "version": "v0.0.1",
                        "bundlePath": "quay.io/random/etcdv0.0.1",
                        "upgradeMode": "semver",
                        "ownedAPIs": [
                            {
                                "group": "etcd.database.coreos.com",
                                "version": "v1alpha1",
                                "kind": "etcd"
                            }
                        ],
                        "requiredAPIs": []
                    },
                ]
            },
            {
                "name": "stable",
                "bundles": [
                    {
                        "version": "v0.0.2",
                        "path": "quay.io/somethingelse/etcdv0.0.2",
                        "upgradeMode": "semver",
                        "ownedAPIs": [
                            {
                                "group": "etcd.database.coreos.com",
                                "version": "v1alpha1",
                                "kind": "etcd"
                            }
                        ],
                        "requiredAPIs": []
                    }
                ]
            }
        ]
    }

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.

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. Once this enhancement is finalized, the opm content management enhancement will discuss ways to leverage the declarative index config to provide a way to do tasks like visualizing the upgrade graph.

yes, that channel definition (or similar) is what I was hoping for!

dmesser commented 4 years ago

@anik120 Why are the ownedAPIs and requiredAPIs present in the update graph definition? Also, isn't the update mode something that we should set on a per channel basis?

anik120 commented 3 years ago

@anik120 Why are the ownedAPIs and requiredAPIs present in the update graph definition?

@dmesser the plan is for all components of OLM to use the json representation of packages inside the built sqllite db it uses currently. Having that information will enable components like dependency resolver to use just the json representation.

anik120 commented 3 years ago

yes, that channel definition (or similar) is what I was hoping for!

@jmccormick2001 updated the enhancement, PTAL, thanks!

dmesser commented 3 years ago

Ah, I see. This will make creating and maintaining the index file more cumbersome, though. Do we think that making the index file not human friendly is outweighed by the benefits that come from saving the bundle extraction for OLM commands?

I am looking at something like this for comparison: https://v3.helm.sh/docs/topics/chart_repository/#the-index-file

On Thu 10. Dec 2020 at 23:39, Anik Bhattacharjee notifications@github.com wrote:

@anik120 https://github.com/anik120 Why are the ownedAPIs and requiredAPIs present in the update graph definition?

@dmesser https://github.com/dmesser the plan https://github.com/operator-framework/enhancements/pull/37/files#diff-1edbfba9fdddd2b74b5cd32c6de146a4f62e1400a93bee78aed2cbf6ebaaa1d7R450 is for all components of OLM to use the json representation of packages inside the built sqllite db it uses currently. Having that information will enable components like dependency resolver to use just the json representation.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/operator-framework/enhancements/pull/37#issuecomment-742844778, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADAT2NNJHLMQHH7QH2BGOXDSUFEYNANCNFSM4OWUY6XA .

-- Daniel Messer

Product Manager Operator Framework & Quay

Red Hat OpenShift

anik120 commented 3 years ago

Ah, I see. This will make creating and maintaining the index file more cumbersome, though. Do we think that making the index file not human friendly is outweighed by the benefits that come from saving the bundle extraction for OLM commands? I am looking at something like this for comparison: https://v3.helm.sh/docs/topics/chart_repository/#the-index-file

On Thu 10. Dec 2020 at 23:39, Anik Bhattacharjee @.***> wrote: @anik120 https://github.com/anik120 Why are the ownedAPIs and requiredAPIs present in the update graph definition? @dmesser https://github.com/dmesser the plan https://github.com/operator-framework/enhancements/pull/37/files#diff-1edbfba9fdddd2b74b5cd32c6de146a4f62e1400a93bee78aed2cbf6ebaaa1d7R450 is for all components of OLM to use the json representation of packages inside the built sqllite db it uses currently. Having that information will enable components like dependency resolver to use just the json representation. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#37 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADAT2NNJHLMQHH7QH2BGOXDSUFEYNANCNFSM4OWUY6XA . -- Daniel Messer Product Manager Operator Framework & Quay Red Hat OpenShift

@dmesser making it as human readable as possible is my primary goal here. However, we're not expecting an end user to create the package representations from scratch by themselves. These will be created for them as a by product of the index add command, and the most they need to interact with the owned/required apis is when they're moving/removing a bundle as a whole. I do propose the option of getting the package representations as either json or yaml files too. Do these points assuage your concern? If not could you elaborate on why you think maintaining the index might get cumbersome with these package representations in the mix?

gallettilance commented 3 years ago

The pipeline will probably maintain these json files in a repo that operator teams can submit PRs against in order to affect changes to the stage and production indexes. This enhancement should include IMO wrapper opm commands that make changes to this config object. For example a team would run locally

    opm index config promote -b=quay.io/my/bundle --from-channel=alpha --to-channel=stable
    opm index config remove -b=quay.io/my/bundle --from-channel=alpha
    opm index config rename-channel --channel=alfa --to=alpha -p=etcd 

to affect the channel promotion, bundle removal, channel renaming etc. before creating a PR. Otherwise this is hardly usable by operator teams managing their own update graph unless they have either a minimal update graph or a very good understanding of OLM mechanics.

bparees commented 3 years ago

@dmesser making it as human readable as possible is my primary goal here. However, we're not expecting an end user to create the package representations from scratch by themselves.

why not? Imho a file format that an operator author can reasonably be expected to create+maintain w/ a text editor should be a starting assumption.

bparees commented 3 years ago

I have a top level concern w/ the direction of this design. My original expectation for "Declarative index config" was that it would be a mechanism independent of "Replaces/skips/skiprange" and "semver", which operator teams could use to define the edges between their bundle versions.

This current design appears to provide for each channel promotion/movement, but it does not allow me to explicitly define edges between bundles, or manage those edges. I'm still stuck w/ the limitations of whatever replaces-mode and semver-mode allow. Am i missing something?

kevinrizza commented 3 years ago

I have a top level concern w/ the direction of this design. My original expectation for "Declarative index config" was that it would be a mechanism independent of "Replaces/skips/skiprange" and "semver", which operator teams could use to define the edges between their bundle versions.

This current design appears to provide for each channel promotion/movement, but it does not allow me to explicitly define edges between bundles, or manage those edges. I'm still stuck w/ the limitations of whatever replaces-mode and semver-mode allow. Am i missing something?

I agree, imo the interesting bit of this design is that it should surface the semantics of the higher order definition that we have in the current registry database today. The abstraction that we use is a little muddy (especially due to skiprange), but at a high level the channel_entry table defines the list of allowed upgrade paths from a given version to another given version. We should have a higher order view into a package's edges and package level metadata that isn't defined in this enhancement yet.

kevinrizza commented 3 years ago

We don't have to reuse it, but https://github.com/openshift/cincinnati-graph-data <- cincinnati has a pretty good definition of the kind of graph level metadata that we want to abstract in a way that is relatively human readable.

anik120 commented 3 years ago

The pipeline will probably maintain these json files in a repo that operator teams can submit PRs against in order to affect changes to the stage and production indexes. This enhancement should include IMO wrapper opm commands that make changes to this config object. For example a team would run locally

    opm index config promote -b=quay.io/my/bundle --from-channel=alpha --to-channel=stable
    opm index config remove -b=quay.io/my/bundle --from-channel=alpha
    opm index config rename-channel --channel=alfa --to=alpha -p=etcd 

to affect the channel promotion, bundle removal, channel renaming etc. before creating a PR.

@gallettilance I think that's exactly what we're trying to avoid. The plan is to give index authors a representation that's easily understandable and thereby modifiable, with existing tooling (grep, sed, whatever's convenient), so that we don't have to keep adding commands to opm and blow the number of sub-commands opm is maintaining

anik120 commented 3 years ago

@dmesser making it as human readable as possible is my primary goal here. However, we're not expecting an end user to create the package representations from scratch by themselves.

why not? Imho a file format that an operator author can reasonably be expected to create+maintain w/ a text editor should be a starting assumption.

@bparees @dmesser updated the enhancement to create two representations per package, one that the index/operator authors will interact with, and the other that will be used by olm components. That helps us service the need of olm components, while maintaining a light UX for end users by getting rid of component important information like owned APIs and required APIs. PTAL

anik120 commented 3 years ago

I have a top level concern w/ the direction of this design. My original expectation for "Declarative index config" was that it would be a mechanism independent of "Replaces/skips/skiprange" and "semver", which operator teams could use to define the edges between their bundle versions. This current design appears to provide for each channel promotion/movement, but it does not allow me to explicitly define edges between bundles, or manage those edges. I'm still stuck w/ the limitations of whatever replaces-mode and semver-mode allow. Am i missing something?

I agree, imo the interesting bit of this design is that it should surface the semantics of the higher order definition that we have in the current registry database today. The abstraction that we use is a little muddy (especially due to skiprange), but at a high level the channel_entry table defines the list of allowed upgrade paths from a given version to another given version. We should have a higher order view into a package's edges and package level metadata that isn't defined in this enhancement yet.

@bparees @kevinrizza updated the enhancement to include channel upgrade information in the package representations, PTAL, thanks!

anik120 commented 3 years ago

@dmesser @kevinrizza updated the enhacement, PTAL, thanks!

anik120 commented 3 years ago

because if teams manage this file directly, that means we have:

  1. operator images get built
  2. team updates json to list new bundle (w/ related image SHAs) (which already means a 2nd step for the team beyond just merging their operator changes)
  3. pipeline updates index w/ json (which will almost certainly fail since the db SHA won't match)

@bparees not sure why we need the related image SHAs step in step 2 separately? Aren't those encoded in the bundle CSV? (eg)

Regarding the SHA check, if it's a hurdle for the pipeline than we can certainly allow overriding of that check as @dmesser mentioned in this comment

bparees commented 3 years ago

@bparees not sure why we need the related image SHAs step in step 2 separately? Aren't those encoded in the bundle CSV?

the SHA in this case is the SHA of the bundle itself, no?

anik120 commented 3 years ago

@bparees not sure why we need the related image SHAs step in step 2 separately? Aren't those encoded in the bundle CSV?

the SHA in this case is the SHA of the bundle itself, no?

@bparees The SHA is the SHA of the related images, not the primary bundle itself.

From the example above, this is the bundle image, these are the related images which differ from the bundle image

dmesser commented 3 years ago

@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 opm in the future. If @anik120 agrees it probably makes sense to open a separate issue for that.

anik120 commented 3 years ago

@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. Also, I've included an example here to illustrate how dependency information will be stored.

kevinrizza commented 3 years ago

/approve