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

Delay renaming for docker containers #5557

Merged
merged 4 commits into from Mar 9, 2019

Conversation

@vlvkobal
Copy link
Member

vlvkobal commented Mar 5, 2019

Summary

When a docker container is starting there is a possibility that the docker ps command will not return the name of the container. An additional renaming attempt after a delay was introduced to deal with the docker lag.

Fixes #5512

Component Name

cgroups.plugin

@vlvkobal vlvkobal requested review from cakrit and ktsaou as code owners Mar 5, 2019
@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Mar 6, 2019

The delay fix seems to be working, i've tested it.

OS: Centos7.
Test: create 100 containers, check their names.

@ktsaou

This comment has been minimized.

Copy link
Member

ktsaou commented Mar 6, 2019

After how much time is checked again?

@vlvkobal

This comment has been minimized.

Copy link
Member Author

vlvkobal commented Mar 6, 2019

After

[plugin:cgroups]
    # check for new cgroups every = 10
@ktsaou

This comment has been minimized.

Copy link
Member

ktsaou commented Mar 6, 2019

ok, I reviewed the code.

Here are some comments:

  1. The script cgroup-name now returns 2 values. The first is a Boolean. I think the return value of the script is more appropriate for this.

  2. There are 2 new cgroup variables introduced: needs_renaming and renaming_delayed.

  • renaming_delayed is set to 1, but never to 0
  • I find the whole idea with these variables a bit complicated. I simpler approach would be to set pending_renames to 2 when the cgroup is to be renamed. Then within cgroup_get_chart_name() first check if pending_renames > 0, and decrement it. If the script returns 0, set pending_renames = 0. This way, cgroup_get_chart_name() should be called only when pending_renames > 0.

What do you think?

@vlvkobal

This comment has been minimized.

Copy link
Member Author

vlvkobal commented Mar 7, 2019

Now script uses an exit code instead of the second parameter.

I replaced two variables by one variable pending_renames. For my taste, two variables reflected the real meaning of the code a little bit more accurately.

@cakrit
cakrit approved these changes Mar 9, 2019
Copy link
Contributor

cakrit left a comment

Tested it and saw it retrying.

@cakrit

This comment has been minimized.

Copy link
Contributor

cakrit commented Mar 9, 2019

Merging myself because I need to work on the same code.

@cakrit cakrit merged commit 2e05371 into netdata:master Mar 9, 2019
12 checks passed
12 checks passed
Header rules - netdata No header rules processed
Details
Pages changed - netdata 2 new files uploaded
Details
Redirect rules - netdata No redirect rules processed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
Mixed content - netdata No mixed content detected
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
netlify/netdata/deploy-preview Deploy preview ready!
Details
@vlvkobal vlvkobal deleted the vlvkobal:delay-renaming branch Mar 10, 2019
@Thatdarkone

This comment has been minimized.

Copy link

Thatdarkone commented Mar 11, 2019

Does this pull resolve the issue of missing interfaces and disks ?
https://github.com/netdata/netdata/issues/5512#issuecomment-467952475

@ilyam8

This comment has been minimized.

Copy link
Member

ilyam8 commented Mar 11, 2019

@Thatdarkone disks are added only if there is any non zero data (so, it will be added later)

Example

  • container with disk
→ cat  /sys/fs/cgroup/blkio/docker/af442bc2a5f272ac045275e99244f8f178d59e36f970d2da1a979b171c4eb532/*
Total 0
Total 0
Total 0
Total 0
Total 0
Total 0
Total 0
Total 0
Total 0
Total 0
Total 0
Total 0
500
cat: /sys/fs/cgroup/blkio/docker/af442bc2a5f272ac045275e99244f8f178d59e36f970d2da1a979b171c4eb532/blkio.reset_stats: Permission denied
8:0 Read 7643136
8:0 Write 0
8:0 Sync 0
8:0 Async 7643136
8:0 Total 7643136
253:0 Read 7643136
253:0 Write 0
253:0 Sync 0
253:0 Async 7643136
253:0 Total 7643136
Total 15286272
8:0 Read 65
8:0 Write 0
8:0 Sync 0
8:0 Async 65
8:0 Total 65
253:0 Read 65
253:0 Write 0
253:0 Sync 0
253:0 Async 65
253:0 Total 65
Total 130
500
0
cat: /sys/fs/cgroup/blkio/docker/af442bc2a5f272ac045275e99244f8f178d59e36f970d2da1a979b171c4eb532/cgroup.event_control: Permission denied
4499
4571
0
4499
4571
  • container w/o disk
→ cat  /sys/fs/cgroup/blkio/docker/1535b27bc0604ed8a4f0712793371b1d86797ee3aa555eadeb176c9c6f60305e/*
Total 0
Total 0
Total 0
Total 0
Total 0
Total 0
Total 0
Total 0
Total 0
Total 0
Total 0
Total 0
500
cat: /sys/fs/cgroup/blkio/docker/1535b27bc0604ed8a4f0712793371b1d86797ee3aa555eadeb176c9c6f60305e/blkio.reset_stats: Permission denied
Total 0
Total 0
500
0
cat: /sys/fs/cgroup/blkio/docker/1535b27bc0604ed8a4f0712793371b1d86797ee3aa555eadeb176c9c6f60305e/cgroup.event_control: Permission denied
6125
6159
0
6125
6159
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.