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

Monitor diff directory for overlay2 #1770

Merged
merged 1 commit into from
Oct 18, 2017
Merged

Monitor diff directory for overlay2 #1770

merged 1 commit into from
Oct 18, 2017

Conversation

dashpole
Copy link
Collaborator

Fixes kubernetes/kubernetes#52336

The directory structure of overlay2 is different from overlay, and we should monitor the contents of /var/lib/docker/overlay2/CONTAINER_ID/diff/ instead of just /var/lib/docker/overlay2/CONTAINER_ID/

@dashpole
Copy link
Collaborator Author

we should cherrypick this onto the v0.27 and v0.26 branch

@dashpole
Copy link
Collaborator Author

cc @tallclair

@dashpole
Copy link
Collaborator Author

actually, lets wait on this until I have done some more digging.

@derekwaynecarr
Copy link
Collaborator

fyi @runcom @mrunalp @sjenning -- input here?

also do we have the same or similar issue w/ crio?

@mrunalp
Copy link
Collaborator

mrunalp commented Oct 12, 2017 •
edited
Loading

@derekwaynecarr Yeah, we are returning the path to container rootfs. So, we need to add "diff" here to do the equivalent for cri-o - https://github.com/google/cadvisor/blob/master/container/crio/handler.go#L144

@runcom
Copy link
Contributor

runcom commented Oct 12, 2017

@derekwaynecarr Yeah, we are returning the path to container rootfs. So, we need to add "diff" here to do the equivalent for cri-o - https://github.com/google/cadvisor/blob/master/container/crio/handler.go#L144

during my testing overlay2 didn't show this issue, I'm not sure...

@runcom
Copy link
Contributor

runcom commented Oct 12, 2017

we need this in cri-o as well @derekwaynecarr @mrunalp

@dashpole
Copy link
Collaborator Author

I tested the directory setup by doing the following:
On a node running ubuntu xenial, docker 1.12.6.
Change storage driver to aufs, restart docker, run docker run -it busybox sh, then touch myfile.
Repeat for overlay and overlay2.

/var/lib/docker# find . | grep myfile
./overlay2/25bee5c56e3f26d2aaaf228a71ffa947970329fc9692e85ae68a5418dcc6387b/diff/myfile
./aufs/diff/6d1dbb63ff495abe6389cdadc9bc49b7621f03c5d98f136e30316f3d0bf8984f/myfile
./overlay/b037efcadbc8129c861ec5e613803697fd09c0c368c02bd48db0e576d8400690/upper/myfile

So we may also have a bug with overlay as well...

@derekwaynecarr
Copy link
Collaborator

@mrunalp @runcom can you do same exercise as @dashpole on crio?

@mrunalp
Copy link
Collaborator

mrunalp commented Oct 12, 2017

If we want to get to the upper layer of the container in crio, it is as I pointed out earlier https://github.com/google/cadvisor/blob/master/container/crio/handler.go#L144 + /diff.

e.g. /var/lib/containers/storage/overlay/1462529fa84a113ec84ef5406c6db96812d792e632c66555e1032404c2431486/diff

@dashpole
Copy link
Collaborator Author

Based on the docker docs, and my own experiments, I think this fixes the overlay2 incorrect root directory issue. @derekwaynecarr would you mind providing a review?

@runcom
Copy link
Contributor

runcom commented Oct 13, 2017

@mrunalp yes, I'm working on a cri-o fix

This was referenced Oct 13, 2017
Closed
Merged
@timothysc
Copy link
Contributor

I think this is cherry-pick worthy fwiw.

@derekwaynecarr
Copy link
Collaborator

@dashpole I will review

derekwaynecarr
derekwaynecarr approved these changes Oct 18, 2017
View reviewed changes
Copy link
Collaborator

@derekwaynecarr derekwaynecarr left a comment

Choose a reason for hiding this comment

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

LGTM

@derekwaynecarr derekwaynecarr merged commit 90bb052 into google:master Oct 18, 2017
@sjenning sjenning mentioned this pull request Oct 18, 2017
Merged
@thomas-riccardi thomas-riccardi mentioned this pull request Oct 18, 2017
Closed
@timothysc
Copy link
Contributor

@dashpole Can we get this all the way onto 1.8.2?

/cc @jpbetz

@dashpole dashpole deleted the fix_overlay2 branch October 20, 2017 19:44
@dashpole dashpole mentioned this pull request Oct 20, 2017
Merged
dashpole added a commit that referenced this pull request Oct 20, 2017
@dashpole
Merge pull request #1775 from dashpole/cherrypick_1770
dd8f682 
Cherrypick of #1770 to 0.27 release branch
openshift-merge-robot added a commit to openshift/origin that referenced this pull request Oct 20, 2017
@openshift-merge-robot
Merge pull request #16931 from sjenning/pick-cadv-1770
a8dde92 
Automatic merge from submit-queue (batch tested with PRs 16912, 16931, 16939, 16967, 16978).

UPSTREAM: google/cadvisor: 1770: Monitor diff directory for overlay2

google/cadvisor#1770

Fixes per-container disk usage accounting when using overlay2

@derekwaynecarr
@dashpole dashpole mentioned this pull request Nov 1, 2017
Merged
k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this pull request Nov 7, 2017
Merge pull request #54958 from dashpole/update_cadvisor_godep_v1.7
97163a4 
Automatic merge from submit-queue.

update cadvisor godeps to v0.26.2

Issue: #52336
This is to cherrypick google/cadvisor#1770 to the 1.7 branch.
First, I cherrypicked it to the cadvisor v0.26 branch, which is the cadvisor release tied to k8s v1.7.
Now, I need to update the vendored version in 1.7 from 0.26.1 to 0.26.2.

```release-note
Fix overlay2 container disk metrics for Docker
```

/assign @wojtek-t @dchen1107
@dashpole dashpole mentioned this pull request Nov 15, 2017
Merged
@warmchang
Copy link
Contributor

👍

@lf1029698952
Copy link

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derekwaynecarr derekwaynecarr derekwaynecarr approved these changes

Assignees

@derekwaynecarr derekwaynecarr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@dashpole @derekwaynecarr @mrunalp @runcom @timothysc @warmchang @lf1029698952 @tallclair