Rewrite finalURLTemplate used only for metrics because of dynamic client change #68530

wenjiaswe · 2018-09-12T01:09:26Z

What this PR does / why we need it:
When new easy-to-use dynamic client introduced in #62913, name and namespace are appended in url and ends up in request.pathPrefix, so original function finalURLTemplate was not able to replace the name of namespaces with string "{namespace}", which cause the overwhelming metrics issue mentioned in #68115 .

This PR edited finalURLTemplate function to replace the name of namespace with string "{namespace}" in the pathPrefix for dynamic client.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #68115

Special notes for your reviewer:

As mentioned in issue Histogram metrics generating overwhelming number of prometheus metrics #68115,

This seems to be a pretty critical bug. Will mark it for v1.12. Please triage.

Please help to review the PR asap if possible. Thanks very much in advance!

This fix only fix the namespace. The name is also impacted since name should be replaced by {name} in finalURLTemplate. The name string is appended to url directly without a prefix like /name/, which is different from namespaces (/namespaces/ACTUALNAMESPACE), it might involve more than just parse the request.pathPrefix. Since it does not impact the metrics, and also considering the time sensitivity during the code freeze, maybe we could consider fix it later? Please let me know if it actually does have impact that big enough to impact 1.12 release.

@saad-ali what do yo think?

Release note:

fix a bug that overwhelming number of prometheus metrics are generated because $NAMESPACE is not replaced by string "{namespace}"

k8s-ci-robot · 2018-09-12T01:09:38Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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. I understand the commands that are listed here.

wenjiaswe · 2018-09-12T01:35:59Z

/cc @deads2k @caesarxuchao @roycaihw

wenjiaswe · 2018-09-12T01:40:39Z

/kind bug

roycaihw · 2018-09-12T01:48:24Z

/ok-to-test

wenjiaswe · 2018-09-12T03:31:58Z

/retest

wenjiaswe · 2018-09-12T04:06:16Z

/milestone v1.12

k8s-ci-robot · 2018-09-12T04:06:17Z

@wenjiaswe: You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to set the milestone.

In response to this:

/milestone v1.12

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.

roycaihw

Thanks for the PR! I left some comments. Given finalURLTemplate is only used by metrics, and prometheus metrics seem to prune resource {name} from URL, I'm okay with fixing {namespace} first and have {name} as a followup after code freeze. I'm not familiar with metrics so I cannot tell the impact.

roycaihw · 2018-09-12T18:09:29Z

staging/src/k8s.io/client-go/rest/request.go

+func replaceNamespace4DynamicClient(oldPath string) string {
+	segments := strings.Split(oldPath, "/")
+	for i := 0; i < len(segments)-1; i++ {
+		if segments[i] == "namespaces" {


One concern is:

if the resource name is "namespaces" (e.g. a node called "namespaces");

if r.baseURL.Path contains "/namespaces/"

I'm not sure if we have constraints (e.g. resources cannot be named "namespaces") that could help us rule out the two cases above. If not, you could check if r.pathPrefix contains r.baseURL.Path, and ignore the baseURL part during parsing. The rest of pathPrefix should follow Kubernetes API convention format (legacy group v.s. named group; cluster-scoped v.s. namespaced).

This also helps you parse the {name} segment.

Another case is the namespace kind, where the url is /api/v1/namespaces/{name}

I checked in my local cluster, it seems like I can create a namespace called "namespace" with no problem. But in that case, the replaceNamespace4DynamicClient function can still handle it by blindly replace whatever segment is following /namespaces/ with {namespace}. r.baseURL and pathPrefix are separate in rest.Request. So I don't get how would checking r.pathPrefix and r.baseURL.Path help?

For example, as in testcase now, if full url is "http://localhost/pre1/namespaces/ns/r1", then r.pathPrefix is "/pre1/namespaces/ns/r1". r.baseURL is "http://localhost"

I am a little skeptical of this approach, but maybe you can make it work. :)

You at least need to check more than one path segment. Here's some test cases off the top of my head.

/api/v1/namespaces/kube-system/services/monitoring-heapster is about a service; it should match.

/api/v1/namespaces/kube-system is about a namespace; it should NOT match (kube-system is the name of the resource in this case, which is not inside a namespace).

/apis/apps/v1/namespaces/foo/deployments/bar is about resource bar in namespace foo, it should match.

/apis/apps/v1/namespaces/namespaces/deployments/namespaces is about resource namespaces in namespace namespaces, it should match.

I just pushed a new commit after talking to @roycaihw offline. It parses the request.pathPrefix based on the type of API groups, either in /api/v1... format for legacy group or /apis/apps/v1... format for named group.

Based on @lavalamp comment, I think there is no rule on nomenclature rule... As far as Daniel's concern, the first, third and forth case should all be fine. However, both Haowei and Daniel mentioned that if we have a dynamic resource client that has no namespace but has a resource called "namespaces", then current fix would blindly replace the segment after the resource name with "{namespace}", which is wrong...

In that case, we will need to pass the dynamicResourceClient struct to rest/request.go so it knows exactly what the structure is instead of blind string manipulation. And it would be more than "only have to do the analysis if you have no hits for the namespace being set." as @deads2k original suggested, let me do more analysis and see if it would have impact on other parts than just metrics.

/api/v1/namespaces/kube-system is about a namespace; it should NOT match

Given the fact that user can choose to set pathPrefix, resource and resourceName at the same time, a pathPrefix can also be part of a /api/v1/namespaces/kube-system/services/monitoring-heapster request, in which case it should match.

We either need to involve more fields in Request into the detection logic, or use some other approach (e.g. modify the dynamic client)

roycaihw · 2018-09-12T18:10:21Z

staging/src/k8s.io/client-go/rest/request_test.go

+		}
+		actualURL := r.finalURLTemplate()
+		actual := actualURL.String()
+		expected := testCase.ExpectedFinalURL


nit: you don't need to redefine expected here

Thanks! Addressed.

lavalamp · 2018-09-17T17:25:03Z

staging/src/k8s.io/client-go/rest/request_test.go

+			Request: NewRequest(nil, "DELETE", uri, "", ContentConfig{GroupVersion: &schema.GroupVersion{Group: "test"}}, Serializers{}, nil, nil, 0).
+				Prefix("/apis/namespaces/namespaces/namespaces/namespaces"),
+			ExpectedFullURL:  "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces",
+			ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bname%7D",


I would have expected apis/namespaces/{namespace}/namespaces/{name}?

wait no I got confused :)

