> 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 > >
