You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Akash Gupta <ak...@hotmail.com> on 2017/11/16 18:18:40 UTC
Review Request 63862: Ported docker_tests.cpp to Windows.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63862/
-----------------------------------------------------------
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 (updated)
-----
src/tests/containerizer/docker_tests.cpp 5cabf5a0b0164f9923008677c58806c8931cbc8d
Diff: https://reviews.apache.org/r/63862/diff/1/
Testing (updated)
-------
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
Re: Review Request 63862: Ported docker_tests.cpp to Windows.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63862/#review191248
-----------------------------------------------------------
PASS: Mesos patch 63862 was successfully built and tested.
Reviews applied: `['63859', '63860', '63861', '63862']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63862
- Mesos Reviewbot Windows
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
>
>
Re: Review Request 63862: Windows: Ported docker_tests.cpp.
Posted by Akash Gupta <ak...@hotmail.com>.
> On Dec. 1, 2017, 2:33 a.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 62 (patched)
> > <https://reviews.apache.org/r/63862/diff/2/?file=1901750#file1901750line62>
> >
> > We should probably comment as to why this doesn't use PowerShell (nanoserver image doesn't have it).
> >
> > Especially considering that `SLEEP_COMMAND` exists elsewhere, someone could easily assume they should change it.
>
> Akash Gupta wrote:
> There's a nanoserver image at `microsoft/powershell`. I can use that.
Added comment on why `SLEEP_COMMAND` can't be used.
> On Dec. 1, 2017, 2:33 a.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 107-111 (patched)
> > <https://reviews.apache.org/r/63862/diff/2/?file=1901750#file1901750line107>
> >
> > Since this could end up being really slow and make the tests appear hung, I think we should `LOG(WARNING) << "Downloading Docker image...";` or something to that effect.
> >
> > (So much better than Hours(1) though.)
Added.
> On Dec. 1, 2017, 2:33 a.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 121-128 (patched)
> > <https://reviews.apache.org/r/63862/diff/2/?file=1901750#file1901750line121>
> >
> > I see where this is quite useful, but perhaps the name could be a bit mores descriptive? I was thinking `from_root_dir` maybe?
Changed.
> On Dec. 1, 2017, 2:33 a.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Line 170 (original), 232-237 (patched)
> > <https://reviews.apache.org/r/63862/diff/2/?file=1901750#file1901750line232>
> >
> > Saw your reply explaining this on the review. I think we need to add a comment with the same explanation.
Yeah. Added an explanation
- Akash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63862/#review192436
-----------------------------------------------------------
On Dec. 7, 2017, 12:05 p.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63862/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2017, 12:05 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Jie Yu, John Kordich, and Joseph Wu.
>
>
> 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 5cabf5a0b0164f9923008677c58806c8931cbc8d
>
>
> Diff: https://reviews.apache.org/r/63862/diff/3/
>
>
> 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
>
>
Re: Review Request 63862: Windows: Ported docker_tests.cpp.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Nov. 30, 2017, 6:33 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Line 318 (original), 391-428 (patched)
> > <https://reviews.apache.org/r/63862/diff/2/?file=1901750#file1901750line391>
> >
> > LGTM though another maintainer may want to take a look.
The current iteration of this this test (revision 5) looks fine. (Marking as Fixed)
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63862/#review192436
-----------------------------------------------------------
On Jan. 5, 2018, 10:33 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63862/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2018, 10:33 a.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
>
>
Re: Review Request 63862: Windows: Ported docker_tests.cpp.
Posted by Akash Gupta <ak...@hotmail.com>.
> On Dec. 1, 2017, 2:33 a.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 62 (patched)
> > <https://reviews.apache.org/r/63862/diff/2/?file=1901750#file1901750line62>
> >
> > We should probably comment as to why this doesn't use PowerShell (nanoserver image doesn't have it).
> >
> > Especially considering that `SLEEP_COMMAND` exists elsewhere, someone could easily assume they should change it.
There's a nanoserver image at `microsoft/powershell`. I can use that.
> On Dec. 1, 2017, 2:33 a.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/docker_tests.cpp
> > Lines 63 (patched)
> > <https://reviews.apache.org/r/63862/diff/2/?file=1901750#file1901750line63>
> >
> > Is this the canonical path it should use on Windows? Seems like we could choose an arbitrary path.
It's an arbitrary path. I just chose it because the Linux test uses `/mnt/sandbox/mesos`
- Akash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63862/#review192436
-----------------------------------------------------------
On Dec. 1, 2017, 2:34 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63862/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 2:34 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Jie Yu, John Kordich, and Joseph Wu.
>
>
> 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 5cabf5a0b0164f9923008677c58806c8931cbc8d
>
>
> Diff: https://reviews.apache.org/r/63862/diff/2/
>
>
> 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
>
>
Re: Review Request 63862: Windows: Ported docker_tests.cpp.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63862/#review192436
-----------------------------------------------------------
src/tests/containerizer/docker_tests.cpp
Lines 62 (patched)
<https://reviews.apache.org/r/63862/#comment270582>
We should probably comment as to why this doesn't use PowerShell (nanoserver image doesn't have it).
Especially considering that `SLEEP_COMMAND` exists elsewhere, someone could easily assume they should change it.
src/tests/containerizer/docker_tests.cpp
Lines 63 (patched)
<https://reviews.apache.org/r/63862/#comment270583>
Is this the canonical path it should use on Windows? Seems like we could choose an arbitrary path.
src/tests/containerizer/docker_tests.cpp
Lines 107-111 (patched)
<https://reviews.apache.org/r/63862/#comment270584>
Since this could end up being really slow and make the tests appear hung, I think we should `LOG(WARNING) << "Downloading Docker image...";` or something to that effect.
(So much better than Hours(1) though.)
src/tests/containerizer/docker_tests.cpp
Lines 121-128 (patched)
<https://reviews.apache.org/r/63862/#comment270593>
I see where this is quite useful, but perhaps the name could be a bit mores descriptive? I was thinking `from_root_dir` maybe?
src/tests/containerizer/docker_tests.cpp
Line 170 (original), 232-237 (patched)
<https://reviews.apache.org/r/63862/#comment270585>
Saw your reply explaining this on the review. I think we need to add a comment with the same explanation.
src/tests/containerizer/docker_tests.cpp
Line 318 (original), 391-428 (patched)
<https://reviews.apache.org/r/63862/#comment270591>
LGTM though another maintainer may want to take a look.
src/tests/containerizer/docker_tests.cpp
Lines 624-630 (patched)
<https://reviews.apache.org/r/63862/#comment270592>
Ah the woes of new technology.
- Andrew Schwartzmeyer
On Nov. 27, 2017, 9:45 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63862/
> -----------------------------------------------------------
>
> (Updated Nov. 27, 2017, 9:45 a.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 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 5cabf5a0b0164f9923008677c58806c8931cbc8d
>
>
> Diff: https://reviews.apache.org/r/63862/diff/2/
>
>
> 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
>
>
Re: Review Request 63862: Windows: Ported docker_tests.cpp.
Posted by Akash Gupta <ak...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63862/
-----------------------------------------------------------
(Updated Nov. 27, 2017, 5:45 p.m.)
Review request for mesos, Andrew Schwartzmeyer and John Kordich.
Changes
-------
Rebased + addressed feedback. I also changed the image to use the nanserver image instead since it is 50x smaller. However, the nanoserver image doesn't work properly with volume mounts in process isolation mode, so I changed the mount tests to use hyper-v isolation, which works fine.
Summary (updated)
-----------------
Windows: Ported docker_tests.cpp.
Bugs: MESOS-7342
https://issues.apache.org/jira/browse/MESOS-7342
Repository: mesos
Description (updated)
-------
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 (updated)
-----
src/tests/containerizer/docker_tests.cpp 5cabf5a0b0164f9923008677c58806c8931cbc8d
Diff: https://reviews.apache.org/r/63862/diff/2/
Changes: https://reviews.apache.org/r/63862/diff/1-2/
Testing (updated)
-------
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
Re: Review Request 63862: Ported docker_tests.cpp to Windows.
Posted by Akash Gupta <ak...@hotmail.com>.
> 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
>
>
Re: Review Request 63862: Windows: Ported docker_tests.cpp.
Posted by Akash Gupta <ak...@hotmail.com>.
> 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?
>
> Akash Gupta wrote:
> 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.
Resolved by having bridge and host = nat.
> 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`?
>
> Akash Gupta wrote:
> Yeah I can remove the loops with `std::algorithm`
eh, I think keeping the loop reads the best in this case.
> 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?
>
> Akash Gupta wrote:
> 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?
after discussing, we agreed to change to TEST_F_TEMP_DISABLED_ON_WINDOWS.
> 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?
>
> Akash Gupta wrote:
> 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?
after discussing, we agreed to change to 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. 27, 2017, 5:45 p.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63862/
> -----------------------------------------------------------
>
> (Updated Nov. 27, 2017, 5:45 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 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 5cabf5a0b0164f9923008677c58806c8931cbc8d
>
>
> Diff: https://reviews.apache.org/r/63862/diff/2/
>
>
> 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
>
>
Re: Review Request 63862: Windows: Ported docker_tests.cpp.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Nov. 16, 2017, 11:25 a.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...
>
> Akash Gupta wrote:
> Hopefully it can be removed when the nanoserver image works since boots up way faster...
Glad it worked out!
> On Nov. 16, 2017, 11:25 a.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()`.
>
> Akash Gupta wrote:
> It can't fail since validate = false, so it just calls a constructor.
Fair enough.
> On Nov. 16, 2017, 11:25 a.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?
>
> Akash Gupta wrote:
> 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.
Ah, we should at least comment this in.
> On Nov. 16, 2017, 11:25 a.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.)
>
> Akash Gupta wrote:
> `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.
Gotcha. Let's get it in a comment.
> On Nov. 16, 2017, 11:25 a.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`?
>
> Akash Gupta wrote:
> 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.
Gotcha.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63862/#review191223
-----------------------------------------------------------
On Nov. 27, 2017, 9:45 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63862/
> -----------------------------------------------------------
>
> (Updated Nov. 27, 2017, 9:45 a.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 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 5cabf5a0b0164f9923008677c58806c8931cbc8d
>
>
> Diff: https://reviews.apache.org/r/63862/diff/2/
>
>
> 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
>
>
Re: Review Request 63862: Ported docker_tests.cpp to Windows.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63862/#review191223
-----------------------------------------------------------
src/tests/containerizer/docker_tests.cpp
Lines 62 (patched)
<https://reviews.apache.org/r/63862/#comment268887>
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.
src/tests/containerizer/docker_tests.cpp
Lines 67 (patched)
<https://reviews.apache.org/r/63862/#comment268889>
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...
src/tests/containerizer/docker_tests.cpp
Lines 79-82 (patched)
<https://reviews.apache.org/r/63862/#comment268891>
Is this assumed not fail? I don't know, but if it can fail, we should `CHECK_SOME` it before `get()`.
src/tests/containerizer/docker_tests.cpp
Lines 84-85 (patched)
<https://reviews.apache.org/r/63862/#comment268894>
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.
src/tests/containerizer/docker_tests.cpp
Lines 90-91 (patched)
<https://reviews.apache.org/r/63862/#comment268897>
Jesus...
src/tests/containerizer/docker_tests.cpp
Lines 121-124 (patched)
<https://reviews.apache.org/r/63862/#comment268896>
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?
src/tests/containerizer/docker_tests.cpp
Lines 125 (patched)
<https://reviews.apache.org/r/63862/#comment268898>
Nit: indentation (I put clang-format up on #windows).
src/tests/containerizer/docker_tests.cpp
Lines 130-133 (patched)
<https://reviews.apache.org/r/63862/#comment268899>
Nit: indentation.
src/tests/containerizer/docker_tests.cpp
Lines 231 (patched)
<https://reviews.apache.org/r/63862/#comment268900>
We're not checking the status code?
src/tests/containerizer/docker_tests.cpp
Lines 316 (patched)
<https://reviews.apache.org/r/63862/#comment268901>
Ditto.
src/tests/containerizer/docker_tests.cpp
Lines 399-401 (patched)
<https://reviews.apache.org/r/63862/#comment268906>
Would this read better with an `all_of`?
src/tests/containerizer/docker_tests.cpp
Lines 400 (patched)
<https://reviews.apache.org/r/63862/#comment268903>
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.)
src/tests/containerizer/docker_tests.cpp
Lines 405-412 (patched)
<https://reviews.apache.org/r/63862/#comment268904>
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`.
src/tests/containerizer/docker_tests.cpp
Lines 407 (patched)
<https://reviews.apache.org/r/63862/#comment268908>
Ditto on the "/".
src/tests/containerizer/docker_tests.cpp
Lines 417 (patched)
<https://reviews.apache.org/r/63862/#comment268907>
Oh I see, actually I think you want `find_if` since we're looking for that particular `containerName`.
src/tests/containerizer/docker_tests.cpp
Lines 420 (patched)
<https://reviews.apache.org/r/63862/#comment268910>
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.
src/tests/containerizer/docker_tests.cpp
Lines 421 (patched)
<https://reviews.apache.org/r/63862/#comment268909>
Ditto on the "/".
src/tests/containerizer/docker_tests.cpp
Lines 422 (patched)
<https://reviews.apache.org/r/63862/#comment268911>
Sweet.
src/tests/containerizer/docker_tests.cpp
Lines 611 (patched)
<https://reviews.apache.org/r/63862/#comment268913>
Should we get an actual Windows temp directory instead of `C:\tmp`?
src/tests/containerizer/docker_tests.cpp
Lines 578-624 (original), 703-752 (patched)
<https://reviews.apache.org/r/63862/#comment268915>
Wait, why did we remove it from the list of skipped tests?
src/tests/containerizer/docker_tests.cpp
Line 582 (original), 708-709 (patched)
<https://reviews.apache.org/r/63862/#comment268914>
Nit: Join these lines again.
src/tests/containerizer/docker_tests.cpp
Lines 629-630 (original), 757-758 (patched)
<https://reviews.apache.org/r/63862/#comment268916>
Nit: Join the lines again if it's < 80.
src/tests/containerizer/docker_tests.cpp
Line 642 (original), 770-774 (patched)
<https://reviews.apache.org/r/63862/#comment268917>
Oh, nit and for elsewhere: `path::join` is preferably to hard coding this.
src/tests/containerizer/docker_tests.cpp
Line 652 (original), 784-788 (patched)
<https://reviews.apache.org/r/63862/#comment268918>
`/mnt/mesos/sandbox/` is now a value in a variable somewhere, isn't it?
src/tests/containerizer/docker_tests.cpp
Lines 790-919 (original), 926-1058 (patched)
<https://reviews.apache.org/r/63862/#comment268919>
Why are these being compiled out?
src/tests/containerizer/docker_tests.cpp
Lines 924-925 (original), 1063-1064 (patched)
<https://reviews.apache.org/r/63862/#comment268920>
Nit: join.
- Andrew Schwartzmeyer
On Nov. 16, 2017, 10:18 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63862/
> -----------------------------------------------------------
>
> (Updated Nov. 16, 2017, 10:18 a.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
>
>