From reviews-return-92523-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Thu Sep 3 08:26:31 2020 Return-Path: X-Original-To: apmail-mesos-reviews-archive@locus.apache.org Delivered-To: apmail-mesos-reviews-archive@locus.apache.org Received: from mailroute1-lw-us.apache.org (mailroute1-lw-us.apache.org [207.244.88.153]) by minotaur.apache.org (Postfix) with ESMTP id 6D9C31A246 for ; Thu, 3 Sep 2020 08:26:31 +0000 (UTC) Received: from mail.apache.org (localhost [127.0.0.1]) by mailroute1-lw-us.apache.org (ASF Mail Server at mailroute1-lw-us.apache.org) with SMTP id 1CF2B121A59 for ; Thu, 3 Sep 2020 08:26:31 +0000 (UTC) Received: (qmail 95456 invoked by uid 500); 3 Sep 2020 08:26:31 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 95430 invoked by uid 500); 3 Sep 2020 08:26:30 -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 95417 invoked by uid 99); 3 Sep 2020 08:26:30 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd2-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 03 Sep 2020 08:26:30 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd2-us-west.apache.org (ASF Mail Server at spamd2-us-west.apache.org) with ESMTP id F11751A4227 for ; Thu, 3 Sep 2020 08:26:29 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd2-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 4.565 X-Spam-Level: **** X-Spam-Status: No, score=4.565 tagged_above=-999 required=6.31 tests=[DKIM_ADSP_CUSTOM_MED=0.001, FORGED_GMAIL_RCVD=1, FREEMAIL_REPLYTO_END_DIGIT=0.25, HEADER_FROM_DIFFERENT_DOMAINS=0.001, HTML_MESSAGE=0.2, KAM_DMARC_NONE=0.25, KAM_DMARC_STATUS=0.01, KAM_LAZY_DOMAIN_SECURITY=1, KAM_LOTSOFHASH=0.25, KHOP_HELO_FCRDNS=0.4, NML_ADSP_CUSTOM_MED=1.2, SPF_HELO_NONE=0.001, SPF_NONE=0.001, URIBL_BLOCKED=0.001] autolearn=disabled Received: from mx1-ec2-va.apache.org ([10.40.0.8]) by localhost (spamd2-us-west.apache.org [10.40.0.9]) (amavisd-new, port 10024) with ESMTP id R4ng1gj11WTE for ; Thu, 3 Sep 2020 08:26:26 +0000 (UTC) Received-SPF: None (mailfrom) identity=mailfrom; client-ip=95.217.165.199; helo=reviews-vm-he-fi.apache.org; envelope-from=noreply@reviews.apache.org; receiver= Received: from reviews-vm-he-fi.apache.org (static.199.165.217.95.clients.your-server.de [95.217.165.199]) by mx1-ec2-va.apache.org (ASF Mail Server at mx1-ec2-va.apache.org) with ESMTP id A2AD5BC27B for ; Thu, 3 Sep 2020 08:26:25 +0000 (UTC) Received: from reviews-vm-he-fi.apache.org (reviews-vm-he-fi.apache.org [127.0.0.1]) by reviews-vm-he-fi.apache.org (Postfix) with ESMTP id BDFE71603B4; Thu, 3 Sep 2020 08:26:24 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============7916622523291849971==" MIME-Version: 1.0 Subject: Re: Review Request 72728: Added tests for the 'volume/csi' isolator. From: Qian Zhang To: Qian Zhang , Andrei Budnik Cc: Mesos Reviewbot , Greg Mann , mesos Date: Thu, 03 Sep 2020 08:26:24 -0000 Message-ID: <20200903082624.2546.6968@reviews-vm-he-fi.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: Qian Zhang X-ReviewGroup: mesos X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/72728/ X-Sender: Qian Zhang References: <20200902133539.13941.93500@reviews-vm-he-fi.apache.org> In-Reply-To: <20200902133539.13941.93500@reviews-vm-he-fi.apache.org> X-ReviewBoard-Diff-For: src/tests/containerizer/volume_csi_isolator_tests.cpp Reply-To: Qian Zhang X-ReviewRequest-Repository: mesos --===============7916622523291849971== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > On Sept. 2, 2020, 9:35 p.m., Qian Zhang wrote: > > src/tests/containerizer/volume_csi_isolator_tests.cpp > > Lines 238 (patched) > > > > > > Do we really need this method? I see it is only called when we create CSI server in `ROOT_PluginConfigAddedAtRuntime`, can we just create the CSI server with NULL secret generator like what we do in `ROOT_InvalidPluginConfig`? > > Greg Mann wrote: > I'd rather leave it in, since it is also used by the subsequent tests in the next patch in this chain. In order to run these tests with authentication enabled, we need to create a secret generator. Since we have some nontrivial authentication code in these code paths, keeping authentication enabled seems like a good idea to me. What do you think? Yeah, I agree. > On Sept. 2, 2020, 9:35 p.m., Qian Zhang wrote: > > src/tests/containerizer/volume_csi_isolator_tests.cpp > > Lines 794 (patched) > > > > > > Usually we use `SUDO_USER` as the non-root user in our unit tests, see https://github.com/apache/mesos/blob/1.10.0/src/tests/containerizer/docker_volume_isolator_tests.cpp#L1335 . And the test name should have the `UNPRIVILEGED_USER_` prefix. > > > > Just realized in a couple of your tests here, we need to pull Docker image `alpine` from Docker Hub, right? Then I think we need to include the prefix `INTERNET_CURL_` in the test name. > > Greg Mann wrote: > Thanks!! > > Greg Mann wrote: > I ended up switching the tests to use a mixture of tasks with a rootfs and tasks without - let me know what you think. SGTM! - Qian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72728/#review221754 ----------------------------------------------------------- On Sept. 3, 2020, 3:05 a.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72728/ > ----------------------------------------------------------- > > (Updated Sept. 3, 2020, 3:05 a.m.) > > > Review request for mesos, Andrei Budnik and Qian Zhang. > > > Bugs: MESOS-10163 > https://issues.apache.org/jira/browse/MESOS-10163 > > > Repository: mesos > > > Description > ------- > > Added tests for the 'volume/csi' isolator. > > > Diffs > ----- > > src/Makefile.am 1043c7b860372a17dba1e84fe5547388cb6a3b63 > src/tests/CMakeLists.txt 6b420d03e85470c16de85c1cbb81ec339142e226 > src/tests/cluster.cpp 3c8685565f9de6c314f6be758368f1eff46e2370 > src/tests/containerizer/volume_csi_isolator_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/72728/diff/14/ > > > Testing > ------- > > `sudo bin/mesos-tests.sh --gtest_filter="*VolumeCSIIsolatorTest*"` > > > Thanks, > > Greg Mann > > --===============7916622523291849971==--