You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2018/02/02 21:43:30 UTC
Re: Review Request 65419: Fixed quoting issues in docker executor
command checks.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65419/#review196734
-----------------------------------------------------------
src/checks/checker_process.cpp
Lines 523-525 (original), 523-526 (patched)
<https://reviews.apache.org/r/65419/#comment276562>
I knew it looked funny.
src/checks/checker_process.cpp
Line 527 (original), 528-535 (patched)
<https://reviews.apache.org/r/65419/#comment276563>
I'm not quite following this. Why do we always push `command.value()` then?
src/checks/checker_process.cpp
Lines 535-537 (original), 543-547 (patched)
<https://reviews.apache.org/r/65419/#comment276564>
Can you explain why this was changed a little more? Maybe an example would help.
- Andrew Schwartzmeyer
On Jan. 30, 2018, 2:12 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65419/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2018, 2:12 a.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/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Akash Gupta
>
>
Re: Review Request 65419: Fixed quoting issues in docker executor
command checks.
Posted by Akash Gupta <ak...@hotmail.com>.
> 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
>
>
Re: Review Request 65419: Fixed quoting issues in docker executor
command checks.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Feb. 2, 2018, 1: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.
>
> Akash Gupta wrote:
> 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)
Hm, take a look at this: https://reviews.apache.org/r/54846/
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65419/#review196734
-----------------------------------------------------------
On Feb. 8, 2018, 9:50 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65419/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2018, 9:50 a.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
>
>