Use reflector for all repeated read operations, set read timeout & a test timeout greater than that, CI stress edition - #330
Open
Jakob-Naucke wants to merge 9 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Jakob-Naucke 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 |
Reviewer's GuideCentralizes Kubernetes operator controller state into a shared reflector-backed OperatorContext with a configured 5-minute read timeout, refactors reference-value and trustee logic to use cached data and server-side patches, tightens attestation secret scoping via labels, and adjusts CI/test timeouts and tests to reflect reduced API interactions and new behaviors. Sequence diagram for ApprovedImage reconciliation using cached PCR ConfigMapsequenceDiagram
participant ImageController
participant OperatorContext
participant Trustee
ImageController->>OperatorContext: image_reconcile(image, ctx)
ImageController->>OperatorContext: get_opt_tec()
alt TrustedExecutionCluster available
ImageController->>OperatorContext: handle_new_image(ctx, image)
note over OperatorContext: may call cached_image_pcrs()
alt PCRs need update
ImageController->>OperatorContext: apply_image_pcrs(ctx, image_pcrs)
OperatorContext->>Trustee: update_reference_values(client, image_pcrs)
else no PCR change
ImageController-->>OperatorContext: return NOT_COMMITTED_REASON_PENDING or existing
end
else no cluster
ImageController-->>OperatorContext: return LONG_REQUEUE
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Jakob-Naucke
force-pushed
the
get-cached-ci
branch
from
August 11, 2026 16:08
ee8469d to
0a80458
Compare
Jakob-Naucke
changed the title
Jakob-Naucke
force-pushed
the
get-cached-ci
branch
from
August 18, 2026 14:27
0a80458 to
7ec60fc
Compare
Jakob-Naucke
changed the title
Member
Author
|
Had one VM that didn't boot up, cause cannot be uncovered from logs (from dmesg one can only tell it has failed, and Trustee was restarted). We are working on improving observability of Trustee, but should also log failures from trustee-attester without leaking secrets. FWIW, the failure could be explained by a missing secret which has been seen on Azure and should improve with #248. For now, rerun. |
Jakob-Naucke
force-pushed
the
get-cached-ci
branch
from
August 19, 2026 11:27
7ec60fc to
1f4f137
Compare
alicefr
reviewed
Aug 19, 2026
| "metadata": { "name": TRUSTEE_DATA_MAP }, | ||
| "data": { REFERENCE_VALUES_FILE: rv_json } | ||
| })); | ||
| let pp = PatchParams::apply("trusted-cluster-operator").force(); |
Member
Author
There was a problem hiding this comment.
Yes, this is an apply patch, not a merge patch:
[2026-08-19T13:28:45Z WARN operator::reference_values] PCR computation for coreos-approved-primary failed: ApiError: PatchOptions.meta.k8s.io "" is invalid: fieldManager: Required value: is required for apply patch: Invalid (Status { status: Some(Failure), code: 422, message: "PatchOptions.meta.k8s.io \"\" is invalid: fieldManager: Required value: is required for apply patch", metadata: Some(ListMeta { continue_: None, remaining_item_count: None, resource_version: None, self_link: None }), reason: "Invalid", details: Some(StatusDetails { name: "", group: "meta.k8s.io", kind: "PatchOptions", uid: "", causes: [StatusCause { reason: "FieldValueRequired", message: "Required value: is required for apply patch", field: "fieldManager" }], retry_after_seconds: 0 }) })
| } | ||
| })); | ||
| let deployments: Api<Deployment> = Api::default_namespaced(client.clone()); | ||
| let pp = PatchParams::apply("trusted-cluster-operator").force(); |
Contributor
There was a problem hiding this comment.
same is the force necessary?
| }], | ||
| path: format!("default/{id}/root"), | ||
| num_retries: None, | ||
| num_retries: Some(NumRetries::Finite(RETRIES)), |
Contributor
There was a problem hiding this comment.
We can also set it to infinite, not sure we are ready for this though. If not can you please add a todo so we don't forget it
| ATTESTATION_KEY_REGISTER_IMAGE=$(REGISTRY)/attestation-key-register:$(TAG) | ||
| TRUSTEE_IMAGE ?= quay.io/trusted-execution-clusters/key-broker-service:v0.17.0 | ||
| TEST_IMAGE ?= quay.io/trusted-execution-clusters/fedora-coreos-kubevirt:42.20260622 | ||
| TEST_IMAGE ?= quay.io/trusted-execution-clusters/fedora-coreos-kubevirt:42.20251012.2.0-console-fwd |
Member
Author
There was a problem hiding this comment.
I want to mention I only meant this for the CI debugging and would have removed before moving out of draft, but I built this with
[Journal]
ForwardToConsole=yesin /usr/lib/systemd/journald.conf.d
Contributor
|
@Jakob-Naucke do we plan to merge this after #248 ? It will cause /again) a lot of rebase otherwise |
Member
Author
I'll respond to your other comments later but I have no objections against merging #248 first and rebasing this |
Jakob-Naucke
commented
Aug 19, 2026
| deployments | ||
| .patch( | ||
| TRUSTEE_DEPLOYMENT, | ||
| &PatchParams::apply("trusted-cluster-operator").force(), |
Member
Author
There was a problem hiding this comment.
Is the force necessary?
@alicefr I'll test without but I'd also like to ask why you added it here itfp see other response
| ATTESTATION_KEY_REGISTER_IMAGE=$(REGISTRY)/attestation-key-register:$(TAG) | ||
| TRUSTEE_IMAGE ?= quay.io/trusted-execution-clusters/key-broker-service:v0.17.0 | ||
| TEST_IMAGE ?= quay.io/trusted-execution-clusters/fedora-coreos-kubevirt:42.20260622 | ||
| TEST_IMAGE ?= quay.io/trusted-execution-clusters/fedora-coreos-kubevirt:42.20251012.2.0-console-fwd |
Member
Author
There was a problem hiding this comment.
I want to mention I only meant this for the CI debugging and would have removed before moving out of draft, but I built this with
[Journal]
ForwardToConsole=yesin /usr/lib/systemd/journald.conf.d
Jakob-Naucke
force-pushed
the
get-cached-ci
branch
from
August 19, 2026 13:35
1f4f137 to
6c1745b
Compare
Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
Extend the use of a reflector cache from attestation keys to the entire operator. Replace AkContextData with OperatorContext to pass caches. Use server-side apply patches instead of get+replace. - Updating image-pcrs without the cache is only used in compute-pcrs now, thus inline the macro - trustee::update_reference_values receives PCRs from argument, it is sometimes available from the call site anyhow Fixes: trusted-execution-clusters#251 Signed-off-by: Jakob Naucke <jnaucke@redhat.com> Assisted-by: AI
so that status is updated in a timely manner Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
for reuse Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
Add a label to secrets and watch only by the label Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
Set read timeout of 295s (same as write) so that hanging operations can retry. Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
Local network on GHA has been seen to be less reliable than local development and production clusters. Set a test timeout multiplier large enough to let one read time out and retry. Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
Jakob-Naucke
force-pushed
the
get-cached-ci
branch
from
August 21, 2026 10:15
6c1745b to
03d070c
Compare
Member
Author
|
This was going to be the PR to let CI spin on it for a while (hence the head name
(see openshift/release#82662 for more information) My hunch is that we merge these changes here (edit: but I'm open to merging #248 first), and consider this DNS failure separately. OpenShift CI will not extend to more tests for now. Something that we could do there already is set the test timeout multiplier like in HEAD here. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
trustee::do_mount_secretthe server-side apply payload hardcodes the container name to"kbs"; this will break if the trustee deployment uses a different container name, so consider deriving the name from the existing pod spec (e.g., the first container) or threading a shared constant instead of duplicating it here. - The
OperatorContext::newconstructor currently creates fresh reflector stores that are immediately overwritten inmain; introducing a constructor that takes all pre-created stores (or a builder) would avoid this double-initialization pattern and make it harder to accidentally use anOperatorContextwith uninitialized/empty stores in other call sites.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `trustee::do_mount_secret` the server-side apply payload hardcodes the container name to `"kbs"`; this will break if the trustee deployment uses a different container name, so consider deriving the name from the existing pod spec (e.g., the first container) or threading a shared constant instead of duplicating it here.
- The `OperatorContext::new` constructor currently creates fresh reflector stores that are immediately overwritten in `main`; introducing a constructor that takes all pre-created stores (or a builder) would avoid this double-initialization pattern and make it harder to accidentally use an `OperatorContext` with uninitialized/empty stores in other call sites.
## Individual Comments
### Comment 1
<location path="operator/src/lib.rs" line_range="45-54" />
<code_context>
+ pub image_store: Store<ApprovedImage>,
+}
+
+impl OperatorContext {
+ pub fn new(client: Client) -> Self {
+ Self {
+ client,
+ tec_store: reflector::store().0,
+ cm_store: reflector::store().0,
+ deployment_store: reflector::store().0,
+ machine_store: reflector::store().0,
+ ak_store: reflector::store().0,
+ secret_store: reflector::store().0,
+ image_store: reflector::store().0,
+ }
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** OperatorContext::new creates unused reflector stores that never receive events
`reflector::store()` returns a `(Store, Writer)` pair, but `OperatorContext::new` keeps only the `Store` and drops the `Writer`, so these stores will never receive events unless replaced later (as in `main`). Any code that constructs `OperatorContext` directly (e.g. tests or future controllers) will get empty, never-updated caches. Consider either requiring callers to pass in the stores, or making `new` private and exposing a constructor that takes the pre-wired stores, so every `OperatorContext` either has an active cache or opts into a no-cache mode explicitly.
Suggested implementation:
```rust
impl OperatorContext {
pub fn new(
client: Client,
tec_store: Store<TrustedExecutionCluster>,
cm_store: Store<ConfigMap>,
deployment_store: Store<Deployment>,
machine_store: Store<Machine>,
ak_store: Store<AttestationKey>,
secret_store: Store<Secret>,
image_store: Store<ApprovedImage>,
) -> Self {
Self {
client,
tec_store,
cm_store,
deployment_store,
machine_store,
ak_store,
secret_store,
image_store,
}
}
```
1. Update all call sites (e.g. in `main` and tests) that currently call `OperatorContext::new(client)` to instead:
- Create each `(Store<T>, Writer<T>)` via `reflector::store()` (or whatever helper is used).
- Wire the `Writer` into the appropriate reflector/controller.
- Pass the `Store` halves into `OperatorContext::new(client, tec_store, cm_store, deployment_store, machine_store, ak_store, secret_store, image_store)`.
2. If you want a no-cache mode for tests, consider adding an explicit `OperatorContext::without_caches(client: Client) -> Self` constructor that clearly documents that it uses empty/non-updating stores, instead of implicitly constructing unused reflectors.
</issue_to_address>
### Comment 2
<location path="operator/src/trustee.rs" line_range="199-207" />
<code_context>
- deployments
- .replace(TRUSTEE_DEPLOYMENT, &Default::default(), &deployment)
- .await?;
+ let patch = Patch::Apply(json!({
+ "apiVersion": "apps/v1",
+ "kind": "Deployment",
+ "metadata": { "name": TRUSTEE_DEPLOYMENT },
+ "spec": {
+ "template": {
+ "spec": {
+ "volumes": volumes,
+ "containers": [{ "name": "kbs", "volumeMounts": vol_mounts }]
+ }
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Hard-coding container name "kbs" in the patch may break deployments with different names or multiple containers
The previous code mutated `containers[0]` from the live `Deployment`, so it worked regardless of the container name. The new patch assumes a single container named `kbs`, which will break if the deployment is renamed or has multiple containers (e.g., patch failing or affecting the wrong container). Instead, derive the container name from the existing deployment (e.g., reuse `container.name`) and patch only the relevant container’s `volumeMounts`, rather than constructing a new `containers` array with a fixed name.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+45
to
+54
| impl OperatorContext { | ||
| pub fn new(client: Client) -> Self { | ||
| Self { | ||
| client, | ||
| tec_store: reflector::store().0, | ||
| cm_store: reflector::store().0, | ||
| deployment_store: reflector::store().0, | ||
| machine_store: reflector::store().0, | ||
| ak_store: reflector::store().0, | ||
| secret_store: reflector::store().0, |
There was a problem hiding this comment.
suggestion (bug_risk): OperatorContext::new creates unused reflector stores that never receive events
reflector::store() returns a (Store, Writer) pair, but OperatorContext::new keeps only the Store and drops the Writer, so these stores will never receive events unless replaced later (as in main). Any code that constructs OperatorContext directly (e.g. tests or future controllers) will get empty, never-updated caches. Consider either requiring callers to pass in the stores, or making new private and exposing a constructor that takes the pre-wired stores, so every OperatorContext either has an active cache or opts into a no-cache mode explicitly.
Suggested implementation:
impl OperatorContext {
pub fn new(
client: Client,
tec_store: Store<TrustedExecutionCluster>,
cm_store: Store<ConfigMap>,
deployment_store: Store<Deployment>,
machine_store: Store<Machine>,
ak_store: Store<AttestationKey>,
secret_store: Store<Secret>,
image_store: Store<ApprovedImage>,
) -> Self {
Self {
client,
tec_store,
cm_store,
deployment_store,
machine_store,
ak_store,
secret_store,
image_store,
}
}- Update all call sites (e.g. in
mainand tests) that currently callOperatorContext::new(client)to instead:- Create each
(Store<T>, Writer<T>)viareflector::store()(or whatever helper is used). - Wire the
Writerinto the appropriate reflector/controller. - Pass the
Storehalves intoOperatorContext::new(client, tec_store, cm_store, deployment_store, machine_store, ak_store, secret_store, image_store).
- Create each
- If you want a no-cache mode for tests, consider adding an explicit
OperatorContext::without_caches(client: Client) -> Selfconstructor that clearly documents that it uses empty/non-updating stores, instead of implicitly constructing unused reflectors.
Comment on lines
+199
to
+207
| let patch = Patch::Apply(json!({ | ||
| "apiVersion": "apps/v1", | ||
| "kind": "Deployment", | ||
| "metadata": { "name": TRUSTEE_DEPLOYMENT }, | ||
| "spec": { | ||
| "template": { | ||
| "spec": { | ||
| "volumes": volumes, | ||
| "containers": [{ "name": "kbs", "volumeMounts": vol_mounts }] |
There was a problem hiding this comment.
issue (bug_risk): Hard-coding container name "kbs" in the patch may break deployments with different names or multiple containers
The previous code mutated containers[0] from the live Deployment, so it worked regardless of the container name. The new patch assumes a single container named kbs, which will break if the deployment is renamed or has multiple containers (e.g., patch failing or affecting the wrong container). Instead, derive the container name from the existing deployment (e.g., reuse container.name) and patch only the relevant container’s volumeMounts, rather than constructing a new containers array with a fixed name.
|
PR needs rebase. DetailsInstructions 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-sigs/prow repository. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Consolidate Kubernetes resource caching and timeout handling to make controller read operations resilient under stressed conditions.
New Features:
Bug Fixes:
Enhancements:
CI:
Tests: