Re: Review Request 44196: Fixed MesosContainerizer orphaned persistent volume recovery.

2016-03-02 Thread Mesos ReviewBot 

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



Bad patch!

Reviews applied: [44196, 44122]

Failed command: ./support/apply-review.sh -n -r 44122

Error:
2016-03-02 11:42:11 URL:https://reviews.apache.org/r/44122/diff/raw/ 
[10904/10904] -> "44122.patch" [1]
error: patch failed: src/tests/containerizer/filesystem_isolator_tests.cpp:73
error: src/tests/containerizer/filesystem_isolator_tests.cpp: patch does not 
apply

Full log: https://builds.apache.org/job/mesos-reviewbot/11776/console

- Mesos ReviewBot


On March 2, 2016, 3:42 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44196/
> ---
> 
> (Updated March 2, 2016, 3:42 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4824
> https://issues.apache.org/jira/browse/MESOS-4824
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds extra mount-table checking logic specifically for orphaned persistent 
> volumes that can be safely cleaned up.  This includes "known" orphans (i.e. 
> containers detected via the `Launcher`).
> 
> Also adds some extra helpers in `slave::paths`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 7fdf518deeb388218438245623719f41613d031b 
>   src/slave/paths.hpp 5ae3a2b86bf76859e0ffb78be2644af56bc88d49 
>   src/slave/paths.cpp 6d9dad59386fb890267923f35edabbdf54fb39c6 
>   src/tests/paths_tests.cpp 4c15ebc514e5d1714b243432eeff5377bb21b93f 
> 
> Diff: https://reviews.apache.org/r/44196/diff/
> 
> 
> Testing
> ---
> 
> Tests added in previous review now pass.  i.e.
> ```
> GLOG_v=1 sudo -E bin/mesos-tests.sh 
> --gtest_filter="*MesosContainerizerRecoverOrphanedVolumes*" --verbose
> GLOG_v=1 sudo -E bin/mesos-tests.sh 
> --gtest_filter="*ChangeRootFilesystemOrphanedPersistentVolume" --verbose
> ```
> 
> Confirmed that the right log messages show up:
> ```
> I0302 00:51:01.100466  1610 linux.cpp:782] Unmounting volume 
> '/tmp/DiskResource_PersistentVolumeTest_ROOT_MesosContainerizerRecoverOrphanedVolumes_0_smk5ZU/slaves/a6cd56eb-a106-4c2a-b2c7-b3b97a7a308f-S0/frameworks/a6cd56eb-a106-4c2a-b2c7-b3b97a7a308f-/executors/cdbae703-603d-4e4c-a026-326dbc8c9ce2/runs/8742378c-9a5d-43d6-af31-0c7fe1f5c4c7/path1'
>  for container 8742378c-9a5d-43d6-af31-0c7fe1f5c4c7
> I0302 00:51:01.100538  1610 linux.cpp:798] Ignoring unmounting sandbox/work 
> directory for container 8742378c-9a5d-43d6-af31-0c7fe1f5c4c7
> ```
> 
> ---
> 
> CI test results:
> 
> ```
>  | OSX | CentOS 7 | CentOS 6 | Debian 8 | Ubuntu 15.10 | Ubuntu 14 | 
> Ubuntu 12 |
>  Non-SSL |  :) | $| *|:)|   :) |:) |  
>:)|
> With-SSL |  :) | $|%*|#@|   ^@ |:) |  
>:)|
> 
>  :) = Passed.
>   $ = ProvisionerDockerRegistryPullerTest.ROOT_INTERNET_CURL_ShellCommand
>   * = DockerContainerizerTest.ROOT_DOCKER_LaunchWithPersistentVolumes
>   % = MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery
>   ^ = CgroupsAnyHierarchyWithFreezerTest.ROOT_CGROUPS_DestroyTracedProcess
>   # = SlaveRecoveryTest/0.Reboot
>   @ = SlaveRecoveryTest/0.RecoverTerminatedExecutor
> 
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 44196: Fixed MesosContainerizer orphaned persistent volume recovery.

2016-03-01 Thread Jie Yu 

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




src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (line 282)


This should be path::getExecutorRunPath(xx,xx,xx,xx). entry.target is the 
directory/mountpoint, not directory itself.


- Jie Yu


On March 2, 2016, 3:42 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44196/
> ---
> 
> (Updated March 2, 2016, 3:42 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4824
> https://issues.apache.org/jira/browse/MESOS-4824
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds extra mount-table checking logic specifically for orphaned persistent 
> volumes that can be safely cleaned up.  This includes "known" orphans (i.e. 
> containers detected via the `Launcher`).
> 
> Also adds some extra helpers in `slave::paths`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 7fdf518deeb388218438245623719f41613d031b 
>   src/slave/paths.hpp 5ae3a2b86bf76859e0ffb78be2644af56bc88d49 
>   src/slave/paths.cpp 6d9dad59386fb890267923f35edabbdf54fb39c6 
>   src/tests/paths_tests.cpp 4c15ebc514e5d1714b243432eeff5377bb21b93f 
> 
> Diff: https://reviews.apache.org/r/44196/diff/
> 
> 
> Testing
> ---
> 
> Tests added in previous review now pass.  i.e.
> ```
> GLOG_v=1 sudo -E bin/mesos-tests.sh 
> --gtest_filter="*MesosContainerizerRecoverOrphanedVolumes*" --verbose
> GLOG_v=1 sudo -E bin/mesos-tests.sh 
> --gtest_filter="*ChangeRootFilesystemOrphanedPersistentVolume" --verbose
> ```
> 
> Confirmed that the right log messages show up:
> ```
> I0302 00:51:01.100466  1610 linux.cpp:782] Unmounting volume 
> '/tmp/DiskResource_PersistentVolumeTest_ROOT_MesosContainerizerRecoverOrphanedVolumes_0_smk5ZU/slaves/a6cd56eb-a106-4c2a-b2c7-b3b97a7a308f-S0/frameworks/a6cd56eb-a106-4c2a-b2c7-b3b97a7a308f-/executors/cdbae703-603d-4e4c-a026-326dbc8c9ce2/runs/8742378c-9a5d-43d6-af31-0c7fe1f5c4c7/path1'
>  for container 8742378c-9a5d-43d6-af31-0c7fe1f5c4c7
> I0302 00:51:01.100538  1610 linux.cpp:798] Ignoring unmounting sandbox/work 
> directory for container 8742378c-9a5d-43d6-af31-0c7fe1f5c4c7
> ```
> 
> ---
> 
> CI test results:
> 
> ```
>  | OSX | CentOS 7 | CentOS 6 | Debian 8 | Ubuntu 15.10 | Ubuntu 14 | 
> Ubuntu 12 |
>  Non-SSL |  :) | $| *|:)|   :) |:) |  
>:)|
> With-SSL |  :) | $|%*|#@|   ^@ |:) |  
>:)|
> 
>  :) = Passed.
>   $ = ProvisionerDockerRegistryPullerTest.ROOT_INTERNET_CURL_ShellCommand
>   * = DockerContainerizerTest.ROOT_DOCKER_LaunchWithPersistentVolumes
>   % = MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery
>   ^ = CgroupsAnyHierarchyWithFreezerTest.ROOT_CGROUPS_DestroyTracedProcess
>   # = SlaveRecoveryTest/0.Reboot
>   @ = SlaveRecoveryTest/0.RecoverTerminatedExecutor
> 
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 44196: Fixed MesosContainerizer orphaned persistent volume recovery.

