Implement ephemeral containers subresource #1153

jmintb · 2023-02-26T20:28:47Z

Motivation

Implement support for another sub resource, as described in #127.

Solution

Support for the ephemeral containers subresource is implemented using a marker trait Ephemeral, which is implemented for the Pod struct. Three methods are implemented for Api<K> where K is bounded with the Ephemeral trait, replace_ephemeral_containers, patch_ephemeral_containers and get_ephemeral_containers. These methods correlate with the operations described in the Kubernetes API, see the section on EphemeralContainers Operations.

Questions

I have a few questions before this should be merged.

I noticed that the patch operation did not return errors for invalid patches. Is that expected behavior when patching a resource? See this comment for examples.
This functionality was stabilized in K8S 1.25, so we need to guard this functionality. A guard was mentioned here but I am not sure how to apply it in this case. Are there other examples in kubers of guarding features based on K8S versions to use as inspiration?
I tried to follow the existing structure for sub resources hence the marker trait, but I am unsure if it makes sense in this case. Since ephemeral containers are only relevant for Pods. Do you think the marker trait makes sense here?

clux · 2023-02-27T03:27:55Z

Thanks a lot for this. Having had a quick look it looks generally sensible and fits in with the existing conventions.

I have a few questions before this should be merged.

I noticed that the patch operation did not return errors for invalid patches. Is that expected behavior when patching a resource? See this comment for examples.

hm.. yeah, that is unfortunate. that does not feel like it's expected. maybe there's an upstream issue for it in kubernetes (or maybe no one has noticed it if everyone so far using it has come from client-go). it's another awkward kubernetes api where they expect you to supply one struct, but simultaneously nest it inside a bigger struct they won't use, so i'm sure there are interesting edge cases.

This functionality was stabilized in K8S 1.25, so we need to guard this functionality. A guard was mentioned here but I am not sure how to apply it in this case. Are there other examples in kubers of guarding features based on K8S versions to use as inspiration?

Ah, yeah, we have used the k8s-openapi macros for this before. Here's the last one we removed.

I tried to follow the existing structure for sub resources hence the marker trait, but I am unsure if it makes sense in this case. Since ephemeral containers are only relevant for Pods. Do you think the marker trait makes sense here?

Yeah, I think so. We have other marker traits that are only relevant to pods (like Evict), and it will become handy to have these traits if we ever get around to switching serialization format to k8s-pb.

clux

some minor nits, interface tweaks, and test structure stuff.

the only thing here that i don't like is the api condition kubernetes is imposing on us (having to append to a vector). if there's any way to get around that through json patches or server-side apply, then that would be a great thing to figure out as a better alternative for us to provide in the docs.

kube-client/src/api/subresource.rs

clux · 2023-02-27T03:18:11Z

kube-client/src/api/subresource.rs

+    /// #[tokio::main]
+    /// async fn main() -> Result<(), Box<dyn std::error::Error>> {


NB: These docs can be simplified to use the async wrapper to make it focused on the actual behavior;

/// # async fn wrapper() -> Result<(), Box<dyn std::error::Error>> { /// let client: Client = todo!(); /// ... actual relevant example part here ; pods, pp, patch /// Ok(()) /// # }

i realise this is something we have not been consistent about everywhere though and this file is using the old style, so we can probably do that in a different PR.

That looks good!, I will use the wrapper.

Nice. Note I am doing the same in core_methods now (but not touching this file to avoid conflicts in this file) in #1158

I took the liberty of updating the other sub resource documentation examples as well.

Might have hidden too many imports in the examples though 🤔

I took the liberty of updating the other sub resource documentation examples as well.

appreciate it!

Might have hidden too many imports in the examples though 🤔

eh, yeah maybe. i would have left the k8s_openapi and kube imports visible but it's not a big deal.

roger that, I have left them visible now.

kube-client/src/api/subresource.rs

kube-client/src/lib.rs

kube-client/src/api/subresource.rs

kube-client/src/lib.rs

kube-client/src/api/mod.rs

kube-client/src/lib.rs

jmintb · 2023-04-07T08:36:11Z

