> On Aug. 1, 2019, 2:15 p.m., Andrei Budnik wrote: > > src/slave/containerizer/mesos/isolators/xfs/disk.cpp > > Lines 335 (patched) > > <https://reviews.apache.org/r/71194/diff/1/?file=2158186#file2158186line335> > > > > According to this comment > > https://github.com/apache/mesos/blob/d8155f8125e38d145d280331146b934c2bb7c842/src/slave/containerizer/mesos/isolators/xfs/disk.cpp#L294-L301, > > this condition can only be met for containers that were launched before > > enabling XFS-isolator? If so, should we use `WARNING` here?
Yep, I agree that `WARNING` seems appropriate here. > On Aug. 1, 2019, 2:15 p.m., Andrei Budnik wrote: > > src/slave/containerizer/mesos/isolators/xfs/disk.cpp > > Lines 425-426 (patched) > > <https://reviews.apache.org/r/71194/diff/1/?file=2158186#file2158186line425> > > > > Why doesn't `ephemeral_volumes` belonging to the `states` cover all > > directories? What is the case when we need this? I'm not fully convinced that overlayfs directories are guaranteed to be in `ContainerState` at recovery time. For example, [here](https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/containerizer.cpp#L1073) we might find that we have an unrecoverable container what won't get a container state, and [here](https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/containerizer.cpp#L1060), we might find the `ContainerState` erroneously missing the ephemeral volumes because the config could not be read. So I felt that we should follow the same strategy as for sandboxes and treat the on-disk state as the source of truth. The consequences of leaking a project ID here are most likely that we would assign a project ID that is in use to a new task and that task could be immediately out of quota. This window should be small since the overlay backend is cleaned up immediately, but it's best to avoid the possibility. > On Aug. 1, 2019, 2:15 p.m., Andrei Budnik wrote: > > src/slave/containerizer/mesos/provisioner/backends/overlay.cpp > > Lines 17 (patched) > > <https://reviews.apache.org/r/71194/diff/1/?file=2158188#file2158188line17> > > > > Move it to the line after `#include "linux/fs.hpp"` (closer to the > > `#include "slave/containerizer/mesos/provisioner/constants.hpp"`)? The counterpart header for the source file should come first since that helps to ensure that the header is self-contained. It's not really spelled out in the style guide, but the example does it: https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md#order-of-includes - James ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71194/#review217030 ----------------------------------------------------------- On Aug. 2, 2019, 2:27 a.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71194/ > ----------------------------------------------------------- > > (Updated Aug. 2, 2019, 2:27 a.m.) > > > Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu. > > > Bugs: MESOS-9900 > https://issues.apache.org/jira/browse/MESOS-9900 > > > Repository: mesos > > > Description > ------- > > Add support for labeling ephemeral volumes with the sandbox XFS > project ID. This makes changes to the container rootfs share the > same disk quota as the sandbox. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/xfs/disk.cpp > 646330c65b24aa28801ec99d7909db08a3e05c79 > src/slave/containerizer/mesos/provisioner/backends/overlay.hpp > 362e02172d2fd8e6e241fb6f5689f569ba74a0d1 > src/slave/containerizer/mesos/provisioner/backends/overlay.cpp > f2040cf36c601a13281a78ff844ebd41000a2d65 > > > Diff: https://reviews.apache.org/r/71194/diff/3/ > > > Testing > ------- > > sudo make check (Frdora 30) > > > Thanks, > > James Peach > >
