API::delete behavior and idempotency

[Matthias247]

Would you like to work on this feature?

None

What problem are you trying to solve?

I'm trying to delete a kubernetes object using Api::delete

Describe the solution you'd like

When deleting the object, for my use-case I would like to know if

• the deletion has started and/or is ongoing
• the object is fully deleted

My understanding of Api::delete was that I would use Ok(Either::Left) to determine deletion is ongoing, Ok(Either::Right) to determine it's completed, and that all errors (Err) are of a more general nature.

However once testing this, I noticed that trying to delete an already deleted object yields an Err. This makes sense according to the documentation

4XX and 5XX status types are returned as an Err(kube_client::Error::Api).

But I don't think it's a natural mapping to the delete semantics, which should be idempotent.
I would have expected a deletion attempt on an object that is not available (404) to be returned via Ok(Either::Right), which is the same as deleting that is immediately gone. In both cases the result for the caller is that we are sure the object is no longer there.

The current setup requires to special case Err(kube::Error::Api(api_error)) if api_error.code == 404 on the caller side and treat it the same as Ok(Either::Right) if one wants to achieve idempotent deletion. In fact in my setup I wasn't even able to observe an Ok(Either::Right) - deletion directly went from Ok(Either::Left) to a 404 error - which I initially not anticipated due to the API description.

I obviously realize a change in behavior would be a breaking change, and that automatically trying to treat 404s and 200 results the same might be a loss of information for some applications, so there's obviously a drawback too.

An other related question I had is what inspection users are expected to be able to perform with a Either::Right, if they just want know that the object is gone. The documentation mentions potentially different status 2xx status codes, but I couldn't find any references in kubernetes docs on whether something else than a 200 status would be expected and what the meaning of other codes is. If someone has a description, it might be helpful to add this to the Api::delete docs.

Describe alternatives you've considered

Manually special casing the 404 error - as described above

Documentation, Adoption, Migration Strategy

No response

Target crate for feature

No response

[clux]

Hm, yeah, you do raise a good point here, and I think what we have is slightly different than what is intended by the upstream api.

While it is more in-line with rust to return actual Error objects for 404s and 5XXs, it's a bit counter-intuitive to give out a response::Status in only a subset of the cases, particularly if the one that's useful to match on the Status::code on needs to be extracted from the an Error.

I wasn't even able to observe an Ok(Either::Right)

IIRC you need to fiddle with the DeleteParams to be able to get one. That said, I've never found matching on the Either to be actually useful in an app setting (if i need to track deletion i generally use the is_deleted Condition in the runtime).

I think there is room to experiment here with a more idempotent Api::delete_force (naming ideas welcome, was thinking rm's -f flag) where we try to find an easier user variant. Note that we did a similar helper for Api::get_opt to avoid breaking Api::get. I don't want to break Api::delete itself at this point. However, if these ideas are popular, then we can probably make them more prominent for the Client V2 goal #1032 (like maybe calling it something like Client::rm)

Feel free to sketch out a PR for ideas on this, and I can review, otherwise i'll leave it as help wanted.

[rakshith-ravi]

Hi @Matthias247 @clux

We faced a similar problem internally and we got around it by doing this. I hope this helps anybody else looking for a similar solution as well:

#[async_trait]
pub trait DeleteOpt<T> {
	async fn delete_opt(
		&self,
		name: &str,
		dp: &DeleteParams,
	) -> Result<Option<Either<T, Status>>>;
}

#[async_trait]
impl<T> DeleteOpt<T> for Api<T>
where
	T: Clone + DeserializeOwned + Debug,
{
	async fn delete_opt(
		&self,
		name: &str,
		dp: &DeleteParams,
	) -> Result<Option<Either<T, Status>>> {
		match self.delete(name, dp).await {
			Ok(obj) => Ok(Some(obj)),
			Err(Error::Api(ErrorResponse { code: 404, .. })) => Ok(None),
			Err(err) => Err(err),
		}
	}
}

We then use this by simply doing:

Api::<Service>::namespaced(kubernetes_client, namespace)
	.delete_opt(
		&service_name,
		&DeleteParams::default(),
	)
	.await?;