Just checking in, is there anything I am missing before the next round of review? :)

If you are just busy then there is no rush 👍 , just wanted to make sure that you weren't waiting for me to do something.

I will have another look at the failing CI and versioning guarding the tests.

clux · 2023-04-07T08:59:56Z

Ah, sorry, I have been a bit busy. Tons of PRs for 0.81 (which i plan on releasing today).
There's been some changes to parameter structs so might be useful to update the branch at this point.

clux · 2023-04-07T09:17:39Z

The cargo deny lint is producing peculiar errors thinking , not sure what is causing this.

Don't worry about that. I expect it will be gone after an update, but it can be resolved separately. The deny dependency duplication is a bit annoying for CI since it can break randomly - but it is useful info for us.

kube-core/src/params.rs

jmintb · 2023-06-25T16:11:04Z

I had to focus on uni for a bit. Will get back on this now.

codecov · 2023-06-25T19:02:45Z

Codecov Report

Merging #1153 (18cec4c) into main (a9f1a4f) will decrease coverage by 0.74%.
The diff coverage is 9.58%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1153      +/-   ##
==========================================
- Coverage   73.28%   72.55%   -0.74%     
==========================================
  Files          75       75              
  Lines        6057     6136      +79     
==========================================
+ Hits         4439     4452      +13     
- Misses       1618     1684      +66

Impacted Files	Coverage Δ
kube-client/src/api/mod.rs	`68.75% <ø> (ø)`
kube-client/src/api/subresource.rs	`44.00% <0.00%> (-7.97%)`	⬇️
kube-client/src/lib.rs	`76.21% <14.00%> (-17.58%)`	⬇️

... and 1 file with indirect coverage changes

clux · 2023-07-22T13:11:01Z

kube-core/src/params.rs

+/// let invalid_patch: PodSpec = serde_json::from_value(serde_json::json!({
+///                 "activeDeadlineSeconds": 5
+/// }))?;
+///
+/// // This will have no effect on mypod.
+/// pods.patch("mypod", &pp, &Patch::Strategic(invalid_patch)).await?;


As a future note to self: I am guessing this happens because activeDeadlineSeconds is not a field on the top level struct expected in the patch on the go side, and thus once it's deserialized, the apiserver is simply asked to do an empty patch, which should never be an error.

This commit implements replace, patch and get operations for the ephemeral containers subresource. Signed-off-by: Jessie Chatham Spencer <jessie@teainspace.com>

clux

Looks great. Thank you so much. Appreciate the perseverance through the amount of annoyingly complex details that arose in here.

jmintb changed the title ~~Implement ephemeral containers subresource~~ Draft: Implement ephemeral containers subresource Feb 26, 2023

clux added the changelog-add changelog added category for prs label Feb 27, 2023

clux reviewed Feb 27, 2023

View reviewed changes

jmintb force-pushed the issue127 branch 4 times, most recently from 80acece to 3e12621 Compare March 9, 2023 09:30

jmintb commented Mar 9, 2023

View reviewed changes

kube-client/src/lib.rs Show resolved Hide resolved

jmintb force-pushed the issue127 branch 2 times, most recently from 0f208b6 to 22b3ed7 Compare March 9, 2023 10:35

jmintb commented Mar 9, 2023

View reviewed changes

kube-client/src/api/mod.rs Show resolved Hide resolved

jmintb force-pushed the issue127 branch from 22b3ed7 to 4c3f6bf Compare March 9, 2023 11:34

clux reviewed Mar 10, 2023

View reviewed changes

kube-client/src/lib.rs Show resolved Hide resolved

clux reviewed Mar 10, 2023

View reviewed changes

kube-client/src/lib.rs Show resolved Hide resolved

clux mentioned this pull request Mar 12, 2023

implement the last subresources #127

Open

11 tasks

jmintb force-pushed the issue127 branch 5 times, most recently from 6713fee to c9503d5 Compare March 18, 2023 08:14

This comment was marked as resolved.

Sign in to view

jmintb mentioned this pull request Mar 18, 2023

Potential extension of PatchParams::validate to cover for invalid patch footgun #1164

Open

jmintb commented Mar 18, 2023

