Re: Review Request 66173: Add test for new `disk/xfs` kill functionality.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66173/#review201956
---



Patch looks great!

Reviews applied: [66001, 66173]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On April 20, 2018, 8:49 p.m., Harold Dost wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66173/
> ---
> 
> (Updated April 20, 2018, 8:49 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/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Harold Dost
> 
>

Re: Review Request 66173: Add test for new `disk/xfs` kill functionality.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66173/#review201940
---


Ship it!




Looks good. There's a coupld of small nits that I'll fix as I commit

- James Peach


On April 20, 2018, 6:49 p.m., Harold Dost wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66173/
> ---
> 
> (Updated April 20, 2018, 6:49 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/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Harold Dost
> 
>

Re: Review Request 66173: Add test for new `disk/xfs` kill functionality.

[Mesos Reviewbot Windows]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66173/#review201647
---



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['66001', '66173']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66173

Relevant logs:

- 
[mesos-tests-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66173/logs/mesos-tests-cmake-stdout.log):

```
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3426):
 warning C4996: 'strerror': This function or variable may be unsafe. Consider 
using strerror_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3500):
 warning C4996: 'inet_ntoa': Use inet_ntop() or InetNtop() instead or define 
_WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3501):
 warning C4996: 'sprintf': This function or variable may be unsafe. Consider 
using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3479):
 warning C4101: 'addrstr': unreferenced local variable 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\recordio.c(170):
 warning C4267: '=': conversion from 'size_t' to 'int32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\mt_adaptor.c(496):
 warning C4244: '=': conversion from 'time_t' to 'int32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(256):
 warning C4090: 'function': different 'const' qualifiers 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(166):
 warning C4716: 'pthread_cond_broadcast': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205):
 warning C4716: 'pthread_cond_wait': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 

Re: Review Request 66173: Add test for new `disk/xfs` kill functionality.

[Harold Dost]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66173/
---

(Updated April 20, 2018, 6:49 p.m.)


Review request for mesos and James Peach.


Changes
---

Update based on changes.


Bugs: MESOS-6575
https://issues.apache.org/jira/browse/MESOS-6575


Repository: mesos


Description
---

MESOS-6575


Diffs (updated)
-

  src/tests/containerizer/xfs_quota_tests.cpp 
64c3e1c3f0bc435897626cb0a13bc19c7cb1a4fe 


Diff: https://reviews.apache.org/r/66173/diff/4/

Changes: https://reviews.apache.org/r/66173/diff/3-4/


Testing
---


Thanks,

Harold Dost

Re: Review Request 66173: Add test for new `disk/xfs` kill functionality.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66173/#review200931
---



I'm getting a test failure on `ROOT_XFS_QuotaTest.RecoverOldContainers` with 
these patches enabled. When we restart the agent with the XFS isolator enabled, 
we skip the running task because there isn't a project ID on the sandbox. Then 
the containerizer tells us to watch that container ID but we return a failure 
because we don't know anything about it. That failure causes the containerizer 
to kill the task. So we need to update `XfsDiskIsolatorProcess::recover` to 
store a placeholder to handle this case.

- James Peach


On April 4, 2018, 5:41 p.m., Harold Dost wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66173/
> ---
> 
> (Updated April 4, 2018, 5:41 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/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Harold Dost
> 
>

Re: Review Request 66173: Add test for new `disk/xfs` kill functionality.

[Mesos Reviewbot Windows]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66173/#review200474
---



PASS: Mesos patch 66173 was successfully built and tested.

Reviews applied: `['66001', '66173']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66173

- Mesos Reviewbot Windows


On April 4, 2018, 5:41 p.m., Harold Dost wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66173/
> ---
> 
> (Updated April 4, 2018, 5:41 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/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Harold Dost
> 
>

Re: Review Request 66173: Add test for new `disk/xfs` kill functionality.

[Harold Dost]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66173/
---

(Updated April 4, 2018, 5:41 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 (updated)
-

  src/tests/containerizer/xfs_quota_tests.cpp 
64c3e1c3f0bc435897626cb0a13bc19c7cb1a4fe 


Diff: https://reviews.apache.org/r/66173/diff/3/

Changes: https://reviews.apache.org/r/66173/diff/2-3/


Testing
---


Thanks,

Harold Dost

Re: Review Request 66173: Add test for new `disk/xfs` kill functionality.

[James Peach]

---
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)


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)


Double newline above here.



src/tests/containerizer/xfs_quota_tests.cpp
Lines 460 (patched)


Need to update this comment for accuracy.



src/tests/containerizer/xfs_quota_tests.cpp
Lines 469 (patched)


Newline before and after this to make it stand out.



src/tests/containerizer/xfs_quota_tests.cpp
Lines 498 (patched)


We can just sleep:
```
"dd if=/dev/zero of=file bs=1048576 count=2 && sleep 10"
```

and update the comment.



src/tests/containerizer/xfs_quota_tests.cpp
Lines 517 (patched)


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)


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)


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
> 
>

Re: Review Request 66173: Add test for new `disk/xfs` kill functionality.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66173/#review199794
---



In one of the basic quota tests (maybe ROOT_XFS_QuotaTest.QuotaGetSet) we 
should add a check to verify that the hard & soft limits get set correctly.

- 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
> 
>

Re: Review Request 66173: Add test for new `disk/xfs` kill functionality.

[Harold Dost]

---
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 (updated)
-

  src/tests/containerizer/xfs_quota_tests.cpp 
64c3e1c3f0bc435897626cb0a13bc19c7cb1a4fe 


Diff: https://reviews.apache.org/r/66173/diff/2/

Changes: https://reviews.apache.org/r/66173/diff/1-2/


Testing
---


Thanks,

Harold Dost

Re: Review Request 66173: Add test for new `disk/xfs` kill functionality.

[Mesos Reviewbot Windows]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66173/#review199554
---



PASS: Mesos patch 66173 was successfully built and tested.

Reviews applied: `['66001', '66173']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66173

- Mesos Reviewbot Windows


On March 20, 2018, 2:34 p.m., Harold Dost wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66173/
> ---
> 
> (Updated March 20, 2018, 2:34 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/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Harold Dost
> 
>

Review Request 66173: Add test for new `disk/xfs` kill functionality.

[Harold Dost]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66173/
---

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/1/


Testing
---


Thanks,

Harold Dost