Re: Review Request 65624: Added fetcher tests for `stderr` output.

[Akash Gupta]

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


Ship it!




Ship It!

- Akash Gupta


On Feb. 20, 2018, 7:40 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65624/
> ---
> 
> (Updated Feb. 20, 2018, 7:40 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher is supposed to log its `stderr` output to a redirected file
> called `stderr` in the given sandbox directory. There previously existed
> a bug on Windows due to incorrect handle inheritance where this
> redirection failed silently, leaving the log empty. These unit tests
> assert that the correct content is logged to the `stderr` file for both
> a success and failure fetch scenario.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 1b4ba1d286f218731c13180e5189e91f90e3d63f 
> 
> 
> Diff: https://reviews.apache.org/r/65624/diff/1/
> 
> 
> Testing
> ---
> 
> Windows (Linux pending):
> ```
> [ RUN  ] FetcherTest.LogSuccessToStderr
> [   OK ] FetcherTest.LogSuccessToStderr (197 ms)
> [ RUN  ] FetcherTest.LogFailureToStderr
> [   OK ] FetcherTest.LogFailureToStderr (320 ms)
> ```
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65624: Added fetcher tests for `stderr` output.

[Andrew Schwartzmeyer]

> On Feb. 20, 2018, 11:43 a.m., Jeff Coffler wrote:
> > src/tests/fetcher_tests.cpp
> > Lines 132 (patched)
> > 
> >
> > These tests pretty much check the fetcher in a "stand-alone" 
> > environment. Could you add some tests to check within a container as well, 
> > particularly since fetcher logging is slightly different within a container?
> > 
> > Note that while fetcher output is slightly different within a 
> > container, fetcher itself isn't (docker is sharing the same directory). But 
> > we don't have any sort of tests to validate that - that I'm aware of anyway.
> 
> Andrew Schwartzmeyer wrote:
> This covers the affected scenarios; nothing changes running "in a 
> container" on Windows compared to this unit test with respect to output 
> redirection. When task is started with a URI to fetch, the fetcher is 
> directly invoked with the sandbox directory as done here to fetch the URI(s), 
> before the container starts. In essence, the fetcher is always run 
> "stand-alone".
> 
> Jeff Coffler wrote:
> That's correct for fetching. However, logging is somewhat different since 
> stderr/stdout logging is somewhat different under Docker vs. on a stand-alone 
> system. That's what I was referring to ...
> 
> Andrew Schwartzmeyer wrote:
> This fetcher is never used under Docker. What scenario are you imagining?
> 
> Akash Gupta wrote:
> There's a `DockerContainerizerTest.ROOT_DOCKER_Logs` test that creates a 
> Fetcher, launches a task and then reads the stdout and stderr files of the 
> container. Is that what you're thinking?

Talked with Jeff, he's looking for something different. Dropping.


- Andrew


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


On Feb. 20, 2018, 11:40 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65624/
> ---
> 
> (Updated Feb. 20, 2018, 11:40 a.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher is supposed to log its `stderr` output to a redirected file
> called `stderr` in the given sandbox directory. There previously existed
> a bug on Windows due to incorrect handle inheritance where this
> redirection failed silently, leaving the log empty. These unit tests
> assert that the correct content is logged to the `stderr` file for both
> a success and failure fetch scenario.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 1b4ba1d286f218731c13180e5189e91f90e3d63f 
> 
> 
> Diff: https://reviews.apache.org/r/65624/diff/1/
> 
> 
> Testing
> ---
> 
> Windows (Linux pending):
> ```
> [ RUN  ] FetcherTest.LogSuccessToStderr
> [   OK ] FetcherTest.LogSuccessToStderr (197 ms)
> [ RUN  ] FetcherTest.LogFailureToStderr
> [   OK ] FetcherTest.LogFailureToStderr (320 ms)
> ```
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65624: Added fetcher tests for `stderr` output.

[Andrew Schwartzmeyer]

> On Feb. 20, 2018, 11:43 a.m., Jeff Coffler wrote:
> > src/tests/fetcher_tests.cpp
> > Lines 132 (patched)
> > 
> >
> > These tests pretty much check the fetcher in a "stand-alone" 
> > environment. Could you add some tests to check within a container as well, 
> > particularly since fetcher logging is slightly different within a container?
> > 
> > Note that while fetcher output is slightly different within a 
> > container, fetcher itself isn't (docker is sharing the same directory). But 
> > we don't have any sort of tests to validate that - that I'm aware of anyway.
> 
> Andrew Schwartzmeyer wrote:
> This covers the affected scenarios; nothing changes running "in a 
> container" on Windows compared to this unit test with respect to output 
> redirection. When task is started with a URI to fetch, the fetcher is 
> directly invoked with the sandbox directory as done here to fetch the URI(s), 
> before the container starts. In essence, the fetcher is always run 
> "stand-alone".
> 
> Jeff Coffler wrote:
> That's correct for fetching. However, logging is somewhat different since 
> stderr/stdout logging is somewhat different under Docker vs. on a stand-alone 
> system. That's what I was referring to ...

This fetcher is never used under Docker. What scenario are you imagining?


- Andrew


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


On Feb. 20, 2018, 11:40 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65624/
> ---
> 
> (Updated Feb. 20, 2018, 11:40 a.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher is supposed to log its `stderr` output to a redirected file
> called `stderr` in the given sandbox directory. There previously existed
> a bug on Windows due to incorrect handle inheritance where this
> redirection failed silently, leaving the log empty. These unit tests
> assert that the correct content is logged to the `stderr` file for both
> a success and failure fetch scenario.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 1b4ba1d286f218731c13180e5189e91f90e3d63f 
> 
> 
> Diff: https://reviews.apache.org/r/65624/diff/1/
> 
> 
> Testing
> ---
> 
> Windows (Linux pending):
> ```
> [ RUN  ] FetcherTest.LogSuccessToStderr
> [   OK ] FetcherTest.LogSuccessToStderr (197 ms)
> [ RUN  ] FetcherTest.LogFailureToStderr
> [   OK ] FetcherTest.LogFailureToStderr (320 ms)
> ```
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65624: Added fetcher tests for `stderr` output.

[Jeff Coffler]

> On Feb. 20, 2018, 7:43 p.m., Jeff Coffler wrote:
> > src/tests/fetcher_tests.cpp
> > Lines 132 (patched)
> > 
> >
> > These tests pretty much check the fetcher in a "stand-alone" 
> > environment. Could you add some tests to check within a container as well, 
> > particularly since fetcher logging is slightly different within a container?
> > 
> > Note that while fetcher output is slightly different within a 
> > container, fetcher itself isn't (docker is sharing the same directory). But 
> > we don't have any sort of tests to validate that - that I'm aware of anyway.
> 
> Andrew Schwartzmeyer wrote:
> This covers the affected scenarios; nothing changes running "in a 
> container" on Windows compared to this unit test with respect to output 
> redirection. When task is started with a URI to fetch, the fetcher is 
> directly invoked with the sandbox directory as done here to fetch the URI(s), 
> before the container starts. In essence, the fetcher is always run 
> "stand-alone".

That's correct for fetching. However, logging is somewhat different since 
stderr/stdout logging is somewhat different under Docker vs. on a stand-alone 
system. That's what I was referring to ...


- Jeff


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


On Feb. 20, 2018, 7:40 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65624/
> ---
> 
> (Updated Feb. 20, 2018, 7:40 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher is supposed to log its `stderr` output to a redirected file
> called `stderr` in the given sandbox directory. There previously existed
> a bug on Windows due to incorrect handle inheritance where this
> redirection failed silently, leaving the log empty. These unit tests
> assert that the correct content is logged to the `stderr` file for both
> a success and failure fetch scenario.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 1b4ba1d286f218731c13180e5189e91f90e3d63f 
> 
> 
> Diff: https://reviews.apache.org/r/65624/diff/1/
> 
> 
> Testing
> ---
> 
> Windows (Linux pending):
> ```
> [ RUN  ] FetcherTest.LogSuccessToStderr
> [   OK ] FetcherTest.LogSuccessToStderr (197 ms)
> [ RUN  ] FetcherTest.LogFailureToStderr
> [   OK ] FetcherTest.LogFailureToStderr (320 ms)
> ```
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65624: Added fetcher tests for `stderr` output.

[Andrew Schwartzmeyer]

> On Feb. 20, 2018, 11:43 a.m., Jeff Coffler wrote:
> > src/tests/fetcher_tests.cpp
> > Lines 132 (patched)
> > 
> >
> > These tests pretty much check the fetcher in a "stand-alone" 
> > environment. Could you add some tests to check within a container as well, 
> > particularly since fetcher logging is slightly different within a container?
> > 
> > Note that while fetcher output is slightly different within a 
> > container, fetcher itself isn't (docker is sharing the same directory). But 
> > we don't have any sort of tests to validate that - that I'm aware of anyway.

This covers the affected scenarios; nothing changes running "in a container" on 
Windows compared to this unit test with respect to output redirection. When 
task is started with a URI to fetch, the fetcher is directly invoked with the 
sandbox directory as done here to fetch the URI(s), before the container 
starts. In essence, the fetcher is always run "stand-alone".


- Andrew


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


On Feb. 20, 2018, 11:40 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65624/
> ---
> 
> (Updated Feb. 20, 2018, 11:40 a.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher is supposed to log its `stderr` output to a redirected file
> called `stderr` in the given sandbox directory. There previously existed
> a bug on Windows due to incorrect handle inheritance where this
> redirection failed silently, leaving the log empty. These unit tests
> assert that the correct content is logged to the `stderr` file for both
> a success and failure fetch scenario.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 1b4ba1d286f218731c13180e5189e91f90e3d63f 
> 
> 
> Diff: https://reviews.apache.org/r/65624/diff/1/
> 
> 
> Testing
> ---
> 
> Windows (Linux pending):
> ```
> [ RUN  ] FetcherTest.LogSuccessToStderr
> [   OK ] FetcherTest.LogSuccessToStderr (197 ms)
> [ RUN  ] FetcherTest.LogFailureToStderr
> [   OK ] FetcherTest.LogFailureToStderr (320 ms)
> ```
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65624: Added fetcher tests for `stderr` output.

[Jeff Coffler]

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




src/tests/fetcher_tests.cpp
Lines 132 (patched)


These tests pretty much check the fetcher in a "stand-alone" environment. 
Could you add some tests to check within a container as well, particularly 
since fetcher logging is slightly different within a container?

Note that while fetcher output is slightly different within a container, 
fetcher itself isn't (docker is sharing the same directory). But we don't have 
any sort of tests to validate that - that I'm aware of anyway.


- Jeff Coffler


On Feb. 20, 2018, 7:40 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65624/
> ---
> 
> (Updated Feb. 20, 2018, 7:40 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher is supposed to log its `stderr` output to a redirected file
> called `stderr` in the given sandbox directory. There previously existed
> a bug on Windows due to incorrect handle inheritance where this
> redirection failed silently, leaving the log empty. These unit tests
> assert that the correct content is logged to the `stderr` file for both
> a success and failure fetch scenario.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 1b4ba1d286f218731c13180e5189e91f90e3d63f 
> 
> 
> Diff: https://reviews.apache.org/r/65624/diff/1/
> 
> 
> Testing
> ---
> 
> Windows (Linux pending):
> ```
> [ RUN  ] FetcherTest.LogSuccessToStderr
> [   OK ] FetcherTest.LogSuccessToStderr (197 ms)
> [ RUN  ] FetcherTest.LogFailureToStderr
> [   OK ] FetcherTest.LogFailureToStderr (320 ms)
> ```
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65624: Added fetcher tests for `stderr` output.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [65397, 65398, 65399, 65400, 65401, 65402, 65405, 65406, 
65407, 65408, 65409, 65465, 65403, 65467, 65574, 65469, 65624]

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 Feb. 13, 2018, 4:41 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65624/
> ---
> 
> (Updated Feb. 13, 2018, 4:41 a.m.)
> 
> 
> Review request for mesos, Akash Gupta and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher is supposed to log its `stderr` output to a redirected file
> called `stderr` in the given sandbox directory. There previously existed
> a bug on Windows due to incorrect handle inheritance where this
> redirection failed silently, leaving the log empty. These unit tests
> assert that the correct content is logged to the `stderr` file for both
> a success and failure fetch scenario.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 1b4ba1d286f218731c13180e5189e91f90e3d63f 
> 
> 
> Diff: https://reviews.apache.org/r/65624/diff/1/
> 
> 
> Testing
> ---
> 
> Windows (Linux pending):
> ```
> [ RUN  ] FetcherTest.LogSuccessToStderr
> [   OK ] FetcherTest.LogSuccessToStderr (197 ms)
> [ RUN  ] FetcherTest.LogFailureToStderr
> [   OK ] FetcherTest.LogFailureToStderr (320 ms)
> ```
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65624: Added fetcher tests for `stderr` output.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65624 was successfully built and tested.

Reviews applied: `['65403', '65467', '65574', '65469', '65624']`

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

- Mesos Reviewbot Windows


On Feb. 13, 2018, 4:41 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65624/
> ---
> 
> (Updated Feb. 13, 2018, 4:41 a.m.)
> 
> 
> Review request for mesos, Akash Gupta and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher is supposed to log its `stderr` output to a redirected file
> called `stderr` in the given sandbox directory. There previously existed
> a bug on Windows due to incorrect handle inheritance where this
> redirection failed silently, leaving the log empty. These unit tests
> assert that the correct content is logged to the `stderr` file for both
> a success and failure fetch scenario.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 1b4ba1d286f218731c13180e5189e91f90e3d63f 
> 
> 
> Diff: https://reviews.apache.org/r/65624/diff/1/
> 
> 
> Testing
> ---
> 
> Windows (Linux pending):
> ```
> [ RUN  ] FetcherTest.LogSuccessToStderr
> [   OK ] FetcherTest.LogSuccessToStderr (197 ms)
> [ RUN  ] FetcherTest.LogFailureToStderr
> [   OK ] FetcherTest.LogFailureToStderr (320 ms)
> ```
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>