View reviewed changes

kube-client/src/lib.rs Outdated Show resolved Hide resolved

jmintb force-pushed the issue127 branch from c9503d5 to 772463e Compare March 18, 2023 09:14

clux modified the milestone: 0.82.0 Apr 7, 2023

clux modified the milestones: 0.83.0, 0.82.1 Apr 8, 2023

jmintb force-pushed the issue127 branch 2 times, most recently from 82512c7 to ca06b4c Compare April 26, 2023 16:58

clux reviewed May 3, 2023

View reviewed changes

kube-core/src/params.rs Outdated Show resolved Hide resolved

clux modified the milestones: 0.83.0, 0.84.0 Jun 4, 2023

jmintb force-pushed the issue127 branch 2 times, most recently from 5d606df to 1c2b97e Compare June 25, 2023 18:54

clux modified the milestones: 0.84.0, 0.85.0 Jul 13, 2023

jmintb force-pushed the issue127 branch 5 times, most recently from 4e04242 to 6125f4a Compare July 22, 2023 11:03

clux reviewed Jul 22, 2023

View reviewed changes

Implement Ephemeral Containers subresource

18cec4c

This commit implements replace, patch and get operations for the ephemeral containers subresource. Signed-off-by: Jessie Chatham Spencer <jessie@teainspace.com>

jmintb force-pushed the issue127 branch from 6125f4a to 18cec4c Compare July 22, 2023 13:32

jmintb mentioned this pull request Jul 22, 2023

Document potential silent failure when patching resources #1262

Open

jmintb changed the title ~~Draft: Implement ephemeral containers subresource~~ Implement ephemeral containers subresource Jul 22, 2023

clux approved these changes Jul 22, 2023

View reviewed changes

clux merged commit ef60e7c into kube-rs:main Jul 22, 2023
15 of 16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ephemeral containers subresource #1153

Implement ephemeral containers subresource #1153

jmintb commented Feb 26, 2023

clux commented Feb 27, 2023

clux left a comment •

edited

Loading

clux Feb 27, 2023

jmintb Feb 28, 2023

clux Mar 3, 2023

jmintb Mar 9, 2023

jmintb Mar 9, 2023

clux Mar 10, 2023 •

edited

Loading

jmintb Mar 18, 2023

This comment was marked as resolved.

jmintb commented Apr 7, 2023

clux commented Apr 7, 2023

clux commented Apr 7, 2023

jmintb commented Jun 25, 2023

codecov bot commented Jun 25, 2023 •

edited

Loading

clux Jul 22, 2023 •

edited

Loading

jmintb Jul 22, 2023

clux left a comment

		/// #[tokio::main]
		/// async fn main() -> Result<(), Box<dyn std::error::Error>> {

Implement ephemeral containers subresource #1153

Implement ephemeral containers subresource #1153

Conversation

jmintb commented Feb 26, 2023

Motivation

Solution

Questions

clux commented Feb 27, 2023

clux left a comment • edited Loading

Choose a reason for hiding this comment

clux Feb 27, 2023

Choose a reason for hiding this comment

jmintb Feb 28, 2023

Choose a reason for hiding this comment

clux Mar 3, 2023

Choose a reason for hiding this comment

jmintb Mar 9, 2023

Choose a reason for hiding this comment

jmintb Mar 9, 2023

Choose a reason for hiding this comment

clux Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

jmintb Mar 18, 2023

Choose a reason for hiding this comment

This comment was marked as resolved.

jmintb commented Apr 7, 2023

clux commented Apr 7, 2023

clux commented Apr 7, 2023

jmintb commented Jun 25, 2023

codecov bot commented Jun 25, 2023 • edited Loading

Codecov Report

clux Jul 22, 2023 • edited Loading

Choose a reason for hiding this comment

jmintb Jul 22, 2023

Choose a reason for hiding this comment

clux left a comment

Choose a reason for hiding this comment

clux left a comment •

edited

Loading

clux Mar 10, 2023 •

edited

Loading

codecov bot commented Jun 25, 2023 •

edited

Loading

clux Jul 22, 2023 •

edited

Loading