Introduce the reflector to reduce the k8s API calls - #267
Conversation
Reviewer's GuideReplaces direct Kubernetes API list/get calls with cached reflectors and shared in-memory stores for TEC, Machine, AttestationKey, Secret, and Deployment resources, wiring these stores into controllers and tests to reduce API traffic while ensuring caches are synced before starting controllers. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The reflector tasks currently discard all events with
.for_each(|_| async {}); consider at least logging or handling errors from thereflectorstream so that broken watches or deserialization issues are visible rather than silently ignored. - In
update_attestation_keys, returning an error when the trusteeDeploymentis not found in the cache (ok_or_else) is a behavioral change from the previous live-GET; if the deployment may legitimately not exist yet or be temporarily absent from the cache, consider treating this as a no-op with a log message instead of a hard error.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The reflector tasks currently discard all events with `.for_each(|_| async {})`; consider at least logging or handling errors from the `reflector` stream so that broken watches or deserialization issues are visible rather than silently ignored.
- In `update_attestation_keys`, returning an error when the trustee `Deployment` is not found in the cache (`ok_or_else`) is a behavioral change from the previous live-GET; if the deployment may legitimately not exist yet or be temporarily absent from the cache, consider treating this as a no-op with a log message instead of a hard error.
## Individual Comments
### Comment 1
<location path="operator/src/attestation_key_register.rs" line_range="298-299" />
<code_context>
info!("Secret reconciliation for AttestationKey secret: {secret_name}");
- let secrets: Api<Secret> = Api::default_namespaced(Arc::unwrap_or_clone(client.clone()));
+ let secrets: Api<Secret> = Api::default_namespaced(ctx.client.clone());
finalizer(&secrets, ATTESTATION_KEY_SECRET_FINALIZER, secret, |ev| async move {
match ev {
Event::Apply(_secret) => {
</code_context>
<issue_to_address>
**issue (bug_risk):** The `ctx` Arc is captured by reference into the finalizer closure, which is likely required to be `'static` and can lead to lifetime issues.
Here `finalizer` likely requires a `'static` closure (e.g., it spawns or stores it). Because the closure is declared with `|ev| async move { ... }`, `ctx` is currently captured by reference from the outer function, which will not satisfy `'static`.
Instead, clone the `Arc` and move it into the closure so it is owned by the closure environment, e.g.:
```rust
let ctx = ctx.clone();
finalizer(&secrets, ATTESTATION_KEY_SECRET_FINALIZER, secret, move |ev| {
let ctx = ctx.clone();
async move {
match ev {
Event::Apply(_secret) => {
trustee::update_attestation_keys(
ctx.client.clone(),
&ctx.secret_store,
&ctx.deployment_store,
)
}
// ...
}
}
})
```
This way the closure owns an `Arc<AkContextData>` and can meet the `'static` bound.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
3dcf664 to
69b3533
Compare
Jakob-Naucke
left a comment
There was a problem hiding this comment.
- don't worry about the CI failure, quay.io is having trouble. That said, ensure your code has been run through formatting (e.g.
cargo fmt) - is this also possible for
ApprovedImages(reference_values.rs:331)? - commit message nits: use infinitive (
Replace direct kube…) removeSubject:
4637c3b to
5c0e978
Compare
Jakob-Naucke
left a comment
There was a problem hiding this comment.
still wondering if it can also be implemented for ApprovedImages (reference_values.rs:331)?
| for machine in ctx.machine_store.state() { | ||
| if ak.spec.uuid.as_ref() == Some(&machine.spec.id) { | ||
| approve_ak(&ak, machine, client.clone()).await?; | ||
| approve_ak(&ak, &machine, ctx.client.clone(), &ctx.secret_store).await?; |
There was a problem hiding this comment.
I think by that point you could just pass a reference to the context
|
@yairpod try to use Opus as model instead of Sonnet. It will give you much better result |
Jakob-Naucke
left a comment
There was a problem hiding this comment.
Can the reflector also be used for reference values?
8f8ee35 to
33ea654
Compare
Jakob-Naucke
left a comment
There was a problem hiding this comment.
nit: use infinitive in the commit message subject (Replace …)
ab39a41 to
e501b73
Compare
Jakob-Naucke
left a comment
There was a problem hiding this comment.
LGTM, would still like @alicefr's opinion
| let err = anyhow::anyhow!( | ||
| "Timed out after {sync_timeout:?} waiting for {name} cache to sync. \ | ||
| Ensure the CRD is installed and the API server is reachable." | ||
| ); |
There was a problem hiding this comment.
if this can be caused by the CRD to not be installed we can error out earlier by getting the CRD. It will also verify that the API server is reachable or not. We can improve this in a follow up PR though.
There was a problem hiding this comment.
Sounds good for a follow up improvement
Until now the operator performed several direct calls to the Kubernetes API server to get and list various objects. Each direct call to the API server incurs network latency and consumes resources on the API server and etcd. Additionally, performing frequent reads against the API server risks hitting rate limits (HTTP 429) during heavy workloads. This commit replaces these direct read operations (get and list) with the kube-rs Reflector. Signed-off-by: Yair Podemsky <ypodemsk@redhat.com> Assisted-by: Opus:4.6
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: alicefr, Jakob-Naucke, yairpod The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
52d492b
into
trusted-execution-clusters:main
As requested in issue 251.
This PR replaces the direct k8s API calls with reflectors.
This should reduce the API server network latency and consumption of resources on the API server and etcd as well as risks hitting rate limits (HTTP 429) during heavy workloads.
The cost is increased memory usage as we cache the information from the API server (the CPU for the watcher threads should be reasonably low).
I have not been able to test the cache memory usage as a kind cluster will have a tiny memory signature anyway.
To test the API call reduction I have run the Integration tests on a clean kind cluster and run the command
kubectl get --raw /metrics | grep apiserver_request_total | grep trusted, then summed the all calls of all types.The unmodified operator had a total of 788 calls.
The PR code had a total of 463 calls.
Summary by Sourcery
Introduce shared reflector-backed caches for custom resources and core objects to reduce direct Kubernetes API usage and wire them into the operator controllers.
New Features:
Enhancements: