Add driver.docker counter metric for OOM Killer events #4185

jesusvazquez · 2018-04-19T15:45:12Z

Hi!

This PR is a proposal to add a new counter metric for OOM Killer events.

The current behaviour is that when a container is killed by the OOM Killer nomad logs the event. The changes I made emit a counter metric after that with some labels that I'd consider useful for later analysis.

Image
ImageID
ContainerID

I'm starting to learn golang so if I got something wrong give me some guidances.

Regards

jesusvazquez · 2018-04-20T06:55:22Z

client/driver/docker.go

@@ -17,12 +17,13 @@ import (
 	"time"

 	"github.com/armon/circbuf"
-	docker "github.com/fsouza/go-dockerclient"
+	"github.com/fsouza/go-dockerclient"


Goimports suggested this change. Please let me know if you want me to rollback this line.

its because the imported package in that folder is already called docker :)

jippi · 2018-04-20T08:06:48Z

client/driver/docker.go

@@ -1924,6 +1925,21 @@ func (h *DockerHandle) run() {
 		h.logger.Printf("[ERR] driver.docker: failed to inspect container %s: %v", h.containerID, ierr)
 	} else if container.State.OOMKilled {
 		werr = fmt.Errorf("OOM Killed")
+		labels := []metrics.Label{


for me the interesting labels would be job, group and task :)

Yes for me too, but i don't know how to access them from here. I'm happy to implement a solution but I'd need some help.

@jesusvazquez you could add those fields to the DockerHandle struct, and then populate them from the DriverContext (I'm preparing a PR so you can have those available).

nomad/client/driver/docker.go

Lines 895 to 909 in fb7e0c1

h := &DockerHandle{

client: client,

waitClient: waitClient,

executor: exec,

pluginClient: pluginClient,

logger: d.logger,

Image: d.driverConfig.ImageName,

ImageID: d.imageID,

containerID: container.ID,

version: d.config.Version.VersionNumber(),

killTimeout: GetKillTimeout(task.KillTimeout, maxKill),

maxKillTimeout: maxKill,

doneCh: make(chan bool),

waitCh: make(chan *dstructs.WaitResult, 1),

}

Oh really nice @jvrplmlmn Ill update this PR as soon as I get back from my vacations.

Adding this fields to the DriverContext object, will allow us to pass them to the drivers. An use case for this, will be to emit tagged metrics in the drivers, which contain all relevant information: - Job - TaskGroup - Task - ... Ref: hashicorp#4185

dadgar · 2018-04-23T16:23:57Z

client/driver/docker.go

+				Value: h.ImageID,
+			},
+			{
+				Name:  "ContainerID",


Is this a useful metric? Seems like the combination of image ID + job information captures all you would need and be a little less noisy.

Yes now that #4196 introduces job information Ill update the labels.

@dadgar Please review the updated labels e7bd558

…ocid

jesusvazquez · 2018-05-04T06:07:02Z

client/driver/docker.go

+		jobName:        d.DriverContext.jobName,
+		taskGroupName:  d.DriverContext.taskGroupName,
+		taskName:       d.DriverContext.taskName,
+		allocID:        d.DriverContext.allocID,


Two notes on this:

AllocID: I've added the allocation id because then we have the full context from the job to the allocation. This is useful to provide user oriented alerts.

We're initializing DockerHandle with 4 out of 8 fields of the DriverContext struct. Then I question myself: Would it make sense to pass the whole struct to DockerHandle?.

Need to add to ID and Open method: https://github.com/hashicorp/nomad/blob/master/client/driver/docker.go#L1769-L1798

jesusvazquez · 2018-05-04T07:09:07Z

client/driver/docker.go

+				Value: h.taskGroupName,
+			},
+			{
+				Name:  "TaskName",


@jippi this is what you meant right?

yeah - looks great!

I personally don't care for the allocation id below, its a very high cardinality field, making influxdb & friends unhappy fairly fast

I understand that from the database and query point of view it probably has no value but from the alerting point of view the user can quickly run nomad alloc-status <alloc-id> and see what is happening.

What would you say here? Should we keep it or drop it.

my personal preference would be to drop it - sounds like an alert in your logging rather than telemetry if you ask me :)

I would drop it. As @jippi points out it will make many telemetry systems unhappy

dadgar · 2018-05-09T21:30:53Z

client/driver/docker.go

+				Value: h.taskGroupName,
+			},
+			{
+				Name:  "TaskName",


I would drop it. As @jippi points out it will make many telemetry systems unhappy

dadgar · 2018-05-09T21:31:56Z

client/driver/docker.go

@@ -1924,6 +1933,25 @@ func (h *DockerHandle) run() {
 		h.logger.Printf("[ERR] driver.docker: failed to inspect container %s: %v", h.containerID, ierr)
 	} else if container.State.OOMKilled {
 		werr = fmt.Errorf("OOM Killed")
+		labels := []metrics.Label{
+			{
+				Name:  "JobName",


Use consistent names: https://github.com/hashicorp/nomad/blob/master/client/task_runner.go#L274: job, task_group, task

dadgar · 2018-05-09T21:34:43Z

client/driver/docker.go

+		jobName:        d.DriverContext.jobName,
+		taskGroupName:  d.DriverContext.taskGroupName,
+		taskName:       d.DriverContext.taskName,
+		allocID:        d.DriverContext.allocID,


Need to add to ID and Open method: https://github.com/hashicorp/nomad/blob/master/client/driver/docker.go#L1769-L1798

dadgar · 2018-05-21T18:17:18Z

Ping on comments

jesusvazquez · 2018-05-21T18:39:46Z

@dadgar

Need to add to ID and Open method: https://github.com/hashicorp/nomad/blob/master/client/driver/docker.go#L1769-L1798

I checked the ID method but I see that the dockerPID struct does not have job, task and task_group values, should I create them? Or maybe I got it wrong, could you give me more detail?

jesusvazquez · 2018-05-25T07:41:12Z

Ping @dadgar ⬆️

github-actions · 2023-03-03T02:47:02Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

jesusvazquez commented Apr 20, 2018

View reviewed changes

jippi reviewed Apr 20, 2018

View reviewed changes

jvrplmlmn mentioned this pull request Apr 22, 2018

DriverContext: Add the TaskGroup and the Job name #4196

Merged

dadgar reviewed Apr 23, 2018

View reviewed changes

Jesus Vazquez added 2 commits May 4, 2018 13:46

Add driver.docker counter metric for OOM Killer events

5cacfaf

Run goimports

54e1788

jesusvazquez force-pushed the add-counter-metric-for-oom-killer-events branch from 6f35d36 to 54e1788 Compare May 4, 2018 05:46

Jesus Vazquez added 3 commits May 4, 2018 14:01

Add Job, taskgroupname, taskname, and allocid to the DockerHandle struct

feeee1b

Initialize dockerhandle with jobname, taskgroupname, taskname and all…

8bc4eb2

…ocid

Update counter driver.docker.oom labels

e7bd558

jesusvazquez commented May 4, 2018

View reviewed changes

dadgar requested changes May 9, 2018

View reviewed changes

jesusvazquez added 4 commits May 21, 2018 20:30

Remove allocid label from driver.docker.oom counter metric

caeb596

Rename labels job, task_group and task

a5166e9

Remove allocID from dockerhandle struct

48c5093

Add job, task, taskgroup to open method

ae0b3d9

dadgar approved these changes Jun 4, 2018

View reviewed changes

dadgar merged commit b7a7caf into hashicorp:master Jun 4, 2018

github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add driver.docker counter metric for OOM Killer events #4185

Add driver.docker counter metric for OOM Killer events #4185

jesusvazquez commented Apr 19, 2018

jesusvazquez Apr 20, 2018

jippi Apr 20, 2018

jippi Apr 20, 2018

jesusvazquez Apr 20, 2018 •

edited

Loading

jvrplmlmn Apr 22, 2018

jvrplmlmn Apr 22, 2018

jesusvazquez Apr 25, 2018

dadgar Apr 23, 2018 •

edited

Loading

jesusvazquez Apr 25, 2018

jesusvazquez May 4, 2018

jesusvazquez May 4, 2018

dadgar May 9, 2018

jesusvazquez May 4, 2018

jippi May 4, 2018 •

edited

Loading

jesusvazquez May 4, 2018

jippi May 4, 2018

dadgar May 9, 2018

dadgar May 9, 2018

dadgar May 9, 2018

dadgar May 9, 2018

dadgar commented May 21, 2018

jesusvazquez commented May 21, 2018

jesusvazquez commented May 25, 2018

github-actions bot commented Mar 3, 2023

	h := &DockerHandle{
	client: client,
	waitClient: waitClient,
	executor: exec,
	pluginClient: pluginClient,
	logger: d.logger,
	Image: d.driverConfig.ImageName,
	ImageID: d.imageID,
	containerID: container.ID,
	version: d.config.Version.VersionNumber(),
	killTimeout: GetKillTimeout(task.KillTimeout, maxKill),
	maxKillTimeout: maxKill,
	doneCh: make(chan bool),
	waitCh: make(chan *dstructs.WaitResult, 1),
	}

Add driver.docker counter metric for OOM Killer events #4185

Add driver.docker counter metric for OOM Killer events #4185

Conversation

jesusvazquez commented Apr 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesusvazquez Apr 20, 2018 • edited Loading

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

dadgar Apr 23, 2018 • edited Loading

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

jippi May 4, 2018 • edited Loading

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

dadgar commented May 21, 2018

jesusvazquez commented May 21, 2018

jesusvazquez commented May 25, 2018

github-actions bot commented Mar 3, 2023

jesusvazquez Apr 20, 2018 •

edited

Loading

dadgar Apr 23, 2018 •

edited

Loading

jippi May 4, 2018 •

edited

Loading