[mesos] 01/02: Erased `Info` struct before unmouting volumes in Docker volume isolator.

[qianzhang]

This is an automated email from the ASF dual-hosted git repository.

qianzhang pushed a commit to branch 1.7.x
in repository https://gitbox.apache.org/repos/asf/mesos.git

commit 819b9d8345e701321067f3b14ad2bb78b60d285c
Author: Qian Zhang <zhq527...@gmail.com>
AuthorDate: Fri May 15 10:23:51 2020 +0800

    Erased `Info` struct before unmouting volumes in Docker volume isolator.
    
    Currently when `DockerVolumeIsolatorProcess::cleanup()` is called, we will
    unmount the volume first, and if the unmount operation fails we will NOT
    erase the container's `Info` struct from `infos`. This is problematic
    because the remaining `Info` in `infos` will cause the reference count of
    the volume is greater than 0, but actually the volume is not being used by
    any containers. That means we may never get a chance to unmount this volume
    on this agent, furthermore if it is an EBS volume, it cannot be used by any
    tasks launched on any other agents since a EBS volume can only be attached
    to one node at a time. The only workaround would manually unmount the 
volume.
    
    So in this patch `DockerVolumeIsolatorProcess::cleanup()` is updated to 
erase
    container's `Info` struct before unmounting volumes.
    
    Review: https://reviews.apache.org/r/72516
---
 .../containerizer/mesos/isolators/docker/volume/isolator.cpp | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
b/src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp
index c924dde..d2d741a 100644
--- a/src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp
+++ b/src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp
@@ -646,6 +646,13 @@ Future<Nothing> DockerVolumeIsolatorProcess::cleanup(
     futures.push_back(this->unmount(volume.driver(), volume.name()));
   }
 
+  // Erase the `Info` struct of this container before unmounting the volumes.
+  // This is to ensure the reference count of the volume will not be wrongly
+  // increased if unmounting volumes fail, otherwise next time when another
+  // container using the same volume is destroyed, we would NOT unmount the
+  // volume since its reference count would be larger than 1.
+  infos.erase(containerId);
+
   return await(futures)
     .then(defer(
         PID<DockerVolumeIsolatorProcess>(this),
@@ -659,8 +666,6 @@ Future<Nothing> DockerVolumeIsolatorProcess::_cleanup(
     const ContainerID& containerId,
     const vector<Future<Nothing>>& futures)
 {
-  CHECK(infos.contains(containerId));
-
   vector<string> messages;
   foreach (const Future<Nothing>& future, futures) {
     if (!future.isReady()) {
@@ -685,9 +690,6 @@ Future<Nothing> DockerVolumeIsolatorProcess::_cleanup(
   LOG(INFO) << "Removed the checkpoint directory at '" << containerDir
             << "' for container " << containerId;
 
-  // Remove all this container's docker volume information from infos.
-  infos.erase(containerId);
-
   return Nothing();
 }