#587 - Disable caching for packageRevisionResources

[kushnaidu]

This change disables caching for packageRevisionResources in the porch controller.
Context -
The use case works for creating a PVS, but after someone creates a PVS, the controller logs are populated with errors saying the controllers cannot 'watch' the packageRevisionResources because there is no 'watch' function on the porch server side.
Example -
E0523 16:49:14.393038 1 reflector.go:140] pkg/mod/k8s.io/client-go@v0.26.9/tools/cache/reflector.go:169: Failed to watch *v1alpha1.PackageRevisionResources: the server does not allow this method on the requested resource (get packagerevisionresources.porch.kpt.dev)

[nagygergo]

This is a generated file based on kubebuilder directives in the controller go code. Please update those instead of this file.

[kushnaidu]

This is a generated file based on kubebuilder directives in the controller go code. Please update those instead of this file.

Sure, I'll make the change.

[liamfallon]

/approve

Looks fine to me, let's get some more reviews.

[efiacor]

If we really need this, which I can't see why we do, maybe refine the roles to only the required ones. list and watch.

[efiacor]

Update controllergen to v0.15.0 and run make generate.
As I mentioned, the setup here is a bit messy. The changes here only get generated if/when we run the "make generate" target. The updated Roles/Bindings are updated but the reality is that the catalog porch pkg itself needs to be updated also. https://github.com/nephio-project/catalog/blob/main/nephio/core/porch/9-porch-controller-packagevariantsets-clusterrole.yaml

[kushnaidu]

Hey Fiachra, yes, I have discussed with greg, we're trying to reproduce this issue and re-verify. Might need an update to the code itself. Also, I just re-used the same roles from packagevariant_controller.go. I will refactor this if it is still needed. I've added the do-not-merge label for now.

[liamfallon]

/remove-approve

[nagygergo]

To my understanding the following is the issue:

1. controller-runtime runs caching clients by default, where the resources are list-and-watched by default. Because of this, when packageVariant controller runs the following line, the controller-runtime client starts watching the packageRevisionResources: client.Get
2. On the porch-server side, there is no implementation of watch for packageRevisionResources, only for packageRevisions. The porch-controllers gets back an error related to this from porch-server, and it gets printed out to the porch-controllers error log.

So the cache was never working, but instead of it failing back ungracefully to requesting the data every time, the cache is disabled by default.

An alternative solution could be that watch is implemented for packageRevisionResources. The metadata updates, including the resourceVersion is shared between the packageRevisions and packageRevisionResources. But because packageRevisionResources include all the data from the kpt package itself, building an in-memory cache of those might be quite expensive. And as I understand, there is no cache eviction policy, and packageRevisions are edited for a short time, then they become dormant as they get approved. All in all, I don't think it's a good idea for any client to cache packageRevisionResources.

[efiacor]

To my understanding the following is the issue:

1. controller-runtime runs caching clients by default, where the resources are list-and-watched by default. Because of this, when packageVariant controller runs the following line, the controller-runtime client starts watching the packageRevisionResources: [client.Get](https://github.com/nordix/porch/blob/07d5d898731592b234bae7e9e3d124df46415eab/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go#L766)

2. On the porch-server side, there is no implementation of watch for packageRevisionResources, only for packageRevisions. The porch-controllers gets back an error related to this from porch-server, and it gets printed out to the porch-controllers error log.

So the cache was never working, but instead of it failing back ungracefully to requesting the data every time, the cache is disabled by default.

An alternative solution could be that watch is implemented for packageRevisionResources. The metadata updates, including the resourceVersion is shared between the packageRevisions and packageRevisionResources. But because packageRevisionResources include all the data from the kpt package itself, building an in-memory cache of those might be quite expensive. And as I understand, there is no cache eviction policy, and packageRevisions are edited for a short time, then they become dormant as they get approved. All in all, I don't think it's a good idea for any client to cache packageRevisionResources.

Nice explanation. Thanks @nagygergo

[efiacor]

/approve

[liamfallon]

/approve

[liamfallon]

/approve
/lgtm

[nephio-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: efiacor, kushnaidu, liamfallon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [efiacor,liamfallon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment