✨ remove unnecessary flag, optimize catalog watch handler, stop watching non-existent unpack pods #941

joelanford · 2024-06-14T18:57:17Z

Description

Reviewer Checklist

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

netlify · 2024-06-14T18:57:33Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`3b172c3`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/666d9ade619dfe000826edff
😎 Deploy Preview	https://deploy-preview-941--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-06-14T19:03:31Z

Codecov Report

Attention: Patch coverage is 81.48148% with 10 lines in your changes missing coverage. Please review.

Project coverage is 80.41%. Comparing base (f301f55) to head (3b172c3).

Files	Patch %	Lines
...nternal/controllers/clusterextension_controller.go	62.50%	5 Missing and 1 partial ⚠️
cmd/manager/main.go	89.47%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #941      +/-   ##
==========================================
+ Coverage   79.34%   80.41%   +1.06%     
==========================================
  Files          16       16              
  Lines        1104     1103       -1     
==========================================
+ Hits          876      887      +11     
+ Misses        158      150       -8     
+ Partials       70       66       -4

Flag	Coverage Δ
e2e	`59.20% <81.48%> (+1.05%)`	⬆️
unit	`53.85% <1.85%> (+0.13%)`	⬆️

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

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

varshaprasad96 · 2024-06-14T19:12:23Z

internal/controllers/clusterextension_controller.go

@@ -574,7 +572,6 @@ func (r *ClusterExtensionReconciler) SetupWithManager(mgr ctrl.Manager) error {
 				return true
 			},
 		}).
-		Watches(&corev1.Pod{}, mapOwneeToOwnerHandler(mgr.GetClient(), mgr.GetLogger(), &ocv1alpha1.ClusterExtension{})).


Since we're on it - we should also be able to remove the rbac to watch pods:

operator-controller/internal/controllers/clusterextension_controller.go

Lines 105 to 107 in cb63023

//+kubebuilder:rbac:groups=core,resources=pods,verbs=list;watch;create;delete

//+kubebuilder:rbac:groups=core,resources=configmaps,verbs=list;watch

//+kubebuilder:rbac:groups=core,resources=pods/log,verbs=get

.

If not in this PR, it can be done in follow up too

I can do it now.

Pushed a change. I removed the pods stuff, and changed the configmaps RBAC (what was that for?) to secrets since the helm stuff needs to manage release secrets.

It's all sorta moot though because:

