Skip to content

Requeue instead of awaiting change #309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Jakob-Naucke wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Requeue instead of awaiting change #309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2f61d4c
rvs: Reconcile images on jobs too
Jakob-Naucke Jul 9, 2026
9edc004
Remove await_change
Jakob-Naucke Jun 26, 2026
7ae47da
tests: Increase deletion timeouts of owned CRs
Jakob-Naucke Jul 7, 2026
ef6d5ca
operator/ak-reg: Use warn!, not eprintln!
Jakob-Naucke Jul 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 10 additions & 12 deletions operator/src/attestation_key_register.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use kube::{
watcher,
},
};
use log::info;
use log::{info, warn};
use serde_json::json;
use std::{collections::BTreeMap, sync::Arc, time::Duration};

Expand Down Expand Up @@ -195,10 +195,10 @@ async fn ak_reconcile(
for machine in ctx.machine_store.state() {
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)));

@alicefr alicefr Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should we define a macro for the default polling time to highlight when this should be the default behavior?

}
}
Ok(Action::await_change())
Ok(Action::requeue(Duration::from_secs(300)))
}

async fn machine_reconcile(
Expand All @@ -216,18 +216,18 @@ async fn machine_reconcile(
"Machine {} is being deleted, updating attestation key volumes",
machine.metadata.name.clone().unwrap_or_default()
);
return Ok(Action::await_change());
return Ok(Action::requeue(Duration::from_secs(60)));
}

for ak in ctx.ak_store.state() {
if let Some(ak_uuid) = &ak.spec.uuid
&& *ak_uuid == machine.spec.id
{
approve_ak(&ak, &machine, &ctx).await?;
return Ok(Action::await_change());
return Ok(Action::requeue(Duration::from_secs(300)));
}
}
Ok(Action::await_change())
Ok(Action::requeue(Duration::from_secs(300)))
}

async fn approve_ak(ak: &AttestationKey, machine: &Machine, ctx: &AkContextData) -> Result<()> {
Expand Down Expand Up @@ -333,9 +333,9 @@ async fn secret_reconcile(
// On creation/update, just update the trustee deployment volumes
trustee::update_attestation_keys(&ctx)
.await
.map(|_| Action::await_change())
.map(|_| Action::requeue(Duration::from_secs(300)))
.map_err(|e| {
eprintln!("Error updating attestation key volumes on secret apply: {e}");
warn!("Error updating attestation key volumes on secret apply: {e}");
finalizer::Error::<ControllerError>::ApplyFailed(e.into())
})
}
Expand All @@ -347,11 +347,9 @@ async fn secret_reconcile(
// Update trustee deployment - secrets with deletion_timestamp will be filtered out
trustee::update_attestation_keys(&ctx)
.await
.map(|_| Action::await_change())
.map(|_| Action::requeue(Duration::from_secs(300)))
.map_err(|e| {
eprintln!(
"Error updating attestation key volumes during secret deletion: {e}"
);
warn!("Error updating attestation key volumes during secret deletion: {e}");
finalizer::Error::<ControllerError>::CleanupFailed(e.into())
})
}
Expand Down
12 changes: 6 additions & 6 deletions operator/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,11 @@ async fn reconcile(
if changed {
update_status!(clusters, name, TrustedExecutionClusterStatus { conditions })?;
}
return Ok(Action::await_change());
return Ok(Action::requeue(Duration::from_secs(60)));
}

if is_installed(cluster.status.clone()) {
return Ok(Action::await_change());
return Ok(Action::requeue(Duration::from_secs(300)));
}

if ctx.tec_store.state().len() > 1 {
Expand Down Expand Up @@ -145,7 +145,7 @@ async fn reconcile(
let status = TrustedExecutionClusterStatus { conditions };
update_status!(clusters, name, status)?;
}
Ok(Action::await_change())
Ok(Action::requeue(Duration::from_secs(300)))
}

async fn install_components(client: &Client, cluster: &TrustedExecutionCluster) -> Result<()> {
Expand Down Expand Up @@ -336,7 +336,7 @@ mod tests {
let mut cluster = dummy_cluster();
cluster.metadata.deletion_timestamp = Some(Time(Timestamp::now()));
let result = reconcile(Arc::new(cluster), Arc::new(dummy_cluster_ctx(client))).await;
assert_eq!(result.unwrap(), Action::await_change());
assert_eq!(result.unwrap(), Action::requeue(Duration::from_secs(60)));
});
}

