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

[docker] Fix pid retrieval #2847

Merged
merged 1 commit into from
Sep 15, 2016
Merged

[docker] Fix pid retrieval #2847

merged 1 commit into from
Sep 15, 2016

Conversation

remh
Copy link

@remh remh commented Sep 14, 2016 •
edited
Loading

What does this PR do?

On kubernetes a cgroup file will look like

root@dd-agent-nightly-rc-jnexb:/host/proc/17916# cat cgroup
10:net_prio:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6a
42772
9:perf_event:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6
a42772
8:blkio:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6a4277
2
7:net_cls:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6a42
772

freezer:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6a4277
2
5:devices:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6a42
772
4:memory:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6a427
72
3:cpuacct:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6a42
772
2:cpu:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6a42772
1:cpuset:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6a427
72

Which was previously ignored. The refactored logic will take that
format in consideration

Motivation

Without this PR the docker daemon check was failing with

   Traceback (most recent call last):
          File "/opt/datadog-agent/agent/checks/__init__.py", line 771, in run
            self.check(copy.deepcopy(instance))
          File "/opt/datadog-agent/agent/checks.d/docker_daemon.py", line 263, in check
            self._report_performance_metrics(containers_by_id)
          File "/opt/datadog-agent/agent/checks.d/docker_daemon.py", line 534, in _report_performance_metrics
            self._report_cgroup_metrics(container, tags)
          File "/opt/datadog-agent/agent/checks.d/docker_daemon.py", line 552, in _report_cgroup_metrics
            stat_file = self._get_cgroup_from_proc(cgroup["cgroup"], container['_pid'], cgroup['file'])
        KeyError: '_pid'

Testing Guidelines

Tested on k8s 1.2, will test on 1.3 as well

@remh remh added this to the 5.9.0 milestone Sep 14, 2016
@remh remh assigned truthbk and hkaj Sep 14, 2016
@remh remh changed the title [docker] Fix pid_retrieval [docker] Fix pid retrieval Sep 14, 2016
@truthbk
Copy link
Member

truthbk commented Sep 14, 2016

The fix looks good too me... 👍

Let's wait for @hkaj before merging :)

hkaj
hkaj reviewed Sep 14, 2016
View reviewed changes
checks.d/docker_daemon.py
@@ -806,6 +806,17 @@ def _parse_blkio_metrics(self, stats):
metrics['io_write'] += int(line.split()[2])
return metrics

def _is_container_cgroup(self, line, selinux_policy):
if line[1] not in ('cpu,cpuacct', 'cpuacct,cpu', 'cpuacct'):
Copy link
Member

Choose a reason for hiding this comment

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

The PR looks good and can be merged as is, but if you change this line into if line[1] not in ('cpu,cpuacct', 'cpuacct,cpu', 'cpuacct') or line[2] == '/docker-daemon': it would also fix #2841 :)

Copy link
Author

Choose a reason for hiding this comment

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

fixed

hkaj
hkaj reviewed Sep 14, 2016
View reviewed changes
Copy link
Member

@hkaj hkaj left a comment

Choose a reason for hiding this comment

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

just a comment about killing two birds with one stone, otherwise LGTM

@hkaj
Copy link
Member

hkaj commented Sep 15, 2016

Tested on 1.3, it works too.
Server Version: version.Info{Major:"1", Minor:"3", GitVersion:"v1.3.6", GitCommit:"ae4550cc9c89a593bcda6678df201db1b208133b", GitTreeState:"clean"}

@remh remh force-pushed the remh/fix_k8s_pid_retrieval branch from ac551f1 to c9f3a97 Compare September 15, 2016 14:27
[docker] Fix pid_retrieval
c9f3a97 
On kubernetes a group file will look like
```
root@dd-agent-nightly-rc-jnexb:/host/proc/17916# cat cgroup
10:net_prio:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6a
42772
9:perf_event:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6
a42772
8:blkio:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6a4277
2
7:net_cls:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6a42
772

freezer:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6a4277
2
5:devices:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6a42
772
4:memory:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6a427
72
3:cpuacct:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6a42
772
2:cpu:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6a42772
1:cpuset:/5e5f675ab3dbd1353837e46d158a6401abe0673324568b204a3a183ea6a427
72
```

Which was previously ignored. The refactored logic will take that
format in consideration
@remh remh merged commit 984d228 into master Sep 15, 2016
@hkaj hkaj mentioned this pull request Sep 15, 2016
Closed
@remh remh deleted the remh/fix_k8s_pid_retrieval branch September 30, 2016 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hkaj hkaj hkaj left review comments

Assignees

@truthbk truthbk

@hkaj hkaj

Labels
None yet
Projects
None yet
Milestone
5.9.0
Development

Successfully merging this pull request may close these issues.
3 participants
@remh @truthbk @hkaj