[BD_annotations] Add relevant Operator level annotations on BD #274
Conversation
This commit makes Operator controller to add annotations on BundleDeployments realted to operator version and channel. Will be useful for runtime based resolutions. Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
| @@ -272,6 +287,10 @@ func (r *OperatorReconciler) generateExpectedBundleDeployment(o operatorsv1alpha | |||
| "kind": rukpakv1alpha1.BundleDeploymentKind, | |||
| "metadata": map[string]interface{}{ | |||
| "name": o.GetName(), | |||
| "annotations": map[string]string{ | |||
| "operator_version": annotations.version, | |||
There was a problem hiding this comment.
The issue also mentions about adding the name of the operator that is being installed. That can be obtained from the operator's spec. However that may cause an additional client call, hence it can be added here if that makes it easier.
| channel, err := entity.ChannelName() | ||
| if err != nil || channel == "" { | ||
| errs = append(errs, fmt.Errorf("unable to find the channel name from resolved entity: %v", err)) | ||
| channel = "unknown" | ||
| } | ||
| bdm.channel = channel |
There was a problem hiding this comment.
I would not expect the resolver to need to know the channel that the currently installed bundle was resolved from because it doesn't really matter.
The channel is just a filtering mechanism. If the same bundle shows up in multiple channels, it is still the same bundle. I think this points to a flaw in our entity generation and channel selection logic. I would expect there to be a single entity per bundle and that entity would include a list of channels that it appears in. Then the channel constraint would interrogate that list to check to see if the desired channel is present.
There was a problem hiding this comment.
I'm probably missing something, but I don't exactly agree. I think the channel matters because, without it, you could upgrade to a bundle in a different channel (e.g. out of stable) and it might put the user on an upgrade path that they don't necessarily want to have. Especially in the case of skips and skips range. You could easily go from the top of the stable channel to the top of the alpha.
There was a problem hiding this comment.
I think having a single bundle entity and multiple/channel is 6 or 1/2 a dozen. It just means we'd need to model the channels and packages as variables as well.
package has a dependency on channels (sorted by default then alphabetical..?), channels have dependencies on bundles. We'd then need to add a mandatory constraint to the required package to force the resolver to pick a channel (and if none are specified the sorting should push the resolver towards the default channel). Then the required-package variable would have a dependency against all the bundles (reverse version order), and the top most version of the chosen channel will be picked. The alternative is to have one bundle per channel, and sort the dependencies based on channel and version. But, I think the result is the same. wdyt?
There was a problem hiding this comment.
@perdasilva and I chatted, and I think we got onto the same page? (correct me if I'm wrong)
- For each bundle, make a variable. The bundle variable does not carry any channel information within itself. The bundle carries its identity (the image ref?)
- For each channel, make a variable. A channel variable knows the bundles that are the members of its channel, so it can generate a dependency constraint like
C = DependsOn(B4, B3, B2, B1), whereB*are the IDs of the bundle variables that correspond to the channel members. If the user specified this channel, the channel variable is marked as mandatory (i.e.X = Mandatory(C)).
These above two items would essentially replicate our existing logic. But this model could be extended to support other kinds of user-defined channel selection, where -- for example -- set-based choices can be made (e.g. in channels 1 and 2, but not 3, in which case 1 and 2 are mandatory and 3 is prohibited.
Then, when we move on to honoring the upgrade edges, the $thing that defines the upgrade edges (in our current FBC model, a channel) has a variable source that (a) figures out the variable ID corresponding to the bundle that is installed, and (b) figures out the variable IDs of all of the possible successors of the currently installed variable ID. So if B1 is installed, and B2 and B3 are both successors of B1, then you'd end up with U = DependsOn(B3, B2, B1), Y = Mandatory(U) -> translation: "I can upgrade to B3 or B2, and I can also stay on the current version, B1"
Getting back to the comments:
I think the channel matters because, without it, you could upgrade to a bundle in a different channel (e.g. out of stable) and it might put the user on an upgrade path that they don't necessarily want to have.
It needs to be possible to upgrade to a different channel though. It doesn't matter what channel a bundle was installed from. What matters is:
- What channel does the user want now?
- Is there an upgrade edge from the currently installed version to a bundle in the channel that I want now? It doesn't and shouldn't matter if the currently installed bundle is also in the channel I want.
In fact, the channel of the currently installed bundle doesn't even make sense, really. Consider this fairly standard scenario:
- We're resolving an
Operatorwith justspec.package: foo - Package
foohas two channels,c1andc2, each containing bundlebar - We resolve
bar. Hooray!
Which channel did we resolve bar from? Conceptually channel had no bearing on the resolution. Why would we arbitrarily (from the user's perspective) pick a channel at this point and then use that in future resolutions?
If, at a later point, a new catalog shows up that has channel c3 with a single member baz and baz skips bar, we absolutely want a user to be able to be upgraded to baz if they specify channel: c3 or if they don't specify a channel at all.
TL;DR:
What this all boils down to, IMO, is that our current model for channels serve two purposes:
- Operator author-defined subset of bundles in a package (which can be used by a cluster admin to filter resolution)
- As a provider of operator author-defined upgrade edges (which are used by default during resolution to allow only certain transitions)
These are disjoint concerns, in my opinion. I think we should model set-based channel membership filtering completely separate from upgrade edge filtering. With the current FBC schemas, they both originate from the same place, but that doesn't mean we need to co-mingle the implementations.
I could see a future where FBC schemas look like this:
---
schema: olm.channel.v2
entries:
- bundle1
- bundle2
- bundle3
---
schema: olm.upgradeEdges
edges:
- from: bundle1
to: bundle2
- from: bundle1
to: bundle3
- from: bundle2
to: bundle3
| errs = append(errs, fmt.Errorf("unable to find the version of operator to be installed from resolved entity: %v", err)) | ||
| version = "unknown" | ||
| } | ||
| bdm.version = version |
There was a problem hiding this comment.
I'm honestly not even sure an annotation-based approach is the right one. The thing that uniquely identifies the thing that we have installed is the bundle image SHA which is already in the BD.
Could the resolver not:
- Find the current image sha of the BD
- Locate all entities whose image == the current image sha
- Find the entities that say they can upgrade from the entities we found in (2)
- Then build a constraint such that the resolved entity must come from set 2 (currently installed) or set 3 (successors of currently installed)
There was a problem hiding this comment.
We would need to think about the ramifications of various scenarios of the size of set (2):
- Size 0: we don't know what's installed, therefore can't upgrade
- Size 1: we know what's installed, therefore we can upgrade or stick to what we're on
- Size >1: there are multiple entities that specify the image ref we're looking for, but we don't know which one we derived the BD from. There are other aspects of the entity (e.g.
olm.bundle.mediatype) that impact the BD, so we would NOT want to end up selecting an entity that resulted in a side-grade to a different BD spec (despite the same bundle image)
There was a problem hiding this comment.
That could be possible, but it could introduce ambiguity. I.e. you could have two bundles pointing to the same image sha, because of reasons. That's why I'd argue it would be better to have an exact 1:1 between the BD and the underlying bundle/entity. Maybe the sane alternative is to rather than break it down along the parameters of the bundle (package, version, channel), just use the entities unique ID. Then we're safe no matter what.
There was a problem hiding this comment.
@perdasilva Agree to what you have mentioned. But I still have questions on the implementation part of it to make it possible.
Locate all entities whose image == the current image sha
How do we work back on locating the entities when they (or neither the solution set) is persisted anywhere during the reconciliation. We would still need to query for the installed BDs on the cluster and provide the necessary info to re-calculate the entities? The installed bundle can be from a catsrc which is not even available on cluster.
There was a problem hiding this comment.
The entity should have an unique id, which I guess is what we should store in the annotations. Then, you could just use the entitysource.Get(id) method to get the entity back on the next resolution. Would that work?
There was a problem hiding this comment.
Straw man: the unique ID is the image SHA.
- The image SHA is content-addressable. If anything about the bundle changes, the SHA changes.
- The bundle image contains ALL of the metadata used for resolution and there is no way to change that metadata without changing the SHA, so if we see an image SHA twice in the catalog, we also know for sure that its resolution parameters are identical.
There was a problem hiding this comment.
We would still need to query for the installed BDs on the cluster and provide the necessary info to re-calculate the entities?
I think that's what I'm suggesting, yes.
The installed bundle can be from a catsrc which is not even available on cluster.
And that's okay. There are two potential scenarios there:
- Nothing currently on the cluster says it upgrades from it, so we just don't upgrade it
- Some other catalog source has an upgrade edge that specifies the currently installed bundle SHA as the
fromversion, which case we can upgrade to thetoversion.
There was a problem hiding this comment.
This is definitely debatable though. I think I could also make a pretty good argument for the unique ID being packageName + bundleVersion + bundleRelease
And maybe if there are two bundles with that same 3 tuple available on the catalog, we fail resolution and ask the user to do provide a includes or excludes configuration on either an entire catalog source or on a catsrc + package tuple.
There was a problem hiding this comment.
The entity should have an unique id, which I guess is what we should store in the annotations. Then, you could just use the entitysource.Get(id) method to get the entity back on the next resolution. Would that work?
If we do that, I think we need a spec for entity ID, and we need to consider what happens if we can't find that entity ID we stored in the BD annotation during the next resolution. For example, right now the catalogd entity source includes catalog.metadata.name in the entity ID. If someone changes the name of a catalog, we probably don't want resolution to stop working for any entities that were installed from the previous name.
|
I'm getting the feeling that this is a problem that we need to noodle on a design for. This honestly feels like it requires a Brief and multiple follow-on RFCs (to use our new favorite terminology) |
|
should we close this PR for now? I think more thought it needed, and we solved the upgrade issue without needing the annotations (we just used the bundle id and documented the limitations). I think that's sufficient for now as we work out the nitty gritty. |
Fair! Closing this |
This commit makes Operator controller to add annotations on BundleDeployments realted to operator version and channel. This will be useful for runtime based resolutions to identify the BDs that are installed by the operator controller and already running on cluster.
Relates to: #268
Description
Reviewer Checklist