Support metric label selector for custom metrics #35

astefanutti · 2018-10-15T14:56:47Z

This PR requires the following to be merged for it to compile and before being merged:

DirectXMan12

generally looks good. Thanks so much for putting this together -- I had been meaning to rebase my changes and push them, but these are in a much better state than my own.

One thing to note: we may have to end up copying a bunch of API machinery over -- I'm expecting that ListerWithOptions may be delayed a while or rejected (somewhat reasonably), since it doesn't have a use beyond the CM adapter, and we've been encouraged in the past to just fork API machinery for the CM adapter.

DirectXMan12 · 2018-11-12T21:39:59Z

pkg/apiserver/installer/cmhandlers.go

@@ -64,6 +64,20 @@ func (ch *CMHandlers) registerResourceHandlers(a *MetricsAPIInstaller, ws *restf
 		return err
 	}

+	listOptions, _, _ := lister.NewListOptions()


please add a comment clarifying why we have both list options and extra list options

As the standard ListOptions is defined as a struct and not an interface, I don't know of a way to use a single object that could be extended with polymorphism. So there is a standard list options as defined by the ListOptions struct, plus the extra options defined by the CM adapter.

I'll add a comment in the code when the question where the ListerWithOptions should go is resolved.

My initial suggestion would be to say that custom list options should ,inline embed the base ListOptions, but I think your idea is actually fine as well :-).

astefanutti · 2018-11-13T09:21:48Z

@DirectXMan12 Thanks for the review. Let me know how we can move the question of the ListerWithOptions forward. If you think the best option is to move the changes from kubernetes/kubernetes#69806 into the CM adapter codebase, I'll take care of updating this PR.

astefanutti · 2018-11-13T09:22:39Z

@DirectXMan12 this PR requires #34 so it may have to be re-open.

DirectXMan12 · 2018-11-13T20:11:32Z

Yep, didn't mean to close it, sorry about that

DirectXMan12 · 2018-11-13T20:13:51Z

Thanks for the review. Let me know how we can move the question of the ListerWithOptions forward

I suspect moving the changes into the CM adapter codebase might be the best option, if it's not too hairy. Alternatively, show up at one of the SIG APIMachinery meetings in person and ask, perhaps? That's generally a quick way to get a resolution. If you have concrete evidence of the maintenance burden/duplicate code, it'd be good evidence for why we should have in in core API machinery.

astefanutti · 2018-11-14T09:45:28Z

Sounds good. I'll try to show up at the APIMachinery SIG ASAP. In the meantime, I'll upgrade this PR by integrating the required bits and we'll have material to iterate on.

astefanutti · 2018-11-15T10:50:43Z

I've updated the PR with 4bf4950 that contains the bits required from the API server, so that removes the pre-requisite on kubernetes/kubernetes#69806.

astefanutti · 2018-11-27T08:28:47Z

Rebased on master branch as #34 has been merged.

DirectXMan12 · 2019-01-04T01:24:10Z

@brancz can you or at @metalmatze or someone PTAL

metalmatze · 2019-01-04T10:08:31Z

Looking at the code this lgtm. I'm missing a higher level description of what actually is going to change with this PR to be honest.

brancz · 2019-01-07T10:57:13Z

While I see the use for this, I'm wondering if it's actually something we should pursue. It maps somewhat ok to the Prometheus data model, but in Prometheus different labels means a different time-series, so should a list return multiple results with the same metrics name but different labels? My feeling is that the changes we have been thinking about regarding configuration to define what a custom metric is to be converted into in Prometheus might be more pragmatic (in regards to the prometheus-k8s-adapter).

DirectXMan12 · 2019-01-11T02:12:05Z

drive-by comment: the adapter probably should try and support most/all of the features in the metrics APIs

fejta-bot · 2019-04-28T07:10:57Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2019-05-28T07:53:36Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

kawych · 2019-06-17T11:01:28Z

/cc @kawych
/uncc @piosz
/remove-lifecycle rotten

kawych · 2019-06-17T11:47:13Z

@astefanutti, can we proceed with this PR? I can approve if we get confirmation from @brancz that it's OK to pursue.

@brancz @DirectXMan12

drive-by comment: the adapter probably should try and support most/all of the features in the metrics APIs

100% agreed. It's really great that this adapter is refactored so that it can be used as the library, but especially now it will be super useful to support all the features, instead of all adapters implementing them for themselves.

I see a discussion on kubernetes/kubernetes#69806 about supporting custom options - it would be ideal if apiserver package could support our use case. I think we should return to the discussion to understand what are the issues with that - if these are serious concerns, maybe this proposal should be reconsidered/updated: https://github.com/kubernetes/enhancements/blob/master/keps/sig-autoscaling/0032-enhance-hpa-metrics-specificity.md#alternatives

brancz · 2019-06-17T11:56:13Z

cc @s-urbaniak (he has been working more closely on metrics API things lately than me so I’d like to hear his opinion).

astefanutti · 2019-06-23T10:05:52Z

@kawych happy to update and proceed with the PR, if we can get an agreement on how to deal with custom list options.

kawych

I've took a closer look at the code and it looks mostly fine - in the comments I've requested a clarification of parts of the code, but the general approach LGTM and the code looks OK. I'm going to give @s-urbaniak at least one more day to voice his doubts and then approve.

kawych · 2019-06-25T10:02:07Z

pkg/apiserver/installer/installer.go

