Store FBC in local directory by anik120 · Pull Request #144 · operator-framework/catalogd

anik120 · 2023-08-24T20:07:19Z

closes #113

codecov · 2023-08-24T20:10:43Z

Codecov Report

Patch coverage: 77.77% and project coverage change: -0.52% ⚠️

Comparison is base (8aab690) 79.61% compared to head (f56fdd2) 79.10%.
Report is 1 commits behind head on main.

❗ Current head f56fdd2 differs from pull request most recent head cd80e99. Consider uploading reports for the commit cd80e99 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     operator-framework/catalogd#144      +/-   ##
==========================================
- Coverage   79.61%   79.10%   -0.52%     
==========================================
  Files           2        3       +1     
  Lines         157      201      +44     
==========================================
+ Hits          125      159      +34     
- Misses         20       26       +6     
- Partials       12       16       +4

Files Changed	Coverage Δ
pkg/storage/localdir.go	`52.94% <52.94%> (ø)`
pkg/controllers/core/catalog_controller.go	`81.31% <92.85%> (+1.96%)`	⬆️

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

everettraven

On first pass this looks pretty good. I have a couple changes in mind though:

stevekuznetsov

Left some comments denoted with a star ([*]) - they are informative. Respond to them if you'd like, but they are not blocking comments and may be things we consider later.

stevekuznetsov · 2023-08-24T22:40:37Z

 			AfterEach(func() {
 				By("tearing down cluster state")
-				Expect(cl.Delete(ctx, catalog)).To(Succeed())
+				Expect(client.IgnoreNotFound(cl.Delete(ctx, catalog))).To(Succeed())


[*] Tests that create complex cluster state in order to run benefit from having a simple toggle to turn off cleanup. When using a persistent test fixture, if I break this test and want to look at the envtest or kind cluster I ran it against, I want a way to keep the test from removing itself from existence on exit.

Do AfterEach blocks run if It's fail? Genuine question. Not sure.

Do AfterEach blocks run if It's fail?

As far as I am aware, yes

stevekuznetsov · 2023-08-24T22:49:36Z

+	"github.com/operator-framework/operator-registry/alpha/property"
+)
+
+// LocalDir is a storage Instance. When Storing a new FBC contained in


You only need to do this on updates, not the first write.

That's a good call out. That means this Store implementation is ready for both create and update.

anik120 · 2023-08-25T15:23:21Z

        - "--metrics-bind-address=127.0.0.1:8080"
        - "--leader-elect"
-        - "--feature-gates=PackagesBundleMetadataAPIs=true,CatalogMetadataAPI=true"
+        - "--feature-gates=PackagesBundleMetadataAPIs=true,CatalogMetadataAPI=true,HTTPServer=false"


Setting this to false for now so that we don't take forever to create all the resources.

anik120 · 2023-08-25T15:35:40Z

+const testBundleTemplate = `---
+image: %s
+name: %s
+schema: olm.bundle
+package: %s
+relatedImages:
+  - name: %s
+    image: %s
+properties:
+  - type: olm.bundle.object
+    value:
+      data: %s
+  - type: some.other
+    value:
+      data: arbitrary-info
+`
+
+const testPackageTemplate = `---
+defaultChannel: %s
+name: %s
+schema: olm.package
+`
+
+const testChannelTemplate = `---
+schema: olm.channel
+package: %s
+name: %s
+entries:
+  - name: %s
+`


There are some common elements between test suites that are starting to emerge that'll probably be better housed in a test_util package. I've reframed from creating the common place in this PR to not increase the scope, but we should look into it in a follow up.

everettraven

A couple nits, but other than that it looks good to me. Approving but would recommend holding merging until others have a chance to take a look

everettraven · 2023-08-28T20:03:14Z

+				Expect(err).To(HaveOccurred())
+				Expect(os.IsNotExist(err)).To(BeTrue())


Nit: the first check seems redundant, but I'm not sure what would happen if you pass a nil error into os.IsNotExist . Also according to the comments on this function we should be using errors.Is instead:

This function predates errors.Is. It only supports errors returned by the os package. New code should use errors.Is(err, fs.ErrNotExist).

stevekuznetsov · 2023-08-28T20:32:51Z

+							}, cmpopts.IgnoreFields(metav1.Condition{}, "Type", "ObservedGeneration", "LastTransitionTime", "Message"))
+							Expect(diff).To(Equal(""))
+						})
+						// TODO: GC doesn't work in evntest, and hence simulating a Storage delete error that causes the deletion to


