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 65419: Fixed quoting issues in docker executor command checks.
Date Thu, 08 Feb 2018 17:50:49 GMT


> On Feb. 2, 2018, 9:43 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.cpp
> > Line 527 (original), 528-535 (patched)
> > <https://reviews.apache.org/r/65419/diff/1/?file=1950448#file1950448line529>
> >
> >     I'm not quite following this. Why do we always push `command.value()` then?

I think the original implementation was incorrectly unconditionally pushing `command.value()`.
The `docker->run` logic was only pushing `command.value()` if it existed, so I made this
match that logic for consistency. The documentation for the `CommandInfo` protobuf just says
that the `.value()` and `.arguments()` are passed into `execlp` as `execlp(commandInfo.value(),
commandInfo.arguments...)`, which doesn't make any sense for docker. The comment just documents
how the protobuf is interpretted inside the executor. Also, I don't know if this segment actually
gets hit since marathon states that all the health checks use the shell form instead of the
exec form.


> On Feb. 2, 2018, 9:43 p.m., Andrew Schwartzmeyer wrote:
> > src/checks/checker_process.cpp
> > Lines 535-537 (original), 543-547 (patched)
> > <https://reviews.apache.org/r/65419/diff/1/?file=1950448#file1950448line544>
> >
> >     Can you explain why this was changed a little more? Maybe an example would help.

Originally, it was setting the `docker exec` in `shell` mode, which wrapped it with `sh -c
''`. So, the command was `sh -c 'docker exec sh -c " <cmd> "'`, which was breaking on
Windows due to quoting. Now, it skips the `sh` wrapper and calls `docker exec sh -c '<cmd>'`
(in exec form, so it's actually `["docker", "exec", "sh", "-c", cmd]`) directly.

This fixes a couple of things:
 - The tests will actually pass on Windows now. Before, due to the crazy quoting, the tests
were failing.
 - On Linux, your command can now actually have `"` in it instead of having to escape it.
For example, `echo "Hello World!"` was originally getting changed to `sh -c 'docker exec sh
-c " echo "Hello World!" "'`, which is broken.
 - On Linux, the mesos-agent shell doesn't accidently interpret the command anymore. For example,
if your command was `ls $HOME`, originally, it was running `sh -c 'docker exec sh -c " ls
$HOME "'`, which meant that `$HOME` was being       evaluated in the context of the mesos-agent
due to the first `sh -c` instead of the docker container.

I've seen a couple of issues caused due to the weird quoting. ( https://issues.apache.org/jira/browse/MESOS-4812
and https://github.com/mesosphere/marathon/issues/5136)


- Akash


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


On Feb. 8, 2018, 5:50 p.m., Akash Gupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65419/
> -----------------------------------------------------------
> 
> (Updated Feb. 8, 2018, 5:50 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston Kleiman,
and Joseph Wu.
> 
> 
> Bugs: MESOS-8498
>     https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The docker executor was wrapping the docker exec command around with
> `sh -c ""`, which was causing quoting issues, especially on Windows.
> Now, the comamnd health check simply runs `docker exec` without any
> wrapping.
> 
> 
> Diffs
> -----
> 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65419/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>


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