Use a read-timeout client for non-watch operations

[Jakob-Naucke]

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:

• pass error if Machine cleanup fails
• reconcile ApprovedImages on job changes too for status updates
• use must-gather in GHA
• fix a log message and uses of warn! in ak-reg
• add timestamps to logging, which

Fixes: #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:

• Add a timed Kubernetes client with a 30-second read timeout for request/response API calls and expose it via the shared library.
• Introduce a KubeClients struct to pass both timed and watch-safe clients throughout the operator, services, test utilities, and virtual machine helpers.
• Extend the reference value image controller to own PCR-compute jobs so that ApprovedImage status and related resources reconcile correctly on job changes.

Bug Fixes:

• Ensure machine cleanup failures in the keygen controller surface as finalizer cleanup errors instead of being silently ignored.
• Use dedicated watch clients for long-lived streams and resource wait helpers to avoid hangs and improve reliability of operator controllers and integration tests.

Enhancements:

• Refactor operator, attestation-key-register, register-server, compute-pcrs, and test utilities to consistently use the new timed client for non-watch operations and a separate client for watches.
• Add timestamps to test logging and enable env_logger humantime formatting for clearer, time-correlated logs.
• Replace ad-hoc eprintln error logging with structured warn logging in the attestation key register secret reconciliation logic.
• Increase certain namespace and resource cleanup timeouts in tests to reduce flakiness on slower clusters.
• Adjust KubeVirt VM helpers to use the appropriate timed or watch clients for resource creation and readiness checks.

CI:

• Add a must-gather collection and artifact upload step to the GitHub Actions integration test workflow to aid in debugging CI failures.

Tests:

• Update integration and attestation tests to use separate timed and watch clients, including for await_condition and wait_for_resource_deleted calls, to improve stability across platforms.

[sourcery-ai]

Reviewer's Guide

Introduce 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

Change	Details	Files
Introduce timed Kubernetes client and shared dual-client abstraction for separating request/response calls from watch streams.	Add KUBE_READ_TIMEOUT constant and timed_client() helper that configures kube::Config.read_timeout for non-watch operations. Define KubeClients struct bundling a timed client and a default watch client for reuse across operator and tests. Switch various binaries and utilities (attestation-key-register, register-server, compute-pcrs, tests) from Client::try_default to timed_client for non-watch use.	lib/src/lib.rs attestation-key-register/src/main.rs register-server/src/main.rs compute-pcrs/src/main.rs
Refactor operator controllers and caches to consume KubeClients, using timed client for writes and watch client for reflectors and Controller sources.	Change ClusterContext and AkContextData constructors to take &KubeClients and store both client and watch_client. Update spawn_reflector and Controller::new calls in attestation_key_register, reference_values, register_server, and main to use watch clients for list/watch and timed clients for reconcile logic. Extend reference value image controller to own Jobs as a secondary resource and make job controller use KubeClients. Adjust main() wiring to build a KubeClients pair and thread it through all controller launch functions instead of raw Client.	operator/src/main.rs operator/src/attestation_key_register.rs operator/src/reference_values.rs operator/src/register_server.rs
Update test utilities and integration tests to use KubeClients, separating watch APIs from standard client calls and increasing cleanup robustness.	Refactor TestContext to hold KubeClients, expose client(), watch_client(), and clients() accessors, and build clients via setup_test_clients(). Split Api constructions so long-running await_condition / wait_for_resource_* calls use watch clients while CRUD and list calls use timed clients. Adjust OpenShift and Kind platform helpers, virt backends, and VM tests to work with KubeClients instead of raw Client. Increase namespace cleanup timeout and error messaging for resource leakage checks.	test_utils/src/lib.rs test_utils/src/virt/mod.rs test_utils/src/virt/kubevirt.rs tests/trusted_execution_cluster.rs tests/attestation.rs
Improve error handling and logging semantics in the operator and test harness, including timestamps and proper log levels.	Add log_time() helper and prepend timestamps to test_info! and test_warn! outputs; enable env_logger humantime feature for better runtime logs. Replace eprintln! calls in secret_reconcile with warn! logging and adjust informational log message when machine is being deleted in machine_reconcile. Ensure Machine cleanup failures in keygen_reconcile return a CleanupFailed finalizer error instead of silently continuing.	test_utils/src/lib.rs operator/src/attestation_key_register.rs operator/src/register_server.rs Cargo.toml
Enhance CI observability by always collecting and uploading must-gather artifacts from integration test runs.	Add steps to integration-tests GitHub Actions workflow to run must-gather/gather regardless of test result and upload the collected artifacts as an action artifact.	.github/workflows/integration-tests.yml

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#227	Add timestamps to logs for confidential-cluster-operator, register-server, and attestation-key-register components.	✅
#227	Migrate logging to a tracing framework for better observability.	❌	The PR keeps using env_logger/log macros and only enables the env_logger humantime feature and adjusts test logging, but does not introduce or switch to a tracing framework (such as tracing and tracing-subscriber).

Possibly linked issues

• #unknown: PR adds timed and watch clients and updates AK controller logic, reducing approval latency noted in issue.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• 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.

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>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[SpaceFace02]

Thanks for the PR! I have a few small suggestions

[SpaceFace02]

nit: Like Sourcery mentioned, the naming could be changed to something less generic.

[SpaceFace02]

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

[Jakob-Naucke]

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

[SpaceFace02]

LGTM 👍

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

Minor nit (but that shouldn't stop this PR from getting merged)

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Jakob-Naucke, SpaceFace02

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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