[BD_annotations] Add relevant Operator level annotations on BD #274
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,6 +52,13 @@ type OperatorReconciler struct { | |
| Resolver *solver.DeppySolver | ||
| } | ||
|
|
||
| // bundleDeploymentMetadata serves as an extensible struct holding | ||
| // any metadata that is to be added to bundleDeployments. | ||
| type bundleDeploymentMetadata struct { | ||
| channel string | ||
| version string | ||
| } | ||
|
|
||
| //+kubebuilder:rbac:groups=operators.operatorframework.io,resources=operators,verbs=get;list;watch | ||
| //+kubebuilder:rbac:groups=operators.operatorframework.io,resources=operators/status,verbs=get;update;patch | ||
| //+kubebuilder:rbac:groups=operators.operatorframework.io,resources=operators/finalizers,verbs=update | ||
|
|
@@ -73,7 +80,7 @@ func (r *OperatorReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c | |
| } | ||
|
|
||
| reconciledOp := existingOp.DeepCopy() | ||
| res, reconcileErr := r.reconcile(ctx, reconciledOp, l) | ||
|
|
||
| // Do checks before any Update()s, as Update() may modify the resource structure! | ||
| updateStatus := !equality.Semantic.DeepEqual(existingOp.Status, reconciledOp.Status) | ||
|
|
@@ -113,7 +120,7 @@ func checkForUnexpectedFieldChange(a, b operatorsv1alpha1.Operator) bool { | |
| // to return different results (e.g. requeue). | ||
| // | ||
| //nolint:unparam | ||
| func (r *OperatorReconciler) reconcile(ctx context.Context, op *operatorsv1alpha1.Operator, log logr.Logger) (ctrl.Result, error) { | ||
| // validate spec | ||
| if err := validators.ValidateOperatorSpec(op); err != nil { | ||
| // Set the TypeInstalled condition to Unknown to indicate that the resolution | ||
|
|
@@ -172,9 +179,17 @@ func (r *OperatorReconciler) reconcile(ctx context.Context, op *operatorsv1alpha | |
| setInstalledStatusConditionFailed(&op.Status.Conditions, err.Error(), op.GetGeneration()) | ||
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| // Get annotations which needs to be added to the bundleDeployment. | ||
| // An error here need not re-trigger a reconcile. | ||
| bundleDeploymentAnnotations := &bundleDeploymentMetadata{} | ||
| if err = bundleDeploymentAnnotations.CompleteBundleDeploymentMetadata(bundleEntity); err != nil { | ||
| log.Error(err, "unable to fetch annotations from the resolved bundleEntity") | ||
| } | ||
|
|
||
| // Ensure a BundleDeployment exists with its bundle source from the bundle | ||
| // image we just looked up in the solution. | ||
| dep := r.generateExpectedBundleDeployment(*op, bundleImage, bundleProvisioner, bundleDeploymentAnnotations) | ||
| if err := r.ensureBundleDeployment(ctx, dep); err != nil { | ||
| // originally Reason: operatorsv1alpha1.ReasonInstallationFailed | ||
| op.Status.InstalledBundleResource = "" | ||
|
|
@@ -260,7 +275,7 @@ func (r *OperatorReconciler) getBundleEntityFromSolution(solution *solver.Soluti | |
| return nil, fmt.Errorf("entity for package %q not found in solution", packageName) | ||
| } | ||
|
|
||
| func (r *OperatorReconciler) generateExpectedBundleDeployment(o operatorsv1alpha1.Operator, bundlePath string, bundleProvisioner string, annotations *bundleDeploymentMetadata) *unstructured.Unstructured { | ||
| // We use unstructured here to avoid problems of serializing default values when sending patches to the apiserver. | ||
| // If you use a typed object, any default values from that struct get serialized into the JSON patch, which could | ||
| // cause unrelated fields to be patched back to the default value even though that isn't the intention. Using an | ||
|
|
@@ -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, | ||
| "operator_channel": annotations.channel, | ||
| }, | ||
| }, | ||
| "spec": map[string]interface{}{ | ||
| // TODO: Don't assume plain provisioner | ||
|
|
@@ -459,3 +478,27 @@ func operatorRequestsForCatalog(ctx context.Context, c client.Reader, logger log | |
| return requests | ||
| } | ||
| } | ||
|
|
||
| // Complete fills in the annotations from the information received from | ||
| // bundleEntities. | ||
| func (bdm *bundleDeploymentMetadata) CompleteBundleDeploymentMetadata(entity *entity.BundleEntity) error { | ||
| var errs []error | ||
|
|
||
| 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 | ||
|
Comment on lines
+487
to
+492
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)
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 Getting back to the comments:
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:
In fact, the channel of the currently installed bundle doesn't even make sense, really. Consider this fairly standard scenario:
Which channel did we resolve If, at a later point, a new catalog shows up that has channel TL;DR: What this all boils down to, IMO, is that our current model for channels serve two purposes:
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 |
||
|
|
||
| var version string | ||
| semverVer, err := entity.Version() | ||
| version = semverVer.String() | ||
| if err != nil || version == "" { | ||
| 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:
There was a problem hiding this comment. We would need to think about the ramifications of various scenarios of the size of set (2):
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.
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.
There was a problem hiding this comment.
I think that's what I'm suggesting, yes.
And that's okay. There are two potential scenarios there:
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 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 There was a problem hiding this comment.
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 |
||
|
|
||
| return utilerrors.NewAggregate(errs) | ||
| } | ||
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 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.