Requeue instead of awaiting change#309
Open
Jakob-Naucke wants to merge 4 commits into
Open
Conversation
so that status is updated in a timely manner 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 GuideThe PR replaces await_change-based controller behavior with explicit requeue delays and extends related test timeouts, while wiring the image controller to own Jobs so status/image reconciliations happen reliably even when events are missed. Sequence diagram for controller requeue behavior instead of await_changesequenceDiagram
participant Controller
participant image_add_reconcile
participant handle_new_image
Controller->>image_add_reconcile: reconcile ApprovedImage
image_add_reconcile->>handle_new_image: handle_new_image(client, image)
handle_new_image-->>image_add_reconcile: Ok(reason)
image_add_reconcile-->>Controller: Action::requeue(Duration::from_secs(300)), reason
alt image_remove_reconcile paths
Controller->>image_remove_reconcile: reconcile ApprovedImage removal
image_remove_reconcile-->>Controller: Action::requeue(Duration::from_secs(60))
end
alt keygen_reconcile cleanup
Controller->>keygen_reconcile: Event::Cleanup(machine)
keygen_reconcile->>trustee_unmount_secret: trustee::unmount_secret(kube_client, id)
trustee_unmount_secret-->>keygen_reconcile: Ok(())
keygen_reconcile-->>Controller: Action::requeue(Duration::from_secs(60))
end
alt keygen_reconcile apply
Controller->>keygen_reconcile: Event::Apply(machine)
keygen_reconcile->>trustee_mount_secret: trustee::mount_secret(kube_client, id)
trustee_mount_secret-->>keygen_reconcile: Ok(())
keygen_reconcile-->>Controller: Action::requeue(Duration::from_secs(300))
end
File-Level Changes
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 2 issues, and left some high level feedback:
- The various requeue intervals (60s vs 300s) and scaled timeouts (360s) are currently hard-coded in multiple places; consider centralizing them into named constants to make the intended behavior clearer and easier to adjust.
- For the new
.owns(jobs, wc)watcher configuration, double-check that the label selector string construction ({JOB_LABEL_KEY}={PCR_COMMAND_NAME}) matches all existing job labels so that reconcile behavior for related jobs is consistent and predictable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The various requeue intervals (60s vs 300s) and scaled timeouts (360s) are currently hard-coded in multiple places; consider centralizing them into named constants to make the intended behavior clearer and easier to adjust.
- For the new `.owns(jobs, wc)` watcher configuration, double-check that the label selector string construction (`{JOB_LABEL_KEY}={PCR_COMMAND_NAME}`) matches all existing job labels so that reconcile behavior for related jobs is consistent and predictable.
## Individual Comments
### Comment 1
<location path="operator/src/reference_values.rs" line_range="340-344" />
<code_context>
pub async fn launch_rv_image_controller(client: Client) {
let images: Api<ApprovedImage> = Api::default_namespaced(client.clone());
+ let jobs: Api<Job> = Api::default_namespaced(client.clone());
+ let wc = watcher::Config::default().labels(&format!("{JOB_LABEL_KEY}={PCR_COMMAND_NAME}"));
tokio::spawn(
Controller::new(images, Default::default())
+ .owns(jobs, wc)
.run(image_reconcile, controller_error_policy, Arc::new(client))
.for_each(controller_info),
</code_context>
<issue_to_address>
**question (bug_risk):** Clarify the implications of restricting owned Jobs via a label selector in the watcher config.
Using `watcher::Config::default().labels(&format!("{JOB_LABEL_KEY}={PCR_COMMAND_NAME}"))` with `.owns(jobs, wc)` limits ownership tracking to Jobs with exactly that label/value. Any relevant Jobs missing this label (or using a different value) will not be reconciled, while unrelated Jobs that reuse the label may be tracked incorrectly. Please either narrow the selector further (e.g., additional labels/owner refs) or confirm that all Jobs created for this controller reliably set this label.
</issue_to_address>
### Comment 2
<location path="tests/trusted_execution_cluster.rs" line_range="120" />
<code_context>
+ wait_for_resource_deleted(&machines, &machine_name, scaled_timeout(360)).await?;
+ wait_for_resource_deleted(&attestation_keys, &ak_name, scaled_timeout(360)).await?;
let secrets_api: Api<Secret> = Api::namespaced(client.clone(), namespace);
wait_for_resource_deleted(&secrets_api, &ak_name, scaled_timeout(120)).await?;
</code_context>
<issue_to_address>
**suggestion (testing):** Consider aligning Secret deletion timeout with the extended 360s timeouts for Machines and AttestationKeys
In the TEC cleanup and attestation key lifecycle tests, Machine and AttestationKey deletions were increased to 360s to tolerate missed reconcile events, but Secret deletion remains at 120s. If Secret deletion uses the same controller path, this shorter timeout can still cause intermittent flakes when reconciles are missed. Please consider increasing this to 360s (or centralizing these timeouts) for consistent test behavior.
Suggested implementation:
```rust
wait_for_resource_deleted(&secrets_api, &ak_name, scaled_timeout(360)).await?;
```
If other tests in this file (or module) also use hard-coded deletion timeouts, consider:
1. Introducing a shared constant (e.g., `const DELETION_TIMEOUT_SECS: u64 = 360;`) and replacing the magic numbers with it.
2. Ensuring all resources that rely on the same controller path use the same timeout to avoid inconsistent behavior across tests.
</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
+340
to
+344
| let jobs: Api<Job> = Api::default_namespaced(client.clone()); | ||
| let wc = watcher::Config::default().labels(&format!("{JOB_LABEL_KEY}={PCR_COMMAND_NAME}")); | ||
| tokio::spawn( | ||
| Controller::new(images, Default::default()) | ||
| .owns(jobs, wc) |
There was a problem hiding this comment.
question (bug_risk): Clarify the implications of restricting owned Jobs via a label selector in the watcher config.
Using watcher::Config::default().labels(&format!("{JOB_LABEL_KEY}={PCR_COMMAND_NAME}")) with .owns(jobs, wc) limits ownership tracking to Jobs with exactly that label/value. Any relevant Jobs missing this label (or using a different value) will not be reconciled, while unrelated Jobs that reuse the label may be tracked incorrectly. Please either narrow the selector further (e.g., additional labels/owner refs) or confirm that all Jobs created for this controller reliably set this label.
| wait_for_resource_deleted(&machines, &machine_name, scaled_timeout(360)).await?; | ||
| wait_for_resource_deleted(&attestation_keys, &ak_name, scaled_timeout(360)).await?; | ||
| let secrets_api: Api<Secret> = Api::namespaced(client.clone(), namespace); | ||
| wait_for_resource_deleted(&secrets_api, &ak_name, scaled_timeout(120)).await?; |
There was a problem hiding this comment.
suggestion (testing): Consider aligning Secret deletion timeout with the extended 360s timeouts for Machines and AttestationKeys
In the TEC cleanup and attestation key lifecycle tests, Machine and AttestationKey deletions were increased to 360s to tolerate missed reconcile events, but Secret deletion remains at 120s. If Secret deletion uses the same controller path, this shorter timeout can still cause intermittent flakes when reconciles are missed. Please consider increasing this to 360s (or centralizing these timeouts) for consistent test behavior.
Suggested implementation:
wait_for_resource_deleted(&secrets_api, &ak_name, scaled_timeout(360)).await?;If other tests in this file (or module) also use hard-coded deletion timeouts, consider:
- Introducing a shared constant (e.g.,
const DELETION_TIMEOUT_SECS: u64 = 360;) and replacing the magic numbers with it. - Ensuring all resources that rely on the same controller path use the same timeout to avoid inconsistent behavior across tests.
Member
|
Looking good! Maybe it is also worth replace the |
Member
I think #302 is still necessary because the following scenario still holds: |
As per kube-rs docs, await_change is unadvisable for cases where eventual consistency is desired because reconcile events can be lost occasionally. We saw this happen in tests where owned resources with finalizers stayed behind after TEC deletion. Instead, requeue after 1 minute (ongoing deletion cases) or 5 minutes (resources applied cases). This is shorter than kube-rs's "sane default" of 60 minutes, but aligns better with testing, user expectations, and is also what KubeVirt appears to be doing. Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
requeue is 5 minutes, so wait 6 minutes for deletion if the event is missed. Also applies to ApprovedImage status update. Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
in consistency with remainder codebase Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
alicefr
reviewed
Jul 13, 2026
| if ak.spec.uuid.as_ref() == Some(&machine.spec.id) { | ||
| approve_ak(&ak, &machine, &ctx).await?; | ||
| return Ok(Action::await_change()); | ||
| return Ok(Action::requeue(Duration::from_secs(300))); |
Contributor
There was a problem hiding this comment.
nit: should we define a macro for the default polling time to highlight when this should be the default behavior?
Contributor
|
@Jakob-Naucke why do we set 5 mins instead of the suggested hour? Can you please give some example of missing events. |
Contributor
|
My worry is that decreasing the polling time is just a mitigation for real bugs. I'd like to understand if it is the operator that just ignores the events or it is kubernetes that just drops those events |
Member
Author
This is a tradeoff between "overhead and log noise" and "how long it takes to reach consistency when events are lost". I went for a duration that also still makes sense for timeouts in testing. (Also considered changing the requeue duration for tests only, but that's a bit of a volkswagenism)
So what's known to have happened is cases where (enhanced, see diff) operator logs suggest that the resource was deleted eventually, but error log suggests that it happened too late: diff --git a/operator/src/reference_values.rs b/operator/src/reference_values.rs
index 4077d9b..c6faa15 100644
--- a/operator/src/reference_values.rs
+++ b/operator/src/reference_values.rs
@@ -324,2 +324,3 @@ async fn image_remove_reconcile(
let name = image.metadata.name.as_ref().unwrap_or(&default);
+ info!("Cleanup for image {image:?}");
if cluster.is_none() {but I understand your concern. I'm considering re-running without this patchset, but with timestamps (hello #227) for more definite proof of what's happening. |
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
3 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.
As per kube-rs docs, await_change is unadvisable for cases where
eventual consistency is desired because reconcile events can be lost
occasionally. We saw this happen in tests where owned resources with
finalizers stayed behind after TEC deletion.
Instead, requeue after 1 minute (ongoing deletion cases) or 5
minutes (resources applied cases). This is shorter than kube-rs's
"sane default" of 60 minutes, but aligns better with testing, user
expectations, and is also what KubeVirt appears to be doing.
Also
IMO this supersedes #302.complements #302. I had a number of successful runs with just this patchset on OpenShift CI, but #302 makes sense logically.Summary by Sourcery
Switch controller reconciler actions from await_change to periodic requeue and adjust related timeouts to improve eventual consistency and test reliability.
Bug Fixes:
Enhancements:
Tests: