----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66173/#review200057 -----------------------------------------------------------
Running the test, I get this failure: ``` ../../src/tests/containerizer/xfs_quota_tests.cpp:533: Failure Failed to wait 15secs for killedStatus ``` Which indicates that the task doesn't get killed. We should also add the following tests: 1. Verify that `flags.xfs_kill_containers` doesn't kill containers that don't violate there limits. You can do this by snsuring that you can kill the container after guaranteeing that at least one disk space usage check has elapsed (this is an argument for keeping the isolator `check` functio, since you can expect it to be called). 2. Add soft limit checks to QuotaGetSet. 3. Add soft limit checks to QuotaLimit. src/tests/containerizer/xfs_quota_tests.cpp Line 147 (original), 147 (patched) <https://reviews.apache.org/r/66173/#comment280684> Since we are now pausing the clock, we need to make sure that it is resumed so that the `losetup` exec works correctly: ``` // Make sure we resume the clock so that we can wait on the // `losetup` process. if (Clock::paused()) { Clock::resume(); } ``` src/tests/containerizer/xfs_quota_tests.cpp Lines 457 (patched) <https://reviews.apache.org/r/66173/#comment280679> Double newline above here. src/tests/containerizer/xfs_quota_tests.cpp Lines 460 (patched) <https://reviews.apache.org/r/66173/#comment280677> Need to update this comment for accuracy. src/tests/containerizer/xfs_quota_tests.cpp Lines 469 (patched) <https://reviews.apache.org/r/66173/#comment280678> Newline before and after this to make it stand out. src/tests/containerizer/xfs_quota_tests.cpp Lines 498 (patched) <https://reviews.apache.org/r/66173/#comment280670> We can just sleep: ``` "dd if=/dev/zero of=file bs=1048576 count=2 && sleep 100000" ``` and update the comment. src/tests/containerizer/xfs_quota_tests.cpp Lines 517 (patched) <https://reviews.apache.org/r/66173/#comment280682> So that you don't have to wait for the full check interval, you can advance the clock: ``` // Create TaskInfo ... Clock::pause(); // Expect TASK_RUNNING ... Clock::advance(flags.container_disk_watch_interval); Clock::settle(); Clock::resume(); // Expect more stuff ... ``` src/tests/containerizer/xfs_quota_tests.cpp Lines 524 (patched) <https://reviews.apache.org/r/66173/#comment280673> Here we should check that the resource in the limitation is what we expect: ``` EXPECT_EQ(TaskStatus::SOURCE_SLAVE, killedStatus->source()); EXPECT_EQ( TaskStatus::REASON_CONTAINER_LIMITATION_DISK, killedStatus->reason()); ASSERT_TRUE(killedStatus->has_limitation()) << JSON::protobuf(killedStatus.get()); Resources limit = Resources(killedStatus->limitation().resources()); // Expect that we were limited on a single disk resource that represents // the amount of disk that the task consumed. EXPECT_EQ(1u, limit.size()); EXPECT_SOME_EQ(Megabytes(2), limit.disk()); ``` src/tests/containerizer/xfs_quota_tests.cpp Lines 528 (patched) <https://reviews.apache.org/r/66173/#comment280680> Double newline after here. - James Peach On March 22, 2018, 2:11 p.m., Harold Dost wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66173/ > ----------------------------------------------------------- > > (Updated March 22, 2018, 2:11 p.m.) > > > Review request for mesos and James Peach. > > > Bugs: MESOS-6575 > https://issues.apache.org/jira/browse/MESOS-6575 > > > Repository: mesos > > > Description > ------- > > MESOS-6575 > > > Diffs > ----- > > src/tests/containerizer/xfs_quota_tests.cpp > 64c3e1c3f0bc435897626cb0a13bc19c7cb1a4fe > > > Diff: https://reviews.apache.org/r/66173/diff/2/ > > > Testing > ------- > > > Thanks, > > Harold Dost > >
