From reviews-return-92095-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Sun Aug 2 14:31:39 2020 Return-Path: X-Original-To: apmail-mesos-reviews-archive@locus.apache.org Delivered-To: apmail-mesos-reviews-archive@locus.apache.org Received: from mxout1-he-de.apache.org (mxout1-he-de.apache.org [95.216.194.37]) by minotaur.apache.org (Postfix) with ESMTP id 8B6C6194D7 for ; Sun, 2 Aug 2020 14:31:39 +0000 (UTC) Received: from mail.apache.org (mailroute1-lw-us.apache.org [207.244.88.153]) by mxout1-he-de.apache.org (ASF Mail Server at mxout1-he-de.apache.org) with SMTP id 43F2262AD1 for ; Sun, 2 Aug 2020 14:31:38 +0000 (UTC) Received: (qmail 52302 invoked by uid 500); 2 Aug 2020 14:31:37 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 52276 invoked by uid 500); 2 Aug 2020 14:31:37 -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 52263 invoked by uid 99); 2 Aug 2020 14:31:36 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd1-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Sun, 02 Aug 2020 14:31:36 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd1-us-west.apache.org (ASF Mail Server at spamd1-us-west.apache.org) with ESMTP id 19B7BC003C for ; Sun, 2 Aug 2020 14:31:36 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd1-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 4.314 X-Spam-Level: **** X-Spam-Status: No, score=4.314 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, KHOP_HELO_FCRDNS=0.4, NML_ADSP_CUSTOM_MED=1.2, SPF_HELO_NONE=0.001, SPF_NONE=0.001] autolearn=disabled Received: from mx1-he-de.apache.org ([10.40.0.8]) by localhost (spamd1-us-west.apache.org [10.40.0.7]) (amavisd-new, port 10024) with ESMTP id 6aSzJJqjpAWU for ; Sun, 2 Aug 2020 14:31:34 +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-he-de.apache.org (ASF Mail Server at mx1-he-de.apache.org) with ESMTP id C8E507F875 for ; Sun, 2 Aug 2020 14:31:33 +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 576B616025E; Sun, 2 Aug 2020 14:31:26 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============5768960154499034930==" MIME-Version: 1.0 Subject: Re: Review Request 72716: Added implementation of the CSI server. From: Qian Zhang To: Qian Zhang , Andrei Budnik Cc: Mesos Reviewbot , Greg Mann , mesos Date: Sun, 02 Aug 2020 14:31:26 -0000 Message-ID: <20200802143126.14498.53159@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/72716/ X-Sender: Qian Zhang References: <20200729152713.20915.97430@reviews-vm-he-fi.apache.org> In-Reply-To: <20200729152713.20915.97430@reviews-vm-he-fi.apache.org> X-ReviewBoard-Diff-For: src/slave/csi_server.cpp Reply-To: Qian Zhang X-ReviewRequest-Repository: mesos --===============5768960154499034930== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > On July 29, 2020, 11:27 p.m., Qian Zhang wrote: > > src/slave/csi_server.cpp > > Lines 194 (patched) > > > > > > I think we should set this based on the CSI services in the related `CSIPluginInfo` rather than hard code here, e.g. in future we may support both node and controller services for Portworx. > > > > Or maybe we should not have this parameter at all? I mean `ServiceManager` should be able to figure out the support services of the plugin based on the related `CSIPluginInfo`, right? > > Greg Mann wrote: > Good point. Yea we may want to remove the service information from the plugin info, I'm not sure. I'll leave it there for now and just initialize the volume manager with the correct services here. Could you plesae elaborate a bit on why we may want to remove the service information from the plugin info? And in the latest patch (revision 3), I see you have called `extractServices` to get services when instantiating `ServiceManager` but still hard code `{csi::NODE_SERVICE}` for volume manager, can we just do the same for volume manager? > On July 29, 2020, 11:27 p.m., Qian Zhang wrote: > > src/slave/csi_server.cpp > > Lines 233-235 (patched) > > > > > > I am just curious what would happen if any of the initialization logic fail, how will the failure be propogated back? > > Greg Mann wrote: > I updated the server so that now `start()` returns a future associated with the initialization. I see. And I guess `CSIServer::start()` will be called in `Slave::registered` and `Slave::reregistered`, right? I am just wondering how we are going to handle the returned future there. Are we going to register an `onAny` callback and log an error message if it is a failed future? > On July 29, 2020, 11:27 p.m., Qian Zhang wrote: > > src/slave/csi_server.cpp > > Lines 244-245 (patched) > > > > > > Do we have to use `started` and `initializationCallbacks`? Can we do the similar with https://github.com/apache/mesos/blob/1.10.0/src/csi/v1_volume_manager.cpp#L1336 ? > > Greg Mann wrote: > The reason it's more complicated here is because we may add more "initialization logic" after server construction if publish/unpublish calls are made before the server is started. So we need an approach which will allow us to add more function calls which are executed during startup. I explored another approach while coding but this is what I ended up settling on, but I'm happy to explore other options if we can think of something better. I see currently you put the "initialization logic" (i.e. generate auth token and intialize plugins) in the constructor of `CSIServerProcess`. Can we instead do that in `CSIServerProcess::start()` and do the following in `CSIServer::start()`. ``` Future CSIServer::start() { started = process::dispatch(process.get(), &CSIServerProcess::start); return started; } ``` And then in `CSIServer::publishVolume` and `CSIServer::unpublishVolume` we could do the following: ``` Future CSIServer::publishVolume( const Volume::Source::CSIVolume& volume) { return started .then(process::defer( process.get(), &CSIServerProcess::publishVolume, volume)); } ``` So any publish and unpublish volume calls can only be executed after CSI server is started. HDYT? - Qian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72716/#review221405 ----------------------------------------------------------- On Aug. 1, 2020, 3 a.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72716/ > ----------------------------------------------------------- > > (Updated Aug. 1, 2020, 3 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 implementation of the CSI server. > > > Diffs > ----- > > src/CMakeLists.txt bcb5128f0e61af0d541502e4ed833da0487b7792 > src/Makefile.am a89919dd7d5ccbc4c5fa79d9a83616608f84ef49 > src/slave/csi_server.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/72716/diff/3/ > > > Testing > ------- > > Details at the end of this chain. > > > Thanks, > > Greg Mann > > --===============5768960154499034930==--