(server) Serve CatalogMetadata resource with aggregated server #107

anik120 · 2023-07-02T22:50:33Z

Closes #39

anik120 · 2023-07-02T23:53:48Z

pkg/controllers/core/catalog_controller.go

@@ -461,7 +463,7 @@ func (r *CatalogReconciler) syncCatalogMetadata(ctx context.Context, fsys fs.FS,
 	ordered := sets.List(sets.KeySet(newCatalogMetadataObjs))
 	for _, catalogMetadataName := range ordered {
 		newcatalogMetadata := newCatalogMetadataObjs[catalogMetadataName]
-		if err := r.Client.Patch(ctx, newcatalogMetadata, client.Apply, &client.PatchOptions{Force: pointer.Bool(true), FieldManager: "catalog-controller"}); err != nil {
+		if err := r.Client.Create(ctx, newcatalogMetadata); err != nil {


Note: The generic storage that the etcd storage implementation is based on that is used by the server only has three interfaces, I.e it supports Create, Update and Delete, but doesn't understand Patch. Switched this out to Create for now, since we're anyway deleting the existing matching resources in L453-L459 above, so Patching seems redundant anyway. Eventually we do have to figure out Patching for any store we use though (see this comment for more details).

So I think we might actually run into an issue with this - the delete above is specifically deleting CatalogMetadata resources that should no longer exist based on the reconciliation of a Catalog resource. I.e if a Catalog resource is updated and is reconciled again it only deletes CatalogMetadata resources that should no longer exist based on the unpacking of the Catalog contents. This PATCH request is making sure that we update any CatalogMetadata resources that should still exist but may need to be updated and updates them using server-side apply.

everettraven

Overall this looks good, great work @anik120!

I do have some comments:

everettraven · 2023-07-03T13:11:39Z

Makefile

+	kubectl	wait --for=condition=Available --namespace=cert-manager deployment/cert-manager --timeout=60s
+	kubectl	wait --for=condition=Available --namespace=cert-manager deployment/cert-manager-cainjector --timeout=60s
+	kubectl	wait --for=condition=Available --namespace=cert-manager deployment/cert-manager-webhook --timeout=60s


Is the weird spacing here just a GH thing or are these intentionally tabbed over?

everettraven · 2023-07-03T13:15:18Z

api/optional/v1alpha1/groupversion_info.go

+
+var (
+	// GroupVersion is group version used to register these objects
+	GroupVersion = schema.GroupVersion{Group: "optional.catalogd.operatorframework.io", Version: "v1alpha1"}


Nit: The Group name containing optional seems like it could be confusing. In reality these apis aren't "optional" in the sense that you can just choose to not use them right? Maybe something like storage.catalogd.operatorframework.io would be better?

Dockerfile

everettraven · 2023-07-03T13:19:18Z

api/optional/doc.go

@@ -0,0 +1,21 @@
+/*
+Copyright 2022.


Nit: Copyright year should be 2023 (should apply for all the added files)

Suggested change

Copyright 2022.

Copyright 2023.

api/optional/v1alpha1/groupversion_info.go

everettraven · 2023-07-03T13:25:47Z

config/rbac/role.yaml

+- apiGroups:
+  - optional.catalogd.operatorframework.io
+  resources:
+  - catalogmetadatas


Nit: Is the additional "s" at the end of the name intentional? I know the Kubernetes pluralization logic isn't the greatest but I think catalogmetadata works as both singular and plural and because of that is, IMO, more intuitive and easier to remember than catalogmetadatas

everettraven · 2023-07-03T14:17:06Z

pkg/controllers/core/catalog_controller.go

@@ -444,7 +446,7 @@ func (r *CatalogReconciler) syncCatalogMetadata(ctx context.Context, fsys fs.FS,
 		return fmt.Errorf("unable to parse declarative config into CatalogMetadata API: %w", err)
 	}

-	var existingCatalogMetadataObjs v1alpha1.CatalogMetadataList
+	var existingCatalogMetadataObjs optionalv1alpha1.CatalogMetadataList
 	if err := r.List(ctx, &existingCatalogMetadataObjs); err != nil {


Not for this PR but something I just noticed - we should add some list options here to ensure that we are only fetching CatalogMetadata resources for the Catalog being reconciled. Right now I think this list call results in all CatalogMetadata being fetched which could result in the deletion of CatalogMetadata resources not owned by this Catalog resource

everettraven · 2023-07-03T14:19:40Z

pkg/controllers/core/catalog_controller.go

@@ -461,7 +463,7 @@ func (r *CatalogReconciler) syncCatalogMetadata(ctx context.Context, fsys fs.FS,
 	ordered := sets.List(sets.KeySet(newCatalogMetadataObjs))
 	for _, catalogMetadataName := range ordered {
 		newcatalogMetadata := newCatalogMetadataObjs[catalogMetadataName]
-		if err := r.Client.Patch(ctx, newcatalogMetadata, client.Apply, &client.PatchOptions{Force: pointer.Bool(true), FieldManager: "catalog-controller"}); err != nil {
+		if err := r.Client.Create(ctx, newcatalogMetadata); err != nil {


So I think we might actually run into an issue with this - the delete above is specifically deleting CatalogMetadata resources that should no longer exist based on the reconciliation of a Catalog resource. I.e if a Catalog resource is updated and is reconciled again it only deletes CatalogMetadata resources that should no longer exist based on the unpacking of the Catalog contents. This PATCH request is making sure that we update any CatalogMetadata resources that should still exist but may need to be updated and updates them using server-side apply.

Closes operator-framework#39 Signed-off-by: Anik <[email protected]>

openshift-merge-robot · 2023-07-10T07:37:50Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

joelanford · 2023-08-15T15:43:55Z

Shall we close this PR?

anik120 requested review from rashmigottipati, everettraven and joelanford July 2, 2023 22:51

anik120 commented Jul 2, 2023

View reviewed changes

anik120 added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 3, 2023

anik120 force-pushed the catalogmetadata-server branch from e516c98 to d89938a Compare July 3, 2023 14:11

everettraven reviewed Jul 3, 2023

View reviewed changes

anik120 force-pushed the catalogmetadata-server branch 2 times, most recently from 98e60c4 to f9c3157 Compare July 3, 2023 16:50

(server) Serve CatalogMetadata resource with aggregated server

0de33f4

Closes operator-framework#39 Signed-off-by: Anik <[email protected]>

anik120 force-pushed the catalogmetadata-server branch from f9c3157 to 0de33f4 Compare July 3, 2023 18:19

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

joelanford closed this Aug 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(server) Serve CatalogMetadata resource with aggregated server #107

(server) Serve CatalogMetadata resource with aggregated server #107

anik120 commented Jul 2, 2023

anik120 Jul 2, 2023

everettraven Jul 3, 2023

everettraven left a comment

everettraven Jul 3, 2023

everettraven Jul 3, 2023

everettraven Jul 3, 2023

everettraven Jul 3, 2023

everettraven Jul 3, 2023

everettraven Jul 3, 2023

openshift-merge-robot commented Jul 10, 2023

joelanford commented Aug 15, 2023

(server) Serve CatalogMetadata resource with aggregated server #107

(server) Serve CatalogMetadata resource with aggregated server #107

Conversation

anik120 commented Jul 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

everettraven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-merge-robot commented Jul 10, 2023

joelanford commented Aug 15, 2023