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

Add ability to patch status of an object #422

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

a-chernykh
Copy link

No description provided.

@cben
Copy link
Collaborator

cben commented Oct 20, 2019

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
Copy link
Collaborator

cben commented Oct 20, 2019

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
Copy link
Collaborator

cben commented Oct 20, 2019

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 👍

@a-chernykh
Copy link
Author

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.

@cben
Copy link
Collaborator

cben commented Oct 22, 2019

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 cben mentioned this pull request Dec 16, 2019
@cben
Copy link
Collaborator

cben commented Mar 11, 2020

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 😉

@shuhaowu
Copy link

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.

@cben
Copy link
Collaborator

cben commented Jun 22, 2020

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 cben mentioned this pull request Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants