Re: Review Request 65127: Windows: Enabled docker health checks.

[Alexander Rukletsov]

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


Fix it, then Ship it!





src/checks/checker_process.cpp
Lines 1157 (patched)


maybe `dockerNetworkRunArgs` or `dockerNetworkRunCommand`?



src/checks/checker_process.cpp
Lines 1153-1154 (original), 1179-1181 (patched)


I'd move this explanation in front of the function declaration and here 
leave a shorter note.



src/checks/checker_process.cpp
Lines 1187 (patched)


`toAdd` is vague. Maybe `httpCheckCommandParameters`?



src/checks/checker_process.cpp
Lines 1202 (patched)


`cbegin() / cend()` please



src/checks/checker_process.cpp
Lines 1367 (patched)


`cbegin / cend`


- Alexander Rukletsov


On Feb. 22, 2018, 1:46 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65127/
> ---
> 
> (Updated Feb. 22, 2018, 1:46 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network health checks called curl and then executed setns to
> enter to container network namespace, which did not work on Windows.
> To do the equivalent, Windows nows calls docker run with powershell's
> curl equivalent (Invoke-WebRequest) and uses the
> network=container: flag to enter the container's namespace. The
> command health check was trivially fixed by replacing the hardcoded
> `sh -c`.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65127/diff/11/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65127: Windows: Enabled docker health checks.

[Akash Gupta]

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

(Updated Feb. 22, 2018, 1:46 a.m.)


Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.


Changes
---

Rebased.


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


Repository: mesos


Description
---

The network health checks called curl and then executed setns to
enter to container network namespace, which did not work on Windows.
To do the equivalent, Windows nows calls docker run with powershell's
curl equivalent (Invoke-WebRequest) and uses the
network=container: flag to enter the container's namespace. The
command health check was trivially fixed by replacing the hardcoded
`sh -c`.


Diffs (updated)
-

  src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
  src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 


Diff: https://reviews.apache.org/r/65127/diff/11/

Changes: https://reviews.apache.org/r/65127/diff/10-11/


Testing
---


Thanks,

Akash Gupta

Re: Review Request 65127: Windows: Enabled docker health checks.

[Akash Gupta]

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

(Updated Feb. 21, 2018, 2:08 a.m.)


Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.


Changes
---

rebased.


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


Repository: mesos


Description
---

The network health checks called curl and then executed setns to
enter to container network namespace, which did not work on Windows.
To do the equivalent, Windows nows calls docker run with powershell's
curl equivalent (Invoke-WebRequest) and uses the
network=container: flag to enter the container's namespace. The
command health check was trivially fixed by replacing the hardcoded
`sh -c`.


Diffs (updated)
-

  src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
  src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 


Diff: https://reviews.apache.org/r/65127/diff/10/

Changes: https://reviews.apache.org/r/65127/diff/9-10/


Testing
---


Thanks,

Akash Gupta

Re: Review Request 65127: Windows: Enabled docker health checks.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Feb. 12, 2018, 3:16 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65127/
> ---
> 
> (Updated Feb. 12, 2018, 3:16 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network health checks called curl and then executed setns to
> enter to container network namespace, which did not work on Windows.
> To do the equivalent, Windows nows calls docker run with powershell's
> curl equivalent (Invoke-WebRequest) and uses the
> network=container: flag to enter the container's namespace. The
> command health check was trivially fixed by replacing the hardcoded
> `sh -c`.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65127/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65127: Windows: Enabled docker health checks.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65127 was successfully built and tested.

Reviews applied: `['65393', '65394', '65395', '65396', '65419', '65127']`

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

- Mesos Reviewbot Windows


On Feb. 12, 2018, 11:16 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65127/
> ---
> 
> (Updated Feb. 12, 2018, 11:16 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network health checks called curl and then executed setns to
> enter to container network namespace, which did not work on Windows.
> To do the equivalent, Windows nows calls docker run with powershell's
> curl equivalent (Invoke-WebRequest) and uses the
> network=container: flag to enter the container's namespace. The
> command health check was trivially fixed by replacing the hardcoded
> `sh -c`.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65127/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65127: Windows: Enabled docker health checks.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On Feb. 12, 2018, 3:16 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65127/
> ---
> 
> (Updated Feb. 12, 2018, 3:16 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network health checks called curl and then executed setns to
> enter to container network namespace, which did not work on Windows.
> To do the equivalent, Windows nows calls docker run with powershell's
> curl equivalent (Invoke-WebRequest) and uses the
> network=container: flag to enter the container's namespace. The
> command health check was trivially fixed by replacing the hardcoded
> `sh -c`.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65127/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65127: Windows: Enabled docker health checks.

[Akash Gupta]

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

(Updated Feb. 12, 2018, 11:16 p.m.)


Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.


Changes
---

Rebased + feedback.


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


Repository: mesos


Description
---

The network health checks called curl and then executed setns to
enter to container network namespace, which did not work on Windows.
To do the equivalent, Windows nows calls docker run with powershell's
curl equivalent (Invoke-WebRequest) and uses the
network=container: flag to enter the container's namespace. The
command health check was trivially fixed by replacing the hardcoded
`sh -c`.


Diffs (updated)
-

  src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
  src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 


Diff: https://reviews.apache.org/r/65127/diff/8/

Changes: https://reviews.apache.org/r/65127/diff/7-8/


Testing
---


Thanks,

Akash Gupta

Re: Review Request 65127: Windows: Enabled docker health checks.

[Gaston Kleiman]

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




src/checks/checker_process.cpp
Lines 1017-1020 (patched)


Why did you introduce a new function? `CheckerProcess::_httpCheck` doesn't 
look like a continuation, since it is called directly (and synchronously) by 
`CheckerProcess::httpCheck`.



src/checks/checker_process.cpp
Lines 1188 (patched)


Add a period at the end of the line.

s/health check/check/


- Gaston Kleiman


On Feb. 8, 2018, 9:51 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65127/
> ---
> 
> (Updated Feb. 8, 2018, 9:51 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network health checks called curl and then executed setns to
> enter to container network namespace, which did not work on Windows.
> To do the equivalent, Windows nows calls docker run with powershell's
> curl equivalent (Invoke-WebRequest) and uses the
> network=container: flag to enter the container's namespace. The
> command health check was trivially fixed by replacing the hardcoded
> `sh -c`.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65127/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65127: Windows: Enabled docker health checks.

[Akash Gupta]

> On Feb. 2, 2018, 9:53 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.hpp
> > Lines 47 (patched)
> > 
> >
> > Should we tag this so a push to their image doesn't break us?

I'm not sure how to solve this problem. The newer images are not backwards 
compatible, so fixed tags will always break on newer Windows releases. 
Personally, I think it's safest to use this tag. Since we require the 
semi-annual channel anyway for this feature, I think the windows hosts will be 
updated and then we'll just pick up the latest powershell image. I don't think 
we can have a concrete solution until the Windows containers guy figure out 
what they want to do with container compatibility with different Windows host.


> On Feb. 2, 2018, 9:53 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.cpp
> > Lines 1144-1151 (patched)
> > 
> >
> > I still feel like there's a better way to output this status code.
> > 
> > 
> > From command prompt:
> > ```
> > C:\Users\andrew>pwsh -c (Invoke-WebRequest -Uri google.com 
> > -UseBasicParsing -SkipCertificateCheck).StatusCode
> > 200
> > 
> > C:\Users\andrew>
> > ```
> > 
> > What's the problem with the output number?

It's "200" in UTF-16, while the common code parsing the http status is 
expecting it in UTF-8, why is why there's all that powershell stuff to write to 
file as ascii. If there is a powershell string.utf16_to_utf8(), I can use that.


- Akash


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


On Feb. 8, 2018, 5:51 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65127/
> ---
> 
> (Updated Feb. 8, 2018, 5:51 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network health checks called curl and then executed setns to
> enter to container network namespace, which did not work on Windows.
> To do the equivalent, Windows nows calls docker run with powershell's
> curl equivalent (Invoke-WebRequest) and uses the
> network=container: flag to enter the container's namespace. The
> command health check was trivially fixed by replacing the hardcoded
> `sh -c`.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65127/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65127: Windows: Enabled docker health checks.

[Akash Gupta]

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

(Updated Feb. 8, 2018, 5:51 p.m.)


Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.


Changes
---

rebased and responded to feedback.


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


Repository: mesos


Description
---

The network health checks called curl and then executed setns to
enter to container network namespace, which did not work on Windows.
To do the equivalent, Windows nows calls docker run with powershell's
curl equivalent (Invoke-WebRequest) and uses the
network=container: flag to enter the container's namespace. The
command health check was trivially fixed by replacing the hardcoded
`sh -c`.


Diffs (updated)
-

  src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
  src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 


Diff: https://reviews.apache.org/r/65127/diff/7/

Changes: https://reviews.apache.org/r/65127/diff/6-7/


Testing
---


Thanks,

Akash Gupta

Re: Review Request 65127: Windows: Enabled docker health checks.

[Andrew Schwartzmeyer]

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




src/checks/checker_process.hpp
Lines 45 (patched)


Nit: s/newere/newer



src/checks/checker_process.hpp
Lines 47 (patched)


Should we tag this so a push to their image doesn't break us?



src/checks/checker_process.cpp
Lines 1144-1151 (patched)


I still feel like there's a better way to output this status code.

From command prompt:
```
C:\Users\andrew>pwsh -c (Invoke-WebRequest -Uri google.com -UseBasicParsing 
-SkipCertificateCheck).StatusCode
200

C:\Users\andrew>
```

What's the problem with the output number?


- Andrew Schwartzmeyer


On Jan. 30, 2018, 2:18 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65127/
> ---
> 
> (Updated Jan. 30, 2018, 2:18 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network health checks called curl and then executed setns to
> enter to container network namespace, which did not work on Windows.
> To do the equivalent, Windows nows calls docker run with powershell's
> curl equivalent (Invoke-WebRequest) and uses the
> network=container: flag to enter the container's namespace. The
> command health check was trivially fixed by replacing the hardcoded
> `sh -c`.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65127/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65127: Windows: Enabled docker health checks.

[Akash Gupta]

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

(Updated Jan. 30, 2018, 10:18 a.m.)


Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.


Changes
---

Separated this patch from the one to fix quoting in `docker exec`.


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


Repository: mesos


Description (updated)
---

The network health checks called curl and then executed setns to
enter to container network namespace, which did not work on Windows.
To do the equivalent, Windows nows calls docker run with powershell's
curl equivalent (Invoke-WebRequest) and uses the
network=container: flag to enter the container's namespace. The
command health check was trivially fixed by replacing the hardcoded
`sh -c`.


Diffs (updated)
-

  src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
  src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 


Diff: https://reviews.apache.org/r/65127/diff/6/

Changes: https://reviews.apache.org/r/65127/diff/5-6/


Testing
---


Thanks,

Akash Gupta

Re: Review Request 65127: Windows: Enabled docker health checks.

[Akash Gupta]

> On Jan. 22, 2018, 9:55 p.m., Joseph Wu wrote:
> > src/checks/checker_process.cpp
> > Lines 479-481 (original), 479-487 (patched)
> > 
> >
> > This is a bit unexpected.  Considering that these arguments will be 
> > joined a few lines down, how is the quoting maintained?
> 
> Akash Gupta wrote:
> It's a bit weird. The implementation of `subprocess` for a shell command 
> creates an argument list that looks like this: `{os::shell::arg0, 
> os::shell::arg1, }`. So, in Windows, `` will then get 
> quoted when it transforms `argv` into a single command line for 
> `CreateProcess`. If you add the quotes here, it won't work, since you will 
> have double quotes.
> 
> Akash Gupta wrote:
> I looked further into this. I'm not sure why the `docker exec` command 
> was wrapped in a shell command. This causes issues with quoting on Windows. 
> It seems better do `.set_shell(false)` and then set the arguments properly.

I posted a review: https://reviews.apache.org/r/65419/ to fix the quoting. 
Let's discuss it there.


- Akash


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


On Jan. 29, 2018, 6:33 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65127/
> ---
> 
> (Updated Jan. 29, 2018, 6:33 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network health checks called curl and then executed setns to
> enter to container network namespace, which did not work on Windows.
> To do the equivalent, Windows nows calls docker run with powershell's
> curl equivalent (Invoke-WebRequest) and uses the
> network=container: flag to enter the container's namespace.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65127/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65127: Windows: Enabled docker health checks.

[Akash Gupta]

> On Jan. 22, 2018, 9:55 p.m., Joseph Wu wrote:
> > src/checks/checker_process.cpp
> > Lines 479-481 (original), 479-487 (patched)
> > 
> >
> > This is a bit unexpected.  Considering that these arguments will be 
> > joined a few lines down, how is the quoting maintained?
> 
> Akash Gupta wrote:
> It's a bit weird. The implementation of `subprocess` for a shell command 
> creates an argument list that looks like this: `{os::shell::arg0, 
> os::shell::arg1, }`. So, in Windows, `` will then get 
> quoted when it transforms `argv` into a single command line for 
> `CreateProcess`. If you add the quotes here, it won't work, since you will 
> have double quotes.

I looked further into this. I'm not sure why the `docker exec` command was 
wrapped in a shell command. This causes issues with quoting on Windows. It 
seems better do `.set_shell(false)` and then set the arguments properly.


- Akash


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


On Jan. 29, 2018, 6:33 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65127/
> ---
> 
> (Updated Jan. 29, 2018, 6:33 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network health checks called curl and then executed setns to
> enter to container network namespace, which did not work on Windows.
> To do the equivalent, Windows nows calls docker run with powershell's
> curl equivalent (Invoke-WebRequest) and uses the
> network=container: flag to enter the container's namespace.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65127/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65127: Windows: Enabled docker health checks.

[Akash Gupta]

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

(Updated Jan. 29, 2018, 6:33 p.m.)


Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.


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


Repository: mesos


Description
---

The network health checks called curl and then executed setns to
enter to container network namespace, which did not work on Windows.
To do the equivalent, Windows nows calls docker run with powershell's
curl equivalent (Invoke-WebRequest) and uses the
network=container: flag to enter the container's namespace.


Diffs
-

  src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
  src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 


Diff: https://reviews.apache.org/r/65127/diff/5/


Testing
---


Thanks,

Akash Gupta

Re: Review Request 65127: Windows: Enabled docker health checks.

[Akash Gupta]

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

(Updated Jan. 29, 2018, 6:33 p.m.)


Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.


Changes
---

Changed with refactor requests.


Repository: mesos


Description
---

The network health checks called curl and then executed setns to
enter to container network namespace, which did not work on Windows.
To do the equivalent, Windows nows calls docker run with powershell's
curl equivalent (Invoke-WebRequest) and uses the
network=container: flag to enter the container's namespace.


Diffs (updated)
-

  src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
  src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 


Diff: https://reviews.apache.org/r/65127/diff/5/

Changes: https://reviews.apache.org/r/65127/diff/4-5/


Testing
---


Thanks,

Akash Gupta

Re: Review Request 65127: Windows: Enabled docker health checks.

[Gaston Kleiman]

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




src/checks/checker_process.cpp
Lines 887-888 (original)


I think this should stay here, it's weird for `createHttpCheckCmd` to log 
this.



src/checks/checker_process.cpp
Lines 991-1004 (patched)






src/checks/checker_process.cpp
Lines 1048-1049 (original)


I think this should stay here, it's weird for `createTcpCheckCmd` to log 
this.


- Gaston Kleiman


On Jan. 16, 2018, 4:09 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65127/
> ---
> 
> (Updated Jan. 16, 2018, 4:09 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network health checks called curl and then executed setns to
> enter to container network namespace, which did not work on Windows.
> To do the equivalent, Windows nows calls docker run with powershell's
> curl equivalent (Invoke-WebRequest) and uses the
> network=container: flag to enter the container's namespace.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65127/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65127: Windows: Enabled docker health checks.

[Akash Gupta]

> On Jan. 22, 2018, 9:55 p.m., Joseph Wu wrote:
> > src/checks/checker_process.cpp
> > Lines 479-481 (original), 479-487 (patched)
> > 
> >
> > This is a bit unexpected.  Considering that these arguments will be 
> > joined a few lines down, how is the quoting maintained?

It's a bit weird. The implementation of `subprocess` for a shell command 
creates an argument list that looks like this: `{os::shell::arg0, 
os::shell::arg1, }`. So, in Windows, `` will then get quoted 
when it transforms `argv` into a single command line for `CreateProcess`. If 
you add the quotes here, it won't work, since you will have double quotes.


- Akash


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


On Jan. 17, 2018, 12:09 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65127/
> ---
> 
> (Updated Jan. 17, 2018, 12:09 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network health checks called curl and then executed setns to
> enter to container network namespace, which did not work on Windows.
> To do the equivalent, Windows nows calls docker run with powershell's
> curl equivalent (Invoke-WebRequest) and uses the
> network=container: flag to enter the container's namespace.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65127/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65127: Windows: Enabled docker health checks.

[Joseph Wu]

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



This patch will likely need a few tweaks based on how the previous patch is 
changed, so I don't have much to say here.


src/checks/checker_process.cpp
Lines 479-481 (original), 479-487 (patched)


This is a bit unexpected.  Considering that these arguments will be joined 
a few lines down, how is the quoting maintained?


- Joseph Wu


On Jan. 16, 2018, 4:09 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65127/
> ---
> 
> (Updated Jan. 16, 2018, 4:09 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network health checks called curl and then executed setns to
> enter to container network namespace, which did not work on Windows.
> To do the equivalent, Windows nows calls docker run with powershell's
> curl equivalent (Invoke-WebRequest) and uses the
> network=container: flag to enter the container's namespace.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65127/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65127: Windows: Enabled docker health checks.

[Akash Gupta]

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

(Updated Jan. 17, 2018, 12:09 a.m.)


Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.


Changes
---

Fixed linux build break.


Repository: mesos


Description
---

The network health checks called curl and then executed setns to
enter to container network namespace, which did not work on Windows.
To do the equivalent, Windows nows calls docker run with powershell's
curl equivalent (Invoke-WebRequest) and uses the
network=container: flag to enter the container's namespace.


Diffs (updated)
-

  src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
  src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 


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

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


Testing
---


Thanks,

Akash Gupta

Re: Review Request 65127: Windows: Enabled docker health checks.

[Gaston Kleiman]

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




src/checks/checker_process.cpp
Lines 990-1004 (patched)


I just found this blog post: 
https://blogs.technet.microsoft.com/virtualization/2017/12/19/tar-and-curl-come-to-windows/

It claims that the nanoserver image is only about 200 Mb big and includes 
`curl`. Can't we use that instead of powershell inside a huge container?


- Gaston Kleiman


On Jan. 16, 2018, 8:41 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65127/
> ---
> 
> (Updated Jan. 16, 2018, 8:41 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network health checks called curl and then executed setns to
> enter to container network namespace, which did not work on Windows.
> To do the equivalent, Windows nows calls docker run with powershell's
> curl equivalent (Invoke-WebRequest) and uses the
> network=container: flag to enter the container's namespace.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65127/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65127: Windows: Enabled docker health checks.

[Akash Gupta]

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

(Updated Jan. 16, 2018, 4:41 p.m.)


Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.


Repository: mesos


Description
---

The network health checks called curl and then executed setns to
enter to container network namespace, which did not work on Windows.
To do the equivalent, Windows nows calls docker run with powershell's
curl equivalent (Invoke-WebRequest) and uses the
network=container: flag to enter the container's namespace.


Diffs (updated)
-

  src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
  src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 


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

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


Testing
---


Thanks,

Akash Gupta

Re: Review Request 65127: Windows: Enabled docker health checks.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65127 was successfully built and tested.

Reviews applied: `['63859', '63860', '63861', '64570', '64386', '65127']`

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

- Mesos Reviewbot Windows


On Jan. 12, 2018, 2:37 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65127/
> ---
> 
> (Updated Jan. 12, 2018, 2:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network health checks called curl and then executed setns to
> enter to container network namespace, which did not work on Windows.
> To do the equivalent, Windows nows calls docker run with powershell's
> curl equivalent (Invoke-WebRequest) and uses the
> network=container: flag to enter the container's namespace.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65127/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Review Request 65127: Windows: Enabled docker health checks.

[Akash Gupta]

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

Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.


Repository: mesos


Description
---

The network health checks called curl and then executed setns to
enter to container network namespace, which did not work on Windows.
To do the equivalent, Windows nows calls docker run with powershell's
curl equivalent (Invoke-WebRequest) and uses the
network=container: flag to enter the container's namespace.


Diffs
-

  src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
  src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 


Diff: https://reviews.apache.org/r/65127/diff/1/


Testing
---


Thanks,

Akash Gupta