kubeclient icon indicating copy to clipboard operation
kubeclient copied to clipboard

Add ability to patch status of an object

Open a-chernykh opened this issue 6 years ago • 8 comments

a-chernykh avatar Oct 18 '19 21:10 a-chernykh

Nice. 2 small concerns:

  1. Do we also need JSON merge patch and JSON patch variants? Might be worth it for orthogonality alone; also, CRDs don't support strategic merge.

  2. Are there any kind pairs "Foo" and "FooStatus"? Then method names would collide. I agree it's unlikely. Could you grep https://github.com/cben/kubernetes-discovery-samples (I'm away from computer today)?

cben avatar Oct 20 '19 12:10 cben

BTW, are these really adding functionality vs doing patch on the whole object only touching status part? I think (untested) that works too.

I suspect the API has foo/status endpoints mostly for ease of whole-sale replacement (PUT). Or maybe for separate RBAC? (E.g. an operator having read access to resources themselves and write to their status). In that use case, I agree this adds functionality.

cben avatar Oct 20 '19 12:10 cben

An alternative could be to not provide these [for now] as generated method names, first as generic method taking the kind of object as parameter.

That is, implement at least patch part of generic methods #332, plus status variants. (Or maybe same method abusing kind: Foo/status but probably not, I prefer #332 layer to match k8s semantics closely)

Don't take this as hard pushback, I'm looking for your opinion, but I also know we want a #332 layer anyway so I'm interested in nudging people in that direction 😉

BTW, kubeclient is generally lacking about foo/bar sub-resources. We have a few specialized methods here and there... Do you think there is use case for more methods like update_foo_status, get_foo_status, watch_foo_status...?

If you have any broader vision how to support sub-resources, please write 👍

cben avatar Oct 20 '19 12:10 cben

Hi, @cben, I appreciate fast turnaround reviewing this PR and thank you for maintaining such a critical open-source project.

To be honest, this was a quick and dirty change I needed to make in order to benefit from custom Pod conditions (trying to fill in some gaps in Kubernetes inability to run slow jobs without interruptions during auto-scaling and deployments).

Unfortunately, I was not able to use the regular PATCH /pods/<pod_name> endpoint in order to update custom status, therefore, I had to implement this _patch method which uses PATCH /pods/<pod_name/status. This endpoint has a separate RBAC as well. I tested this change in our AWS EKS cluster and it works as expected.

a-chernykh avatar Oct 22 '19 04:10 a-chernykh

Oh i now remembered there is a starightforward way to add full set of operations on these foo/status sub-resources: I believe we're already getting them listed in discovery, and we're deliberately ignoring ones that contain a slash. We could stop ignoring those and decide how to map them to method names, e.g. replace slash with underscores.

cben avatar Oct 22 '19 17:10 cben

Sorry, I effectively blocked this by saying "there is a better way" but never implementing it :-| I won't have time to work on this until 5.0 release (#435). Why don't I just merge your PR for now and extend later? I probably could. But I wanted to verify the more general approach would result in interface as this "special case" approach...

Are in interested in tackling the general approach yourself?

  • modify next if resource['name'].include?('/') line in load_entities to allow things ending in /status.
  • modify parse_definition to translate that to _status suffix
  • tests...

Alternatively, please ping me periodically :wink:

cben avatar Mar 11 '20 13:03 cben

This is required to patch status right? Trying to update a single status field by re-POSTING the entire resource doesn't seem to actually change the field in the status.

shuhaowu avatar Jun 22 '20 14:06 shuhaowu

The PUT and POST verbs on objects MUST ignore the "status" values, to avoid accidentally overwriting the status in read-modify-write scenarios. A /status subresource MUST be provided to enable system components to update statuses of resources they manage.

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#spec-and-status

cben avatar Jun 22 '20 15:06 cben