Convert impl Condition return signature to Option<bool>

[clux]

To allow taking advantage of ? on Option to allow simpler Condition impls.

    fn is_pod_running() -> impl Condition<Pod> {
        |obj: Option<&Pod>| Some(obj?.status.as_ref()?.phase.as_ref()? == "Running")
    }

This also technically allows for distinguishing between;

• condition definitively does not match the condition
• condition could not check properties on the object

Generally, this is probably not hugely valuable, but it allows us to have a clearer answer rather than to conflate missing with either true or false.

Breaking

This is generally not a breaking change. The matches_object fn retains its original signature + behavior. It does this by using the new trait method matches with unwrap_or_default:

    fn matches_object(&self, obj: Option<&K>) -> bool {
        self.matches(obj).unwrap_or_default()
    }

However;

This is a breaking change only for implementors of Condition who now need to return an Option<bool> rather than bool for impl Condition. My thinking is that being able to use option-try makes this worth it. It can be easily communicated that you take the value and put it in a Some otherwise.

This is also a slight breaking change for the newly introduced (#1710) conditions::is_service_loadbalancer_provisioned which was returning true for services the condition did not apply for. Now it returns Some(false) for such services.

[Danil-Grigorev]

Lookup of the conditions is one of the most common scenarios in controller code. If anything can make it simpler, I think it is worth implementing.

[codecov]

Codecov Report

Attention: Patch coverage is 89.74359% with 4 lines in your changes missing coverage. Please review.

Project coverage is 76.0%. Comparing base (57a5e14) to head (e3b341c).

Files with missing lines	Patch %	Lines
kube-runtime/src/wait.rs	88.9%	4 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1498     +/-   ##
=======================================
- Coverage   76.1%   76.0%   -0.0%     
=======================================
  Files         84      84             
  Lines       7859    7843     -16     
=======================================
- Hits        5976    5960     -16     
  Misses      1883    1883             

Files with missing lines	Coverage Δ
kube/src/lib.rs	89.0% <100.0%> (+0.5%)	⬆️
kube-runtime/src/wait.rs	92.1% <88.9%> (-1.3%)	⬇️

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[clux]

I have revamped this after seeing new verbose Condition impls, and thinking this is probably worth it with some care. Now doing it with quite minimal set of breaking changes. Have updated the PR body with details.

[clux]

hey @detjensrobert doing a follow-up on conditions. wondering if you had a particular reason to return true here rather than false for this condition. feels to me we shouldn't consider a condition "matching" on a service that's not even of the right type.

also if you have opinions on this pr, happy to hear 🙏

EDIT: sorry about spam, put comment on wrong place with wrong account.

[detjensrobert]

My thought was that since non-LoadBalancer services do not need to wait for a cloud LB, they are (almost) immediately ready and do not need any checks. I can see this wanting to fail on the wrong type of service though.

[nightkr]

The ergonomic improvement is compelling.

I don't love the idea of having two separate-but-equivalent values (None and Some(false)). We could use Option<()> instead, but that makes the match logic itself less ergonomic (a == b would become, at best, (a == b).then_some(())).

I especially don't think Condition should allow distinguishing between these conditions, both because there's no clear semantic on the distinction, and because it breaks identities like !!x == x.

This is a breaking change only for implementors of Condition who now need to return an Option rather than bool for impl Condition.

I'd still consider this "very breaking", given how the API is designed to make implementing custom Conditions a very routine and ergonomic thing to do ("just return a lambda!").

We could avoid the breakage and support multiple alternatives using an intermediate trait (https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=575f74b0a646dd428a25ffb284189850), but I'm not sure the juice is worth the squeeze.

[detjensrobert]

If we want to keep the bool return type, we could use a nested Option function (or the upcoming try { } block) to get the early return. I had used this in my own versions of the conditions upstreamed in #1710:

await_condition(api, &object.name_any(), |d: Option<&Deployment>| {
  // Use a nested function so that we can use Option? returns (the outer closure returns `bool`)
  // TODO: switch to try { } when that is standardized
  
  /// Replicate the upstream deployment complete check
  /// https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#complete-deployment
  fn _inner(d: Option<&Deployment>) -> Option<bool> {
      Some(d?.status.as_ref()?.conditions.as_ref()?.iter().any(|c| {
          c.reason == Some("NewReplicaSetAvailable".to_string()) && c.status == "True"
      }))
  }
  
  _inner(d).unwrap_or(false)
})

This doesn't require any changes to the type since this is now all internal to the existing closure, but is getting weird with now two levels of inline functions/closures which kinda feels like a code smell.

Related, what was the rationale for conditions returning a lambda instead of the condition function itself implementing the check directly? Is that a limitation of the trait type?

[nightkr]

You can also (currently) do await_condition(..., |d: Option<&Deployment>| d.and_then(|d| d.status.as_ref()?.conditions.as_ref()?.iter().find(|c| c.reason.as_deref() == Some("NewReplicaSetAvailable"))).map(|c| c.status.as_deref()) == "True")