KEP 2211: Omit managedFields server-side #2212
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: knight42 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
sig/api-machinery
| FIXME: we need to set the `Accept` header on the client-side and perform client-side stripping if the API server still | ||
| returns objects with managedFields. I am not sure we should do this in `client-go` and `cli-runtime` or only in `kubectl get`. | ||
|
|
||
| /cc @apelisse |
There was a problem hiding this comment.
what about controller-runtime, or is that taken care of by the client-go ?
There was a problem hiding this comment.
My proposed change is providing a helper function in client-go which is able to set the Accept header in rest.Config, so that users could tell the API server they want to omit the managed fields.
Then you could use the modified rest.Config everywhere that needs it and the managed fields should be dropped if the API server recognize the new Accept header.
There was a problem hiding this comment.
I'm still really not convinced we need a mechanism to set a single Accept header.
There was a problem hiding this comment.
I wouldn't say need but the header we propose for fallback etc. might not be too known or familiar as an option here that I wouldn't mind having a simpler option that does it for you.
Like PreferNoManagedFields() = header with q= fallback and RequestNoManagedFields() = no fallback.
There was a problem hiding this comment.
@kwiesmueller yes that's what I mean by saying "providing a helper function", but it seems there is more we could do on client-side if we need to consider the case that API server cannot recognize this new header and we still want to omit the managed fields.
| } | ||
| ``` | ||
|
|
||
| ### Client-side |
There was a problem hiding this comment.
@julianvmodesto We'll try to get this KEP merged as provisional quickly, so that you can fill in that section if you're ok with it :-)
There was a problem hiding this comment.
The plan is to implement this using Accept: application/yaml+unmanaged,application/yaml;q=0.9, or simply: Accept: application/yaml+unmanaged,application/yaml
There was a problem hiding this comment.
what does unmanaged mean here ?
Omit the managed fields.
|
PoC of the server-side changes: kubernetes/kubernetes@master...knight42:feat/strip-managed-fields-server-side |
|
|
||
| ManagedFields is used to keep track of who has changed a field of an object, and Server-side Apply relies on such information. | ||
| While the managedFields is beneficial to resolving conflicts on updates, it is a bit verbose to many users because this field | ||
| could be quite large and generally users are not interested in this information. |
There was a problem hiding this comment.
users and controllers, also impact bandwidth usage, size of internal caches
| While the managedFields is beneficial to resolving conflicts on updates, it is a bit verbose to many users because this field | ||
| could be quite large and generally users are not interested in this information. | ||
|
|
||
| This KEP is proposing stripping the managedFields on the server-side when users request objects to make the response less verbose. |
There was a problem hiding this comment.
Not only when user "requests objects", since pretty much all operations return the final object.
There was a problem hiding this comment.
Hopefully we are talking about omitting them from the response and not stripping them from the stored object.
There was a problem hiding this comment.
Why not omitting it from stored object as well ? If a cluster operator has made a conscious decision, it would save a lot of storage for him.
There was a problem hiding this comment.
Hopefully we are talking about omitting them from the response and not stripping them from the stored object.
Yes, the original object saved in etcd is left intact.
Why not omitting it from stored object as well
The Server-side Apply feature needs the managedFields.
There was a problem hiding this comment.
If a cluster operator has made a conscious decision, it would save a lot of storage for him.
It's unclear if the trade-offs they're making would be all that conscious.
I'm happy to discuss if there is a specific use-case that you have in mind, but the "Accept" header is not where we would want to strip managedFields from storage.
There was a problem hiding this comment.
Why not omitting it from stored object as well ?
- it loses valuable context about the object that other aspects of the system need
- this would not be the right mechanism for that at all, we'd need to make a setting otherwise it would come back
- this is primarily about read operations, although it will also work on write operations
There was a problem hiding this comment.
+1 to Daniel. Managed field is crucial to automatic config mgmt and a depency for system needs. This proposal should be a minimal UI fix IMO.
A off-topic idea is we may want to bring up some real-world cases how this tech could be properly used by the platform team in server-side apply doc. The main use case on our side is k-o-k configuration mgmt scenario, but it should be more.
There was a problem hiding this comment.
Sounds good thanks. I agree a different setting is needed. I think until this becomes widespread in usage, the default should be ignored in returns and not stored as well. This is ahead of its time , but definitely a really nice concept.
| [90066]: https://github.com/kubernetes/kubernetes/issues/90066 | ||
|
|
||
| At the same time, it is not ideal to only drop the managedFields on the client-side, because that would require every client | ||
| (such as kubectl or CLI tools written by users using client-go) to do the same thing. |
There was a problem hiding this comment.
That's a good point, along with extra bandwidth
| * Add three content types to denote the client expects the API server to strip the managedFields. | ||
| * `application/json+unmanaged` | ||
| * `application/yaml+unmanaged` | ||
| * `application/vnd.kubernetes.protobuf+unmanaged` |
There was a problem hiding this comment.
Even if we don't introduce it just yet, we might have other versions for the encoding of managed fields in the future. Currently, we only have v1. Can we describe how the header would look like if I wanted to request a specific format version of managed fields in the future?
There was a problem hiding this comment.
"unmanaged" isn't the right word. "omitting.managed.fields", "sans-managed" perhaps.
There was a problem hiding this comment.
I'd prefer something that was a positive specification rather than negative. So, like we could add "managed.v1" (the default) and "managed.omit" (the new option), which leaves room for v2 etc.
There was a problem hiding this comment.
This needs to compose with the tabular output content type.
Also I think clients would specify these in the "Accept" header. Ah, it's mentioned below. But better to be specific.
There was a problem hiding this comment.
Can we describe how the header would look like if I wanted to request a specific format version of managed fields in the future
I guess this is out of the scope of this KEP, but we could still leave room for it.
@lavalamp IIUC are you suggesting changing the Content-Type to application/{json,yaml,profobuf}.managed.omit?
There was a problem hiding this comment.
let's also look at this really quick: https://trac.tools.ietf.org/html/draft-ietf-appsawg-media-type-regs-14#section-6
|
I'm happy to merge this as provisional for now. |
k8s-ci-robot
added
the
lgtm
keps/sig-api-machinery/2211-strip-managed-fields-server-side/README.md
Outdated
Show resolved
Hide resolved
| * `application/json+unmanaged` | ||
| * `application/yaml+unmanaged` | ||
| * `application/vnd.kubernetes.protobuf+unmanaged` | ||
| * Add a new serializer, say `UnmanagedSerializer`, which is able to strip the managedFields when encoding objects. |
There was a problem hiding this comment.
The serializer would ideally just take the version (either v1 or omit) rather than needing a different one for each version.
There was a problem hiding this comment.
Technically, it will be wrapper around existing serializers I think.
|
|
||
| ContentTypeUnmanagedJSON string = "application/json+unmanaged" | ||
| ContentTypeUnmanagedYAML string = "application/yaml+unmanaged" | ||
| ContentTypeUnmanagedProtobuf string = "application/vnd.kubernetes.protobuf+unmanaged" |
There was a problem hiding this comment.
I don't think we should encourage a combinatorial explosion. We should represent this with a {json/yaml/proto, managed-fields:v1/omit} pair.
There was a problem hiding this comment.
@lavalamp Could you elaborate more on the representation please?
There was a problem hiding this comment.
+1 to Daniel (modulo the fact that for backward compatibility we need to default to v1 if unset)
This BTW will also reflect the implementation.
| * Add three content types to denote the client expects the API server to strip the managedFields. | ||
| * `application/json+unmanaged` | ||
| * `application/yaml+unmanaged` | ||
| * `application/vnd.kubernetes.protobuf+unmanaged` |
| * `application/json+unmanaged` | ||
| * `application/yaml+unmanaged` | ||
| * `application/vnd.kubernetes.protobuf+unmanaged` | ||
| * Add a new serializer, say `UnmanagedSerializer`, which is able to strip the managedFields when encoding objects. |
There was a problem hiding this comment.
Technically, it will be wrapper around existing serializers I think.
| FIXME: we need to set the `Accept` header on the client-side and perform client-side stripping if the API server still | ||
| returns objects with managedFields. I am not sure we should do this in `client-go` and `cli-runtime` or only in `kubectl get`. | ||
|
|
||
| /cc @apelisse |
|
|
||
| ContentTypeUnmanagedJSON string = "application/json+unmanaged" | ||
| ContentTypeUnmanagedYAML string = "application/yaml+unmanaged" | ||
| ContentTypeUnmanagedProtobuf string = "application/vnd.kubernetes.protobuf+unmanaged" |
There was a problem hiding this comment.
+1 to Daniel (modulo the fact that for backward compatibility we need to default to v1 if unset)
This BTW will also reflect the implementation.
| owning-sig: sig-api-machinery | ||
| participating-sigs: [sig-cli] | ||
| reviewers: ['@apelisse', '@kwiesmueller'] | ||
| approvers: ['@wojtek-t'] |
There was a problem hiding this comment.
you need sig-apimachinery approver too (it seems @lavalamp is already looking into it)
| alpha: v1.21 | ||
| beta: v1.22 | ||
| stable: v1.23 | ||
| # TODO: do we need a feature gate? |
knight42
force-pushed
the
feat/strip-managed-fields-server-side
branch
from
373f5a3
to
9137444
Compare
|
New changes are detected. LGTM label has been removed. |
k8s-ci-robot
removed
the
lgtm
knight42
force-pushed
the
feat/strip-managed-fields-server-side
branch
from
9137444
to
cd4c958
Compare
|
|
||
| ## Design Details | ||
|
|
||
| ### Server-side |
There was a problem hiding this comment.
it's not clear to me why the server-side piece is required... that seems significantly more complex and doesn't remove the need for client-side logic to pass alternate content-type headers. Is the bandwidth savings significant? Why would we not just drop the field in the client?
There was a problem hiding this comment.
Correct, it's ~30% of the object size if you're in proto mode.
There was a problem hiding this comment.
Now that you mention it though, why don't we just implement in kubectl and see if controller operators complain? So far all the complaints have just been from users that don't want to see it AFAIK.
There was a problem hiding this comment.
why don't we just implement in kubectl and see if controller operators complain?
I would definitely start there (or somewhere client-side... client-go or cli-runtime or kubectl). If there's good evidence we need to push it further up the stack, doing it with a ;omit=metadata.managedFields modifier or something in a later iteration could be reasonable
There was a problem hiding this comment.
We already have complaints about the network impact:
kubernetes/kubernetes#90066 (comment)
Hi, @apelisse, Due to addition managedFields metadata, payload of the every object got increased by 40%. For example, for simple nginx pod, before <1.18, response was ~6KB and >=1.18, response is 10KB. Ours is monitoring application, we use k8s apis to get the response of all objects (chunked fashion) and we dont use this field. The additional payload significantly costing the cpu and memory. is there any way to express the k8s API to exclude in the response ? If not, having functionality like this will significantly helpful for the clients.
kubernetes/kubernetes#90066 (comment)
As I've mentioned before and as @ganga1980 mentioned as well, the main managed fields problem is not a cosmetic one of hiding them in kubectl, it's a performance one, they increase the CPU, memory and bandwidth utilisation, solving this at the client side is not enough.
it's unclear from these comments what CPU/memory is being affected here.
There was a problem hiding this comment.
+1 on doing on server side, we have lot of querying and storing the k8s resources, and a 40% increase with no specific near time use will help with bandwidth and processing power of our clients which are making this query continuously. What version was this managedFields introduced in responses from the server side, we might have to plan for this ?
There was a problem hiding this comment.
If you're worried about storage, just clear client-side. You only need to clear serverside if there's a problem with network bandwidth.
There was a problem hiding this comment.
client-side already has some changes, see kubernetes/kubernetes#91946 which dropped those from edit, but I keep on hearing this over and over that folks want to see those stripped also when doing get.
|
|
||
| ### Server-side changes | ||
|
|
||
| * Add three content types: |
There was a problem hiding this comment.
Ugh, we already have content type modifiers defined for metadata lists. Why are we not using that mechanism?
There was a problem hiding this comment.
This is what I was trying to ask for 😅
There was a problem hiding this comment.
I didn't know about these 🤷♂️
There was a problem hiding this comment.
@smarterclayton - you mean thins like "as=Table", "as=PartialObjectMetadata" etc?
Shouldn't those be generic types?
[If you meant those - these doesn't benefit from this optimization yet: https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1152-less-object-serializations
- and i think that we don't want to drop this.]
[added later]
It seems that "as" is one potential modifier, another one is e.g. "pretty", which is supported by the optimization above. So I guess we can implement the new modifier to take advantage of the optimization above.
| 1. Add a content type suffix to `k8s.io/apimachinery/pkg/runtime/types.go`, which can be used to construct the new content type | ||
| along with the existing ones: | ||
| ```go | ||
| const ContentTypeSuffixOmitManagedFields = "+managed.omit" |
There was a problem hiding this comment.
I would prefer to leverage the existing negotiation modifier mechanism, which already handles partial objects. How it's modelled is open to debate, but this feels way too specific to managed fields and has no future proofing.
There was a problem hiding this comment.
handled at https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/negotiation/negotiate.go#L172-L179, used for things like application/json;as=Table;v=v1;g=meta.k8s.io
|
|
||
| ### Server-side changes | ||
|
|
||
| * Add three content types: |
There was a problem hiding this comment.
@smarterclayton - you mean thins like "as=Table", "as=PartialObjectMetadata" etc?
Shouldn't those be generic types?
[If you meant those - these doesn't benefit from this optimization yet: https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1152-less-object-serializations
- and i think that we don't want to drop this.]
[added later]
It seems that "as" is one potential modifier, another one is e.g. "pretty", which is supported by the optimization above. So I guess we can implement the new modifier to take advantage of the optimization above.
| * Add three content types: | ||
| * `application/json+managed.omit` | ||
| * `application/yaml+managed.omit` | ||
| * `application/vnd.kubernetes.protobuf+managed.omit` |
|
|
||
| #### Alternative 1: New dedicated flag, for example `--show-managed-fields` | ||
|
|
||
| The value of flag `--show-managed-fields` could be `omit`, `default` and a version name(in case users want to |
There was a problem hiding this comment.
Where is this flag? kubectl only?
| participating-sigs: [sig-cli] | ||
| reviewers: ['@apelisse', '@kwiesmueller'] | ||
| approvers: ['@wojtek-t', '@lavalamp'] | ||
| prr-approvers: ['@wojtek-t'] |
There was a problem hiding this comment.
If you want to target alpha in 1.21, you need to fill PRR and get approval before feature freeze.
|
|
||
| ### Client-side changes | ||
|
|
||
| Implement a flag in `kubectl` to let users choose to omit the managedFields. |
There was a problem hiding this comment.
There are parallel efforts for keeping kubectl preferences, compare #2111
| 1. We could add the original content type to the `Accept` header as a fallback, such as | ||
| `Accept: application/json+managed.omit, application/json;q=0.9`, therefore the API server could return JSON-formatted | ||
| objects rather than reject the request with a 406(Not Acceptable) error if it is too old to recognize the new content type. | ||
| 2. Then make `kubectl get` to drop managedFields if user wants to omit managedFields, but the API server doesn't realize that. |
There was a problem hiding this comment.
If the functionality doesn't exist on the server then we do nothing. I don't want to repeat the mechanism the sever already have. It'll just be a no-op in those cases.
|
|
||
| ## Design Details | ||
|
|
||
| ### Server-side |
There was a problem hiding this comment.
client-side already has some changes, see kubernetes/kubernetes#91946 which dropped those from edit, but I keep on hearing this over and over that folks want to see those stripped also when doing get.
|
|
||
| #### Alternative 1: New dedicated flag, for example `--show-managed-fields` | ||
|
|
||
| The value of flag `--show-managed-fields` could be `omit`, `default` and a version name(in case users want to |
There was a problem hiding this comment.
👍 for sig-cli pov, it allows much more flexibility for the future.
|
|
||
| #### Alternative 1: New dedicated flag, for example `--show-managed-fields` | ||
|
|
||
| The value of flag `--show-managed-fields` could be `omit`, `default` and a version name(in case users want to |
There was a problem hiding this comment.
I'm even inclined to have it default to omit.
| Cons: | ||
| * The new flag depends on the `-o` flag, it only works with `-o (json|yaml)`. In other cases, it is a no-op. | ||
|
|
||
| #### Alternative 2: New output format, for example `-o yaml+managed.omit` |
There was a problem hiding this comment.
I'm not a fan of this approach, I'd prefer seeing the other flag with omit being the default.
|
For some reason github won't let me reply directly to some of @soltysh's comments above, so: I have a new thought, which is to combine this feature with a filtering feature, so basically there'd be one flag. The paradigm is that this flag consumes the managed fields, and uses them to filter the object:
Could be a weird idea, could be useful, what do you think? |
|
@soltysh my problem with the omit flag is, that it doesn't seem consistent. Aside from that technicality, the omit flag doesn't seem like fully deterministic UX to me. The -o flag would directly translate to the Accept header (imo) so if it can not be provided, it would act just as if you request a format that does not exist. If the flag would be I know this is discussing on a very granular level and making the topic probably more complicated than necessary. There was a vote on Slack and the flag won so I'll go with it. But as you just commented here, I want to raise my concerns while I still can. |
I'd love to have default be to filter out managed fields. Rarely folks invoking kubectl care about those managed fields.
Is it possible that a field is not managed by anyone?
@lavalamp I assume this flag would directly translate to params being passed to the server or are you referring to client-side filtering? I still would be a big proponent of having this mechanism on the server. The proposal itself seems reasonable although I'm worried about too much complexity when specifying those, but that's still on the table. |
|
@kwiesmueller although |
Do we want to have this mechanism available to control the output of every command, including |
@soltysh I think it'd be best to implement on the client. I don't really see much benefit in having the server do this for you (it's fairly trivial with the structured-merge-diff library). Maybe if it begins to be a blocker to api clients in other languages. |
I was only thinking about this for get commands personally. For get commands, my first thought is that it's not good to make users pipe the output through a separate |
|
Yeah, we have a get in-flight client-side, see kubernetes/kubernetes#96878 |
knight42
force-pushed
the
feat/strip-managed-fields-server-side
branch
from
bb249fb
to
061d0fd
Compare
|
As for the server-side changes, it looks like we all agree it is better to leverage the existing negotiation modifier mechanism, so I have updated the design detail in kep, and here is a PoC: kubernetes/kubernetes@master...knight42:feat/server-omit-managed-fields. Reviewers please take another look. |
|
@knight42 - are we going to proceed with this? Or can we close it? |
|
@wojtek-t I would love to proceed, but no one has commented on the updated KEP 😢 , so I am not sure whether it needs further improvement. |
|
I think we don't have enough evidence that we need this at that time. We can close it and see if people have stronger use-cases that require this, or we can let open. 🤷 |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
With SSA in GA now, I still think there can be use-cases where omitting the managedFields server-side could be beneficial. Sadly we didn't get enough energy here to get to a positive conclusion. |
|
@kwiesmueller: The In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@kwiesmueller: The In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Nevermind, PRs can't be frozen. |
|
@kwiesmueller: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Hi k8s experts, I just want to add another data point related to the size issue of managedFields. Basically for operators, e.g. KCC watch a great amount of objects, say over 20K objects, even with metadata-only watch enabled, we have still see a good portion of memory footprint reduction (over 30%) if watches can trim Looking at the previous discussion about solving and mitigating the issue on the client side. The ask to reduce memory footprint for watches can potentially belong to controller-runtime library. Can anyone comment on the current thinking about this kep? If it's more appropriate to purpose the light watch in controller-runtime layer? |
This KEP proposes to omit the managedFields of objects on the server-side if users send a request with a special content type modifier.
Motivation: kubernetes/kubernetes#90066
Rendered