🐛 incremental improvement in catalogmetadata cache/client performance by joelanford · Pull Request #965 · operator-framework/operator-controller

joelanford · 2024-06-21T20:20:37Z

Description

This partially resolves #914 by doing two things:

Exploding the all.json response into separate directories organized by package
Updating the BundleProvider to require a package name to be provided in the request, which results in a much smaller set of FBC that needs to be re-parsed into catalogmetadata objects on each reconcile.

This is still not completely resolved though because step (1) still happens synchronously during the reconciliation of a ClusterExtension. In order to finish the improvement (and to complete #948), we need to implement a controller that watches and reconciles ClusterCatalog changes so that it can create (and delete) caches for operator-controller.

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-21T20:20:54Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`82303d7`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/6679c2f361c8f9000742ebe9
😎 Deploy Preview	https://deploy-preview-965--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.

joelanford · 2024-06-21T20:39:05Z

internal/catalogmetadata/client/client.go

+		// if the catalog has not been successfully unpacked, report an error. This ensures that our
+		// reconciles are deterministic and wait for all desired catalogs to be ready.
 		if !meta.IsStatusConditionPresentAndEqual(catalog.Status.Conditions, catalogd.TypeUnpacked, metav1.ConditionTrue) {
+			errs = append(errs, fmt.Errorf("catalog %q is not unpacked", catalog.Name))


This is a change in semantics. If we ignore catalogs that are not unpacked, users could run into issues if the same package exists in two different catalogs and they are expecting a specific result.

The new semantics are similar to the behavior in OLMv0, and there have been some complaints about it. For example, "I want to install CNV, I don't care if the community catalog is having issues". In order to solve this problem, I think we need to have a knob on the ClusterExtension API to allow users to select which ClusterCatalogs to resolve from (likely using a label selector)

Do we have this captured as an issue somewhere or do you anticipate this falling under the umbrella of #740?

I am going to make a separate issue for this. Just wanted to get consensus on this approach before I did.

joelanford · 2024-06-21T20:41:14Z

internal/catalogmetadata/client/client_test.go

 )

 func TestClient(t *testing.T) {
-	t.Run("Bundles", func(t *testing.T) {


I removed this outer/unnecessary t.Run wrapper (mainly because it caused my IDE to not be able to find the individual test cases to run/debug separately).

In order to review changes in this test, I advise ignoring whitespace.

joelanford · 2024-06-21T20:42:20Z

internal/catalogmetadata/cache/cache_test.go

 )

-func TestCache(t *testing.T) {
-	t.Run("FetchCatalogContents", func(t *testing.T) {


Similar to my other comment, I removed this unnecessary t.Run wrapper. When reviewing, it's much easier to see actual changes if you ignore whitespace.

joelanford · 2024-06-21T20:48:07Z

internal/testutil/fake_catalog_client.go

Note that I moved this into internal. I noticed go-apidiff complain that I changed the Bundles() function signature realized we shouldn't have a test utility in our public API, especially since it returns a slice of objects that are also in an internal package.

grokspawn · 2024-06-21T20:51:18Z

internal/catalogmetadata/cache/cache.go

+	tmpDir, err := os.MkdirTemp(fsc.cachePath, fmt.Sprintf(".%s-", catalog.Name))
 	if err != nil {
-		return nil, fmt.Errorf("error creating cache file for Catalog %q: %s", catalog.Name, err)
+		return nil, fmt.Errorf("error creating temporary directory to unpack catalog metadata: %s", err)


Suggested change

return nil, fmt.Errorf("error creating temporary directory to unpack catalog metadata: %s", err)

return nil, fmt.Errorf("error creating temporary directory to unpack catalog metadata: %v", err)

joelanford · 2024-06-21T21:12:58Z

Makefile

 UNIT_TEST_DIRS := $(shell go list ./... | grep -v /test/)
 test-unit: $(SETUP_ENVTEST) #HELP Run the unit tests
-	eval $$($(SETUP_ENVTEST) use -p env $(ENVTEST_VERSION) $(SETUP_ENVTEST_BIN_DIR_OVERRIDE)) && go test -count=1 -short $(UNIT_TEST_DIRS) -coverprofile cover.out
+	eval $$($(SETUP_ENVTEST) use -p env $(ENVTEST_VERSION) $(SETUP_ENVTEST_BIN_DIR_OVERRIDE)) && CGO_ENABLED=1 go test -count=1 -race -short $(UNIT_TEST_DIRS) -coverprofile cover.out


CGO is required for the race detector. TIL

yeah - got caught on that one rejiggering the v0 Makefile

codecov · 2024-06-22T03:11:36Z

Codecov Report

Attention: Patch coverage is 72.97297% with 20 lines in your changes missing coverage. Please review.

Project coverage is 79.86%. Comparing base (6c61a78) to head (82303d7).
Report is 1 commits behind head on main.

Files	Patch %	Lines
internal/catalogmetadata/cache/cache.go	41.37%	11 Missing and 6 partials ⚠️
internal/catalogmetadata/client/client.go	91.66%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #965      +/-   ##
==========================================
+ Coverage   79.76%   79.86%   +0.09%     
==========================================
  Files          16       17       +1     
  Lines        1107     1157      +50     
==========================================
+ Hits          883      924      +41     
- Misses        155      161       +6     
- Partials       69       72       +3

Flag	Coverage Δ
e2e	`58.00% <54.41%> (+0.64%)`	⬆️
unit	`55.05% <72.97%> (+1.21%)`	⬆️

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.

perdasilva · 2024-06-24T06:15:58Z

internal/catalogmetadata/cache/cache.go

-	if _, err = file.Seek(0, 0); err != nil {
-		return nil, fmt.Errorf("error resetting offset for cache file reader for Catalog %q: %s", catalog.Name, err)
+	if err := os.Rename(tmpDir, cacheDir); err != nil {
+		return nil, fmt.Errorf("error moving temporary directory to cache directory: %v", err)


curious to know what happens if fail here? Just run of the mill expbackoff? what happens to resolution during that period? Same thing?

Yes to both.

everettraven · 2024-06-24T18:00:39Z

internal/catalogmetadata/client/client.go

+			deprecationsMu sync.Mutex
+		)
+
+		if err := declcfg.WalkMetasFS(ctx, packageFS, func(_ string, meta *declcfg.Meta, err error) error {


Is this function now guaranteed to run concurrently? Is there any guarantees on the same *declcfg.Meta object not being processed more than once?

It runs concurrently by default (based on number of CPUs Go thinks are available). There is an exactly once guarantee for an individual *declcfg.Meta object.

Ultimately, we're parsing n files at a time (where n is number of goroutines). You may notice that the cache writes metas into their own files. This is on purpose to make as many files as possible so that as many goroutines as possible (up to n, at least) have work to do.

everettraven · 2024-06-24T18:06:28Z

internal/catalogmetadata/client/client.go

+	for _, bundle := range bundles {
+		slices.SortFunc(bundle.InChannels, func(a, b *catalogmetadata.Channel) int { return cmp.Compare(a.Name, b.Name) })
+	}


Could you elaborate on the purpose of this?

Because of the concurrency introduced in WalkMetasFS the order that channels are added processed is non-deterministic. That non-determinism manifests as non-deterministic ordering of bundle.InChannels. So this function gets us back to a deterministic order.

It doesn't really matter except that tests become more annoying to implement if the order isn't deterministic.

Would you mind adding a comment just highlighting the why?

https://github.com/operator-framework/operator-controller/compare/0661a996c07672f58b91a4e18ac35ba00710fd43..3df60bbed3d17645aec5eae832f884bb51a7b9a9

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>

joelanford requested a review from a team as a code owner June 21, 2024 20:20

joelanford force-pushed the improve-catalog-reconcile-time branch 2 times, most recently from 822f614 to cbac64e Compare June 21, 2024 20:34

joelanford commented Jun 21, 2024

View reviewed changes

grokspawn reviewed Jun 21, 2024

View reviewed changes

joelanford force-pushed the improve-catalog-reconcile-time branch from cbac64e to 0661a99 Compare June 21, 2024 21:05

joelanford commented Jun 21, 2024

View reviewed changes

perdasilva reviewed Jun 24, 2024

View reviewed changes

everettraven reviewed Jun 24, 2024

View reviewed changes

joelanford force-pushed the improve-catalog-reconcile-time branch 2 times, most recently from 3df60bb to b86e629 Compare June 24, 2024 18:54

joelanford mentioned this pull request Jun 24, 2024

[epic] ClusterExtension supports selecting from specific catalogs #968

Closed

incremental improvement in catalogmetadata cache/client performance

82303d7

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>

joelanford force-pushed the improve-catalog-reconcile-time branch from b86e629 to 82303d7 Compare June 24, 2024 19:03

everettraven approved these changes Jun 24, 2024

View reviewed changes

joelanford added this pull request to the merge queue Jun 24, 2024

Merged via the queue into operator-framework:main with commit 2eca31d Jun 24, 2024

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

joelanford deleted the improve-catalog-reconcile-time branch October 22, 2024 13:02

	return nil, fmt.Errorf("error creating temporary directory to unpack catalog metadata: %s", err)
	return nil, fmt.Errorf("error creating temporary directory to unpack catalog metadata: %v", err)

Conversation

joelanford commented Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewer Checklist

Uh oh!

netlify bot commented Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

joelanford Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joelanford commented Jun 21, 2024 •

edited

Loading

netlify bot commented Jun 21, 2024 •

edited

Loading

joelanford Jun 21, 2024 •

edited

Loading

codecov bot commented Jun 22, 2024 •

edited

Loading

joelanford Jun 24, 2024 •

edited

Loading