Use a read-timeout client for non-watch operations - #324
Conversation
in consistency with remainder codebase Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
for crates and testing Fixes: trusted-execution-clusters#227 Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
|
[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 GuideIntroduce a dual Kubernetes client abstraction with read timeouts for non-watch operations while keeping separate watch clients for long-lived streams, refactor operator/tests/utilities to use it, tighten controller behaviors, and improve logging and CI observability. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 1 issue, and left some high level feedback:
- The
KubeClientsstruct currently uses fairly generic field names (clientandwatch); consider renaming these to something more self-describing (e.g.request_client/watch_client) to make call-site intent clearer and avoid confusion when both are in scope. - Several paths now clone full
KubeClientsinstances when only one client is used (e.g. inlaunch_keygen_controller,launch_rv_*_controller, and test helpers); you could reduce unnecessary cloning by passing references or extracting just the needed client to keep ownership and lifetimes simpler.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `KubeClients` struct currently uses fairly generic field names (`client` and `watch`); consider renaming these to something more self-describing (e.g. `request_client` / `watch_client`) to make call-site intent clearer and avoid confusion when both are in scope.
- Several paths now clone full `KubeClients` instances when only one client is used (e.g. in `launch_keygen_controller`, `launch_rv_*_controller`, and test helpers); you could reduce unnecessary cloning by passing references or extracting just the needed client to keep ownership and lifetimes simpler.
## Individual Comments
### Comment 1
<location path="operator/src/register_server.rs" line_range="182-183" />
<code_context>
);
return Ok(Action::await_change());
}
+ Err(e) => {
+ let err = anyhow!("{e}").into();
+ return Err(finalizer::Error::<ControllerError>::CleanupFailed(err));
+ }
</code_context>
<issue_to_address>
**issue:** Wrapping `e` with `anyhow!("{e}")` discards the original error context and backtrace.
In the new `Err(e)` branch, converting `e` to `anyhow!("{e}")` before wrapping it in `finalizer::Error::CleanupFailed` loses the original structured error. Prefer propagating `e` directly (e.g. `finalizer::Error::CleanupFailed(e.into())`) so callers and logs preserve its full context and backtrace.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
SpaceFace02
left a comment
SpaceFace02
left a comment
There was a problem hiding this comment.
Thanks for the PR! I have a few small suggestions
| if: always() | ||
| uses: actions/upload-artifact@v7 | ||
| with: | ||
| name: must-gather |
There was a problem hiding this comment.
Maybe we can differentiate each GHA must-gather run by some unique identifier? Not sure if GHA adds a unique id based on which workflow created the file.
There was a problem hiding this comment.
Good idea, I've added the source branch. We could add the artifact ID but IMO the naming you get from the browser downloads (e.g. must-gather-timed-client.zip, must-gather-timed-client.zip(1)) is more intuitive.
| uses: actions/upload-artifact@v7 | ||
| with: | ||
| name: must-gather | ||
| path: must-gather-output/ |
There was a problem hiding this comment.
Any way to configure retention period for this, otherwise these files may pile up to use a lot of space.
There was a problem hiding this comment.
For public GitHub projects like this one, you get 90 days. It's possible to set something lower here, but as long as GitHub lets us I don't really see a reason to. FWIW one of these is about 120K zipped when all went well, about 750K with failures, so at 30 a week, I don't really see the total size exceeding 500M which is less than what you get with e.g. a free Dropbox account. (The size of OpenShift CI artifacts is another story… 🥲)
| return Ok(Action::await_change()); | ||
| } | ||
| Err(e) => { | ||
| let err = anyhow!("{e}").into(); |
There was a problem hiding this comment.
Q: When you say GET request to TEC fails occasionally, do you mean a request timeout, or a 4xx/5xx error which is returned? Why does it fail (due to flakiness or other reason)?
There was a problem hiding this comment.
The request would be launched and never return because prior to this PR, it has no timeout. From a log with UUID per request, which is printed again upon return:
2026-07-28T15:32:33Z GET /api/v1/namespaces/test-4a79467d/secrets/trustee-auth "" 46a1e037-ff78-4a05-97b4-ecefc2ac314d
2026-07-28T15:32:33Z 46a1e037-ff78-4a05-97b4-ecefc2ac314d
and then this GET never returning occurred often:
[2026-07-28T15:33:19Z INFO operator::register_server] get TEC
2026-07-28T15:33:19Z GET /apis/trusted-execution-clusters.io/v1alpha1/namespaces/test-4a79467d/trustedexecutionclusters/trusted-execution-cluster "" 1426a6a1-91fb-4d4f-856d-5871491635d8
(with that UUID never seen again, despite the log continuing until 15:38)
| /// (Controllers, Reflectors, `await_condition`). | ||
| #[derive(Clone)] | ||
| pub struct KubeClients { | ||
| pub client: Client, |
There was a problem hiding this comment.
nit: Like Sourcery mentioned, the naming could be changed to something less generic.
There was a problem hiding this comment.
nit: Also I don't know what would be the ideal way to name this, whether it should have the 's' in the end, indicating a plural. It is one object after all, with different functionalities available
| if machine.metadata.deletion_timestamp.is_some() { | ||
| info!( | ||
| "Machine {} is being deleted, updating attestation key volumes", | ||
| "Machine {} is being deleted, skipping update of attestation key volumes", |
There was a problem hiding this comment.
We are skipping update of attestation key volumes because as per the latest PR by Roy, attestation keys are no longer stored in volumes, but are stored in reference values?
If so, skipping updates might be a misnomer (because there is no attestationkey volume which can be skipped anyways). Ideally we should have some job that deletes that short lived attestationKey from trustee reference values)
There was a problem hiding this comment.
If so, skipping updates might be a misnomer
yes, and this comment is not touched in #248, could you raise that there?
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>
Default kube-rs client has no read timeout so that watch operations are supported. Some non-watch operations have been seen to hang. Use a timeout of 30 seconds for such operations. Introduce a structure to pass both clients where needed. Signed-off-by: Jakob Naucke <jnaucke@redhat.com> Assisted-by: AI
GET on trustedexecutionclusters is known to fail occasionally. This should return an error, which will lead to a requeue. Thus, also increase base deletion timeout to 120s (30s timeout on failure, 60s requeue). Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
Jakob-Naucke
left a comment
Jakob-Naucke
left a comment
There was a problem hiding this comment.
Several paths now clone full KubeClients instances when only one client is used (e.g. in launch_keygen_controller, launch_rv_*_controller, and test helpers); you could reduce unnecessary cloning by passing references or extracting just the needed client to keep ownership and lifetimes simpler.
@sourcery-ai these all use both
| if: always() | ||
| uses: actions/upload-artifact@v7 | ||
| with: | ||
| name: must-gather |
There was a problem hiding this comment.
Good idea, I've added the source branch. We could add the artifact ID but IMO the naming you get from the browser downloads (e.g. must-gather-timed-client.zip, must-gather-timed-client.zip(1)) is more intuitive.
| uses: actions/upload-artifact@v7 | ||
| with: | ||
| name: must-gather | ||
| path: must-gather-output/ |
There was a problem hiding this comment.
For public GitHub projects like this one, you get 90 days. It's possible to set something lower here, but as long as GitHub lets us I don't really see a reason to. FWIW one of these is about 120K zipped when all went well, about 750K with failures, so at 30 a week, I don't really see the total size exceeding 500M which is less than what you get with e.g. a free Dropbox account. (The size of OpenShift CI artifacts is another story… 🥲)
| return Ok(Action::await_change()); | ||
| } | ||
| Err(e) => { | ||
| let err = anyhow!("{e}").into(); |
There was a problem hiding this comment.
The request would be launched and never return because prior to this PR, it has no timeout. From a log with UUID per request, which is printed again upon return:
2026-07-28T15:32:33Z GET /api/v1/namespaces/test-4a79467d/secrets/trustee-auth "" 46a1e037-ff78-4a05-97b4-ecefc2ac314d
2026-07-28T15:32:33Z 46a1e037-ff78-4a05-97b4-ecefc2ac314d
and then this GET never returning occurred often:
[2026-07-28T15:33:19Z INFO operator::register_server] get TEC
2026-07-28T15:33:19Z GET /apis/trusted-execution-clusters.io/v1alpha1/namespaces/test-4a79467d/trustedexecutionclusters/trusted-execution-cluster "" 1426a6a1-91fb-4d4f-856d-5871491635d8
(with that UUID never seen again, despite the log continuing until 15:38)
| if machine.metadata.deletion_timestamp.is_some() { | ||
| info!( | ||
| "Machine {} is being deleted, updating attestation key volumes", | ||
| "Machine {} is being deleted, skipping update of attestation key volumes", |
There was a problem hiding this comment.
If so, skipping updates might be a misnomer
yes, and this comment is not touched in #248, could you raise that there?
2d7d477 to
1ec63fe
Compare
2 participants
Default kube-rs client has no read timeout so that watch operations are supported. Some non-watch operations have been seen to hang. Use a timeout of 30 seconds for such operations.
Introduce a structure to pass both clients where needed.
This yielded all successful runs for many hours on GHA CI and 4 passes on the notoriously flaky OpenShift CI.
Also:
warn!in ak-regFixes: #227
@iroykaufman I also tested this with #248, here's the rerere cache already in case we merge this first: rerere-timed-client.tar.gz
Summary by Sourcery
Introduce a dual Kubernetes client setup with read timeouts for non-watch operations while preserving non-timed clients for watch-based controllers and tests, and update logging, controllers, tests, and CI accordingly.
New Features:
Bug Fixes:
Enhancements:
CI:
Tests: