Skip to content
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.

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

Generic wait-for-condition Condition #679

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
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
73 changes: 71 additions & 2 deletions kube-runtime/src/wait.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ where
/// }
/// }
/// ```
pub trait Condition<K> {
pub trait Condition<K: ?Sized> {
fn matches_object(&self, obj: Option<&K>) -> bool;

/// Returns a `Condition` that holds if `self` does not
Expand Down Expand Up @@ -146,7 +146,7 @@ pub trait Condition<K> {
}
}

impl<K, F: Fn(Option<&K>) -> bool> Condition<K> for F {
impl<K: ?Sized, F: Fn(Option<&K>) -> bool> Condition<K> for F {
fn matches_object(&self, obj: Option<&K>) -> bool {
(self)(obj)
}
Expand All @@ -157,6 +157,7 @@ pub mod conditions {
pub use super::Condition;
use k8s_openapi::apiextensions_apiserver::pkg::apis::apiextensions::v1::CustomResourceDefinition;
use kube_client::Resource;
use serde::Serialize;

/// An await condition that returns `true` once the object has been deleted.
///
Expand Down Expand Up @@ -192,6 +193,74 @@ pub mod conditions {
}
}

/// An await condition that returns `true` if the object exists.
///
/// NOTE: If waiting for an object to be deleted, do _not_ [invert](`Condition::not`) this [`Condition`].
/// Instead, use [`is_deleted`], which considers a deleted-then-recreated object to have been deleted.
#[must_use]
pub fn exists<K>() -> impl Condition<K> {
|obj: Option<&K>| obj.is_some()
}

/// A condition that returns true if an arbitrary condition matches a condition value
///
/// # Value condition
///
/// The value condition is passed `None` if the object does not exist or does not have the given condition (combine with
/// [`exists`] if you need to validate whether the object exists). Otherwise, the value should be one of `"True"`, `"False"`, or `"Unknown"`
/// (see <https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.22/#condition-v1-meta> for more details).
///
/// # Stability
///
/// This is an experimental API that should be expected to change. It has a few particular problems:
///
/// 1. It is completely untyped
/// 2. It makes fairly deep assumptions about the structure of the object and its status
/// 3. It doesn't have any way to signal errors gracefully
/// 4. It has some unfortunate lifetime problems that prevent bringing in a closure context
///
/// # Usage
///
/// ```rust
/// # use k8s_openapi::api::core::v1::{Pod, PodCondition, PodStatus};
/// # use kube_runtime::wait::{conditions::unstable_has_status_condition, Condition};
Copy link
Member

Choose a reason for hiding this comment

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

for reference; in docs you can use kube::runtime in doc paths for consistency. i've done this everywhere in all crates to refer to kube

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a doctest, which is built before the kube crate is. Either way, the line is hidden from readers (by prefixing with #).

/// let pod = |ready: String| Pod {
/// status: Some(PodStatus {
/// conditions: Some(vec![PodCondition {
/// type_: "Ready".to_string(),
/// status: ready,
/// ..PodCondition::default()
/// }]),
/// ..PodStatus::default()
/// }),
/// ..Pod::default()
/// };
/// let cond_status_ready: fn(Option<&str>) -> bool = |status| status == Some("True");
/// let cond_pod_ready = unstable_has_status_condition("Ready", cond_status_ready);
Comment on lines +238 to +239
Copy link
Member

Choose a reason for hiding this comment

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

I almost feel like two &str inputs here are better than this closure. It's a struggle to understand why cond_status_ready needs so much wrapping from a consumer standpoint when all you are really doing here is passing in the value of an expected string.

Copy link
Member Author

Choose a reason for hiding this comment

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

That doesn't cover something like |status| status == Some("Unknown") || status == Some("True"). It's also worth keeping in mind that status can ultimately be any string, the True/False/Unknown troolean is just a convention.

Then again, I'm not sure those advanced use-cases are something people actually want?

/// assert!(!cond_pod_ready.matches_object(Some(&pod("False".to_string()))));
/// assert!(cond_pod_ready.matches_object(Some(&pod("True".to_string()))));
/// ```
#[must_use]
pub fn unstable_has_status_condition<'a, K: Serialize + Resource, StatusCond: Condition<str> + 'a>(
Comment on lines +243 to +244
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should look into something like https://docs.rs/stability/0.1.0/stability/attr.unstable.html rather than prefixing fn names

Copy link
Member Author

Choose a reason for hiding this comment

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

That kind of makes sense. I'd almost rather have this be something that the end user has to opt into specifically with a --cfg, rather than a feature you can inherit by accident from a dependency.

Copy link
Member

Choose a reason for hiding this comment

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

There is the tokio solution for this as well #508 (comment). Anyway, I've made notes about this in the big stability roadmap issue, because it's something we should solve for that.

condition_type: &'a str,
status_cond: StatusCond,
) -> impl Condition<K> + 'a {
move |obj: Option<&K>| {
let serialized_obj = serde_json::to_value(obj).ok();
status_cond.matches_object(serialized_obj.as_ref().and_then(|obj| {
obj.get("status")?
.get("conditions")?
Comment on lines +249 to +252
Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally we'd get around this with a HasConditions trait, but that would have to be upstream in k8s-openapi to be of much help

Copy link
Member

Choose a reason for hiding this comment

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

I think those types of traits we'd need to define in the protobuf repo. I'll open some issues there to track.

.as_array()?
.iter()
.find(|cond| {
cond.get("type").and_then(serde_json::Value::as_str) == Some(condition_type)
})?
.get("status")?
.as_str()
Comment on lines +253 to +259
Copy link
Member Author

Choose a reason for hiding this comment

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

🤮

Copy link
Member

Choose a reason for hiding this comment

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

hah, I thought we were going ot start with DynamicObject first. This is pretty gross :D

}))
}
}

/// See [`Condition::not`]
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub struct Not<A>(pub(super) A);
Expand Down