Return-Path: X-Original-To: apmail-mesos-reviews-archive@minotaur.apache.org Delivered-To: apmail-mesos-reviews-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 7193118390 for ; Wed, 9 Mar 2016 18:03:36 +0000 (UTC) Received: (qmail 40505 invoked by uid 500); 9 Mar 2016 18:03:36 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 40479 invoked by uid 500); 9 Mar 2016 18:03:36 -0000 Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: reviews@mesos.apache.org Delivered-To: mailing list reviews@mesos.apache.org Received: (qmail 40462 invoked by uid 99); 9 Mar 2016 18:03:36 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 09 Mar 2016 18:03:36 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 1684A2AAA9C; Wed, 9 Mar 2016 18:03:35 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============7712272195563553870==" MIME-Version: 1.0 Subject: Re: Review Request 44342: XFS disk resource isolator. From: Jiang Yan Xu To: Jie Yu , Jiang Yan Xu Cc: James Peach , mesos Date: Wed, 09 Mar 2016 18:03:35 -0000 Message-ID: <20160309180335.3912.80706@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: Jiang Yan Xu X-ReviewGroup: mesos X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/44342/ X-Sender: Jiang Yan Xu References: <20160307183846.8639.56910@reviews.apache.org> In-Reply-To: <20160307183846.8639.56910@reviews.apache.org> X-ReviewBoard-Diff-For: src/slave/containerizer/mesos/isolators/disk/xfs.hpp X-ReviewBoard-Diff-For: src/slave/containerizer/mesos/isolators/disk/xfs.cpp Reply-To: Jiang Yan Xu X-ReviewRequest-Repository: mesos --===============7712272195563553870== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44342/#review122156 ----------------------------------------------------------- Partial review. Continuing on the latest revision. It would be great to have two sets of tests. One that tests the XFS operations and one that tests the isolator. In fact XFS operations and the isolator can be splitted into two reviews so each is verified by tests. configure.ac (line 923) Where is the corresponding AC_ARG_WITH? i.e. we should only do this if the user wants to use XFS. src/Makefile.am No need to kill this line as often two lines are used to delimit sections. src/slave/containerizer/mesos/isolators/disk/xfs.hpp (line 38) Kill the line. src/slave/containerizer/mesos/isolators/disk/xfs.hpp (line 84) 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`? src/slave/containerizer/mesos/isolators/disk/xfs.hpp (line 87) Ditto about uint32_t vs prid_t. src/slave/containerizer/mesos/isolators/disk/xfs.hpp (line 94) Kill the line. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 17) Add a link that explains this? src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 60) Kill the line. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 65) { on a new line. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 69) Add a line. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 70) This comment is probably unnecessary. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 83) Add a line. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 128) Can this and other xfs* methods be put under a xfs namespace instead? src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 154) This doesn't appear to be XFS specific. Can we pull this into stout? At lease name it such that it's clear. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 163 - 165) This utility method itself doesn't care if the path is a directory or not, right? src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 167) Can we use `os::stat::dev()` directly? src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 186) Use `devname.isError()`. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 190 - 194) Why list initialize it here? src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 191 - 194) Why a union here? src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 196 - 199) These require more comments. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 203) We are effectively not using the soft limits. Is it OK to not set them? src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 205 - 206) This indentation style is unconventional. Consider this: ``` if (::quotactl(QCMD(Q_XSETQLIM, PRJQUOTA), devname.get().c_str(), projid, ptr.c) == -1) { ... } ``` src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 211 - 212) It would be more useful to include containerIds in the log lines and they can be put inside the isolator. This applies to a couple of other log lines as well. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 365) This looks like a generic utility which we can leverage & add to ls.hpp. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 396) It's unclear why this additional check is needed here so can you comment on it? src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 456) 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/`? src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 470) Use `lambda::_1` instead. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 489 - 493) 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. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 513) What privledges does XFS isolator require? Looks to me some operations require CAP_SYS_ADMIN. We often just check if the user is root. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 531) Are we losing the last element of each range if the interval set is open on the right side? src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 553) We'll likely see this a lot when we first turn on this feature. LOG(WARNING) is better IMO. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 573) No biggie but it's more customary to add a `:` after TODO(jpeach). src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 573 - 576) We do need to handle orphans, as you've commented in `cleanup()`. `orphans` here are known to the containerizer so `cleanup()` will be called by it to remove the quota. However if there's not an entry for it in `infos`, `cleanup()` ignores it. The concern is that if the project ID of the orphan is not unassigned from the sandbox and the ID is not tracked by the isolator, next time it's used by a new task, it'll share the same quota with the orphan. - Jiang Yan Xu On March 7, 2016, 10:38 a.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44342/ > ----------------------------------------------------------- > > (Updated March 7, 2016, 10:38 a.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 > > --===============7712272195563553870==--