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: Ported docker_tests.cpp to Windows.
Date Fri, 17 Nov 2017 22:23:17 GMT


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 62 (patched)
> > <https://reviews.apache.org/r/63862/diff/1/?file=1893974#file1893974line62>
> >
> >     If you want to use it, we have a `SLEEP_COMMAND` macro that takes an int and
generates either `sleep x` on Linuxish and `powershell -noprofile -command start-sleep x`
on Windows. Using `ping` can be annoying because it emits output.

I'm hoping to eventually change the `windowsservercore` image to `nanoserver`. `nanoserver`
doesn't come with `powershell` by default, so I'm trying to avoid `powershell` in the docker
commands.


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 67 (patched)
> > <https://reviews.apache.org/r/63862/diff/1/?file=1893974#file1893974line67>
> >
> >     You could declare a `Seconds(30)` and not have to use  `Seconds(DOCKER_WAIT)`
below.
> >     
> >     It'd be nicer if we didn't have to adjust the timeouts at all...

Hopefully it can be removed when the nanoserver image works since boots up way faster...


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 79-82 (patched)
> > <https://reviews.apache.org/r/63862/diff/1/?file=1893974#file1893974line79>
> >
> >     Is this assumed not fail? I don't know, but if it can fail, we should `CHECK_SOME`
it before `get()`.

It can't fail since validate = false, so it just calls a constructor.


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 84-85 (patched)
> > <https://reviews.apache.org/r/63862/diff/1/?file=1893974#file1893974line84>
> >
> >     If you don't override it, you get this from the base class MesosTest : TemporaryDirectoryTest
[here](https://github.com/apache/mesos/blob/72752fc6deb8ebcbfbd5448dc599ef3774339d31/3rdparty/stout/include/stout/tests/utils.hpp#L41),
so you could call `MesosTest::SetUp()` and get the freebies from the base classes.

Nice. Ill try it. Although, I should probably just change it to `SetUpTestCase()`, so it just
executes this once before all the docker_tests run


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 90-91 (patched)
> > <https://reviews.apache.org/r/63862/diff/1/?file=1893974#file1893974line90>
> >
> >     Jesus...

Yeah most of the test is just downloading the image if it doesn't already exist lol.


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 121-124 (patched)
> > <https://reviews.apache.org/r/63862/diff/1/?file=1893974#file1893974line121>
> >
> >     Would it be possible to instead change the default network setting to `bridge`?
We may have talked about this, I don't remember. What should the default be if `host` doesn't
work on Windows?

I'm not sure. The default is set in the protobuf, so I'm not sure how the default can be changed
for Linux vs Windows. I guess we can have `host` AND `bridge` = `nat`, but that seems hacky.


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 231 (patched)
> > <https://reviews.apache.org/r/63862/diff/1/?file=1893974#file1893974line231>
> >
> >     We're not checking the status code?

On Hyper-V isolation, the status code is -1, which causes the `AWAIT_EXPECT_WEXITSTATUS_EQ`
macro to fail since it thinks that the process failed to start. On process isolation, the
status code is `STATUS_CONTROL_C_EXIT`. So, I just decided to check if there was a status
code, and then rely on `docker ps` to determine if the container actually stopped.


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 316 (patched)
> > <https://reviews.apache.org/r/63862/diff/1/?file=1893974#file1893974line316>
> >
> >     Ditto.

See previous comment


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 399-401 (patched)
> > <https://reviews.apache.org/r/63862/diff/1/?file=1893974#file1893974line399>
> >
> >     Would this read better with an `all_of`?

Yeah I can remove the loops with `std::algorithm`


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 400 (patched)
> > <https://reviews.apache.org/r/63862/diff/1/?file=1893974#file1893974line400>
> >
> >     Should we maybe do `strings::remove(PREFIX, "/", container.name)` for a safer
test? (Otherwise we're just assuming here that `container.name` will always be prefixed with
a slash.)

`docker inspect` will always have a / in front of the container name. This is because it's
more like a path, so /X means container X on the local machine. So, the output would actually
be wrong if it didn't have the slash prefix. The weird part is that Mesos `docker->ps()`
isn't actually `docker ps`, but more like `docker inspect` on all containers returned by `docker
ps (-a)`, so I check for the slash here.


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 405-412 (patched)
> > <https://reviews.apache.org/r/63862/diff/1/?file=1893974#file1893974line405>
> >
> >     Would this read better with an `any_of`? We're really just checking that at
least one of the elements of `containers.get()` matches `containerName`.

Should be `find_if`


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 407 (patched)
> > <https://reviews.apache.org/r/63862/diff/1/?file=1893974#file1893974line407>
> >
> >     Ditto on the "/".

See previous comment on why the "/" is expected.


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 420 (patched)
> > <https://reviews.apache.org/r/63862/diff/1/?file=1893974#file1893974line420>
> >
> >     This is checking that the ID is not an empty string, but the coment says we're
checking that it hasn't changed. We should probably save the ID earlier and check its value
matches here.

good catch. I will save the id and check.


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 611 (patched)
> > <https://reviews.apache.org/r/63862/diff/1/?file=1893974#file1893974line611>
> >
> >     Should we get an actual Windows temp directory instead of `C:\tmp`?

It's just an arbitary path in the container, so the specific place doesn't matter. The container
is going to be deleted anyway at the end of the test.


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 578-624 (original), 703-752 (patched)
> > <https://reviews.apache.org/r/63862/diff/1/?file=1893974#file1893974line703>
> >
> >     Wait, why did we remove it from the list of skipped tests?

I #ifdef'd out the tests that won't work due to limitations in Windows at the OS level based
off this JIRA issue https://issues.apache.org/jira/browse/MESOS-6392 . Should they still be
TEST_F_TEMP_DISABLED_ON_WINDOWS?


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Line 652 (original), 784-788 (patched)
> > <https://reviews.apache.org/r/63862/diff/1/?file=1893974#file1893974line784>
> >
> >     `/mnt/mesos/sandbox/` is now a value in a variable somewhere, isn't it?

yeah, I'll use path::join for these.


> On Nov. 16, 2017, 7:25 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 790-919 (original), 926-1058 (patched)
> > <https://reviews.apache.org/r/63862/diff/1/?file=1893974#file1893974line926>
> >
> >     Why are these being compiled out?

I #ifdef'd out the tests that won't work due to limitations in Windows at the OS level based
off this JIRA issue https://issues.apache.org/jira/browse/MESOS-6392 . Should they still be
TEST_F_TEMP_DISABLED_ON_WINDOWS?


- Akash


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


On Nov. 16, 2017, 6:18 p.m., Akash Gupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63862/
> -----------------------------------------------------------
> 
> (Updated Nov. 16, 2017, 6:18 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> 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 were
> simply #ifdef'd out:
>   - 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 5cabf5a0b0164f9923008677c58806c8931cbc8d 
> 
> 
> Diff: https://reviews.apache.org/r/63862/diff/1/
> 
> 
> 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