Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#7968 enhancement closed fixed (fixed)

Docker overlayfs doesn't support INotify, we shouldn't enable support for it

Reported by: hawkowl Owned by: hawkowl
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/lxc-inotify-7968
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl

Description (last modified by hawkowl)

We should check for Docker, and abort if we're inside it, rather than provide broken support for it.

Change History (12)

comment:1 Changed 2 years ago by hawkowl

Author: hawkowl
Branch: branches/lxc-inotify-7968

(In [45238]) Branching to lxc-inotify-7968.

comment:2 Changed 2 years ago by hawkowl

Keywords: review added

The docker builders don't fail now. Please review.

comment:3 Changed 2 years ago by hawkowl

Milestone: Twisted 15.3

comment:4 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Thanks for working on this... I will soon prepare the travis branch and will give it a spin to test the code.

The code looks like a detection for cgroups, maybe we should move it into t.python.runtime.Platform and write it as a new method isLXC and update the current supportsINotify to make use of the new method.

What do you think?

If you still want to keep the current code, I don't like the way initCGroups is created and then removed :) Why not make it a private method and call?

Thanks!

comment:5 Changed 2 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

Hi Adi,

Thanks for the review!

Good idea, I've done the moving into platform. Builders spun, looking green, please review.

comment:6 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Thanks for the changes. It look better.

Does this code work?

For example I see that on the fedora17-x86_64-py2.7 the INotify tests were not previously skipped and now they are skipped:

a build on another branch: https://buildbot.twistedmatrix.com/builders/fedora17-x86_64-py2.7/builds/2992

build on this branch: https://buildbot.twistedmatrix.com/builders/fedora17-x86_64-py2.7/builds/2989

I have tried them on my Ubuntu 14.04 which is not a container and I see that tests are not skipped... I don't know why they are skipped for the fedora builder.

for me the whole slaves/builder is a mess as they lack documentation.

I think that we need a test for the new code.


For my project I have a wiki page where all slaves are documented. There is also a rule to name the slaves - builders.

Each slave has info about CPU + OS + OS version + RAM + system type (bare metal/vbox/lxc/wpar/vwpar/zone/hp-ux container)

Based on that, I have a tests which checks for various things like INotify support based on an hardcoded list of slave names. It is a bit of work to maintain these tests but in this way you are sure that a test for lxc is executed on a LXC env and a non-LXC container.

The tests are skipped if the CI env variable is not found as developers will also run them outside of the buildbot, on arbitrary systems.

For me is better than no having tests, like the case for the new isLXC code and INotify changes.

Thanks and sorry for being a PITA.

comment:7 Changed 2 years ago by hawkowl

Description: modified (diff)
Summary: lxc overlayfs doesn't support INotify, we shouldn't enable support for itDocker overlayfs doesn't support INotify, we shouldn't enable support for it

comment:8 Changed 2 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

There's now tests for isDocker (since that's more accurate), and one of the test cases uses the cgroups file from Fedora 17, which is slightly different.

Builders spun, please review.

comment:9 Changed 2 years ago by Adi Roiban

Looks better... but they are just unit tests... in case docker infrastructure is changed and cgroups is no longer used our tests will produce false negatives.

To check that all is ok I run the following manual test: Check fedora builder and see that inotify tests are not skipepd and then check the new debian builder and see that inotify are skipped. Maybe we can automate this test.

I am confused... I don't know much about Docker but I was under the impression that Docker is just an automation tools.

The inotify problem is not due to the direct use of Docker but because docker uses unionfs/overlayfs.

In case twisted runs on ufs outside of a docker environment I assume that we experience the same issues. Is that correct?

I am leaving this in review so that other developers could check it... maybe someone from ClusterHQ :)

Thanks!

comment:10 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

I think that this patch is good to be merged.

Here is the chat log:

(11:55:52) hawkowl: well the first bit
(11:55:59) hawkowl: docker is on top of cgroups, it will forever be
(11:56:09) hawkowl: we can change our stuff later
(11:56:12) fijal left the room (quit: Ping timeout: 264 seconds).
(11:57:15) hawkowl: the thing is that right *now*, inotify is broken on docker on recommended and default deploys
(11:57:33) hawkowl: if they change that, we can remove it or make the check finer grained, because twisted is time released
(11:57:44) hawkowl: 15.3 will not be supported in 3 months
(11:58:12) hawkowl: so it doesn't matter if it's wrong because someone else's trash was broken and we had to work around it
(11:58:37) hawkowl: the failure cases are that we diagnose docker when it's not, in which case non-essenital and buggy to start with functionality is disabled
(11:58:49) hawkowl: or we diagnose no docker when it is, in which case we'll get explosions
(12:00:16) adiroiban: so, Inotify is broken due to some custom Docker stuff, or due to the fact that docker used unionfs ?
(12:01:09) adiroiban: how can you know that docker will always be on top of cgroups ? maybe we will see a new fancy feature in linux which will replace cgroups and docker will start using it
(12:05:02) adiroiban: my comment is based on the fact that this behaviour was also observed on travis-ci long time ago, when they were still using virtualbox
(12:05:45) hawkowl: adiroiban: no, they were using docker at that point for new builds
(12:05:55) hawkowl: unless i am mistaken
(12:06:12) ***hawkowl checks
(12:06:48) adiroiban: ok
(12:06:54) adiroiban: my builds were done in Feb 2014
(12:07:08) adiroiban: and I see This job ran on our legacy infrastructure.
(12:07:36) hawkowl: hum
(12:07:41) hawkowl: okay maybe something else was up
(12:07:49) adiroiban: so, again
(12:08:06) adiroiban: is the Inotify caused by Docker custom code, or by docker using unionfs ?
(12:09:03) hawkowl: it's by using unionfs/whatever the new version is called
(12:09:15) hawkowl: but there's no way to detect that (except that inotfify is busted)
(12:11:13) hawkowl: i am considering moving all but the debian ones off my infra
(12:11:17) hawkowl: which will sidestep this for now
(12:11:27) hawkowl: because they
(12:11:31) hawkowl: will be on xen
(12:12:31) hawkowl: yeah i did move one
(12:12:37) hawkowl: py3k warnings needs to be moved too
(12:12:47) hawkowl: or maybe just completely removed, idk how useful it is
(12:15:04) adiroiban: ok. thanks
(12:15:19) adiroiban: can you have a docker without unionfs ?
(12:17:52) hawkowl: adiroiban: it's not supported by them
(12:17:57) hawkowl: and it's not in the default install
(12:19:11) adiroiban: what I am looking for, is to prevent adding a bug in the case someone configures a docker with explicit inotify support (even if inotify is not enabled by default) and then have Twisted reject it just based on the fact that docker is used
(12:20:01) adiroiban: or maybe we should update the news file to inform that inotify is not avaialbe for docker, no mater how you configure docker
(12:21:06) hawkowl: the newsfile does
(12:21:13) hawkowl: "Additionally, Platform.supportsINotify() now returns False if isDocker() is True, because of many Docker storage layers having broken INotify."

so there is no way to detect the usage of unionfs ... so the next best thing we can do is to detect docker and assume that docker uses unionfs.

feel free to merge it.

Thanks!

comment:11 Changed 2 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [45291]) Merge lxc-inotify-7968: Docker overlayfs doesn't support INotify, we shouldn't enable support for it

Author: hawkowl Reviewer: adiroiban Fixes: #7968

comment:12 Changed 2 years ago by hawkowl

Milestone: Twisted 15.3

Ticket retargeted after milestone deleted

Note: See TracTickets for help on using tickets.