2016-03-01 Thread Jie Yu 

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


Fix it, then Ship it!




Thanks! I'll fix the issues for you and committing it now.


src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (line 255)


I would put the following code in else, instead of using a continue here.



src/slave/paths.cpp (line 61)


Let's use const char XXX_YYY[] = ""; here to avoid non-POD global 
variables.



src/slave/paths.cpp (line 75)


you can use `path::join(_rootDir, "");` here


- Jie Yu


On March 2, 2016, 3:42 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44196/
> ---
> 
> (Updated March 2, 2016, 3:42 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4824
> https://issues.apache.org/jira/browse/MESOS-4824
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds extra mount-table checking logic specifically for orphaned persistent 
> volumes that can be safely cleaned up.  This includes "known" orphans (i.e. 
> containers detected via the `Launcher`).
> 
> Also adds some extra helpers in `slave::paths`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 7fdf518deeb388218438245623719f41613d031b 
>   src/slave/paths.hpp 5ae3a2b86bf76859e0ffb78be2644af56bc88d49 
>   src/slave/paths.cpp 6d9dad59386fb890267923f35edabbdf54fb39c6 
>   src/tests/paths_tests.cpp 4c15ebc514e5d1714b243432eeff5377bb21b93f 
> 
> Diff: https://reviews.apache.org/r/44196/diff/
> 
> 
> Testing
> ---
> 
> Tests added in previous review now pass.  i.e.
> ```
> GLOG_v=1 sudo -E bin/mesos-tests.sh 
> --gtest_filter="*MesosContainerizerRecoverOrphanedVolumes*" --verbose
> GLOG_v=1 sudo -E bin/mesos-tests.sh 
> --gtest_filter="*ChangeRootFilesystemOrphanedPersistentVolume" --verbose
> ```
> 
> Confirmed that the right log messages show up:
> ```
> I0302 00:51:01.100466  1610 linux.cpp:782] Unmounting volume 
> '/tmp/DiskResource_PersistentVolumeTest_ROOT_MesosContainerizerRecoverOrphanedVolumes_0_smk5ZU/slaves/a6cd56eb-a106-4c2a-b2c7-b3b97a7a308f-S0/frameworks/a6cd56eb-a106-4c2a-b2c7-b3b97a7a308f-/executors/cdbae703-603d-4e4c-a026-326dbc8c9ce2/runs/8742378c-9a5d-43d6-af31-0c7fe1f5c4c7/path1'
>  for container 8742378c-9a5d-43d6-af31-0c7fe1f5c4c7
> I0302 00:51:01.100538  1610 linux.cpp:798] Ignoring unmounting sandbox/work 
> directory for container 8742378c-9a5d-43d6-af31-0c7fe1f5c4c7
> ```
> 
> ---
> 
> CI test results:
> 
> ```
>  | OSX | CentOS 7 | CentOS 6 | Debian 8 | Ubuntu 15.10 | Ubuntu 14 | 
> Ubuntu 12 |
>  Non-SSL |  :) | $| *|:)|   :) |:) |  
>:)|
> With-SSL |  :) | $|%*|#@|   ^@ |:) |  
>:)|
> 
>  :) = Passed.
>   $ = ProvisionerDockerRegistryPullerTest.ROOT_INTERNET_CURL_ShellCommand
>   * = DockerContainerizerTest.ROOT_DOCKER_LaunchWithPersistentVolumes
>   % = MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery
>   ^ = CgroupsAnyHierarchyWithFreezerTest.ROOT_CGROUPS_DestroyTracedProcess
>   # = SlaveRecoveryTest/0.Reboot
>   @ = SlaveRecoveryTest/0.RecoverTerminatedExecutor
> 
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 44196: Fixed MesosContainerizer orphaned persistent volume recovery.

2016-03-01 Thread Joseph Wu 

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

(Updated March 1, 2016, 7:42 p.m.)


Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.


Changes
---

Reworked according to offline comments.


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


Repository: mesos


Description
---

Adds extra mount-table checking logic specifically for orphaned persistent 
volumes that can be safely cleaned up.  This includes "known" orphans (i.e. 
containers detected via the `Launcher`).

Also adds some extra helpers in `slave::paths`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
7fdf518deeb388218438245623719f41613d031b 
  src/slave/paths.hpp 5ae3a2b86bf76859e0ffb78be2644af56bc88d49 
  src/slave/paths.cpp 6d9dad59386fb890267923f35edabbdf54fb39c6 
  src/tests/paths_tests.cpp 4c15ebc514e5d1714b243432eeff5377bb21b93f 

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


Testing (updated)
---

Tests added in previous review now pass.  i.e.
```
GLOG_v=1 sudo -E bin/mesos-tests.sh 
--gtest_filter="*MesosContainerizerRecoverOrphanedVolumes*" --verbose
GLOG_v=1 sudo -E bin/mesos-tests.sh 
--gtest_filter="*ChangeRootFilesystemOrphanedPersistentVolume" --verbose
```

Confirmed that the right log messages show up:
```
I0302 00:51:01.100466  1610 linux.cpp:782] Unmounting volume 
'/tmp/DiskResource_PersistentVolumeTest_ROOT_MesosContainerizerRecoverOrphanedVolumes_0_smk5ZU/slaves/a6cd56eb-a106-4c2a-b2c7-b3b97a7a308f-S0/frameworks/a6cd56eb-a106-4c2a-b2c7-b3b97a7a308f-/executors/cdbae703-603d-4e4c-a026-326dbc8c9ce2/runs/8742378c-9a5d-43d6-af31-0c7fe1f5c4c7/path1'
 for container 8742378c-9a5d-43d6-af31-0c7fe1f5c4c7
