This is an automated email from the ASF dual-hosted git repository. qianzhang pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 2845330fbd78a80fb7e71c6101724655fa254392 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 c547696..2f2f624 100644 --- a/src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp +++ b/src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp @@ -658,6 +658,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), @@ -671,8 +678,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()) { @@ -697,9 +702,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(); }