-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Draft proposal for watch support on metric APIs #1013
Conversation
Thank you for kickstarting this. How'd you expect collaboration to happen? PRs to your fork? |
Good question, I didn't collaborate on a KEP before. Maybe just PRs modifying the KEP? At least once this one gets merged. If merging gets delayed however, then yes, I guess PRs to my fork make sense. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should merge this early in the provisional state so that the KEP can be collaborated/iterated on through the PR process (as per sig-arch guidance earlier today).
/lgtm
Merging as status is provisional, this definitely still needs lots of investigation before we can move ahead with implementation, but in general we had consensus this is a generally a good idea. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brancz, logicalhan, x13n The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
appears. Metrics don't contain any resourceVersion associated with them, so it | ||
won't be possible to retrieve old values by passing `resourceVersion` parameter. | ||
Instead, this parameter will be ignored and all recent data points will be | ||
returned instead. This means metrics APIs will never return `410 Gone` error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait - i think that either the description here is unclear (at least to me) or I see a significant issues with what was described here:
- Maybe I'm missing something, but what do you mean by "old metric values are never modified"?
While conceptually this is true, the API objects are around Pod, Node, etc.
So actually, the object (unless I'm signiifcantly missing something) actually changes.
I actually believe that sending ADDED event on every event is wrong - it's simply not what it should be (and it would also break all the libraries build, like reflector/informer framework). - Conceptually, what you really implement here is the existing "resourceVersion=0" semantic of watch:
Change Watch behavior for resourceVersion=0 in v2 API kubernetes#13969
If that's really the case, we should simply make it explicit, and support only watch with RV=0
The high level comment is that we shouldn't reuse the existing options/fields and give them different semantics than in all other places - it would be extremely misleading for users.
@kubernetes/sig-api-machinery-feature-requests @kubernetes/sig-api-machinery-api-reviews @jpbetz
I think this is something that requires much closer collaboration wich apimachinery team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only went ahead with merging this, as it’s provisional, not accepted, this is a work in progress state, but generally watch is desirable. I agree this needs very very close collaboration with apimachinery and autoscaling, I’ve mentioned this to the authors repeatedly :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Yes, I agree apimachinery input will be very valuable here.
Re 1 - The objects in *.metrics.k8s.io
, while reference "regular" k8s objects like Pods, are entities on their own. One difference is that they don't use the resourceVersion. Because of that, MODIFIED is not feasible unless we emulate resourceVersion with e.g. unix timestamps. However, with this approach, the backend would have to detect when metrics become missing. I guess my silent assumption here was that metric clients don't care when existing data becomes old, while they do care when new value appears, so old values detection is not really required. If my proposal would break the existing clients then of course I agree it should be changed.
Re 2 - In the linked issue you're suggesting removal of this "hidden feature". Is it likely to happen in foreseeable future? I don't see a lot of value in introducing something knowing it will become deprecated soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that they don't use RV doesn't change anything - if you don't support it, you can validate the request and just support RV=0 (or sth like that).
But if the metrics for a given Pod or Node change, what you get is the same object (it has the same name, namespace, etc., right?), not a new object (so you can't send ADD, without sending DELETE for the previous one first).
- I don't know - I would like to see it removed, but I'm afraid we won't have manpower to do it anytime soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This KEP proposes adding watch capability to metric APIs. Details are left out for now - this is an initial draft to start discussions.
/sig instrumentation
/sig autoscaling
@kubernetes/sig-instrumentation-proposals
@brancz @markusthoemmes @josephburnett @mwielgus