Re: Review Request 38910: Added `-v` flag to `docker rm`.

[Timothy Chen]

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

Ship it!


Ship It!

- Timothy Chen


On Oct. 7, 2015, 5:34 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38910/
> ---
> 
> (Updated Oct. 7, 2015, 5:34 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2613
> https://issues.apache.org/jira/browse/MESOS-2613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `-v` flag to `docker rm`.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp afcedf1f1a309bd0626c33ee25694ac1b43bdec7 
> 
> Diff: https://reviews.apache.org/r/38910/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> DockerContainerizerTest.ROOT_DOCKER_Launch_Executor fails on my Ubuntu 14.04 
> VM, but this is a known issue: MESOS-3123
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 38910: Added `-v` flag to `docker rm`.

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 十月 7, 2015, 5:34 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38910/
> ---
> 
> (Updated 十月 7, 2015, 5:34 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2613
> https://issues.apache.org/jira/browse/MESOS-2613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `-v` flag to `docker rm`.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp afcedf1f1a309bd0626c33ee25694ac1b43bdec7 
> 
> Diff: https://reviews.apache.org/r/38910/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> DockerContainerizerTest.ROOT_DOCKER_Launch_Executor fails on my Ubuntu 14.04 
> VM, but this is a known issue: MESOS-3123
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 38910: Added `-v` flag to `docker rm`.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38910]

All tests passed.

- Mesos ReviewBot


On Oct. 7, 2015, 5:34 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38910/
> ---
> 
> (Updated Oct. 7, 2015, 5:34 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2613
> https://issues.apache.org/jira/browse/MESOS-2613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `-v` flag to `docker rm`.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp afcedf1f1a309bd0626c33ee25694ac1b43bdec7 
> 
> Diff: https://reviews.apache.org/r/38910/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> DockerContainerizerTest.ROOT_DOCKER_Launch_Executor fails on my Ubuntu 14.04 
> VM, but this is a known issue: MESOS-3123
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 38910: Added `-v` flag to `docker rm`.

[Greg Mann]

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

(Updated Oct. 7, 2015, 5:34 p.m.)


Review request for mesos, Gilbert Song, Jojy Varghese, and Timothy Chen.


Changes
---

Addressed comments.


Bugs: MESOS-2613
https://issues.apache.org/jira/browse/MESOS-2613


Repository: mesos


Description
---

Added `-v` flag to `docker rm`.


Diffs (updated)
-

  src/docker/docker.cpp afcedf1f1a309bd0626c33ee25694ac1b43bdec7 

Diff: https://reviews.apache.org/r/38910/diff/


Testing
---

`make check`

DockerContainerizerTest.ROOT_DOCKER_Launch_Executor fails on my Ubuntu 14.04 
VM, but this is a known issue: MESOS-3123


Thanks,

Greg Mann

Re: Review Request 38910: Added `-v` flag to `docker rm`.

[Guangya Liu]

> On 十月 1, 2015, 4:31 a.m., Jojy Varghese wrote:
> > src/docker/docker.cpp, line 681
> > 
> >
> > wondering this behavior should be defaulted or not. We might be 
> > overloading stop with more than what it should be doing isnt it? Do we 
> > always want to force remove the volumes when docker stops?
> 
> Greg Mann wrote:
> My & Tim's original thought was that because persistent volumes aren't 
> explicitly implemented for the Docker containerizer currently, we could 
> safely make `-v` the default behavior. However, after considering a bit more, 
> I'm thinking that existing user workflows might establish a Docker volume on 
> an agent with the intention of returning to it later with a subsequent task, 
> in which case defaulting to `-v` would break this workflow, so making this an 
> optional argument may be a good idea. I'll have to think about what is the 
> best way to pass this option to the agent, any thoughts on that matter would 
> be appreciated!
> 
> Greg Mann wrote:
> After further review, I do think that we should make the default behavior 
> include the `-v` flag. By default, Docker will create directories in 
> `/var/lib/docker/aufs/`, `/var/lib/docker/vfs`, etc. depending on the 
> filesystem used. These directories contain information related to the Docker 
> image itself, and they will not be deleted if the `-v` flag isn't used. Thus, 
> if an agent runs long enough and runs enough new Docker images, it can fill 
> up the disk with these default volumes.
> 
> While it's possible that a user might hack together a "persistent volume" 
> using Docker volumes as they're currently implemented, we should discourage 
> this in favor of utilizing Mesos-monitored persistent volumes. Always using 
> `docker rm -v` allows us to properly clean the agent, leaving it in a good 
> state after the containerized task has completed.
> 
> It's also worth noting that `docker rm -v` will NOT remove volumes where 
> a host path has been explicitly mounted into the container (i.e. using 
> `docker run -v /host/path:/container/path mycontainer`). So, in fact, some 
> volumes will be unaffected by this change. The primary purpose of this patch 
> is to clean the agent by removing the default volumes created in 
> `/var/lib/docker/`.
> 
> Guangya Liu wrote:
> Greg, so if I was using -v /host/path:/container/path when create a 
> container, I will always need to remove the files explictly on the docker 
> server when the container stopped?
> 
> Greg Mann wrote:
> Guangya, yes you have to remove these volumes manually. Even if the 
> directory /host/path does not exist prior to running the container, Docker 
> does not remove volumes mounted in that way, and I don't believe Mesos 
> removes such volumes either. This is perhaps another feature that we should 
> add into the Docker containerizer: when attaching a volume of the form `-v 
> /host/path:/container/path`, Mesos could check to see if that directory 
> exists already; if it doesn't, then it could be removed along with the 
> container after the task is complete.

Thanks Greg, +1 on using -v as default.


- Guangya


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


On 九月 30, 2015, 11:51 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38910/
> ---
> 
> (Updated 九月 30, 2015, 11:51 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2613
> https://issues.apache.org/jira/browse/MESOS-2613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `-v` flag to `docker rm`.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp afcedf1f1a309bd0626c33ee25694ac1b43bdec7 
> 
> Diff: https://reviews.apache.org/r/38910/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> DockerContainerizerTest.ROOT_DOCKER_Launch_Executor fails on my Ubuntu 14.04 
> VM, but this is a known issue: MESOS-3123
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 38910: Added `-v` flag to `docker rm`.

[Timothy Chen]

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



src/docker/docker.cpp (line 681)


Mounts doesn't work that way where you can remove the directory in the 
container.
IMO this is not something we are looking to support since we don't have a 
strong use case for this.

+1 for what Greg said and we should make -v the default.


- Timothy Chen


On Sept. 30, 2015, 11:51 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38910/
> ---
> 
> (Updated Sept. 30, 2015, 11:51 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2613
> https://issues.apache.org/jira/browse/MESOS-2613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `-v` flag to `docker rm`.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp afcedf1f1a309bd0626c33ee25694ac1b43bdec7 
> 
> Diff: https://reviews.apache.org/r/38910/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> DockerContainerizerTest.ROOT_DOCKER_Launch_Executor fails on my Ubuntu 14.04 
> VM, but this is a known issue: MESOS-3123
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 38910: Added `-v` flag to `docker rm`.

[Greg Mann]

> On Oct. 1, 2015, 4:31 a.m., Jojy Varghese wrote:
> > src/docker/docker.cpp, line 681
> > 
> >
> > wondering this behavior should be defaulted or not. We might be 
> > overloading stop with more than what it should be doing isnt it? Do we 
> > always want to force remove the volumes when docker stops?
> 
> Greg Mann wrote:
> My & Tim's original thought was that because persistent volumes aren't 
> explicitly implemented for the Docker containerizer currently, we could 
> safely make `-v` the default behavior. However, after considering a bit more, 
> I'm thinking that existing user workflows might establish a Docker volume on 
> an agent with the intention of returning to it later with a subsequent task, 
> in which case defaulting to `-v` would break this workflow, so making this an 
> optional argument may be a good idea. I'll have to think about what is the 
> best way to pass this option to the agent, any thoughts on that matter would 
> be appreciated!
> 
> Greg Mann wrote:
> After further review, I do think that we should make the default behavior 
> include the `-v` flag. By default, Docker will create directories in 
> `/var/lib/docker/aufs/`, `/var/lib/docker/vfs`, etc. depending on the 
> filesystem used. These directories contain information related to the Docker 
> image itself, and they will not be deleted if the `-v` flag isn't used. Thus, 
> if an agent runs long enough and runs enough new Docker images, it can fill 
> up the disk with these default volumes.
> 
> While it's possible that a user might hack together a "persistent volume" 
> using Docker volumes as they're currently implemented, we should discourage 
> this in favor of utilizing Mesos-monitored persistent volumes. Always using 
> `docker rm -v` allows us to properly clean the agent, leaving it in a good 
> state after the containerized task has completed.
> 
> It's also worth noting that `docker rm -v` will NOT remove volumes where 
> a host path has been explicitly mounted into the container (i.e. using 
> `docker run -v /host/path:/container/path mycontainer`). So, in fact, some 
> volumes will be unaffected by this change. The primary purpose of this patch 
> is to clean the agent by removing the default volumes created in 
> `/var/lib/docker/`.
> 
> Guangya Liu wrote:
> Greg, so if I was using -v /host/path:/container/path when create a 
> container, I will always need to remove the files explictly on the docker 
> server when the container stopped?

Guangya, yes you have to remove these volumes manually. Even if the directory 
/host/path does not exist prior to running the container, Docker does not 
remove volumes mounted in that way, and I don't believe Mesos removes such 
volumes either. This is perhaps another feature that we should add into the 
Docker containerizer: when attaching a volume of the form `-v 
/host/path:/container/path`, Mesos could check to see if that directory exists 
already; if it doesn't, then it could be removed along with the container after 
the task is complete.


- Greg


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


On Sept. 30, 2015, 11:51 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38910/
> ---
> 
> (Updated Sept. 30, 2015, 11:51 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2613
> https://issues.apache.org/jira/browse/MESOS-2613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `-v` flag to `docker rm`.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp afcedf1f1a309bd0626c33ee25694ac1b43bdec7 
> 
> Diff: https://reviews.apache.org/r/38910/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> DockerContainerizerTest.ROOT_DOCKER_Launch_Executor fails on my Ubuntu 14.04 
> VM, but this is a known issue: MESOS-3123
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 38910: Added `-v` flag to `docker rm`.

[Guangya Liu]

> On 十月 1, 2015, 4:31 a.m., Jojy Varghese wrote:
> > src/docker/docker.cpp, line 681
> > 
> >
> > wondering this behavior should be defaulted or not. We might be 
> > overloading stop with more than what it should be doing isnt it? Do we 
> > always want to force remove the volumes when docker stops?
> 
> Greg Mann wrote:
> My & Tim's original thought was that because persistent volumes aren't 
> explicitly implemented for the Docker containerizer currently, we could 
> safely make `-v` the default behavior. However, after considering a bit more, 
> I'm thinking that existing user workflows might establish a Docker volume on 
> an agent with the intention of returning to it later with a subsequent task, 
> in which case defaulting to `-v` would break this workflow, so making this an 
> optional argument may be a good idea. I'll have to think about what is the 
> best way to pass this option to the agent, any thoughts on that matter would 
> be appreciated!
> 
> Greg Mann wrote:
> After further review, I do think that we should make the default behavior 
> include the `-v` flag. By default, Docker will create directories in 
> `/var/lib/docker/aufs/`, `/var/lib/docker/vfs`, etc. depending on the 
> filesystem used. These directories contain information related to the Docker 
> image itself, and they will not be deleted if the `-v` flag isn't used. Thus, 
> if an agent runs long enough and runs enough new Docker images, it can fill 
> up the disk with these default volumes.
> 
> While it's possible that a user might hack together a "persistent volume" 
> using Docker volumes as they're currently implemented, we should discourage 
> this in favor of utilizing Mesos-monitored persistent volumes. Always using 
> `docker rm -v` allows us to properly clean the agent, leaving it in a good 
> state after the containerized task has completed.
> 
> It's also worth noting that `docker rm -v` will NOT remove volumes where 
> a host path has been explicitly mounted into the container (i.e. using 
> `docker run -v /host/path:/container/path mycontainer`). So, in fact, some 
> volumes will be unaffected by this change. The primary purpose of this patch 
> is to clean the agent by removing the default volumes created in 
> `/var/lib/docker/`.

Greg, so if I was using -v /host/path:/container/path when create a container, 
I will always need to remove the files explictly on the docker server when the 
container stopped?


- Guangya


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


On 九月 30, 2015, 11:51 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38910/
> ---
> 
> (Updated 九月 30, 2015, 11:51 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2613
> https://issues.apache.org/jira/browse/MESOS-2613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `-v` flag to `docker rm`.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp afcedf1f1a309bd0626c33ee25694ac1b43bdec7 
> 
> Diff: https://reviews.apache.org/r/38910/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> DockerContainerizerTest.ROOT_DOCKER_Launch_Executor fails on my Ubuntu 14.04 
> VM, but this is a known issue: MESOS-3123
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 38910: Added `-v` flag to `docker rm`.

[Greg Mann]

> On Oct. 1, 2015, 4:31 a.m., Jojy Varghese wrote:
> > src/docker/docker.cpp, line 681
> > 
> >
> > wondering this behavior should be defaulted or not. We might be 
> > overloading stop with more than what it should be doing isnt it? Do we 
> > always want to force remove the volumes when docker stops?
> 
> Greg Mann wrote:
> My & Tim's original thought was that because persistent volumes aren't 
> explicitly implemented for the Docker containerizer currently, we could 
> safely make `-v` the default behavior. However, after considering a bit more, 
> I'm thinking that existing user workflows might establish a Docker volume on 
> an agent with the intention of returning to it later with a subsequent task, 
> in which case defaulting to `-v` would break this workflow, so making this an 
> optional argument may be a good idea. I'll have to think about what is the 
> best way to pass this option to the agent, any thoughts on that matter would 
> be appreciated!

After further review, I do think that we should make the default behavior 
include the `-v` flag. By default, Docker will create directories in 
`/var/lib/docker/aufs/`, `/var/lib/docker/vfs`, etc. depending on the 
filesystem used. These directories contain information related to the Docker 
image itself, and they will not be deleted if the `-v` flag isn't used. Thus, 
if an agent runs long enough and runs enough new Docker images, it can fill up 
the disk with these default volumes.

While it's possible that a user might hack together a "persistent volume" using 
Docker volumes as they're currently implemented, we should discourage this in 
favor of utilizing Mesos-monitored persistent volumes. Always using `docker rm 
-v` allows us to properly clean the agent, leaving it in a good state after the 
containerized task has completed.

It's also worth noting that `docker rm -v` will NOT remove volumes where a host 
path has been explicitly mounted into the container (i.e. using `docker run -v 
/host/path:/container/path mycontainer`). So, in fact, some volumes will be 
unaffected by this change. The primary purpose of this patch is to clean the 
agent by removing the default volumes created in `/var/lib/docker/`.


- Greg


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


On Sept. 30, 2015, 11:51 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38910/
> ---
> 
> (Updated Sept. 30, 2015, 11:51 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2613
> https://issues.apache.org/jira/browse/MESOS-2613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `-v` flag to `docker rm`.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp afcedf1f1a309bd0626c33ee25694ac1b43bdec7 
> 
> Diff: https://reviews.apache.org/r/38910/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> DockerContainerizerTest.ROOT_DOCKER_Launch_Executor fails on my Ubuntu 14.04 
> VM, but this is a known issue: MESOS-3123
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 38910: Added `-v` flag to `docker rm`.

[Greg Mann]

> On Oct. 1, 2015, 4:31 a.m., Jojy Varghese wrote:
> > src/docker/docker.cpp, line 681
> > 
> >
> > wondering this behavior should be defaulted or not. We might be 
> > overloading stop with more than what it should be doing isnt it? Do we 
> > always want to force remove the volumes when docker stops?

My & Tim's original thought was that because persistent volumes aren't 
explicitly implemented for the Docker containerizer currently, we could safely 
make `-v` the default behavior. However, after considering a bit more, I'm 
thinking that existing user workflows might establish a Docker volume on an 
agent with the intention of returning to it later with a subsequent task, in 
which case defaulting to `-v` would break this workflow, so making this an 
optional argument may be a good idea. I'll have to think about what is the best 
way to pass this option to the agent, any thoughts on that matter would be 
appreciated!


- Greg


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


On Sept. 30, 2015, 11:51 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38910/
> ---
> 
> (Updated Sept. 30, 2015, 11:51 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2613
> https://issues.apache.org/jira/browse/MESOS-2613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `-v` flag to `docker rm`.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp afcedf1f1a309bd0626c33ee25694ac1b43bdec7 
> 
> Diff: https://reviews.apache.org/r/38910/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> DockerContainerizerTest.ROOT_DOCKER_Launch_Executor fails on my Ubuntu 14.04 
> VM, but this is a known issue: MESOS-3123
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 38910: Added `-v` flag to `docker rm`.

[Guangya Liu]

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



src/docker/docker.cpp (line 681)


Echo Jojy's comments, shall we make this as configurable?


- Guangya Liu


On 九月 30, 2015, 11:51 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38910/
> ---
> 
> (Updated 九月 30, 2015, 11:51 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2613
> https://issues.apache.org/jira/browse/MESOS-2613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `-v` flag to `docker rm`.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp afcedf1f1a309bd0626c33ee25694ac1b43bdec7 
> 
> Diff: https://reviews.apache.org/r/38910/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> DockerContainerizerTest.ROOT_DOCKER_Launch_Executor fails on my Ubuntu 14.04 
> VM, but this is a known issue: MESOS-3123
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 38910: Added `-v` flag to `docker rm`.

[Jojy Varghese]

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



src/docker/docker.cpp (line 681)


wondering this behavior should be defaulted or not. We might be overloading 
stop with more than what it should be doing isnt it? Do we always want to force 
remove the volumes when docker stops?


- Jojy Varghese


On Sept. 30, 2015, 11:51 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38910/
> ---
> 
> (Updated Sept. 30, 2015, 11:51 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2613
> https://issues.apache.org/jira/browse/MESOS-2613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `-v` flag to `docker rm`.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp afcedf1f1a309bd0626c33ee25694ac1b43bdec7 
> 
> Diff: https://reviews.apache.org/r/38910/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> DockerContainerizerTest.ROOT_DOCKER_Launch_Executor fails on my Ubuntu 14.04 
> VM, but this is a known issue: MESOS-3123
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 38910: Added `-v` flag to `docker rm`.

[haosdent huang]

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



src/docker/docker.cpp (line 681)


The comment line should be move up before "const string cmd"?


- haosdent huang


On Sept. 30, 2015, 11:51 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38910/
> ---
> 
> (Updated Sept. 30, 2015, 11:51 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2613
> https://issues.apache.org/jira/browse/MESOS-2613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `-v` flag to `docker rm`.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp afcedf1f1a309bd0626c33ee25694ac1b43bdec7 
> 
> Diff: https://reviews.apache.org/r/38910/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> DockerContainerizerTest.ROOT_DOCKER_Launch_Executor fails on my Ubuntu 14.04 
> VM, but this is a known issue: MESOS-3123
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 38910: Added `-v` flag to `docker rm`.

[haosdent huang]

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

Ship it!


Ship It!

- haosdent huang


On Sept. 30, 2015, 11:51 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38910/
> ---
> 
> (Updated Sept. 30, 2015, 11:51 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2613
> https://issues.apache.org/jira/browse/MESOS-2613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `-v` flag to `docker rm`.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp afcedf1f1a309bd0626c33ee25694ac1b43bdec7 
> 
> Diff: https://reviews.apache.org/r/38910/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> DockerContainerizerTest.ROOT_DOCKER_Launch_Executor fails on my Ubuntu 14.04 
> VM, but this is a known issue: MESOS-3123
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 38910: Added `-v` flag to `docker rm`.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38910]

All tests passed.

- Mesos ReviewBot


On Sept. 30, 2015, 11:51 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38910/
> ---
> 
> (Updated Sept. 30, 2015, 11:51 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2613
> https://issues.apache.org/jira/browse/MESOS-2613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `-v` flag to `docker rm`.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp afcedf1f1a309bd0626c33ee25694ac1b43bdec7 
> 
> Diff: https://reviews.apache.org/r/38910/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> DockerContainerizerTest.ROOT_DOCKER_Launch_Executor fails on my Ubuntu 14.04 
> VM, but this is a known issue: MESOS-3123
> 
> 
> Thanks,
> 
> Greg Mann
> 
>