I0302 00:51:01.100538  1610 linux.cpp:798] Ignoring unmounting sandbox/work 
directory for container 8742378c-9a5d-43d6-af31-0c7fe1f5c4c7
```

---

CI test results:

```
 | OSX | CentOS 7 | CentOS 6 | Debian 8 | Ubuntu 15.10 | Ubuntu 14 | 
Ubuntu 12 |
 Non-SSL |  :) | $| *|:)|   :) |:) |
 :)|
With-SSL |  :) | $|%*|#@|   ^@ |:) |
 :)|

 :) = Passed.
  $ = ProvisionerDockerRegistryPullerTest.ROOT_INTERNET_CURL_ShellCommand
  * = DockerContainerizerTest.ROOT_DOCKER_LaunchWithPersistentVolumes
  % = MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery
  ^ = CgroupsAnyHierarchyWithFreezerTest.ROOT_CGROUPS_DestroyTracedProcess
  # = SlaveRecoveryTest/0.Reboot
  @ = SlaveRecoveryTest/0.RecoverTerminatedExecutor

```


Thanks,

Joseph Wu

Re: Review Request 44196: Fixed MesosContainerizer orphaned persistent volume recovery.

2016-03-01 Thread Joseph Wu 

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

(Updated March 1, 2016, 5:11 p.m.)


Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.


Changes
---

Rework the fix into what we (Jie & I) discussed offline.


Summary (updated)
-

Fixed MesosContainerizer orphaned persistent volume recovery.


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


Repository: mesos


Description (updated)
---

Adds extra mount-table checking logic specifically for orphaned persistent 
volumes that can be safely cleaned up.  This includes "known" orphans (i.e. 
containers detected via the `Launcher`).

Also adds some extra helpers in `slave::paths`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
7fdf518deeb388218438245623719f41613d031b 
  src/slave/paths.hpp 5ae3a2b86bf76859e0ffb78be2644af56bc88d49 
  src/slave/paths.cpp 6d9dad59386fb890267923f35edabbdf54fb39c6 
  src/tests/paths_tests.cpp 4c15ebc514e5d1714b243432eeff5377bb21b93f 

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


Testing (updated)
---

Tests added in previous review now pass.  i.e.
```
GLOG_v=1 sudo -E bin/mesos-tests.sh 
--gtest_filter="*MesosContainerizerRecoverOrphanedVolumes*0" --verbose
GLOG_v=1 sudo -E bin/mesos-tests.sh 
--gtest_filter="*ChangeRootFilesystemOrphanedPersistentVolume" --verbose
```

Confirmed that the right log messages show up:
```
I0302 00:51:01.100466  1610 linux.cpp:782] Unmounting volume 
'/tmp/DiskResource_PersistentVolumeTest_ROOT_MesosContainerizerRecoverOrphanedVolumes_0_smk5ZU/slaves/a6cd56eb-a106-4c2a-b2c7-b3b97a7a308f-S0/frameworks/a6cd56eb-a106-4c2a-b2c7-b3b97a7a308f-/executors/cdbae703-603d-4e4c-a026-326dbc8c9ce2/runs/8742378c-9a5d-43d6-af31-0c7fe1f5c4c7/path1'
 for container 8742378c-9a5d-43d6-af31-0c7fe1f5c4c7
I0302 00:51:01.100538  1610 linux.cpp:798] Ignoring unmounting sandbox/work 
directory for container 8742378c-9a5d-43d6-af31-0c7fe1f5c4c7
```

---

CI test results (running)...


Thanks,

Joseph Wu