OCPBUGS-37982: Bug fix: Reduce Frequency of Update Requests for Copied CSVs

[bentito]

Description of the change:

Please checkout this doc on scoping out this change: https://docs.google.com/document/d/1P4cSYEP05vDyuhBfilyuWgL5d5OOD5z7JlAyOxpPqps

In this PR we are resurrecting #3411 with the intent to fix what that PR was originally going to fix. Follow on work will address the then revealed problem with metadata.generation|resourceVersion as per the doc comment by @tmshort

Motivation for the change:
[from the linked doc, "How Did We Get Here:]

• Original Change for Memory Optimization: Sixteen months ago, we merged a PR in OLMv0 that converted the cache of copied ClusterServiceVersions (CSVs) to use PartialObjectMetadata types instead of caching the full CSV objects. This change was crucial for memory utilization performance gains, enabling OLM to run efficiently on MicroShift, a lightweight Kubernetes distribution designed for edge computing.
• Limited Access to Spec/Status: By using PartialObjectMetadata, we only have access to the metadata of copied CSVs, not their spec or status fields. This means the operator lacks the information needed to compare the full content of the copied CSVs with the originals.
• Removal of “Hash and Compare” Logic: The change inadvertently removed a core piece of the “hash and compare” logic. Previously, the operator used annotations containing hashes of the non-status and status fields of the original CSV to determine if a copied CSV needed updating. These annotations were not set on the copied CSVs after the change.
• Resulting in Excessive Updates: Without the ability to compare hashes, the operator began issuing updates for copied CSVs 100% of the time, regardless of whether they were in sync with the originals. This behavior introduced a high load on the Kubernetes API server, especially in environments with many namespaces and CSVs installed in AllNamespace mode. The increased load also led to higher audit log volumes, impacting users with increased logging costs.

Architectural changes:

• Reintroducing Annotations: A proposed fix PR adds back the annotations olm.operatorframework.io/nonStatusCopyHash and olm.operatorframework.io/statusCopyHash to the copied CSVs. These annotations store hashes of the non-status and status fields of the original CSV, respectively.
• Reducing Unnecessary Updates: By comparing these hashes, the operator can determine if the copied CSVs are out of sync with the originals and only issue updates when necessary. This reduces the frequency of update requests to the API server and lowers audit log volumes.
• Uncovering a New Bug: However, reintroducing the hash comparison logic will uncover a bug due to the use of PartialObjectMetadata for caching copied CSVs. Since we only have access to metadata, if a user manually modifies the spec or status of a copied CSV without changing the hash annotations, the operator cannot detect the change. The operator would incorrectly assume the copied CSV is in sync with the original, leading to potential inconsistencies.

Testing remarks:

Except from the expected changes around the inability to track copied CSV changes made by a user, we should be careful to test the following:

• Cannot Revert Memory Optimization: Reverting the changes that introduced PartialObjectMetadata caching is not feasible. The memory optimization is critical for running OLM on MicroShift and supporting edge computing scenarios where resources are constrained.

Reviewer Checklist

• Implementation matches the proposed design, or proposal is updated to match implementation
• Sufficient unit test coverage
• Sufficient end-to-end test coverage
• Bug fixes are accompanied by regression test(s)
• e2e tests and flake fixes are accompanied evidence of flake testing, e.g. executing the test 100(0) times
• tech debt/todo is accompanied by issue link(s) in comments in the surrounding code
• Tests are comprehensible, e.g. Ginkgo DSL is being used appropriately
• Docs updated or added to /doc
• Commit messages sensible and descriptive
• Tests marked as [FLAKE] are truly flaky and have an issue
• Code is properly formatted

[camilamacedo86]

From the code implemented in this PR to the current state, the main addition seems to be this else block (beyond tests).

I’m not entirely sure I fully understand—are we also looking to implement what’s outlined in the Proposed Fixes section of this document? How/where are we addressing the concerns raised in the: Why don’t we just merge the [fix PR](https://github.com/operator-framework/operator-lifecycle-manager/pull/3411) as-is? section?

[bentito]

this is first pass, basically, just merge the old PR. With this PR we're taking path of #4 in the scoping doc: merge the PR with some possible problems, they should be a minor use case: users changing the copied CSVs

but the else is not the only thing done here, the main thing added is the tracking hashes so we can tell what's in need of update.

[camilamacedo86]

this is first pass, basically, just merge the old PR.

We previously agreed that the old PR wasn't quite the right approach, correct? Given that, I’m not sure it makes sense to merge it as-is.

If we need to do a release before we have the proper solution in place, we might include a change we don’t want. That doesn’t seem ideal to me. That is a case that I would request changes since it does not provide the desired solution, or fix the problem accordingly as defined in the doc. See that the doc has a section about that Why don’t we just merge the PR as it is?

It’s fine to add it as you did, but what do you think about creating a commit on top with the solution we intend to use? Could we focus on implementing the correct fix for the bug instead?
Is there any reason we need to merge the old PR without the correct fix? Can we not do that as proposed?

c/c @tmshort

[bentito]

Please see @tmshort comment on the doc. Idea being, merging this PR is a first step, it gives some relief, then we'll make another pass after this settles. Settling involves seeing much less API activity, especially on clusters with many namespaces for the CSV to be copied to.

[bentito]

and settling also involves seeing if the things mentioned in the doc, primarily whether OLM not correcting user-modified copied CSVs, will be a real-world problem.

[perdasilva]

My understanding is we have two problems:

1. spamming logs and api server requests
2. changes to copied csvs won't be detected and therefore will linger

The proposed approach is to separate these two problems by resolving 1 (which has a material impact on the cluster and customer's bottom line) and later handling 2.

It's my understanding that changes to copied csvs don't carry any behavioral changes in the system anyway. They only exist to make it possible to discover which operators are available in a particular namespace with a kubectl command. Also, I'd assume that write access to CSVs will be restricted in most real world cases to the cluster admin and the namespace admin. If these two assumptions hold, I think the blast radius of modifying the copied CSV and not having it reconcile back to the intended spec should be pretty small. So, I tend to agree with the approach here. Let's address the big problem of api server/log spamming, then worry about the relatively small problem of inconsistent copied csvs.

[tmshort]

I think there might be some simplification that can be done with the setting of the status/nonstatus annotations.

[tmshort]

I'm assuming all the changes here are due to lint?

[bentito]

yeah, and I just ran make lint locally to make sure nothing changed. nothing changed.

[tmshort]

Because copyToNamespace is called in a loop, prototype, being a pointer, is reused multiple times. Which means that these annotations may already be set. Is there any reason why these annotations simply aren't set in ensureCSVsInNamesapces() where the hashes are calculcated?

[bentito]

good point possibly. checking...

[bentito]

So looking at it closer it seems like we shouldn't change it, here's my reasoning:

Keeping the annotation logic here, in copyToNamespace(), encapsulate the update semantics so each call handles its own CSV's state reliably.

We're reusing prototype and accounting for possibly set annotations. If we move the logic to ensureCSVsInNamesapces(), we'll have to duplicate the annotation checking logic because the logic for handling those annotations is tightly coupled with the CSV’s create/update lifecycle.

In copyToNamespace() we need to:
• Distinguish between a new creation (where the annotations don’t exist yet) and an update (where the annotations might already be set but could be outdated).
• Apply the updates in a specific order (first updating the non-status hash, then the status hash, including a status update to avoid mismatches).
• Ensure that each target CSV reflects the current state as expected for that specific namespace.

Aside from the hash handling we'd still need to be doing the above work in copyToNamespace()

.

[camilamacedo86]

Hi @bentito @tmshort

Could we address the properly solution on this one as defined in the doc instead of get merged the old pr fix? See: #3497 (comment)

I do not want to block anything, request changes regards: #3497 (review) (so dismissing)

[perdasilva]

In general, I think it lgtm. I'm curious why we need the annotations at all. Would it be sufficient to do something like:

generate the desired copied csv from the parent csv
get the existing copied csv
if not found => create desired copied csv and return
else => compute desired and existing non-status hashes
if desiredNonStatusHash != existingNonStatusHash => update copied csv and return
else, compute desired and existing status hashes
if desiredStatusHash != existingStatusHash => update status and return
else, return

I think the answer might be because the sync function is being driven off parent/non-copied csv events. Even so, we should be able to use the general approach outlined above (minus the returns). wdyt? I guess the only major difference is swapping memory (storing the hashes in the annotations) for computation (calculating both desired and current hashes). Maybe that's cleaner and adds fewer implementation concerns to the copied csv?

[everettraven]

In general, I think it lgtm. I'm curious why we need the annotations at all. Would it be sufficient to do something like:

generate the desired copied csv from the parent csv get the existing copied csv if not found => create desired copied csv and return else => compute desired and existing non-status hashes if desiredNonStatusHash != existingNonStatusHash => update copied csv and return else, compute desired and existing status hashes if desiredStatusHash != existingStatusHash => update status and return else, return

I think the answer might be because the sync function is being driven off parent/non-copied csv events. Even so, we should be able to use the general approach outlined above (minus the returns). wdyt? I guess the only major difference is swapping memory (storing the hashes in the annotations) for computation (calculating both desired and current hashes). Maybe that's cleaner and adds fewer implementation concerns to the copied csv?

I've not looked at the exact changes in the PR, but having been originally involved in bug when it first rolled around the reason I had determined we couldn't do this is because we only ever cached the copied CSV metadata and not the whole copied CSV.

IIRC the bug was originally concerned with the numerous update calls, which were happening because we always decided we needed to update the CSVs.

At the time, it was also determined that we could change what we cached because edge-computing solutions like MicroShift needed the reduction in memory footprint to run OLM.

Because of this, you would always have to make a GET call to the kube-apiserver for every copied CSV you need to evaluate. This could lead to at worst making twice the API calls we were already making -- a GET followed by an UPDATE as opposed to always issuing an UPDATE. At best, this was exactly the same -- always making a GET request.

If the constraints of not being able to change how the data is cached are still in play, you'll end up with the same problem. The bug was originally filed as a "OLM makes too many calls to the Kubernetes API server, inflating the audit logs and thus our log ingestion/monitoring bill". Switching from always issuing an UPDATE to always issuing a GET and possibly an UPDATE would make this worse than just not fixing the bug.

[perdasilva]

Because of this, you would always have to make a GET call to the kube-apiserver for every copied CSV you need to evaluate. This could lead to at worst making twice the API calls we were already making -- a GET followed by an UPDATE as opposed to always issuing an UPDATE. At best, this was exactly the same -- always making a GET request.

Gotcha. I missed that we weren't doing a live GET call. But, rather pulling it from cache. That all makes sense. Thank you =D

[perdasilva]

should we update the status hash post status update, i.e. with the observed resource version and generation annotations (for the same reason described in line 897)?

[bentito]

done in soon to be pushed commit 4b21aa6e

[perdasilva]

It would be cool if we could add a regression test of some sort to measure the number of api server requests or log size or something that helps us verify/measure the effectiveness of the fix (and make sure we don't fall in the same trap again). Though, only if there is a relatively easy way to do it. Otherwise, could we post up some manual verification? i.e. pre-fix logs are big, post-fix logs are ok. Or maybe grabbing some api server stats or something...? wdyt?

[bentito]

@per replying to your comment:

Would it be sufficient to do something like:
generate the desired copied csv from the parent csv

Why annotations vs “just re-hash everything on the fly”

1. Performance & churn
   Marshaling the entire CSV spec+status and hashing it on every parent-CSV event could get expensive (especially in big clusters with many CSVs) and can lead to unnecessary Update calls (and status flaps) if you miss a subtle drift.

2. Persistence across restarts
   We need to remember what we last applied, even if the controller pod restarts or skips an event. Annotations live on the resource itself, so we don’t need an external cache or to recompute “last applied” from scratch.

3. Robust “drift-guard”
   By also storing observed generation & resourceVersion, we catch manual edits made directly to the copied CSV (spec drift or status drift) even if no new parent-CSV event fires. Without annotations you’d have to separately watch the copied CSVs, trigger extra reconciles, and still figure out whether you should stomp on user edits or not.

4. Simpler reconciliation logic
   Instead of a big “compute desired, compute existing, diff non-status, diff status, decide what to do” every single time, the annotations let you do a cheap string compare in most cases (“is desiredHash == lastAppliedHash?”) and skip all the heavy lifting.

   You’re absolutely right that a pure-compute approach is conceptually possible, but to get the same guarantees (no missed updates, no unnecessary writes, survive restarts, catch manual drift) you’d end up
   re-implementing a lot of what annotations give you “for free.” That was the driving motivation here: swap a tiny bit of metadata on the resource for a much simpler, more robust controller loop.