Expand Down Expand Up @@ -416,7 +416,7 @@ mod tests {
conditions: Some(vec![foreign_condition]),
});
let result = reconcile(Arc::new(cluster), Arc::new(dummy_cluster_ctx(client))).await;
assert_eq!(result.unwrap(), Action::await_change());
assert_eq!(result.unwrap(), Action::requeue(Duration::from_secs(60)));
});
}

Expand Down Expand Up @@ -493,7 +493,7 @@ mod tests {
});
count_check!(10, clos, |client| {
let result = reconcile(Arc::new(cluster), Arc::new(dummy_cluster_ctx(client))).await;
assert_eq!(result.unwrap(), Action::await_change());
assert_eq!(result.unwrap(), Action::requeue(Duration::from_secs(300)));
});
}

Expand Down
11 changes: 7 additions & 4 deletions operator/src/reference_values.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ async fn image_add_reconcile(
}

let (action, reason) = match handle_new_image(client.clone(), image).await {
Ok(reason) => (Action::await_change(), reason),
Ok(reason) => (Action::requeue(Duration::from_secs(300)), reason),
Err(e) => {
warn!("PCR computation for {name} failed: {e}");
let action = Action::requeue(Duration::from_secs(60));
Expand Down Expand Up @@ -320,7 +320,7 @@ async fn image_remove_reconcile(
let name = image.metadata.name.as_ref().unwrap_or(&default);
if cluster.is_none() {
info!("No TrustedExecutionCluster found, skipping disallow_image for {name}");
return Ok(Action::await_change());
return Ok(Action::requeue(Duration::from_secs(60)));
}
let cluster = cluster.unwrap();
let tec_name = cluster.metadata.name.unwrap_or("<no name>".to_string());
Expand All @@ -329,16 +329,19 @@ async fn image_remove_reconcile(
"TrustedExecutionCluster {tec_name} is being deleted, \
skipping disallow_image for {name}"
);
return Ok(Action::await_change());
return Ok(Action::requeue(Duration::from_secs(60)));
}
disallow_image(client, name).await?;
Ok(Action::await_change())
Ok(Action::requeue(Duration::from_secs(60)))
}

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)
Comment on lines +340 to +344

@sourcery-ai sourcery-ai Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.run(image_reconcile, controller_error_policy, Arc::new(client))
.for_each(controller_info),
);
Expand Down
10 changes: 5 additions & 5 deletions operator/src/register_server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use kube::runtime::{
};
use kube::{Api, Client, Resource};
use log::info;
use std::{collections::BTreeMap, sync::Arc};
use std::{collections::BTreeMap, sync::Arc, time::Duration};

