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.

Sign up for GitHub

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

Jump to bottom

e2e: adapt kubelet_perf.go to use the new summary metrics API #24003

Merged
merged 1 commit into from
Apr 12, 2016
Merged

e2e: adapt kubelet_perf.go to use the new summary metrics API #24003

merged 1 commit into from
Apr 12, 2016
+142 −91

Conversation

yujuhong
Copy link
Contributor

@yujuhong yujuhong commented Apr 7, 2016

This commit switch most functions in kubelet_stats.go to use the new API.
However, the functions that perform one-time resource usage retrieval remain
unchanged to be compatible with reource_usage_gatherer.go. They should be
handled separately.

Also, the new summary API does not provide the RSS memory yet, so all memory
checking tests will always pass. We plan to add this metrics in the API and
restore the functionality of the test.

@yujuhong yujuhong added sig/node Categorizes an issue or PR as relevant to SIG Node. area/test and removed cla: yes labels Apr 7, 2016
@yujuhong
Copy link
Contributor Author

yujuhong commented Apr 7, 2016

This should fix #23961

Some notes:

  • resource_usage_gatherer.go is not changed to use the new api yet (cc @gmarek).
  • RSS is not supported in the summary API yet, so memory checking will always pass.

Tested the PR by running the resource usage tracking test in my own cluster.

/cc @dchen1107 @timstclair @sjpotter @andyzheng0831

@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Apr 7, 2016
@yujuhong yujuhong mentioned this pull request Apr 7, 2016
Closed
@k8s-bot
Copy link

k8s-bot commented Apr 7, 2016

GCE e2e build/test passed for commit 8e7802e4aa2f6138f9b7a2d4a522f9e908234c7b.

@yujuhong yujuhong added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Apr 7, 2016
@dchen1107 dchen1107 assigned dchen1107 and unassigned ixdy Apr 7, 2016
timstclair
timstclair reviewed Apr 7, 2016
View reviewed changes
test/e2e/kubelet_stats.go

