mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 65419: Fixed quoting issues in docker executor command checks.
Date Wed, 21 Feb 2018 13:58:14 GMT


> On Feb. 7, 2018, 1:07 p.m., Alexander Rukletsov wrote:
> > Is this related to https://issues.apache.org/jira/browse/MESOS-4812 ?
> 
> Akash Gupta wrote:
>     Yeah, I think it will fix that issue, but I have to confirm it.
> 
> Alexander Rukletsov wrote:
>     Please do! And if it does fix it, adjust the ticket reference accordingly.
> 
> Akash Gupta wrote:
>     Alex, I checked MESOS-4812 and also this issue https://github.com/mesosphere/marathon/issues/5136.
With this patch, both of them work as expected without any work arounds. To quickly test this,
I just tried changing `ROOT_DOCKER_DockerHealthyTask` with the following:
>     
>     
>     ```
>       ContainerInfo::DockerInfo dockerInfo;
>       dockerInfo.set_image("nginx");  // tomcat image with ps installed for github issue.
>       containerInfo.mutable_docker()->CopyFrom(dockerInfo);
>     
>       vector<TaskInfo> tasks = populateTasks(
>           "nginx -g 'daemon off;'", // `catalina.sh run` for github issue
>           "bash -c \"</dev/tcp/127.0.0.1/80\"",  // if `[ -z $(ps -e | grep java)
]; then exit 1; fi` for github issue.
>           offers.get()[0],
>           0,
>           None(),
>           None(),
>           containerInfo);
>     ```
>     
>     So, both issues should be resolved. Note that the workaround for MESOS-4812 (escape
all `"` in the command, so `bash -c \\"</dev/tcp/127.0.0.1/80\\"`) doesn't work anymore,
so people will need to update their scripts to avoid the workaround. I don't know how much
of a backwards compatability concern that is though.

I think this is fine:
1) This was always a workaround and a known bug. We are not going to maintain old bugs for
convenience until people get extremely mad at us ; )
2) Cluster upgrades should not be affected. Marathon task scale operations will, but this
is probably fine as people expect to do them in steps and have the ability to spot the change
and adjust the task definition.


- Alexander


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


On Feb. 21, 2018, 2:07 a.m., Akash Gupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65419/
> -----------------------------------------------------------
> 
> (Updated Feb. 21, 2018, 2:07 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston Kleiman,
and Joseph Wu.
> 
> 
> Bugs: MESOS-4812 and MESOS-8498
>     https://issues.apache.org/jira/browse/MESOS-4812
>     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/5/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>


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