This is correct.

lavalamp · 2018-09-17T17:26:14Z

staging/src/k8s.io/client-go/rest/request_test.go

+			Request: NewRequest(nil, "DELETE", uri, "", ContentConfig{GroupVersion: &schema.GroupVersion{Group: "test"}}, Serializers{}, nil, nil, 0).
+				Prefix("/apis/namespaces/namespaces/namespaces/namespaces/status"),
+			ExpectedFullURL:  "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/status",
+			ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bname%7D/status",


I'd expect{namespace} here too?

Scratch this, it's correct.

lavalamp · 2018-09-17T17:29:30Z

/lgtm
/approve

Test cases seem thorough :)

k8s-ci-robot · 2018-09-17T17:29:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, wenjiaswe

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:

~~staging/src/k8s.io/client-go/OWNERS~~ [lavalamp]

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

alvaroaleman · 2018-09-19T09:06:23Z

@wenjiaswe This is a pretty big issue actually, is there a chance this fix can get cherrrypicked/backported to 1.11?

wenjiaswe · 2018-09-19T16:39:29Z

@alvaroaleman thanks for reminding! Let me see if I could get the final fix #68690 done soon enough, then I will back port the final fix. Otherwise I will back port this one first.

lavalamp · 2018-09-19T16:43:27Z

It's probably best to just backport this fix, and only worry about getting the final fix into master. Since the behavior should be the same?

…

On Wed, Sep 19, 2018 at 9:41 AM Wenjia ***@***.***> wrote: @alvaroaleman <https://github.com/alvaroaleman> thanks for reminding! Let me see if I could get the final fix #68690 <#68690> done soon enough, then I will back port the final fix. Otherwise I will back port this one first. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#68530 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnglpOZwJ8ZW6Gg2GNoytKW3LvS0I5yks5ucnPRgaJpZM4WkeXY> .

wenjiaswe · 2018-09-19T17:26:59Z

@lavalamp you are right, the worries we had about this temp fix doesn't exist in 1.11 anyway. I opened a PR for backporting. Thanks.

After v1.11, the amount of metrics generated by kube-controller-manager increased several times. See kubernetes/kubernetes#68530 We drop all those metrics, but it takes extra time for Prometheus to scrape this endpoint.

…530-upstream-release-1.11 Automated cherry pick of #68530: Rewrite finalURLTemplate used only for metrics of dynamic client

In kubernetes/kubernetes#68530 a bug will be fixed with v1.12 of kubernetes, which floods the metrics created by the controller-manager. This fix will drop all rest_* series from the controller-manager when v1.11.x is used.

* Mitigate prometheus RAM flooding In kubernetes/kubernetes#68530 a bug will be fixed with v1.12 of kubernetes, which floods the metrics created by the controller-manager. This fix will drop all rest_* series from the controller-manager when v1.11.x is used. * fix tests * update fixtures * restrict prometheus drop rule to 1.11.0-1.11.3 as it got fixed in the versions above * fix test

* Mitigate prometheus RAM flooding In kubernetes/kubernetes#68530 a bug will be fixed with v1.12 of kubernetes, which floods the metrics created by the controller-manager. This fix will drop all rest_* series from the controller-manager when v1.11.x is used. * fix tests * update fixtures * restrict prometheus drop rule to 1.11.0-1.11.3 as it got fixed in the versions above * fix test (cherry picked from commit 20aeed6)

