Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/docker.cpp (line 1746)


Fix the indentation here.



src/slave/containerizer/docker.cpp (lines 1856 - 1861)


Can you move that to `___destroy`?



src/tests/containerizer/docker_containerizer_tests.cpp (line 1447)


Kill this line here.


- Jie Yu


On Feb. 18, 2016, 6:16 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43015/
> ---
> 
> (Updated Feb. 18, 2016, 6:16 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3413
> https://issues.apache.org/jira/browse/MESOS-3413
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed persistent volumes with docker tasks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
>   src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 645bdcf095145097d8b8c65d592c787417883145 
> 
> Diff: https://reviews.apache.org/r/43015/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

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

(Updated Feb. 18, 2016, 6:16 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Fixed persistent volumes with docker tasks.


Diffs (updated)
-

  src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
  src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
  src/tests/containerizer/docker_containerizer_tests.cpp 
645bdcf095145097d8b8c65d592c787417883145 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

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

(Updated Feb. 18, 2016, 9:12 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Fixed persistent volumes with docker tasks.


Diffs (updated)
-

  src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
  src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
  src/tests/containerizer/docker_containerizer_tests.cpp 
645bdcf095145097d8b8c65d592c787417883145 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Jie Yu]

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




src/slave/containerizer/docker.cpp (lines 1785 - 1790)


the destroy can be initiated by the slave, right? it's possible that 
`docker->stop` above failed (meaning that the container is still running). You 
should not remove mounts in that case, isn't it?


- Jie Yu


On Feb. 16, 2016, 3:33 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43015/
> ---
> 
> (Updated Feb. 16, 2016, 3:33 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3413
> https://issues.apache.org/jira/browse/MESOS-3413
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed persistent volumes with docker tasks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
>   src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 645bdcf095145097d8b8c65d592c787417883145 
> 
> Diff: https://reviews.apache.org/r/43015/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

> On Feb. 16, 2016, 11:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, lines 1785-1790
> > 
> >
> > Hum, I think we shouldn't remove the volumes if the container might be 
> > still running. So can we move the cleanup logics to `___destroy()`?

We don't unmount when it's running, it's only running when it's in the RUNNING 
state.


- Timothy


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


On Feb. 16, 2016, 3:33 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43015/
> ---
> 
> (Updated Feb. 16, 2016, 3:33 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3413
> https://issues.apache.org/jira/browse/MESOS-3413
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed persistent volumes with docker tasks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
>   src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 645bdcf095145097d8b8c65d592c787417883145 
> 
> Diff: https://reviews.apache.org/r/43015/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

> On Feb. 16, 2016, 11:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, lines 1701-1708
> > 
> >
> > Why do you need to do this? Looks like we don't mount until pulling is 
> > done. Am I missing something?

The reason is that I didn't add a new state that distinguishes mounting the 
persistent volumes from pulling, so when we destroy when a container is in 
PULLING state I assume we might already mounted volumes. I've added a new state 
so we're now more explicit.


- Timothy


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


On Feb. 16, 2016, 3:33 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43015/
> ---
> 
> (Updated Feb. 16, 2016, 3:33 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3413
> https://issues.apache.org/jira/browse/MESOS-3413
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed persistent volumes with docker tasks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
>   src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 645bdcf095145097d8b8c65d592c787417883145 
> 
> Diff: https://reviews.apache.org/r/43015/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

> On Feb. 16, 2016, 11:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, lines 473-488
> > 
> >
> > There's a os::exists check in linux fs isolator which you removed. This 
> > is fine because we won't update volumes when containerizer->update is 
> > called. I think we should at least drop a TODO here
> > so that we don't forget that when we want to support 
> > containerizer->update.
> > 
> > Alternatively, we can kill updatePersistentVolumes and inline it into 
> > mountPersistentVolumes.
> 
> Timothy Chen wrote:
> I think I simply forgot about it, let me add them.

Actually I'll just drop a TODO since we don't support update as you mentioned.


- Timothy


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


On Feb. 16, 2016, 3:33 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43015/
> ---
> 
> (Updated Feb. 16, 2016, 3:33 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3413
> https://issues.apache.org/jira/browse/MESOS-3413
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed persistent volumes with docker tasks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
>   src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 645bdcf095145097d8b8c65d592c787417883145 
> 
> Diff: https://reviews.apache.org/r/43015/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

> On Feb. 16, 2016, 11:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, lines 413-414
> > 
> >
> > Should we ignore those volumes that have absolute/nested container path 
> > (like we did in linux fs isolator). What's the reason removing it?

Ah yes I think we should. Let me update this.


- Timothy


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


On Feb. 16, 2016, 3:33 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43015/
> ---
> 
> (Updated Feb. 16, 2016, 3:33 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3413
> https://issues.apache.org/jira/browse/MESOS-3413
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed persistent volumes with docker tasks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
>   src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 645bdcf095145097d8b8c65d592c787417883145 
> 
> Diff: https://reviews.apache.org/r/43015/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

> On Feb. 16, 2016, 11:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, lines 473-488
> > 
> >
> > There's a os::exists check in linux fs isolator which you removed. This 
> > is fine because we won't update volumes when containerizer->update is 
> > called. I think we should at least drop a TODO here
> > so that we don't forget that when we want to support 
> > containerizer->update.
> > 
> > Alternatively, we can kill updatePersistentVolumes and inline it into 
> > mountPersistentVolumes.

I think I simply forgot about it, let me add them.


- Timothy


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


On Feb. 16, 2016, 3:33 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43015/
> ---
> 
> (Updated Feb. 16, 2016, 3:33 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3413
> https://issues.apache.org/jira/browse/MESOS-3413
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed persistent volumes with docker tasks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
>   src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 645bdcf095145097d8b8c65d592c787417883145 
> 
> Diff: https://reviews.apache.org/r/43015/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

> On Feb. 16, 2016, 11:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, lines 161-162
> > 
> >
> > Can you wrap using 70 char width here?

Sure, I thought we removed that restriction now?


- Timothy


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


On Feb. 16, 2016, 3:33 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43015/
> ---
> 
> (Updated Feb. 16, 2016, 3:33 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3413
> https://issues.apache.org/jira/browse/MESOS-3413
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed persistent volumes with docker tasks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
>   src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 645bdcf095145097d8b8c65d592c787417883145 
> 
> Diff: https://reviews.apache.org/r/43015/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Jie Yu]

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




src/slave/containerizer/docker.cpp (lines 161 - 162)


Can you wrap using 70 char width here?



src/slave/containerizer/docker.cpp (lines 413 - 414)


Should we ignore those volumes that have absolute/nested container path 
(like we did in linux fs isolator). What's the reason removing it?



src/slave/containerizer/docker.cpp (line 419)


Indentation.



src/slave/containerizer/docker.cpp (line 442)


indentation



src/slave/containerizer/docker.cpp (lines 457 - 458)


Ditto. Why remove the container_path check?



src/slave/containerizer/docker.cpp (lines 473 - 488)


There's a os::exists check in linux fs isolator which you removed. This is 
fine because we won't update volumes when containerizer->update is called. I 
think we should at least drop a TODO here
so that we don't forget that when we want to support containerizer->update.

Alternatively, we can kill updatePersistentVolumes and inline it into 
mountPersistentVolumes.



src/slave/containerizer/docker.cpp (line 881)


Can you add a TODO stating that this is a hack for now. We need to revisit 
if docker containers have other mounts that may container 'containerId'



src/slave/containerizer/docker.cpp (lines 918 - 921)


Indentation.
```
futures.push_back(
docker->stop(
container.id,
flags.docker_stop_timeout,
true)
  .then([id]() { return id.get(); }));
```



src/slave/containerizer/docker.cpp (lines 1701 - 1708)


Why do you need to do this? Looks like we don't mount until pulling is 
done. Am I missing something?



src/slave/containerizer/docker.cpp (lines 1785 - 1790)


Hum, I think we shouldn't remove the volumes if the container might be 
still running. So can we move the cleanup logics to `___destroy()`?



src/tests/containerizer/docker_containerizer_tests.cpp (lines 1178 - 1217)


ok, can you instead, launch a slave to do that. This is pretty hacky to me.

You can split this test into two: 1) for running containers 2) for orphan 
containers.

Take a look at SlaveRecoveryTest see how to simulate slave failover.



src/tests/containerizer/docker_containerizer_tests.cpp (lines 3217 - 3221)


You can do CREATE+LAUNCH in the same offer cycle, right?



src/tests/containerizer/docker_containerizer_tests.cpp (line 3236)


I might want to write some data to the persistent volume and make sure that 
file exists in the end. See AccessPersistentVolume in PersistentVolumeTest.



src/tests/containerizer/docker_containerizer_tests.cpp (line 3268)


insert a blank line above.



src/tests/containerizer/docker_containerizer_tests.cpp (line 3270)


insert a blank line above.



src/tests/containerizer/docker_containerizer_tests.cpp (line 3285)


Does this still work? Looks like you're not doing rmdir anymore. Am i 
missing something?


- Jie Yu


On Feb. 16, 2016, 3:33 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43015/
> ---
> 
> (Updated Feb. 16, 2016, 3:33 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3413
> https://issues.apache.org/jira/browse/MESOS-3413
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed persistent volumes with docker tasks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
>   src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 645bdcf095145097d8b8c65d592c787417883145 
> 
> Diff: https://reviews.apache.org/r/43015/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

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

(Updated Feb. 16, 2016, 3:33 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Fixed persistent volumes with docker tasks.


Diffs (updated)
-

  src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
  src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
  src/tests/containerizer/docker_containerizer_tests.cpp 
645bdcf095145097d8b8c65d592c787417883145 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

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

(Updated Feb. 16, 2016, 3:01 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Fixed persistent volumes with docker tasks.


Diffs (updated)
-

  src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
  src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
  src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
cb181265482c884b02bdfc576f906aa0dd9f00f2 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
7fdf518deeb388218438245623719f41613d031b 
  src/tests/containerizer/docker_containerizer_tests.cpp 
645bdcf095145097d8b8c65d592c787417883145 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

> On Feb. 16, 2016, 12:29 a.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, lines 1707-1719
> > 
> >
> > Do you need this? Looks like 'unmountPersistentVolumes' will take care 
> > of the umount.

We do still need this just in case the container is destroyed right before it's 
launched but the persistent volumes are already mounted. I think it's better to 
use unmountVolumes here since we only LOG(ERROR) with custom executors case.


- Timothy


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


On Feb. 15, 2016, 3:39 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43015/
> ---
> 
> (Updated Feb. 15, 2016, 3:39 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3413
> https://issues.apache.org/jira/browse/MESOS-3413
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed persistent volumes with docker tasks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
>   src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 645bdcf095145097d8b8c65d592c787417883145 
> 
> Diff: https://reviews.apache.org/r/43015/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Jie Yu]

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




src/slave/containerizer/docker.cpp (line 396)


Looks like we use 'directory' consistently in this file for container 
working directory. So please

s/sandboxDirectory/directory/



src/slave/containerizer/docker.cpp (line 397)


s/currentResources/current/



src/slave/containerizer/docker.cpp (line 398)


s/newResources/updated/



src/slave/containerizer/docker.cpp (lines 420 - 424)


You may want to make sure `slave.work_dir` is a shared mount in its own 
peer group (see code in `LinuxFilesystemIsolatorProcess::create` and also 
ticket https://issues.apache.org/jira/browse/MESOS-3483).

The reason we need to do that is because when we fork an executor process 
(in Mesos containerizer) with a new mount namespace, if the mount is a private 
mount, the mount in the new mount namespace will be a private mount as well, 
holding an extra reference to the mount. That mount can be for other 
containers. When the slave remove the mount followed by an rmdir (removing the 
mount point), the rmdir could fail due to the extra references to the mount.

Although we did some best effort umount in the new mount namespace (trying 
to umount any irrelevant mounts), but race still exists. For instance, while 
we're umounting, the slave tries to umount+rmdir for some mounts.



src/slave/containerizer/docker.cpp (lines 512 - 513)


Returning a failure doesn't match the existing semantics. Could you instead 
use a LOG(ERROR) here?



src/slave/containerizer/docker.cpp (line 518)


Hum, can you make 'updatePersistentVolumes' a member function so that you 
have access to 'flags' and do not need to pass in this parameter.



src/slave/containerizer/docker.cpp (line 522)


Insert a blank line above.



src/slave/containerizer/docker.cpp (lines 1281 - 1289)


This is useless right now, isn't it? Can you remove it and instead, drop a 
TODO here. To fully support updating volumes, we need to have docker's mount 
propagation support.



src/slave/containerizer/docker.cpp (lines 1707 - 1719)


Do you need this? Looks like 'unmountPersistentVolumes' will take care of 
the umount.


- Jie Yu


On Feb. 15, 2016, 3:39 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43015/
> ---
> 
> (Updated Feb. 15, 2016, 3:39 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3413
> https://issues.apache.org/jira/browse/MESOS-3413
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed persistent volumes with docker tasks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
>   src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 645bdcf095145097d8b8c65d592c787417883145 
> 
> Diff: https://reviews.apache.org/r/43015/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

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

(Updated Feb. 15, 2016, 3:39 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Fixed persistent volumes with docker tasks.


Diffs (updated)
-

  src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
  src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
  src/tests/containerizer/docker_containerizer_tests.cpp 
645bdcf095145097d8b8c65d592c787417883145 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

> On Feb. 15, 2016, 3:47 a.m., Guangya Liu wrote:
> > src/slave/containerizer/docker.cpp, line 505
> > 
> >
> > What about add `containerId` in the log message?

It's not added in all the other places. I see you have a commit adding it to 
the failure message, let's rebase it once this is merged soon.


- Timothy


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


On Feb. 14, 2016, 8:38 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43015/
> ---
> 
> (Updated Feb. 14, 2016, 8:38 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3413
> https://issues.apache.org/jira/browse/MESOS-3413
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed persistent volumes with docker tasks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
>   src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 645bdcf095145097d8b8c65d592c787417883145 
> 
> Diff: https://reviews.apache.org/r/43015/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

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

(Updated Feb. 15, 2016, 3:38 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Fixed persistent volumes with docker tasks.


Diffs (updated)
-

  src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
  src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
  src/tests/containerizer/docker_containerizer_tests.cpp 
645bdcf095145097d8b8c65d592c787417883145 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Guangya Liu]

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




src/slave/containerizer/docker.cpp (line 505)


What about add `containerId` in the log message?



src/tests/containerizer/docker_containerizer_tests.cpp (line 3149)


s/by default/the default

There are indeed patches trying to update all `by default` to `the 
default`: https://reviews.apache.org/r/42689/



src/tests/containerizer/docker_containerizer_tests.cpp (line 3160)


Why not `ASSERT_EQ(1u, offers2.get().size());`? This seems more accurate


- Guangya Liu


On 二月 14, 2016, 8:38 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43015/
> ---
> 
> (Updated 二月 14, 2016, 8:38 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3413
> https://issues.apache.org/jira/browse/MESOS-3413
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed persistent volumes with docker tasks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
>   src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 645bdcf095145097d8b8c65d592c787417883145 
> 
> Diff: https://reviews.apache.org/r/43015/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43015]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 14, 2016, 8:38 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43015/
> ---
> 
> (Updated Feb. 14, 2016, 8:38 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3413
> https://issues.apache.org/jira/browse/MESOS-3413
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed persistent volumes with docker tasks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
>   src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 645bdcf095145097d8b8c65d592c787417883145 
> 
> Diff: https://reviews.apache.org/r/43015/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

> On Feb. 8, 2016, 8:44 p.m., Jie Yu wrote:
> > I realized a tricky part in the recovery path while dealing with potential 
> > conflicts with MesosContainerizer (LinuxFilesystemIsolator). For instance, 
> > will the LinuxFilesystemIsolator try to umount some persistent volume 
> > mounts that belongs to docker containerizer because it'll treat them as 
> > orphans. After checking the code, we're lucky here because of the way 
> > LinxuFilesystemIsolator detects orphans in LinuxFilesystemIsolator. There 
> > will be a mount from 'directory' (e.g., /var/lib/mesos/...) -> 'sandbox' 
> > (e.g., rootfs/mnt/mesos/sandbox). If we cannot find such a mount in 
> > LinuxFilesystemIsolator, it'll not try to umount volumes under 'directory'.
> > 
> > However, I don't see any cleanup code during recovery in this patch. Do we 
> > need to cleanup volume mounts for orphan docker containers?

Yes you're right, I've added clean up code in recovery now.


- Timothy


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


On Feb. 9, 2016, 2:32 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43015/
> ---
> 
> (Updated Feb. 9, 2016, 2:32 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3413
> https://issues.apache.org/jira/browse/MESOS-3413
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed persistent volumes with docker tasks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
>   src/slave/containerizer/docker.cpp 2887cb4a01febbbf276026e584ffc466289e10c9 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 645bdcf095145097d8b8c65d592c787417883145 
> 
> Diff: https://reviews.apache.org/r/43015/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

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

(Updated Feb. 14, 2016, 8:38 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Fixed persistent volumes with docker tasks.


Diffs (updated)
-

  src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
  src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc 
  src/tests/containerizer/docker_containerizer_tests.cpp 
645bdcf095145097d8b8c65d592c787417883145 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

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

(Updated Feb. 9, 2016, 2:32 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Fixed persistent volumes with docker tasks.


Diffs
-

  src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
  src/slave/containerizer/docker.cpp 2887cb4a01febbbf276026e584ffc466289e10c9 
  src/tests/containerizer/docker_containerizer_tests.cpp 
645bdcf095145097d8b8c65d592c787417883145 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Jie Yu]

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



I realized a tricky part in the recovery path while dealing with potential 
conflicts with MesosContainerizer (LinuxFilesystemIsolator). For instance, will 
the LinuxFilesystemIsolator try to umount some persistent volume mounts that 
belongs to docker containerizer because it'll treat them as orphans. After 
checking the code, we're lucky here because of the way LinxuFilesystemIsolator 
detects orphans in LinuxFilesystemIsolator. There will be a mount from 
'directory' (e.g., /var/lib/mesos/...) -> 'sandbox' (e.g., 
rootfs/mnt/mesos/sandbox). If we cannot find such a mount in 
LinuxFilesystemIsolator, it'll not try to umount volumes under 'directory'.

However, I don't see any cleanup code during recovery in this patch. Do we need 
to cleanup volume mounts for orphan docker containers?

- Jie Yu


On Jan. 30, 2016, 10:43 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43015/
> ---
> 
> (Updated Jan. 30, 2016, 10:43 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed persistent volumes with docker tasks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
>   src/slave/containerizer/docker.cpp 2887cb4a01febbbf276026e584ffc466289e10c9 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 645bdcf095145097d8b8c65d592c787417883145 
> 
> Diff: https://reviews.apache.org/r/43015/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

> On Jan. 30, 2016, 7:31 p.m., Joerg Schad wrote:
> > src/docker/executor.cpp, line 135
> > 
> >
> > I am not sure why we are printing this, but shouldn't we at least check 
> > for errors on os::shell?

Thought I alrady removed all of these, good catch.


- Timothy


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


On Jan. 30, 2016, 5:25 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43015/
> ---
> 
> (Updated Jan. 30, 2016, 5:25 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed persistent volumes with docker tasks.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
>   src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
>   src/slave/containerizer/docker.cpp 2887cb4a01febbbf276026e584ffc466289e10c9 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 645bdcf095145097d8b8c65d592c787417883145 
> 
> Diff: https://reviews.apache.org/r/43015/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

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

(Updated Jan. 30, 2016, 10:43 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Fixed persistent volumes with docker tasks.


Diffs (updated)
-

  src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
  src/slave/containerizer/docker.cpp 2887cb4a01febbbf276026e584ffc466289e10c9 
  src/tests/containerizer/docker_containerizer_tests.cpp 
645bdcf095145097d8b8c65d592c787417883145 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

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




src/slave/containerizer/docker.cpp (line 992)


It can't


- Timothy Chen


On Jan. 30, 2016, 5:25 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43015/
> ---
> 
> (Updated Jan. 30, 2016, 5:25 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed persistent volumes with docker tasks.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
>   src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
>   src/slave/containerizer/docker.cpp 2887cb4a01febbbf276026e584ffc466289e10c9 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 645bdcf095145097d8b8c65d592c787417883145 
> 
> Diff: https://reviews.apache.org/r/43015/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 43015: Fixed persistent volumes with docker tasks.

[Joerg Schad]

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



Thanks for fixing this! Is there a ticket with the issue? I will do a more 
complete review tomorrow.


src/docker/executor.cpp (line 135)


I am not sure why we are printing this, but shouldn't we at least check for 
errors on os::shell?



src/slave/containerizer/docker.cpp (line 522)


Just for  my understanding: We assume the docker containerizer to only run 
on linux? Maybe we could add a short comment.



src/slave/containerizer/docker.cpp (line 992)


This looks like it could fit in a single line.


- Joerg Schad


On Jan. 30, 2016, 5:25 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43015/
> ---
> 
> (Updated Jan. 30, 2016, 5:25 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed persistent volumes with docker tasks.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
>   src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
>   src/slave/containerizer/docker.cpp 2887cb4a01febbbf276026e584ffc466289e10c9 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 645bdcf095145097d8b8c65d592c787417883145 
> 
> Diff: https://reviews.apache.org/r/43015/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Review Request 43015: Fixed persistent volumes with docker tasks.

[Timothy Chen]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Fixed persistent volumes with docker tasks.


Diffs
-

  src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
  src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 
  src/slave/containerizer/docker.cpp 2887cb4a01febbbf276026e584ffc466289e10c9 
  src/tests/containerizer/docker_containerizer_tests.cpp 
645bdcf095145097d8b8c65d592c787417883145 

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


Testing
---

make check


Thanks,

Timothy Chen