> 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?
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 > 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. 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. > 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? 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? - Alexander ----------------------------------------------------------- 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 > >
