----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63652/#review190962 -----------------------------------------------------------
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. src/linux/fs.hpp Lines 180 (patched) <https://reviews.apache.org/r/63652/#comment268547> Let's spell this out for the casual reader: ``` ... supports the `d_type` field in `struct dirent`. `` src/linux/fs.hpp Lines 184 (patched) <https://reviews.apache.org/r/63652/#comment268544> Let's rename this to `dtypeSupported()` to be consistent with other APIs. src/linux/fs.cpp Lines 110 (patched) <https://reviews.apache.org/r/63652/#comment268549> You don't need the `Try` since there is an implicit conversion. We can also separate the variable declarations from clearing `errno` to improve readability: ``` bool result = true; struct dirent* entry; errno = 0; while ( ...) { ... } ``` src/linux/fs.cpp Lines 113 (patched) <https://reviews.apache.org/r/63652/#comment268548> Please add braces: ``` if (entry->d_type == DT_UNKNOWN) { result = false; break; } ``` src/slave/containerizer/mesos/provisioner/provisioner.cpp Lines 126 (patched) <https://reviews.apache.org/r/63652/#comment268550> This comment should be a full sentence. Let's capture your experience testing this. Consider: ``` // Check whether d_type is supported. We need to create a directory entry to probe this since `.` and `..` will be marked `DT_DIR` even if the filesystem doesn't otherwise have d_type support. ``` src/slave/containerizer/mesos/provisioner/provisioner.cpp Lines 127 (patched) <https://reviews.apache.org/r/63652/#comment268552> Can we give this a more descriptive name, e.g. `.probe` or `.dtype`? src/slave/containerizer/mesos/provisioner/provisioner.cpp Lines 146 (patched) <https://reviews.apache.org/r/63652/#comment268558> Let's be consistent about formatting the `Error` line wrapping. Everywhere else here is doing: ``` return Error( "foo" + "bar"); ^ 4-space indent ``` src/slave/containerizer/mesos/provisioner/provisioner.cpp Lines 153 (patched) <https://reviews.apache.org/r/63652/#comment268555> `due to missing d_type support`? Also, this will read `is not supported on this 'XFS' due to ...` which sounds awkward. How about: ``` return Error( "Backend '" + stringify(OVERLAY_BACKEND) + "' is not supported due to missing d_type support " + "on the underlying filesystem"); ``` src/slave/containerizer/mesos/provisioner/provisioner.cpp Lines 154 (patched) <https://reviews.apache.org/r/63652/#comment268554> The caller is responsible for logging the error. Let's be consistent and remove this `LOG`. - James Peach On Nov. 13, 2017, 10:03 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63652/ > ----------------------------------------------------------- > > (Updated Nov. 13, 2017, 10:03 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/3/ > > > Testing > ------- > > Manually tested slave creation with default and overlayfs backends on XFS > based work_dir with different ftype mount options. > > > Thanks, > > Meng Zhu > >
