----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44948/#review126845 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/xfs/disk.hpp (line 33) <https://reviews.apache.org/r/44948/#comment190019> Doesn't look like the utils header is used in this header? Otherwise add a blank line above. src/slave/containerizer/mesos/isolators/xfs/disk.hpp (line 100) <https://reviews.apache.org/r/44948/#comment190020> const Flags flags; src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 57 - 58) <https://reviews.apache.org/r/44948/#comment189960> We should probably also validate the upper bound of the ranges because they are `uint64`. ``` static Try<IntervalSet<prid_t>> getIntervalSet( const Value::Ranges& ranges) ``` ? src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 112 - 114) <https://reviews.apache.org/r/44948/#comment189938> There is also the `uid.isNone()` case. We can just ``` if (!user.isSome()) { return Error("Failed to determine user: " + (uid.isError() ? uid.error() : "uid not found")); } ``` src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 140 - 144) <https://reviews.apache.org/r/44948/#comment189953> I suggested this earlier and this was marked as resolved: If the utils provdes ``` Option<Error> validateProjectIds(IntervalSet<prid_t>); ``` Then the isolator doesn't need to know about `NON_PROJECT_ID` anymore and it can stay as a constant in utils.cpp. src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 172 - 199) <https://reviews.apache.org/r/44948/#comment190032> We can consolidate this with the orphan case because in both cases we need to do all of these to read its project ID and construct an `info`. src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 216 - 217) <https://reviews.apache.org/r/44948/#comment189977> This fits in one line. src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 222) <https://reviews.apache.org/r/44948/#comment190044> IMO it's pretty clean and explicit to consolicate the recovery for both live containers and known orphans if we add comments here to make it clear that ``` // We recover the sandboxes for both live containers and known orphans // and only clean up the unknown orphans here. ``` src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 231 - 234) <https://reviews.apache.org/r/44948/#comment190041> This case shouldn't happen anymore if we consolidate. We can do a CHECK here. src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 237) <https://reviews.apache.org/r/44948/#comment190040> In the code above you had ``` // We should always be able to get a project ID. Maybe the directory // is not on an XFS filesystem, or something equally fatal happened. ``` And you return a failure. I think this applies here as well regarless of whether the sandbox is alive or known/unkown orphans because this indicates problems that affect more than just this one container. We should return a failure here as well. src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 238 - 239) <https://reviews.apache.org/r/44948/#comment190018> Single quote the sandbox. src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 239) <https://reviews.apache.org/r/44948/#comment190039> Move one space to the right. src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 243) <https://reviews.apache.org/r/44948/#comment190035> Let's move over the comments you had above. ``` // If there is no project ID, don't worry about it. This can happen the // first time an operator enables the XFS disk isolator and we recover a // set of containers that we did not isolate. ``` src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 253) <https://reviews.apache.org/r/44948/#comment190053> I don't think we need to wait for the result of the unknown orphan recovery because the cleanup could be expensive as it processes every file in the sandbox. We can call `cleanup(containerId)` for every unknown orphan without collecting them. At the bottom we just `return Nothing()`. src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 258) <https://reviews.apache.org/r/44948/#comment189942> 2 space indentation here. src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 281 - 286) <https://reviews.apache.org/r/44948/#comment190256> This is sort of safe in our case because we expect an empty sandbox. In a general case `xfs::setProjectId()` could partially fail which makes it unsafe to return the project ID. I think we can just do a hard CHECK before calling to make sure the directory is empty. If no such method is directory usable right now, add a TODO and let's add one to stout. src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 332 - 333) <https://reviews.apache.org/r/44948/#comment190201> The 'resources' here can be long as it includes all resources. We probably don't need to log here as we are already logging all outcomes. src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 338 - 340) <https://reviews.apache.org/r/44948/#comment190203> We can take care of this in a subsequent review but let's insert a LOG(WARNING) here and add a TODO: semantially we should set the quota to 0 but given XFS' enforceability limitation it's going to be limited at 1 basic block. src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 436 - 440) <https://reviews.apache.org/r/44948/#comment190204> We don't need to dispatch again. `_cleanup`'s content can just be moved here. - Jiang Yan Xu On April 4, 2016, 10:28 a.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44948/ > ----------------------------------------------------------- > > (Updated April 4, 2016, 10:28 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 > ----- > > src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 > src/slave/containerizer/mesos/containerizer.cpp > a5dd22380066aa85de04d485052084e2629681c0 > src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION > src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION > src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION > src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION > src/slave/flags.hpp d0c606eea74e1a2e69067c43a267047e65a22a04 > src/slave/flags.cpp 0551ec334c6747507bf7bb068d27d67f3fdd6c83 > > Diff: https://reviews.apache.org/r/44948/diff/ > > > Testing > ------- > > Make check. Manual testing. Tests in subsequent patches. > > > Thanks, > > James Peach > >
