Opened 7 years ago

Last modified 6 years ago

#7968 enhancement new

— at Docker overlayfs doesn't support INotify, we shouldn't enable support for itVersion 7

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 (7)

comment:1 Changed 7 years ago by hawkowl

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

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

comment:2 Changed 7 years ago by hawkowl

Keywords: review added

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

comment:3 Changed 7 years ago by hawkowl

Milestone: Twisted 15.3

comment:4 Changed 7 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 7 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 7 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 7 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
Note: See TracTickets for help on using tickets.