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

Add design proposal for accelerator monitoring. #1420

Merged

Conversation

rohitagarwal003
Copy link
Member

@rohitagarwal003 rohitagarwal003 commented Nov 20, 2017

For kubernetes/enhancements#369, google/cadvisor#1762 and kubernetes/kubernetes#55188

Conversion to markdown from Google doc: https://docs.google.com/document/d/13O4HNrB7QFpKQcLcJm28R-QBH3Xo0VmJ7w_Pkvmsf68/edit
(accessible to anyone who is a member of kubernetes-dev@ or kubernetes-users@ Google Groups). Lots of discussion on the doc which is hard to recreate here now.

This was discussed on this doc: https://docs.google.com/document/d/13O4HNrB7QFpKQcLcJm28R-QBH3Xo0VmJ7w_Pkvmsf68/edit
(accessible to anyone who is a member of kubernetes-dev or kubernetes-users Google Groups).

This was presented during the sig-node meeting on 2017-10-10.

Adding this to this repo, so it's more accessible.
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 20, 2017
@rohitagarwal003
Copy link
Member Author

/assign @vishh @derekwaynecarr


// Total acclerator memory.
// unit: bytes
MemoryTotal uint64 `json:"memory_total"`
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: either fix the json tags (camel case) or remove them.

Copy link
Member Author

Choose a reason for hiding this comment

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

This section is for what's added to cAdvisor which uses snake_case.

The following new metrics would be exposed per container from cAdvisor:

I mention below that we will add the same thing to the summary API.

We will update kubelet’s summary API to also add these metrics.

@vishh
Copy link
Contributor

vishh commented Nov 21, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 21, 2017
@vishh
Copy link
Contributor

vishh commented Nov 21, 2017

Let's wait for @derekwaynecarr to read through this before merging.

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue.

@k8s-github-robot k8s-github-robot merged commit 1d86f60 into kubernetes:master Nov 21, 2017
@rohitagarwal003
Copy link
Member Author

There's a submit queue now that automatically merges LGTM'd PRs in this repo. I will send an update PR if Derek has any comments.

@derekwaynecarr
Copy link
Member

@vishh @mindprince - no major concerns. thanks for getting this in the repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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
Development

Successfully merging this pull request may close these issues.

5 participants