mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Akash Gupta <akash-gu...@hotmail.com>
Subject Re: Review Request 63862: Windows: Ported docker_tests.cpp.
Date Wed, 17 Jan 2018 21:22:41 GMT


> On Jan. 10, 2018, 1:27 p.m., Alexander Rukletsov wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 139-153 (patched)
> > <https://reviews.apache.org/r/63862/diff/5/?file=1932789#file1932789line139>
> >
> >     Again, this is something we should likely do in a broader scope. Chances are,
someone else in a different place will try to solve the same problem.
> 
> Andrew Schwartzmeyer wrote:
>     Same as above. At the point of someone else solving the same problem, then it would
make sense to move the code into a more general purpose location and reuse it. But we shouldn't
do it until then, because otherwise we're just guessing at future technical needs.
> 
> Akash Gupta wrote:
>     This is specific to Docker and only for test code that checks the container exit
value, which is onlt used here. So, I don't think other places will ever use this.
> 
> Andrew Schwartzmeyer wrote:
>     I was talking with Alex, and while I don't think `stout` is necessarily the right
place for this kind of code, there is potential clean up here:
>     
>     ```
>     src/tests/containerizer/docker_containerizer_tests.cpp
>     1288:  AWAIT_EXPECT_WEXITSTATUS_EQ(128 + SIGKILL, orphanRun);
>     1425:  AWAIT_EXPECT_WEXITSTATUS_EQ(128 + SIGKILL, orphanRun);
>     
>     src/tests/containerizer/ns_tests.cpp
>     101:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s->status());
>     
>     src/tests/containerizer/docker_tests.cpp
>     170:  AWAIT_EXPECT_WEXITSTATUS_EQ(128 + SIGKILL, status);
>     250:  AWAIT_EXPECT_WEXITSTATUS_EQ(128 + SIGKILL, status);
>     318:  AWAIT_EXPECT_WEXITSTATUS_EQ(128 + SIGKILL, run);
>     449:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, run);
>     528:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, run);
>     574:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, run);
>     623:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, run);
>     844:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, status);
>     918:  AWAIT_EXPECT_WEXITSTATUS_EQ(128 + SIGKILL, status);
>     
>     src/tests/containerizer/capabilities_tests.cpp
>     98:  AWAIT_EXPECT_WEXITSTATUS_NE(0, s->status());
>     122:  AWAIT_EXPECT_WEXITSTATUS_NE(0, s->status());
>     151:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s->status());
>     
>     3rdparty/libprocess/src/tests/reap_tests.cpp
>     98:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, status);
>     172:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, status);
>     
>     3rdparty/libprocess/include/process/gtest.hpp
>     623:#define AWAIT_EXPECT_WEXITSTATUS_EQ_FOR(expected, actual, duration)     \
>     627:#define AWAIT_EXPECT_WEXITSTATUS_EQ(expected, actual)                   \
>     628:  AWAIT_EXPECT_WEXITSTATUS_EQ_FOR(expected, actual, Seconds(15))
>     664:#define AWAIT_EXPECT_WEXITSTATUS_NE_FOR(expected, actual, duration)     \
>     668:#define AWAIT_EXPECT_WEXITSTATUS_NE(expected, actual)                   \
>     669:  AWAIT_EXPECT_WEXITSTATUS_NE_FOR(expected, actual, Seconds(15))
>     
>     3rdparty/libprocess/src/tests/subprocess_tests.cpp
>     72:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
>     249:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
>     264:  AWAIT_EXPECT_WEXITSTATUS_EQ(1, s.get().status());
>     323:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
>     344:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
>     371:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
>     411:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
>     442:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
>     465:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
>     497:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
>     531:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
>     562:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
>     599:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
>     617:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
>     700:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
>     763:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
>     789:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
>     818:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
>     847:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
>     879:  AWAIT_EXPECT_WEXITSTATUS_EQ(0, s.get().status());
>     ```
>     
>     Especially the ones that do `128 + SIGKILL`; just WTF is that even doing? No one
should have to go to [Stack Overflow](https://unix.stackexchange.com/a/99134) to understand
what an assertion in a test is doing.
>     
>     It may be reasonable to go to `include/process/gtest.hpp` and make this easier, with
`AWAIT_EXPECT_EXIT_SIGKILL(status)` (and like `AWAIT_EXPECT_EXIT_SUCCESS(status)` for `==
0`).

The weird exit code only happens to docker containers getting stopped/killed, so I think only
the `128+SIGKILL` ones need to be cleaned up in `docker_tests.cpp` and `docker_containerizer_tests.cpp`


- Akash


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


On Jan. 5, 2018, 6:33 p.m., Akash Gupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63862/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2018, 6:33 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston Kleiman,
Jie Yu, John Kordich, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-7342
>     https://issues.apache.org/jira/browse/MESOS-7342
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Ported the disabled tests to run on Windows. The following tests
> could not be ported due to Windows platform limitations and remain
> diabled:
>   - ROOT_DOCKER_MountRelativeContainerPath (can't mount volumes inside
>     sandbox).
>   - ROOT_DOCKER_NVIDIA_GPU_DeviceAllow (no GPU container support).
>   - ROOT_DOCKER_NVIDIA_GPU_InspectDevices (no GPU container support).
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/docker_tests.cpp 0ac4a79e03d5e11ead5a57a9749e26c20a1ddd57 
> 
> 
> Diff: https://reviews.apache.org/r/63862/diff/5/
> 
> 
> Testing
> -------
> 
> Windows mesos-test:
> [==========] 754 tests from 77 test cases ran. (270586 ms total)
> [  PASSED  ] 754 tests.
> 
> 
> Windows DockerTest only:
> [==========] 11 tests from 1 test case ran. (85617 ms total)
> [  PASSED  ] 11 tests.
> 
> Linux DockerTest (only 12 tests instead of 14, because I don't have Nvidia GPU):
> [==========] 12 tests from 1 test case ran. (12413 ms total)
> [  PASSED  ] 12 tests.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message