@@ -286,3 +286,9 @@ func restfulListResource(r rest.Lister, rw rest.Watcher, scope handlers.RequestS
 		handlers.ListResource(r, rw, scope, forceWatch, minRequestTimeout)(res.ResponseWriter, req.Request)


Is this function still used?

Yes, for external metrics.

kawych · 2019-06-25T11:17:27Z

pkg/apiserver/endpoints/handlers/get.go

+	cm_rest "github.com/kubernetes-incubator/custom-metrics-apiserver/pkg/apiserver/registry/rest"
+)
+
+func ListResourceWithOptions(r cm_rest.ListerWithOptions, rw rest.Watcher, scope handlers.RequestScope, forceWatch bool, minRequestTimeout time.Duration) http.HandlerFunc {


AFAIU the watch is not supported and forceWatch is unused, please remove

kawych · 2019-06-25T11:32:22Z

pkg/apiserver/registry/rest/rest.go

+	List(ctx context.Context, options *metainternalversion.ListOptions, extraOptions runtime.Object) (runtime.Object, error)
+
+	// NewListOptions returns an empty options object that will be used to pass extra options
+	// to the List method. It may return a bool and a string, if true, the


Can you please explain better the third returned value? AFAIU the second one indicates whether the third one is present?

Your understanding is correct. It is taken verbatim from NewGetterOptions.

kawych · 2019-06-25T11:33:40Z

pkg/apiserver/endpoints/handlers/get.go

+		// Log only long List requests (ignore Watch).
+		defer trace.LogIfLong(500 * time.Millisecond)
+		trace.Step("About to List from storage")
+		extraOpts, subpath, subpathKey := r.NewListOptions()


nit: subpath is not the best name for boolean value, maybe hasSubpath?

astefanutti · 2019-06-25T19:23:32Z

@kawych thanks for the review. I’ll be AFK until end of this week. I’ll update the PR next week with your comments.

astefanutti · 2019-07-01T20:28:25Z

I've just rebased the PR and updated it with the review.

kawych · 2019-07-02T12:46:16Z

/lgtm
/approve

k8s-ci-robot · 2019-07-02T12:46:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kawych

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:

~~OWNERS~~ [kawych]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

s-urbaniak · 2019-07-15T07:45:21Z

post-merge lgtm from my side 👍 Sorry, I have been on a long PTO.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 15, 2018

k8s-ci-robot requested review from DirectXMan12 and piosz October 15, 2018 14:56

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 15, 2018

astefanutti mentioned this pull request Oct 15, 2018

Support List with options for RESTful storage services kubernetes/kubernetes#69806

Closed

DirectXMan12 suggested changes Nov 12, 2018

View reviewed changes

astefanutti force-pushed the pr-cm-labels branch from cf00c84 to 3ec1933 Compare November 15, 2018 10:46

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 15, 2018

astefanutti force-pushed the pr-cm-labels branch from 3ec1933 to 4bf4950 Compare November 15, 2018 10:58

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 16, 2018

astefanutti force-pushed the pr-cm-labels branch from db19feb to ce275f5 Compare November 27, 2018 08:24

astefanutti force-pushed the pr-cm-labels branch from ce275f5 to e9da6de Compare November 27, 2018 08:58

DirectXMan12 mentioned this pull request Dec 4, 2018

[feature] Allow label value interpolation in metric names kubernetes-sigs/prometheus-adapter#137

Closed

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 28, 2019

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 28, 2019

k8s-ci-robot added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label May 28, 2019

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jun 17, 2019

k8s-ci-robot requested review from kawych and removed request for piosz June 17, 2019 11:01

kawych suggested changes Jun 25, 2019

View reviewed changes

astefanutti added 3 commits July 1, 2019 22:16

Support metric label selector for custom metrics

71d134e

Support List with options for custom metrics label selector

17cb7c8

Reduce API server code duplication

2a23342

astefanutti force-pushed the pr-cm-labels branch from e9da6de to 2a23342 Compare July 1, 2019 20:25

k8s-ci-robot assigned kawych Jul 2, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 2, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 2, 2019

k8s-ci-robot merged commit 8fcee3f into kubernetes-sigs:master Jul 2, 2019

		@@ -286,3 +286,9 @@ func restfulListResource(r rest.Lister, rw rest.Watcher, scope handlers.RequestS
		handlers.ListResource(r, rw, scope, forceWatch, minRequestTimeout)(res.ResponseWriter, req.Request)

Support metric label selector for custom metrics #35

Support metric label selector for custom metrics #35

Conversation

astefanutti commented Oct 15, 2018 • edited

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefanutti commented Nov 13, 2018

astefanutti commented Nov 13, 2018

DirectXMan12 commented Nov 13, 2018

DirectXMan12 commented Nov 13, 2018

astefanutti commented Nov 14, 2018

astefanutti commented Nov 15, 2018 • edited

astefanutti commented Nov 27, 2018

DirectXMan12 commented Jan 4, 2019

metalmatze commented Jan 4, 2019

brancz commented Jan 7, 2019

DirectXMan12 commented Jan 11, 2019

fejta-bot commented Apr 28, 2019

fejta-bot commented May 28, 2019

kawych commented Jun 17, 2019

kawych commented Jun 17, 2019

brancz commented Jun 17, 2019

astefanutti commented Jun 23, 2019 • edited

kawych left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefanutti commented Jun 25, 2019

astefanutti commented Jul 1, 2019

kawych commented Jul 2, 2019

k8s-ci-robot commented Jul 2, 2019

s-urbaniak commented Jul 15, 2019

astefanutti commented Oct 15, 2018 •

edited

astefanutti commented Nov 15, 2018 •

edited

astefanutti commented Jun 23, 2019 •

edited