From reviews-return-5925-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Mon Jul 27 23:58:41 2015 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 D422E18123 for ; Mon, 27 Jul 2015 23:58:41 +0000 (UTC) Received: (qmail 75186 invoked by uid 500); 27 Jul 2015 23:58:29 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 75162 invoked by uid 500); 27 Jul 2015 23:58:29 -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 75144 invoked by uid 99); 27 Jul 2015 23:58:29 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 27 Jul 2015 23:58:29 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 3F5ADD6959; Mon, 27 Jul 2015 23:58:28 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============0159398642532313209==" MIME-Version: 1.0 Subject: Re: Review Request 36429: Add filesystem/linux isolator for persistent volumes. From: "Jie Yu" To: "Vinod Kone" , "Timothy Chen" , "Jie Yu" Cc: "Ian Downes" , "mesos" Date: Mon, 27 Jul 2015 23:58:28 -0000 Message-ID: <20150727235828.28761.12339@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/36429/ X-Sender: "Jie Yu" References: <20150712044624.1572.25304@reviews.apache.org> In-Reply-To: <20150712044624.1572.25304@reviews.apache.org> Reply-To: "Jie Yu" X-ReviewRequest-Repository: mesos --===============0159398642532313209== 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/36429/#review93143 ----------------------------------------------------------- Please do a rebase first since quite a lot of the stuff have been changed. Also, there's no test yet. Do you have tests in a subsequent patch? I think we might need a TestProvisioner that's based on copying the host file systems so that we can test the file system isolators. We can unify this with the `BasicLinuxChroot` in launch_tests.cpp. What do you think? src/slave/containerizer/isolators/filesystem/linux.cpp (lines 130 - 148) Can you elaborate in the comment about why this is needed? I don't follow this part. Also, I am wondering what if 'directory' itself contains symbolic links? For example, 'directory=/var/lib/mesos/...' and '/var' is a symbolic link to '/xxx/var'. Is that a real issue observerd? Or we can drop a TODO instead for now? src/slave/containerizer/isolators/filesystem/linux.cpp (line 165) s/!rootfs.isSome()/rootfs.isNone() src/slave/containerizer/isolators/filesystem/linux.cpp (lines 176 - 195) Again, this part is not quite obvious to me. Can you explain why this is needed? src/slave/containerizer/isolators/filesystem/linux.cpp (line 185) Failed to create 'container_path'? src/slave/containerizer/isolators/filesystem/linux.cpp (line 188) What if 'rootfs' itself has symbolic links? Do we have code to guarantee that it won't be the case? src/slave/containerizer/isolators/filesystem/linux.cpp (line 214) Can you print the container type if this check fails: ``` CHECK(...) << "Unexpected container type " << ... ``` src/slave/containerizer/isolators/filesystem/linux.cpp (lines 320 - 323) You may want to check if some volumes already exist before mounting them because of the same reason you mentioned here. src/slave/containerizer/isolators/filesystem/linux.cpp (lines 373 - 374) The logic here needs to be adjusted. Please refer to https://issues.apache.org/jira/browse/MESOS-3124 https://reviews.apache.org/r/36684 src/slave/containerizer/isolators/filesystem/linux.cpp (line 428) I still don't get this part. Correct me if I'm wrong. Say the sandbox in the host file system (i.e., `info->directory`) is `DIRECTORY`. Persistent volumes are mounted at `$DIRECTORY/volume1`, `$DIRECTORY/volume2`, right? Those volumes's target does not meet `strings::startsWith(entry->target, info->rootfs.get()`, right? Also, if `info->rootfs.isNone()`, do you also need to cleanup persistent volume mounts? - Jie Yu On July 12, 2015, 4:46 a.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36429/ > ----------------------------------------------------------- > > (Updated July 12, 2015, 4:46 a.m.) > > > Review request for mesos, Jie Yu, Timothy Chen, and Vinod Kone. > > > Repository: mesos > > > Description > ------- > > Moved filesystem/linux from review https://reviews.apache.org/r/34135/ > > > Diffs > ----- > > src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c > src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION > src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION > src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 > > Diff: https://reviews.apache.org/r/36429/diff/ > > > Testing > ------- > > > Thanks, > > Ian Downes > > --===============0159398642532313209==--