Add monitoring support for hardware accelerators #55188

rohitagarwal003 · 2017-11-06T23:51:30Z

Currently only NVIDIA GPU monitoring is implemented.

Feature repo issue: kubernetes/enhancements#369
cAdvisor PR: google/cadvisor#1762

/kind feature
/sig node
/sig instrumentation
/area hw-accelerators

Release note:

Kubelet now exposes metrics for NVIDIA GPUs attached to the containers.

rohitagarwal003 · 2017-11-06T23:52:16Z

/assign @vishh @yujuhong

vishh · 2017-11-07T00:10:04Z

pkg/kubelet/apis/stats/v1alpha1/types.go

+	// unit: bytes
+	MemoryUsed uint64 `json:"memory_used"`
+
+	// Percent of time over the past sample period during which


Define what the sampling period is ? It is 10 seconds right?

vishh · 2017-11-07T00:11:09Z

This PR LGTM. I'd recommend making the cAdvisor update in a separate PR though.
This PR can have just the summary API changes, ideally with corresponding e2es updates as well.

andyxning · 2017-11-08T05:56:10Z

@mindprince @vishh I can help to add GPU info to heapster based on this.

BTW, nice work~

Also update golang.org/x/sys because of google/cadvisor#1786

assert.Contains() checks if its second argument (which is supposed to be a single element) is contained in its first argument (which is supposed to be a slice/map etc.) The third and following arguments are supposed to be message and args for the output in case of failure. Because of this bad form, a failure was hidden, the system container is named "misc", not "system".

rohitagarwal003 · 2017-11-11T02:35:17Z

/assign @dchen1107
This is ready to go in.

abdasgupta · 2017-11-13T11:16:28Z

@mindprince One thing for my clarification. Will this PR show the GPU metrics on the following endpoint: http://KUBELET-IP:10255/stats/summary ?

rohitagarwal003 · 2017-11-13T18:41:31Z

@abdasgupta Yes.

rohitagarwal003 · 2017-11-13T18:42:30Z

/cc @dashpole @Random-Liu @yguo0905
Adding more people from sig node who have worked with the summary API.

yguo0905

LGTM

yguo0905 · 2017-11-14T01:53:34Z

pkg/kubelet/server/stats/summary_test.go

