From reviews-return-25547-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Wed Mar 2 05:56:17 2016 Return-Path: X-Original-To: apmail-mesos-reviews-archive@minotaur.apache.org Delivered-To: apmail-mesos-reviews-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id A8138184F4 for ; Wed, 2 Mar 2016 05:56:17 +0000 (UTC) Received: (qmail 68757 invoked by uid 500); 2 Mar 2016 05:56:17 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 68725 invoked by uid 500); 2 Mar 2016 05:56:17 -0000 Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: reviews@mesos.apache.org Delivered-To: mailing list reviews@mesos.apache.org Received: (qmail 68706 invoked by uid 99); 2 Mar 2016 05:56:17 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 02 Mar 2016 05:56:17 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 279C92E775B; Wed, 2 Mar 2016 05:56:16 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============8012600024422581556==" MIME-Version: 1.0 Subject: Re: Review Request 44196: Fixed MesosContainerizer orphaned persistent volume recovery. From: Jie Yu To: Timothy Chen , Jie Yu , Artem Harutyunyan Cc: Joseph Wu , mesos Date: Wed, 02 Mar 2016 05:56:16 -0000 Message-ID: <20160302055616.26940.35470@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: Jie Yu X-ReviewGroup: mesos X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/44196/ X-Sender: Jie Yu X-ReviewBoard-ShipIt: 1 References: <20160302034243.26940.33121@reviews.apache.org> In-Reply-To: <20160302034243.26940.33121@reviews.apache.org> Reply-To: Jie Yu X-ReviewRequest-Repository: mesos --===============8012600024422581556== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- 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[] = "zzzz"; 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-0000/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 > > --===============8012600024422581556==--