> On Nov. 14, 2017, 9:32 a.m., James Peach wrote: > > This is looking pretty good. > > > > You should be able to write a test for this using the > > [ROOT_XFS_TestBase](https://github.com/apache/mesos/blob/master/src/tests/containerizer/xfs_quota_tests.cpp) > > fixture. Probably a reasonable approach would be to make a XFS filesystem > > with and without `d_type` support and verify that `Provisioner::create` > > succeeds or fails.
Thank you for the quick and informative review, James. I was planning to add a unit test. But the problem is that it would require XFS installed. We will have to add a new configuration flag which seems to overkill. Thus I only did manual tests following a similar procedure. What do you think? > On Nov. 14, 2017, 9:32 a.m., James Peach wrote: > > src/slave/containerizer/mesos/provisioner/provisioner.cpp > > Lines 154 (patched) > > <https://reviews.apache.org/r/63652/diff/3/?file=1891098#file1891098line154> > > > > The caller is responsible for logging the error. Let's be consistent > > and remove this `LOG`. I am worrying that, in the case of default backend selection, if the underlying, say XFS, does not support `d_type`, the provisioner will just silently eat the error message (near line 275) and fall back to a less ideal backend. The user would have no clue of what happened. Putting the warning message here is a simple thing that we can do to give some hint to the user. What do you think? - Meng ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63652/#review190962 ----------------------------------------------------------- On Nov. 14, 2017, 1:30 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63652/ > ----------------------------------------------------------- > > (Updated Nov. 14, 2017, 1:30 p.m.) > > > Review request for mesos, Gilbert Song and James Peach. > > > Bugs: MESOS-8121 > https://issues.apache.org/jira/browse/MESOS-8121 > > > Repository: mesos > > > Description > ------- > > In unified containerizer, the d_type cannot be 1 > if we are using the overlay fs backend. > Raise error if user specifies overlayfs as backend. > Fallback to other backends in the default case and > raise a warning. > > > Diffs > ----- > > src/linux/fs.hpp cbc8bf79083ce2bc34fa698808eaf92764a577a9 > src/linux/fs.cpp 40c31a3ad2ccbeb59868c5e2d7b1809c613da8de > src/slave/containerizer/mesos/provisioner/provisioner.cpp > 450a3b32d69d2882973a6ed4e94e169a0256056b > > > Diff: https://reviews.apache.org/r/63652/diff/4/ > > > Testing > ------- > > Manually tested slave creation with default and overlayfs backends on XFS > based work_dir with different ftype mount options. > > > Thanks, > > Meng Zhu > >