@@ -46,7 +46,7 @@ func TestSummaryProvider(t *testing.T) {
 		node         = &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "test-node"}}
 		nodeConfig   = cm.NodeConfig{
 			RuntimeCgroupsName: "/runtime",
-			SystemCgroupsName:  "/system",
+			SystemCgroupsName:  "/misc",


Just curious why do we need this change?

I explain this here: 238b4a0

Fix TestSummaryProvider. assert.Contains() checks if its second argument (which is supposed to be a single element) is contained in its first argument (which is supposed to be a slice/map etc.) The third and following arguments are supposed to be message and args for the output in case of failure. Because of this bad form, a failure was hidden, the system container is named "misc", not "system".

dchen1107 · 2017-11-15T17:52:20Z

I only reviewed the last commit: Expose accelerator metrics in the summary API.

/lgtm

dchen1107 · 2017-11-15T17:52:58Z

/approve

k8s-github-robot · 2017-11-15T17:53:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, mindprince

Associated issue: 369

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [dchen1107]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

rohitagarwal003 · 2017-11-15T18:49:47Z

/retest

andyxning · 2017-11-16T07:26:34Z

Any thing wrong. Seems submit queue is pending.

k8s-github-robot · 2017-11-16T11:09:18Z

Automatic merge from submit-queue (batch tested with PRs 55798, 49579, 54862, 55188, 51990). If you want to cherry-pick this change to another branch, please follow the instructions here.

smarterclayton · 2017-11-16T23:10:06Z

How was this merged without a proposal merged to kubernetes/community?

smarterclayton · 2017-11-16T23:11:52Z

pkg/kubelet/apis/stats/v1alpha1/types.go

+
+	// Percent of time over the past sample period (10s) during which
+	// the accelerator was actively processing.
+	DutyCycle uint64 `json:"duty_cycle"`


This is not consistent with our API conventions - use of snake_case is not allowed in public APIs.

smarterclayton · 2017-11-16T23:11:57Z

pkg/kubelet/apis/stats/v1alpha1/types.go

+
+	// Total accelerator memory.
+	// unit: bytes
+	MemoryTotal uint64 `json:"memory_total"`


This is not consistent with our API conventions - use of snake_case is not allowed in public APIs.

smarterclayton · 2017-11-16T23:12:02Z

pkg/kubelet/apis/stats/v1alpha1/types.go

+
+	// Total accelerator memory allocated.
+	// unit: bytes
+	MemoryUsed uint64 `json:"memory_used"`


This is not consistent with our API conventions - use of snake_case is not allowed in public APIs.

smarterclayton · 2017-11-16T23:12:35Z

Looks like there are some API errors on this - @mindprince please open a new PR for 1.9 that fixes the API to be covered by our API conventions.

smarterclayton · 2017-11-16T23:14:34Z

@kubernetes/sig-testing-feature-requests I think I need to make sure pkg/kubelet/apis appears under the API changes label (it wasn't flagged here and so I missed it). Can someone point me to where that can be done?

rohitagarwal003 · 2017-11-16T23:23:17Z

@smarterclayton #55908

vishh · 2017-11-16T23:33:14Z

@smarterclayton can we build some CI tooling to enforce these semantics?
I feel a full design review just for the sake of API schema review is heavy weight.
The design doc was shared as a google doc and can now be committed to k8s/community repo if that will be useful.
However, it is not clear if the entire golang object definition is expected in design docs though.

Automatic merge from submit-queue (batch tested with PRs 55908, 55829, 55293, 55653, 55665). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Fix accelerator stats API to follow API conventions. Introduced in #55188 **Release note**: ```release-note None ```

Automatic merge from submit-queue. Add design proposal for accelerator monitoring. 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.

k8s-ci-robot assigned vishh and yujuhong Nov 6, 2017

vishh reviewed Nov 7, 2017

View reviewed changes

rohitagarwal003 force-pushed the accelerator-monitoring branch 2 times, most recently from 292c044 to d5dc8bf Compare November 7, 2017 20:54

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 7, 2017

rohitagarwal003 force-pushed the accelerator-monitoring branch from d5dc8bf to 6cfe359 Compare November 8, 2017 01:16

rohitagarwal003 added 2 commits November 10, 2017 14:54

Update cAdvisor.

fe5ef1b

Also update golang.org/x/sys because of google/cadvisor#1786

rohitagarwal003 force-pushed the accelerator-monitoring branch from 6cfe359 to 7a68e8a Compare November 10, 2017 22:55

Expose accelerator metrics in the summary API.

9c38abd

rohitagarwal003 force-pushed the accelerator-monitoring branch from 7a68e8a to 9c38abd Compare November 10, 2017 22:59

rohitagarwal003 mentioned this pull request Nov 10, 2017

Update gonvml to workaround bazelbuild/rules_go#1003. google/cadvisor#1801

Merged

k8s-ci-robot assigned dchen1107 Nov 11, 2017

k8s-ci-robot requested a review from dashpole November 13, 2017 18:42

k8s-ci-robot requested review from Random-Liu and yguo0905 November 13, 2017 18:42

yguo0905 reviewed Nov 14, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2017

k8s-github-robot merged commit 7791056 into kubernetes:master Nov 16, 2017

smarterclayton reviewed Nov 16, 2017

View reviewed changes

k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Nov 16, 2017

rohitagarwal003 mentioned this pull request Nov 16, 2017

Fix accelerator stats API to follow API conventions. #55908

Merged

dashpole mentioned this pull request Nov 16, 2017

[Failing Test] [k8s.io] Summary API when querying /stats/summary should report resource usage through the stats api #55909

Closed

rohitagarwal003 mentioned this pull request Nov 18, 2017

Update kubelet summary api vendor kubernetes-retired/heapster#1878

Merged

rohitagarwal003 mentioned this pull request Nov 20, 2017

Add design proposal for accelerator monitoring. kubernetes/community#1420

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add monitoring support for hardware accelerators #55188

Add monitoring support for hardware accelerators #55188

rohitagarwal003 commented Nov 6, 2017

rohitagarwal003 commented Nov 6, 2017

vishh Nov 7, 2017

rohitagarwal003 Nov 10, 2017

vishh commented Nov 7, 2017

andyxning commented Nov 8, 2017

rohitagarwal003 commented Nov 11, 2017

abdasgupta commented Nov 13, 2017 •

edited

rohitagarwal003 commented Nov 13, 2017

rohitagarwal003 commented Nov 13, 2017

yguo0905 left a comment

yguo0905 Nov 14, 2017

rohitagarwal003 Nov 14, 2017

dchen1107 commented Nov 15, 2017

dchen1107 commented Nov 15, 2017

k8s-github-robot commented Nov 15, 2017

rohitagarwal003 commented Nov 15, 2017

andyxning commented Nov 16, 2017

k8s-github-robot commented Nov 16, 2017

smarterclayton commented Nov 16, 2017

smarterclayton Nov 16, 2017

smarterclayton Nov 16, 2017

smarterclayton Nov 16, 2017

smarterclayton commented Nov 16, 2017

smarterclayton commented Nov 16, 2017

rohitagarwal003 commented Nov 16, 2017

vishh commented Nov 16, 2017

Add monitoring support for hardware accelerators #55188

Add monitoring support for hardware accelerators #55188

Conversation

rohitagarwal003 commented Nov 6, 2017

rohitagarwal003 commented Nov 6, 2017

vishh Nov 7, 2017

Choose a reason for hiding this comment

rohitagarwal003 Nov 10, 2017

Choose a reason for hiding this comment

vishh commented Nov 7, 2017

andyxning commented Nov 8, 2017

rohitagarwal003 commented Nov 11, 2017

abdasgupta commented Nov 13, 2017 • edited

rohitagarwal003 commented Nov 13, 2017

rohitagarwal003 commented Nov 13, 2017

yguo0905 left a comment

Choose a reason for hiding this comment

yguo0905 Nov 14, 2017

Choose a reason for hiding this comment

rohitagarwal003 Nov 14, 2017

Choose a reason for hiding this comment

dchen1107 commented Nov 15, 2017

dchen1107 commented Nov 15, 2017

k8s-github-robot commented Nov 15, 2017

rohitagarwal003 commented Nov 15, 2017

andyxning commented Nov 16, 2017

k8s-github-robot commented Nov 16, 2017

smarterclayton commented Nov 16, 2017

smarterclayton Nov 16, 2017

Choose a reason for hiding this comment

smarterclayton Nov 16, 2017

Choose a reason for hiding this comment

smarterclayton Nov 16, 2017

Choose a reason for hiding this comment

smarterclayton commented Nov 16, 2017

smarterclayton commented Nov 16, 2017

rohitagarwal003 commented Nov 16, 2017

vishh commented Nov 16, 2017

abdasgupta commented Nov 13, 2017 •

edited