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

Draft proposal for watch support on metric APIs #1013

Merged
merged 1 commit into from May 2, 2019

Conversation

x13n
Copy link
Member

@x13n x13n commented Apr 29, 2019

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

@k8s-ci-robot k8s-ci-robot added sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. kind/design Categorizes issue or PR as related to design. sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 29, 2019
@k8s-ci-robot k8s-ci-robot requested review from brancz and piosz Apr 29, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory labels Apr 29, 2019
@markusthoemmes
Copy link

markusthoemmes commented Apr 29, 2019

Thank you for kickstarting this. How'd you expect collaboration to happen? PRs to your fork?

@x13n
Copy link
Member Author

x13n commented Apr 29, 2019

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.

Copy link
Contributor

@logicalhan logicalhan left a comment

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

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 2, 2019
@brancz
Copy link
Member

brancz commented May 2, 2019

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
/approve

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 2, 2019

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 2, 2019
@k8s-ci-robot k8s-ci-robot merged commit e30f724 into kubernetes:master May 2, 2019
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
Copy link
Member

@wojtek-t wojtek-t May 6, 2019

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:

  1. 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).
  2. 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.

Copy link
Member

@brancz brancz May 6, 2019

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 :)

Copy link
Member Author

@x13n x13n May 6, 2019

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.

Copy link
Member

@lavalamp lavalamp May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liggitt, @deads2k, or myself should probably make a pass over this.

Copy link
Member

@wojtek-t wojtek-t May 7, 2019

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).

  1. 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.

Copy link
Member Author

@x13n x13n May 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lavalamp, @liggitt, @deads2k - Whom should I add as an approver from apimachinery side?

I'll prepare a follow-up PR to define details of the API.

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/feature Categorizes issue or PR as related to a new feature. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels May 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/design Categorizes issue or PR as related to design. kind/feature Categorizes issue or PR as related to a new feature. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm Indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants