Add annotations to `BundleDeployment` #442

m1kola · 2023-10-06T10:02:01Z

Description

This adds annotations to BundleDeployment objects created by operator-controller in response to Operator objects.

This makes it easier to filter bundles during resolution and makes us less dependant on image bundle format.

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

codecov · 2023-10-06T10:04:47Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (65bfc69) 84.00% compared to head (799c4e9) 83.79%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #442      +/-   ##
==========================================
- Coverage   84.00%   83.79%   -0.21%     
==========================================
  Files          23       23              
  Lines         844      858      +14     
==========================================
+ Hits          709      719      +10     
- Misses         93       96       +3     
- Partials       42       43       +1

Flag	Coverage Δ
e2e	`65.96% <69.44%> (-0.15%)`	⬇️
unit	`76.68% <77.77%> (-0.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...ternal/catalogmetadata/filter/bundle_predicates.go	`92.85% <100.00%> (ø)`
...al/resolution/variablesources/bundle_deployment.go	`89.47% <100.00%> (+16.74%)`	⬆️
...al/resolution/variablesources/installed_package.go	`75.00% <83.33%> (+9.78%)`	⬆️
internal/controllers/operator_controller.go	`78.21% <60.00%> (-2.61%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

m1kola · 2023-10-06T10:05:02Z

internal/resolution/variablesources/installed_package.go

-	sort.SliceStable(resultSet, func(i, j int) bool {
-		return catalogsort.ByVersion(resultSet[i], resultSet[j])
-	})


I think we no longer need sorting by version since we filter bundles by bundle version amongst other things. I assume previously we were doing sorting beucase multiple bundles could in theory use the same image (.e.g some sort of fake version).

Sorting, I think, was to make sure the resolver favored high semver versions over low semver versions. Maybe different context though?

For upgrade edges there is a separate sorting below in this file (not in the diff) which is still needed.

This piece of code is where we look for a currently installed bundle by image ref. I suspect that there may be multiple bundles with the same image, but different versions 🤷‍♂️. So in the current implementation we need sorting to prefer higher version.

But when filtering bundles by of package name, bundle name and bundle version we should be able to uniquely identify a bundle. So I expect there always will be 1 or 0 bundles.

@jmprusi might be able to give some more context

Got it. It looks like we check the result set for 0 bundles, but not more than 1. Can we add another check that errors if there is more than 1 bundle?

m1kola · 2023-10-06T10:11:29Z

internal/resolution/variablesources/installed_package.go

-	// TODO: fast follow - we should check whether we are already supporting the channel attribute in the operator spec.
-	//       if so, we should take the value from spec of the operator CR in the owner ref of the bundle deployment.
-	//       If that channel is set, we need to update the filter above to filter by channel as well.


I believe this is no longer relevant given that we filter by package name, bundle name and bundle version.

m1kola · 2023-10-06T10:37:27Z

internal/resolution/variablesources/bundle_deployment.go

+		bundleName := bundleDeployment.Annotations[AnnotationBundleName]
+		bundleVersion := bundleDeployment.Annotations[AnnotationBundleVersion]
+		if pkgName == "" || bundleName == "" || bundleVersion == "" {
+			continue


Do we need to care about bundle deployments created directly? These annotations will only be present on bundle deployments reconciled by operator-controller.

Edit: I think we do need to care so this whole thing might not fly.

For now, I don't think we need to care, at least in this context.

I sorta think this context is: is there a BD for this operator already? (And maybe that means we should have a label on the BD like operators.operatorframework.io/operator-name whose value is Operator.metadata.name?

Perhaps in the future, there's a separate feature that helps the resolver become aware of other BDs, but I don't think we need to do that now.

joelanford · 2023-10-09T13:43:44Z

internal/controllers/operator_controller.go

+			"annotations": map[string]string{
+				variablesources.AnnotationPackageName:   bundle.Package,
+				variablesources.AnnotationBundleName:    bundle.Name,
+				variablesources.AnnotationBundleVersion: bundleVersion.String(),
+			},


Should we make these labels (rather than annotations) so that we can use a label selector to populate our BD cache with just BDs that we are managing?

@joelanford I think we already do that but in a different way.

We set owner reference on a BD here:

operator-controller/internal/controllers/operator_controller.go

Lines 297 to 306 in 205a487

bd.SetOwnerReferences([]metav1.OwnerReference{

{

APIVersion: operatorsv1alpha1.GroupVersion.String(),

Kind: "Operator",

Name: o.Name,

UID: o.UID,

Controller: pointer.Bool(true),

BlockOwnerDeletion: pointer.Bool(true),

},

})

And only reconcile owned objects:

operator-controller/internal/controllers/operator_controller.go

Line 316 in 205a487

Owns(&rukpakv1alpha1.BundleDeployment{}).

Kubernetes does not support setting up informers to filter on the server-side using owner references though. So my suggestion is to use labels so that we can also configure the cache to select only BDs that have the expected label set.

internal/controllers/operator_controller.go

This adds annotations to `BundleDeployment` objects created by operator-controller in response to `Operator` objects. This makes it easier to filter bundles during resolution and makes us less dependant on image bundle format. Signed-off-by: Mikalai Radchuk <[email protected]>

m1kola · 2023-10-12T16:04:02Z

I've been thinking more about this:

In main we have BundleDeploymentVariableSource which looks into .Spec.Template.Spec.Source.Image.Ref to find an image we currently have installed. I think it should look at the actual state instead of a desired state. We already have .Status.InstalledBundleResource on Operator and can probably use this instead.
I think instead of adding labels/annotations into BundleDeployments we should probably add this information into Operator's status. This will also solve Surface information about installed version in Operator CR #270

I'll bring this up for a discussion and will create a new PR or repoen this one based on the outcome.

Closing for now.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 6, 2023

m1kola force-pushed the bundledeployment_annotations branch from 5aab955 to 7d2a8f7 Compare October 6, 2023 10:02

m1kola force-pushed the bundledeployment_annotations branch 2 times, most recently from 6b5528c to e30a14f Compare October 6, 2023 10:35

m1kola commented Oct 6, 2023

View reviewed changes

m1kola force-pushed the bundledeployment_annotations branch from e30a14f to 799c4e9 Compare October 6, 2023 10:42

joelanford reviewed Oct 9, 2023

View reviewed changes

stevekuznetsov reviewed Oct 10, 2023

View reviewed changes

internal/controllers/operator_controller.go Outdated Show resolved Hide resolved

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2023

m1kola force-pushed the bundledeployment_annotations branch from 799c4e9 to 67ccd0f Compare October 12, 2023 15:47

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 12, 2023

m1kola closed this Oct 12, 2023

This was referenced Oct 18, 2023

Remove randomness in package uniqueness ordering #461

Merged

Possibility of an upgrade from one package to another #466

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add annotations to `BundleDeployment` #442

Add annotations to `BundleDeployment` #442

m1kola commented Oct 6, 2023

codecov bot commented Oct 6, 2023 •

edited

Loading

m1kola Oct 6, 2023

joelanford Oct 6, 2023 •

edited

Loading

m1kola Oct 6, 2023 •

edited

Loading

joelanford Oct 9, 2023

m1kola Oct 6, 2023

m1kola Oct 6, 2023 •

edited

Loading

joelanford Oct 9, 2023

joelanford Oct 9, 2023

m1kola Oct 11, 2023

joelanford Oct 11, 2023

m1kola commented Oct 12, 2023

	bd.SetOwnerReferences([]metav1.OwnerReference{
	{
	APIVersion: operatorsv1alpha1.GroupVersion.String(),
	Kind: "Operator",
	Name: o.Name,
	UID: o.UID,
	Controller: pointer.Bool(true),
	BlockOwnerDeletion: pointer.Bool(true),
	},
	})

Add annotations to BundleDeployment #442

Add annotations to BundleDeployment #442

Conversation

m1kola commented Oct 6, 2023

Description

Reviewer Checklist

codecov bot commented Oct 6, 2023 • edited Loading

Codecov Report

m1kola Oct 6, 2023

Choose a reason for hiding this comment

joelanford Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

m1kola Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

joelanford Oct 9, 2023

Choose a reason for hiding this comment

m1kola Oct 6, 2023

Choose a reason for hiding this comment

m1kola Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

joelanford Oct 9, 2023

Choose a reason for hiding this comment

joelanford Oct 9, 2023

Choose a reason for hiding this comment

m1kola Oct 11, 2023

Choose a reason for hiding this comment

joelanford Oct 11, 2023

Choose a reason for hiding this comment

m1kola commented Oct 12, 2023

Add annotations to `BundleDeployment` #442

Add annotations to `BundleDeployment` #442

codecov bot commented Oct 6, 2023 •

edited

Loading

joelanford Oct 6, 2023 •

edited

Loading

m1kola Oct 6, 2023 •

edited

Loading

m1kola Oct 6, 2023 •

edited

Loading