use crate::trustee;
use operator::*;
Expand Down Expand Up @@ -142,7 +142,7 @@ async fn keygen_reconcile(
trustee::mount_secret(kube_client, id).await
}
.await
.map(|_| Action::await_change())
.map(|_| Action::requeue(Duration::from_secs(300)))
.map_err(|e| finalizer::Error::<ControllerError>::ApplyFailed(e.into()))
}
Event::Cleanup(machine) => {
Expand All @@ -168,7 +168,7 @@ async fn keygen_reconcile(
skipping unmount_secret for Machine {}",
machine.metadata.name.as_deref().unwrap_or("unknown")
);
return Ok(Action::await_change());
return Ok(Action::requeue(Duration::from_secs(60)));
}
Err(kube::Error::Api(ae)) if ae.code == 404 => {
// TEC already deleted, skip unmount_secret
Expand All @@ -177,7 +177,7 @@ async fn keygen_reconcile(
skipping unmount_secret for Machine {}",
machine.metadata.name.as_deref().unwrap_or("unknown")
);
return Ok(Action::await_change());
return Ok(Action::requeue(Duration::from_secs(60)));
}
_ => {
// TEC exists and is not being deleted, proceed with unmount_secret
Expand All @@ -187,7 +187,7 @@ async fn keygen_reconcile(

trustee::unmount_secret(kube_client, id)
.await
.map(|_| Action::await_change())
.map(|_| Action::requeue(Duration::from_secs(60)))
.map_err(|e| finalizer::Error::<ControllerError>::CleanupFailed(e.into()))
}
}
Expand Down
2 changes: 1 addition & 1 deletion test_utils/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ impl TestContext {

pub async fn cleanup(&self) -> Result<()> {
self.delete_trusted_execution_cluster().await?;
let timeout = scaled_duration(60);
let timeout = scaled_duration(360);
let msg = format!("Resources were left behind after {timeout:?}");
let poller = Poller::new().with_timeout(timeout).with_error_message(msg);
let chk = || async move {
Expand Down
14 changes: 7 additions & 7 deletions tests/trusted_execution_cluster.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,10 @@ named_test!(
wait_for_resource_deleted(&deployments_api, REGISTER_SERVER_DEPLOYMENT, timeout).await?;

let images_api: Api<ApprovedImage> = Api::namespaced(client.clone(), namespace);
wait_for_resource_deleted(&images_api, APPROVED_IMAGE_NAME, scaled_timeout(120)).await?;
wait_for_resource_deleted(&images_api, APPROVED_IMAGE_NAME, scaled_timeout(360)).await?;

wait_for_resource_deleted(&machines, &machine_name, scaled_timeout(120)).await?;
wait_for_resource_deleted(&attestation_keys, &ak_name, scaled_timeout(120)).await?;
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?;

@sourcery-ai sourcery-ai Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  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.


Expand Down Expand Up @@ -328,9 +328,9 @@ async fn test_attestation_key_lifecycle() -> anyhow::Result<()> {
machines.delete(&machine_name, &dp).await?;
test_ctx.info(format!("Deleted Machine: {machine_name}"));

wait_for_resource_deleted(&machines, &machine_name, scaled_timeout(120)).await?;
wait_for_resource_deleted(&machines, &machine_name, scaled_timeout(360)).await?;
test_ctx.info("Machine successfully deleted");
wait_for_resource_deleted(&attestation_keys, &ak_name, scaled_timeout(120)).await?;
wait_for_resource_deleted(&attestation_keys, &ak_name, scaled_timeout(360)).await?;
test_ctx.info("AttestationKey successfully deleted");
wait_for_resource_deleted(&secrets_api, &ak_name, scaled_timeout(120)).await?;
test_ctx.info("Secret successfully deleted");
Expand Down Expand Up @@ -367,7 +367,7 @@ async fn test_nonexistent_approved_image() -> anyhow::Result<()> {
};
let done = await_condition(images, "coreos1", is_pending);
let ctx = "waiting for ApprovedImage coreos1 to be PodPending";
timeout(scaled_duration(30), done).await.context(ctx)??;
timeout(scaled_duration(360), done).await.context(ctx)??;

test_ctx.cleanup().await?;
Ok(())
Expand Down Expand Up @@ -398,7 +398,7 @@ async fn test_approved_image_readoption() -> anyhow::Result<()> {
test_ctx.info(format!("Deleting TrustedExecutionCluster {TEC_NAME}"));
clusters.delete(TEC_NAME, &Default::default()).await?;
wait_for_resource_deleted(&configmaps, TRUSTEE_CONFIG_MAP, scaled_timeout(60)).await?;
wait_for_resource_deleted(&images, APPROVED_IMAGE_NAME, scaled_timeout(60)).await?;
wait_for_resource_deleted(&images, APPROVED_IMAGE_NAME, scaled_timeout(360)).await?;
test_ctx.info(format!("Configmap {TRUSTEE_CONFIG_MAP} was removed"));

let image = ApprovedImage {
Expand Down