We give ourselves */*/* in the next line.

We'll eventually drop the secrets permission here because we'll expect the SA to have the necessary permission to handle their own release secret.

and changed the configmaps RBAC (what was that for?)

I think it may have gotten ported over from Extension, where Kapp needed CRUD permissions for configmaps.

varshaprasad96

/lgtm
/approve

tmshort · 2024-06-14T20:40:40Z

Looks like 'make verify' or equivalent is needed.
/hold

joelanford · 2024-06-14T21:05:00Z

Oops forgot to re-gen RBAC. Will do!

joelanford · 2024-06-14T23:55:26Z

/hold cancel

- remove unnecessary flag - optimize catalog watch handler - fix finalizers (also stop using rukpak-based finalizers and keys) - Add some reminder TODO comments about more improvements (need to convert to issues) Signed-off-by: Joe Lanford <joe.lanford@gmail.com>

joelanford · 2024-06-15T23:21:58Z

internal/controllers/clusterextension_registryv1_validation_test.go

@@ -116,6 +117,7 @@ func TestClusterExtensionRegistryV1DisallowDependencies(t *testing.T) {
 				ActionClientGetter:    helmClientGetter,
 				Unpacker:              unpacker,
 				InstalledBundleGetter: mockInstalledBundleGetter,
+				Finalizers:            crfinalizer.NewFinalizers(),


It's a bit disconcerting to me that so much of the ClusterExtensionReconciler can be constructed so differently than the real one can. We should discuss further, but off the top of my head, I wonder if we can use the exact same reconciler as is configured in main.go.

For catalogd we can use httptest to setup a mock catalogd server and use the real ClusterExtension http client.

An image registry is an http server. I wonder if there is an in-process implementation that would be easy to run for test purposes.

Use t.TempDir() subdirectories for any necessary local storage and caching that happens during reconcile.

For the kubernetes cluster, we use envtest.

If we had that setup, we wouldn't actually need to configure the ClusterExtension reconciler any differently.

More immediately, we should have unit tests that make sure finalizers are actually being handled properly during reconcile.

It's a bit disconcerting to me that so much of the ClusterExtensionReconciler can be constructed so differently than the real one can.

This is definitely up for discussion and needs separate set of effort. The whole point of these unit tests is to make sure that the reconciler code path traversed during various instances is as expected. Which is why we ensure that the mock unpacker returns quickly as expected. The code coverage for unpacking and storage has separate unit tests (currently in rukpak).

I'm not sure if we immediately need to focus on this - rather if possible try finishing up #879 as immediately as possible (even if it requires us to mock some of the components) to ensure we don't skip any nuances in the reconciler code.

varshaprasad96

/lgtm
/approve

varshaprasad96 · 2024-06-17T12:33:31Z

internal/controllers/clusterextension_registryv1_validation_test.go

@@ -116,6 +117,7 @@ func TestClusterExtensionRegistryV1DisallowDependencies(t *testing.T) {
 				ActionClientGetter:    helmClientGetter,
 				Unpacker:              unpacker,
 				InstalledBundleGetter: mockInstalledBundleGetter,
+				Finalizers:            crfinalizer.NewFinalizers(),


It's a bit disconcerting to me that so much of the ClusterExtensionReconciler can be constructed so differently than the real one can.

This is definitely up for discussion and needs separate set of effort. The whole point of these unit tests is to make sure that the reconciler code path traversed during various instances is as expected. Which is why we ensure that the mock unpacker returns quickly as expected. The code coverage for unpacking and storage has separate unit tests (currently in rukpak).

I'm not sure if we immediately need to focus on this - rather if possible try finishing up #879 as immediately as possible (even if it requires us to mock some of the components) to ensure we don't skip any nuances in the reconciler code.

joelanford requested a review from a team as a code owner June 14, 2024 18:57

varshaprasad96 reviewed Jun 14, 2024

View reviewed changes

joelanford force-pushed the random-cleanup branch 2 times, most recently from 186c4ce to 64420af Compare June 14, 2024 19:39

varshaprasad96 previously approved these changes Jun 14, 2024

View reviewed changes

openshift-ci bot assigned varshaprasad96 Jun 14, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2024

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 14, 2024

joelanford dismissed varshaprasad96’s stale review via e0b00b4 June 14, 2024 23:04

joelanford force-pushed the random-cleanup branch from 64420af to e0b00b4 Compare June 14, 2024 23:04

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2024

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 14, 2024

joelanford force-pushed the random-cleanup branch 2 times, most recently from e437d99 to 1749fb1 Compare June 15, 2024 13:28

joelanford force-pushed the random-cleanup branch from 1749fb1 to 3b172c3 Compare June 15, 2024 13:45

joelanford commented Jun 15, 2024

View reviewed changes

varshaprasad96 approved these changes Jun 17, 2024

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2024

tmshort approved these changes Jun 17, 2024

View reviewed changes

tmshort added this pull request to the merge queue Jun 17, 2024

Merged via the queue into operator-framework:main with commit b4c928c Jun 17, 2024
17 checks passed

joelanford deleted the random-cleanup branch June 18, 2024 00:07

joelanford mentioned this pull request Jun 21, 2024

Fix: finalizers not set on ClusterExtension #942

Closed

joelanford mentioned this pull request Jun 21, 2024

🐛 fix: remove unnecessary bundle cache volume #964

Merged

4 tasks

This was referenced Jul 2, 2024

✨ Adding a token getter to get service account tokens #1006

Merged

Fixes and Improvements for TokenGetter #1013

Closed

✨ Fixes and Improvements for TokenGetter #1014

Merged

skattoju mentioned this pull request Sep 9, 2024

🌱 Descriptive Upgrade Constraint Policy constants #1233

Merged

skattoju mentioned this pull request Sep 25, 2024

✨ Remove unpacked status #1309

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ remove unnecessary flag, optimize catalog watch handler, stop watching non-existent unpack pods #941

✨ remove unnecessary flag, optimize catalog watch handler, stop watching non-existent unpack pods #941

joelanford commented Jun 14, 2024

netlify bot commented Jun 14, 2024 •

edited

Loading

codecov bot commented Jun 14, 2024 •

edited

Loading

varshaprasad96 Jun 14, 2024 •

edited

Loading

joelanford Jun 14, 2024

joelanford Jun 14, 2024

varshaprasad96 Jun 14, 2024

varshaprasad96 left a comment

tmshort commented Jun 14, 2024

joelanford commented Jun 14, 2024

joelanford commented Jun 14, 2024

joelanford Jun 15, 2024

joelanford Jun 15, 2024 •

edited

Loading

varshaprasad96 Jun 17, 2024

varshaprasad96 left a comment

varshaprasad96 Jun 17, 2024

	//+kubebuilder:rbac:groups=core,resources=pods,verbs=list;watch;create;delete
	//+kubebuilder:rbac:groups=core,resources=configmaps,verbs=list;watch
	//+kubebuilder:rbac:groups=core,resources=pods/log,verbs=get

✨ remove unnecessary flag, optimize catalog watch handler, stop watching non-existent unpack pods #941

✨ remove unnecessary flag, optimize catalog watch handler, stop watching non-existent unpack pods #941

Conversation

joelanford commented Jun 14, 2024

Description

Reviewer Checklist

netlify bot commented Jun 14, 2024 • edited Loading

✅ Deploy Preview for olmv1 ready!

codecov bot commented Jun 14, 2024 • edited Loading

Codecov Report

varshaprasad96 Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

joelanford Jun 14, 2024

Choose a reason for hiding this comment

joelanford Jun 14, 2024

Choose a reason for hiding this comment

varshaprasad96 Jun 14, 2024

Choose a reason for hiding this comment

varshaprasad96 left a comment

Choose a reason for hiding this comment

tmshort commented Jun 14, 2024

joelanford commented Jun 14, 2024

joelanford commented Jun 14, 2024

joelanford Jun 15, 2024

Choose a reason for hiding this comment

joelanford Jun 15, 2024 • edited Loading

Choose a reason for hiding this comment

varshaprasad96 Jun 17, 2024

Choose a reason for hiding this comment

varshaprasad96 left a comment

Choose a reason for hiding this comment

varshaprasad96 Jun 17, 2024

Choose a reason for hiding this comment

netlify bot commented Jun 14, 2024 •

edited

Loading

codecov bot commented Jun 14, 2024 •

edited

Loading

varshaprasad96 Jun 14, 2024 •

edited

Loading

joelanford Jun 15, 2024 •

edited

Loading