var data []byte
if subResourceProxyAvailable {
data, err = c.Post().
Copy link

Choose a reason for hiding this comment

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

(nit) prefer get, same below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. I copied from the old code which has to use post for stats.

Copy link
Member

Choose a reason for hiding this comment

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

Can we address this comment? We are ready to go.

@dchen1107 dchen1107 mentioned this pull request Apr 8, 2016
Closed
@dchen1107 dchen1107 added this to the v1.2 milestone Apr 8, 2016
@dchen1107 dchen1107 added cherrypick-candidate lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Apr 8, 2016
@dchen1107
Copy link
Member

cc/ @roberthbailey

@dchen1107 dchen1107 removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 8, 2016
@gmarek
Copy link
Contributor

gmarek commented Apr 8, 2016

@yujuhong - can you shortly write what are advantages of this API instead of 'old' that resource-gatherer uses? We need the latter to track master components resources to look for regressions. @roberthbailey @jlowdermilk

@yujuhong
Copy link
Contributor Author

yujuhong commented Apr 8, 2016

This PR needs to wait until #24015 lands so I can add the RSS usage.

@yujuhong
Copy link
Contributor Author

yujuhong commented Apr 8, 2016

@yujuhong - can you shortly write what are advantages of this API instead of 'old' that resource-gatherer uses? We need the latter to track master components resources to look for regressions

You can still get all the pods (including the master components) from the stats/summary endpoint.

The main reason for this PR is that we don't want to deal with the raw cgroup names, and the summary api provides fixed names for the daemons (e.g., runtime, kubelet). In general, we are migrating the code to use the new summary api (see #12483 for details).

@k8s-bot
Copy link

k8s-bot commented Apr 8, 2016

GCE e2e build/test passed for commit ead9a71f9b1d459432f967d7fd6261504b52fd29.

@gmarek
Copy link
Contributor

gmarek commented Apr 8, 2016

@yujuhong - Is this a cleanup, or you're going to provide some more data?

@dchen1107
Copy link
Member

This is a cleanup and fixes the broken tests on systemd nodes too. We might introduce more data later, not with one.

@gmarek
Copy link
Contributor

gmarek commented Apr 8, 2016

OK, I'm assuming that it's OK to leave resource gatherer as-is for now. Note that it currently is only for tracking usage but it'll become critical before v1.3, when we'll add meta-test for resource usage regression.

@yujuhong
Copy link
Contributor Author

yujuhong commented Apr 8, 2016

@gmarek, it's OK for now, but I'd still suggest adopting the summary api if possible. One caveat is that you don't get multiple records, so it might be more problematic to get the cpu usage through one request.

@yujuhong
Copy link
Contributor Author

yujuhong commented Apr 8, 2016

@dchen1107 @andyzheng0831, I verified that the test passes as long as "--runtime-cgroups" is not set. Will rebase after #24015 lands.

@andyzheng0831
Copy link

Awesome! Thank you for fixing it

@gmarek
Copy link
Contributor

gmarek commented Apr 9, 2016

I don't think I'll have time for this before v1.3:/

@yujuhong
e2e: adapt kubelet_perf.go to use the new summary metrics API
a8c6859 
This commit switch most functions in kubelet_stats.go to use the new API.
However, the functions that perform one-time resource usage retrieval remain
unchanged to be compatible with reource_usage_gatherer.go. They should be
handled separately.

Also, the new summary API does not provide the RSS memory yet, so all memory
checking tests will *always* pass. We plan to add this metrics in the API and
restore the functionality of the test.
@yujuhong
Copy link
Contributor Author

Rebased and added back RSS.

@dchen1107
Copy link
Member

LGTM

@dchen1107 dchen1107 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 12, 2016

GCE e2e build/test passed for commit 39b1054f960e09a640c29604dd1434a5f3209349.

@k8s-bot
Copy link

k8s-bot commented Apr 12, 2016

GCE e2e build/test passed for commit a8c6859.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Apr 12, 2016

GCE e2e build/test passed for commit a8c6859.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit e9c1d59 into kubernetes:master Apr 12, 2016
@zmerlynn zmerlynn added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Apr 13, 2016
@roberthbailey
Copy link
Contributor

@yujuhong - cherry pick approved; can you create a cherry pick pull request?

This was referenced Apr 13, 2016
Merged
Merged
roberthbailey added a commit that referenced this pull request Apr 19, 2016
@roberthbailey
Merge pull request #24227 from yujuhong/automated-cherry-pick-of-#24015-
15f1165 
#24003-#24205-upstream-release-1.2

Automated cherry pick of #24015 #24003 #24205
@yujuhong yujuhong mentioned this pull request May 10, 2016
Closed
@yujuhong yujuhong deleted the metrics_test branch November 7, 2016 21:36
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@roberthbailey
Merge pull request kubernetes#24227 from yujuhong/automated-cherry-pi…
e5a7ab6 
…ck-of-#24015-kubernetes#24003-kubernetes#24205-upstream-release-1.2

Automated cherry pick of kubernetes#24015 kubernetes#24003 kubernetes#24205
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
@roberthbailey
Merge pull request kubernetes#24227 from yujuhong/automated-cherry-pi…
7e85f8e 
…ck-of-#24015-kubernetes#24003-kubernetes#24205-upstream-release-1.2

Automated cherry pick of kubernetes#24015 kubernetes#24003 kubernetes#24205
openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Oct 25, 2019
@k8s-publishing-bot
Merge pull request kubernetes#24003 from p0lyn0mial/pick_insecure_proxy
d142f6b 
UPSTREAM: 83419: Insecure backend proxy

Origin-commit: a90a57708e0e28d225ff3f8df72a0567a3cadb23
p0lyn0mial pushed a commit to p0lyn0mial/kubernetes that referenced this pull request Dec 20, 2019
@openshift-merge-robot
Merge pull request kubernetes#24003 from p0lyn0mial/pick_insecure_proxy
49a6b7b 
UPSTREAM: 83419: Insecure backend proxy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@dchen1107 dchen1107

Labels
area/test cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.