> On March 9, 2016, 6:03 p.m., Jiang Yan Xu wrote: > > src/slave/containerizer/mesos/isolators/disk/xfs.hpp, line 84 > > <https://reviews.apache.org/r/44342/diff/3/?file=1279807#file1279807line84> > > > > About the type: should it be `prid_t` which more accurately describes > > what it is? > > > > About the name: unless this naming convention in univeral in XFS can we > > spell it out as `projectId`?
Renamed project ID variables to ``projectId`` everywhere. ``prid_t`` is more correct than ``uint32_t``, however I don't want to pollute the global namespace with XFS types, which would be necessary if I used ``prod_t`` in the header. An alternative is to move the whole declaration of ``XfsDiskIsolatorProcess`` into the cpp file and just leave the static ``create()`` factory function in the header. I like this idea since is hides the implementation better and helps with compile times, but it is inconsistent with the rest of the code. > On March 9, 2016, 6:03 p.m., Jiang Yan Xu wrote: > > src/slave/containerizer/mesos/isolators/disk/xfs.cpp, line 17 > > <https://reviews.apache.org/r/44342/diff/3/?file=1279808#file1279808line17> > > > > Add a link that explains this? > > James Peach wrote: > A link to what? I described the problem in more detail. > On March 9, 2016, 6:03 p.m., Jiang Yan Xu wrote: > > src/slave/containerizer/mesos/isolators/disk/xfs.cpp, lines 163-165 > > <https://reviews.apache.org/r/44342/diff/3/?file=1279808#file1279808line163> > > > > This utility method itself doesn't care if the path is a directory or > > not, right? Nope. > On March 9, 2016, 6:03 p.m., Jiang Yan Xu wrote: > > src/slave/containerizer/mesos/isolators/disk/xfs.cpp, line 167 > > <https://reviews.apache.org/r/44342/diff/3/?file=1279808#file1279808line167> > > > > Can we use `os::stat::dev()` directly? I'd have to add support for ``DO_NOT_FOLLOW_SYMLINK``, which would be a separate commit and review request. Maybe do that after this one? > On March 9, 2016, 6:03 p.m., Jiang Yan Xu wrote: > > src/slave/containerizer/mesos/isolators/disk/xfs.cpp, lines 191-194 > > <https://reviews.apache.org/r/44342/diff/3/?file=1279808#file1279808line191> > > > > Why a union here? > > James Peach wrote: > The union is to avoid a ``reinterpret_cast`` later. Removed the union in favour of ``reinterpret_cast``. > On March 9, 2016, 6:03 p.m., Jiang Yan Xu wrote: > > src/slave/containerizer/mesos/isolators/disk/xfs.cpp, line 203 > > <https://reviews.apache.org/r/44342/diff/3/?file=1279808#file1279808line203> > > > > We are effectively not using the soft limits. Is it OK to not set them? IMHO it is kinder to set them both. That is consistent and doesn't leave anyone wondering whether they are different for a reason. > On March 9, 2016, 6:03 p.m., Jiang Yan Xu wrote: > > src/slave/containerizer/mesos/isolators/disk/xfs.cpp, line 456 > > <https://reviews.apache.org/r/44342/diff/3/?file=1279808#file1279808line456> > > > > The naming is inconsistent with the reset of xfs* methods but moreover, > > can we pull out things that aren't XFS specific to stout headers and group > > XFS methods under namespace `mesos::internal::slave::xfs` instead? In fact > > perhaps put them under `linux/xfs/`? The ``xfs*`` nomenclature is used only for functions that wrap the XFS system calls. Everything else is named after what it does. I'll work on separating the ``xfs*`` APIs into ``linux/xfs``. > On March 9, 2016, 6:03 p.m., Jiang Yan Xu wrote: > > src/slave/containerizer/mesos/isolators/disk/xfs.cpp, lines 489-493 > > <https://reviews.apache.org/r/44342/diff/3/?file=1279808#file1279808line489> > > > > This feels like a TODO. > > > > FWIW currently persistent volumes are not removed at all. We can try to > > make the behavior consistent with posix/disk and also enforce quota on them > > and update its logic later when we work on the ticket that handles > > persitent volume deletion. > > > > Otherwise such limitation needs to be prominently documentated. (e.g. > > in the class-level comments and user docs) with a TODO or JIRA. > > > > Let's evaluate what to do for now. Added the ``TODO``. I'd like to do volume support as a separate JIRA. > On March 9, 2016, 6:03 p.m., Jiang Yan Xu wrote: > > src/slave/containerizer/mesos/isolators/disk/xfs.cpp, line 513 > > <https://reviews.apache.org/r/44342/diff/3/?file=1279808#file1279808line513> > > > > What privledges does XFS isolator require? > > > > Looks to me some operations require CAP_SYS_ADMIN. We often just check > > if the user is root. Yes you need ``CAP_SYS_ADMIN``. > On March 9, 2016, 6:03 p.m., Jiang Yan Xu wrote: > > src/slave/containerizer/mesos/isolators/disk/xfs.cpp, line 531 > > <https://reviews.apache.org/r/44342/diff/3/?file=1279808#file1279808line531> > > > > Are we losing the last element of each range if the interval set is > > open on the right side? Right. For the range ``5000-1000``, the first project ID we would consume is ``5000`` and the last is ``9999``. - James ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44342/#review122156 ----------------------------------------------------------- On March 7, 2016, 6:38 p.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44342/ > ----------------------------------------------------------- > > (Updated March 7, 2016, 6:38 p.m.) > > > Review request for mesos, Jie Yu and Jiang Yan Xu. > > > Bugs: MESOS-4828 > https://issues.apache.org/jira/browse/MESOS-4828 > > > Repository: mesos > > > Description > ------- > > Track sandbox directory usage by dynamically assigning XFS project > quotas. We track a range of XFS project IDs, assigning a project ID > and a project quota to each sandbox as it is created. When the task > reaches the quota, writes will fail with EDQUOT, and the task will have > an opportunity to handle that. > > Quotas are not applied to volume resources since the isolator interface > has no insight into the volume lifecycle. Thus it is not currently > possible to accurately assign and reclaim project IDs. > > If LOW is the lower bound of the project ID range and HIGH is the upper > bound, you can show the currently allocated project quotas using the > xfs_quota command: > > $ xfs_quota -x -c "report -a -n -L LOW -U HIGH" > > To show the project ID assigned to the file PATH, use the xfs_io command: > > $ xfs_io -r -c stat PATH > > > Diffs > ----- > > configure.ac a20382e8d425eb297492a6e6c2c75ea59be097c2 > docs/configuration.md 305ba2c801c2060db6dcb4ef83c1043aaa7d520c > docs/mesos-containerizer.md 15fb5bdbe74e059614b8948108f32cd04b623305 > src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 > src/slave/containerizer/mesos/containerizer.cpp > af3ff5750649497d8852b4761c78d4cae5455a02 > src/slave/containerizer/mesos/isolators/disk/xfs.hpp PRE-CREATION > src/slave/containerizer/mesos/isolators/disk/xfs.cpp PRE-CREATION > src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 > src/slave/flags.cpp 6e3fd69c06eefd40bc0e5c222ea72f34144c5534 > > Diff: https://reviews.apache.org/r/44342/diff/ > > > Testing > ------- > > Manual testing on Fedora 23 w/ XFS. Make check on Fedora and OS X. > > > Thanks, > > James Peach > >