nit: prefer to capture this in an issue vs commented code, which will rot

FWIW, rukpak has a TODO issue creator (I think in a github action), that makes sure that issues are created for any TODO comments that are merged.

Also, what's the GC that we're expecting to happen here? As I understand it, the HTTPServer feature gate will no longer use underlying objects. And we don't expect anything to GC the catalog itself (that's a user-created object from the perspective of catalogd).

@joelanford there's more context here: https://redhat-internal.slack.com/archives/GCV27CWNP/p1692993854012409

My read is

If:

I'm using envtest

I put a finalizer on my object

I delete my object

Then:

My object actually gets fully deleted even though it has a finalizer on it

Is that right? If so, that smells funny to me. I'm almost positive that the apiserver (which runs with envtest) is the thing looking at finalizers and deciding when to actually delete the object from etcd. Are you sure there wasn't something else about your test setup that was giving you a incorrect impression?

Well so the funny thing that you're smelling is this setup of the MockStore, which is actually not doing the job of signaling an error in deleting the stored content.

joelanford · 2023-08-28T21:12:51Z

+						// })
+					})
+
+					When("there is an error storing the fbc", func() {


If we don't have one already, we need to capture an issue for handling the case where:

We've successfully reconciled a catalog and unpacked it successfully

The spec changed to something that we could not unpack

At this point, what are we doing with the previous successful state, if anything? Is it still active? If so, how do we convey in the status that "we're having trouble reaching desired state, but the previous state is still active"

joelanford · 2023-08-29T19:34:48Z

        - "./manager"
        args:
        - --leader-elect
+        - "--catalogs-storage-dir=/var/cache/catalogs"


This ends up being gobbled up by the manager_auth_proxy_patch.yaml patch. If you actually want to explicitly set this value on the deployment, you need to copy it to that file as well.

Did you mean "as well"? I removed it from this file and only included it in manager_auth_proxy_patch.yaml. Slightly bothered by the fact that the file is called auth_proxy_path but we're including patches that are not auth proxy related, it should probably be in a different patch file.

Noticed the manager_config_patch.yaml file wasn't being used at all, moved this arg and the feature gate arg to this previously unused file. There are other fields that should probably go in the new file too but refrained from doing that in this PR.

You're right, it only has to show up in the patch file right now. That is overriding the base. And yeah this could use some refactoring and simplifying in a separate PR.

closes operator-framework#113 Signed-off-by: Anik <anikbhattacharya93@gmail.com>

joelanford

lgtm!

anik120 requested a review from a team as a code owner August 24, 2023 20:07

anik120 mentioned this pull request Aug 24, 2023

Store(local dir) and Serve(http server) FBC #135

Closed

anik120 force-pushed the storage branch 4 times, most recently from c040464 to 2f9c96d Compare August 24, 2023 20:35

everettraven suggested changes Aug 24, 2023

View reviewed changes

Comment thread pkg/controllers/core/catalog_controller_test.go Outdated

Comment thread pkg/controllers/core/catalog_controller_test.go Outdated

everettraven reviewed Aug 24, 2023

View reviewed changes

Comment thread api/core/v1alpha1/catalog_types.go Outdated

everettraven reviewed Aug 24, 2023

View reviewed changes

Comment thread pkg/storage/localdir.go Outdated

stevekuznetsov reviewed Aug 24, 2023

View reviewed changes

anik120 force-pushed the storage branch from 2f9c96d to c48be99 Compare August 25, 2023 15:13

anik120 commented Aug 25, 2023

View reviewed changes

anik120 force-pushed the storage branch 2 times, most recently from 18a2e5d to f075286 Compare August 25, 2023 20:48

stevekuznetsov mentioned this pull request Aug 28, 2023

>We can assume that catalogs being added here were built in a pipeline that ran opm validate <directory> --flags as the final step. operator-framework/operator-controller#1594

Closed

anik120 force-pushed the storage branch 2 times, most recently from bc1035b to a2f92c2 Compare August 28, 2023 19:53

everettraven approved these changes Aug 28, 2023

View reviewed changes

stevekuznetsov reviewed Aug 28, 2023

View reviewed changes