After v1.11, the amount of metrics generated by kube-controller-manager increased several times. See kubernetes/kubernetes#68530 We drop all those metrics, but it takes extra time for Prometheus to scrape this endpoint.

k8s-ci-robot requested review from nikhiljindal and resouer September 12, 2018 01:09

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 12, 2018

wenjiaswe force-pushed the 68115 branch from 0689617 to bc6bc00 Compare September 12, 2018 01:11

k8s-ci-robot requested review from caesarxuchao, deads2k and roycaihw September 12, 2018 01:35

wenjiaswe force-pushed the 68115 branch from bc6bc00 to 50d171b Compare September 12, 2018 01:38

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 12, 2018

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 12, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 12, 2018

roycaihw reviewed Sep 12, 2018

View reviewed changes

wenjiaswe force-pushed the 68115 branch 2 times, most recently from b1dc3a9 to abbef82 Compare September 12, 2018 20:58

lavalamp reviewed Sep 17, 2018

View reviewed changes

k8s-ci-robot assigned lavalamp Sep 17, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 17, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 17, 2018

k8s-ci-robot merged commit 817d420 into kubernetes:master Sep 17, 2018

wenjiaswe deleted the 68115 branch September 17, 2018 19:46

cbeneke mentioned this pull request Sep 18, 2018

Mitigate prometheus RAM flooding kubermatic/kubermatic#2020

Merged

wenjiaswe mentioned this pull request Sep 19, 2018

Automated cherry pick of #68530: Rewrite finalURLTemplate used only for metrics of dynamic client #68842

Merged

dadux mentioned this pull request Sep 26, 2018

Kubernetes rest_client_* metrics within controller-manager explode in cardinality when deleting many uniquely named namespaces #68918

Closed

mvladev mentioned this pull request Oct 8, 2018

Increase scrape_timeout for kube-controller-manager gardener/gardener#424

Merged

k8s-ci-robot added a commit that referenced this pull request Oct 10, 2018

Merge pull request #68842 from wenjiaswe/automated-cherry-pick-of-#68…

0116476

…530-upstream-release-1.11 Automated cherry pick of #68530: Rewrite finalURLTemplate used only for metrics of dynamic client

wenjiaswe mentioned this pull request Nov 8, 2018

REQUEST: New membership for @wenjiaswe kubernetes/org#227

Closed

6 tasks

liggitt mentioned this pull request Jan 4, 2019

url label in rest client Prometheus metrics causes cardinality problems #72523

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite finalURLTemplate used only for metrics because of dynamic client change #68530

Rewrite finalURLTemplate used only for metrics because of dynamic client change #68530

wenjiaswe commented Sep 12, 2018 •

edited

k8s-ci-robot commented Sep 12, 2018

wenjiaswe commented Sep 12, 2018

wenjiaswe commented Sep 12, 2018

roycaihw commented Sep 12, 2018

wenjiaswe commented Sep 12, 2018

wenjiaswe commented Sep 12, 2018

k8s-ci-robot commented Sep 12, 2018

roycaihw left a comment

roycaihw Sep 12, 2018

roycaihw Sep 12, 2018

wenjiaswe Sep 12, 2018

lavalamp Sep 12, 2018

wenjiaswe Sep 12, 2018 •

edited

roycaihw Sep 12, 2018 •

edited

roycaihw Sep 12, 2018

wenjiaswe Sep 12, 2018

lavalamp Sep 17, 2018

lavalamp Sep 17, 2018

lavalamp Sep 17, 2018

lavalamp Sep 17, 2018

lavalamp commented Sep 17, 2018

k8s-ci-robot commented Sep 17, 2018

alvaroaleman commented Sep 19, 2018

wenjiaswe commented Sep 19, 2018

lavalamp commented Sep 19, 2018 via email

wenjiaswe commented Sep 19, 2018

Rewrite finalURLTemplate used only for metrics because of dynamic client change #68530

Rewrite finalURLTemplate used only for metrics because of dynamic client change #68530

Conversation

wenjiaswe commented Sep 12, 2018 • edited

k8s-ci-robot commented Sep 12, 2018

wenjiaswe commented Sep 12, 2018

wenjiaswe commented Sep 12, 2018

roycaihw commented Sep 12, 2018

wenjiaswe commented Sep 12, 2018

wenjiaswe commented Sep 12, 2018

k8s-ci-robot commented Sep 12, 2018

roycaihw 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

wenjiaswe Sep 12, 2018 • edited

Choose a reason for hiding this comment

roycaihw Sep 12, 2018 • edited

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

lavalamp commented Sep 17, 2018

k8s-ci-robot commented Sep 17, 2018

alvaroaleman commented Sep 19, 2018

wenjiaswe commented Sep 19, 2018

lavalamp commented Sep 19, 2018 via email

wenjiaswe commented Sep 19, 2018

wenjiaswe commented Sep 12, 2018 •

edited

wenjiaswe Sep 12, 2018 •

edited

roycaihw Sep 12, 2018 •

edited