mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Akash Gupta <akash-gu...@hotmail.com>
Subject Re: Review Request 64386: Windows: Enabled docker health checks.
Date Thu, 11 Jan 2018 22:10:56 GMT


> On Jan. 10, 2018, 1:16 p.m., Alexander Rukletsov wrote:
> > src/checks/checker.hpp
> > Lines 73 (patched)
> > <https://reviews.apache.org/r/64386/diff/5/?file=1938002#file1938002line73>
> >
> >     I think all such commands should not be passed from outside, but be part of
the library itself. The library shall be capable of determining the target platform and use
an appropriate command.
> 
> Akash Gupta wrote:
>     Hm, on Windows, the library would need to detect if the mesos or docker executor
called it. The mesos executor doesn't do any namespace changes, so it can just call the command
directly, but the docker executor needs run the command in the container's namespace, so it
needs to do `docker run --network=container:ID `. Should the library check if the command
needs to run in the containers namesapce (via the namespaces vector), and if so, simply hard
code docker for Windows?
> 
> Alexander Rukletsov wrote:
>     I would favour this approach. We can also move this inside the library as well: https://github.com/apache/mesos/blob/9c03a463c1ac8f63dc00255945a04016c45f04e9/src/docker/executor.cpp#L607-L644

Sounds good.


> On Jan. 10, 2018, 1:16 p.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.cpp
> > Lines 1009-1013 (original), 1016-1025 (patched)
> > <https://reviews.apache.org/r/64386/diff/5/?file=1938005#file1938005line1016>
> >
> >     This means mesos-tcp-connect is not used for tcp checks with docker executor?
Why? Is the solution your propose more efficient?
> 
> Akash Gupta wrote:
>     `mesos-tcp-connect` will be still used in the Linux docker executor, but it cannot
be used for the Windows docker executor. On Linux, Mesos runs `mesos-tcp-connect` and then
changes the network namespace of the process to the container. Windows doesn't have this functionality,
so the only way to get the equivalent behavior is to run a container inside another container's
network namespace. So, either we ship a `mesos-tcp-connect` docker image or we just ignore
it and use powershell to do the TCP connection test. I opted to do the latter.
> 
> Alexander Rukletsov wrote:
>     I'm surprised to hear that there is no way to run an executable in a namespace of
a docker container, but then this is how it is. Let's make it clear this distinction clear
in the library, i.e., put your comment here into a comment in the code.

Sounds good.


> On Jan. 10, 2018, 1:16 p.m., Alexander Rukletsov wrote:
> > src/docker/executor.cpp
> > Lines 665-675 (patched)
> > <https://reviews.apache.org/r/64386/diff/5/?file=1938009#file1938009line665>
> >
> >     Why is this not part of the health check library? This way, the code related
to http and tcp checking is scattered over the codebase.
> 
> Akash Gupta wrote:
>     Copied question from one of my responses: Should the library check if the command
needs to run in the containers namesapce (via the namespaces vector), and if so, simply hard
code docker for Windows?
> 
> Alexander Rukletsov wrote:
>     I think it is fine to pass a flag into the library, e.g., "docker", or, if you want
clean abstractions, create an enum "ContainerRuntime" with values `{Docker, Mesos, Default}`
and have different paths in the library based on the enum value. Is my suggestion clear?

Yeah, makes sense to me.


- Akash


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


On Jan. 10, 2018, 1:28 a.m., Akash Gupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64386/
> -----------------------------------------------------------
> 
> (Updated Jan. 10, 2018, 1:28 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston Kleiman,
Jie Yu, Joseph Wu, and Michael Park.
> 
> 
> 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:<ID> flag to enter the container's namespace.
> 
> 
> Diffs
> -----
> 
>   src/checks/checker.hpp 93502270f31e80c5f7c94b5b456625e9cdea1837 
>   src/checks/checker.cpp fff0aac504b4283a210f936e00c977fa60d88b3d 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
>   src/checks/health_checker.hpp 019fbd791f250ecc28ff59d779f90e7ccbf0c685 
>   src/checks/health_checker.cpp eaf9a18817eeeff7c29c7a4b9d1b183f398760a3 
>   src/docker/docker.hpp d9e71f8841a868082170d28fc4f3d495e2eb1e61 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
>   src/launcher/executor.cpp 794bf7ca07e68c7d83993546c134f85cac5a68e3 
> 
> 
> Diff: https://reviews.apache.org/r/64386/diff/5/
> 
> 
> Testing
> -------
> 
> See https://reviews.apache.org/r/64387/
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>


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