From reviews-return-92297-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Tue Aug 18 16:49:05 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 6630D1A517 for ; Tue, 18 Aug 2020 16:49:05 +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 169CF125008 for ; Tue, 18 Aug 2020 16:49:05 +0000 (UTC) Received: (qmail 12764 invoked by uid 500); 18 Aug 2020 16:49:04 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 12733 invoked by uid 500); 18 Aug 2020 16:49:04 -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 12705 invoked by uid 99); 18 Aug 2020 16:49:04 -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; Tue, 18 Aug 2020 16:49:04 +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 B8124C0267 for ; Tue, 18 Aug 2020 16:49:03 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd1-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 4.56 X-Spam-Level: **** X-Spam-Status: No, score=4.56 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.249, HTML_MESSAGE=0.2, KAM_DMARC_NONE=0.25, KAM_DMARC_STATUS=0.01, KAM_LAZY_DOMAIN_SECURITY=1, KHOP_HELO_FCRDNS=0.398, NML_ADSP_CUSTOM_MED=1.2, SPF_HELO_NONE=0.001, SPF_NONE=0.001] autolearn=disabled Received: from mx1-ec2-va.apache.org ([10.40.0.8]) by localhost (spamd1-us-west.apache.org [10.40.0.7]) (amavisd-new, port 10024) with ESMTP id XvPrGh1DpUAo for ; Tue, 18 Aug 2020 16:49:01 +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 28258C2550 for ; Tue, 18 Aug 2020 16:09:52 +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 CB0A4160ECB; Tue, 18 Aug 2020 16:09:50 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============2684069921084258490==" MIME-Version: 1.0 Subject: Re: Review Request 72759: Improved CSI service manager to set node ID for managed CSI plugins. From: Qian Zhang To: Andrei Budnik , Greg Mann Cc: Mesos Reviewbot , Qian Zhang , mesos Date: Tue, 18 Aug 2020 16:09:50 -0000 Message-ID: <20200818160950.26191.65485@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/72759/ X-Sender: Qian Zhang References: <20200813222111.6277.45425@reviews-vm-he-fi.apache.org> In-Reply-To: <20200813222111.6277.45425@reviews-vm-he-fi.apache.org> Reply-To: Qian Zhang X-ReviewRequest-Repository: mesos --===============2684069921084258490== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > On Aug. 14, 2020, 6:21 a.m., Greg Mann wrote: > > src/csi/service_manager.cpp > > Lines 740 (patched) > > > > > > Where does this env var name come from, 'MESOS_NODE_ID'? > > Qian Zhang wrote: > Orginially I wanted to just name it `NODE_ID`, however I think it is too generic and may be conflict with other env var used by the CSI plugin. So I changed to add a `MESOS_` prefix just like other env vars that Mesos sets for containers, e.g. `MESOS_FRAMEWORK_ID`, `MESOS_SLAVE_ID`, `MESOS_AGENT_ENDPOINT`. Or maybe we should name it `MESOS_AGENT_IP`? > > Greg Mann wrote: > Hm, I think this may not yield a good ID for the agent, since I think it's possible that the value of `self().address.ip` could be the same across all nodes in the cluster - I think it could be `0.0.0.0`/`INADDR_ANY`, for example. Do you think we need to inject the Mesos agent ID into the service manager so that we can use it as the node ID here? I think `self().address.ip` could not be `0.0.0.0/INADDR_ANY`, see https://github.com/apache/mesos/blob/1.10.0/3rdparty/libprocess/src/process.cpp#L1142:L1159 for details. > Do you think we need to inject the Mesos agent ID into the service manager so that we can use it as the node ID here? Yeah, that's my original thought, but it may involve a bit more code changes in some other components, like CSI server, SLRP. But after second thought I think it should be OK and makes more sense to use Mesos agent ID. I have updated this patch accordingly. And could you please update https://reviews.apache.org/r/72761 by passing Mesos agent ID to CSI server (e.g. as a parameter of `CSIServer::start()`). - Qian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72759/#review221574 ----------------------------------------------------------- On Aug. 12, 2020, 7:47 p.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72759/ > ----------------------------------------------------------- > > (Updated Aug. 12, 2020, 7:47 p.m.) > > > Review request for mesos, Andrei Budnik and Greg Mann. > > > Bugs: MESOS-10175 > https://issues.apache.org/jira/browse/MESOS-10175 > > > Repository: mesos > > > Description > ------- > > Improved CSI service manager to set node ID for managed CSI plugins. > > > Diffs > ----- > > src/csi/service_manager.cpp 7a8d8e5dc3c3bae5251284652d73b33ca554f783 > > > Diff: https://reviews.apache.org/r/72759/diff/1/ > > > Testing > ------- > > > Thanks, > > Qian Zhang > > --===============2684069921084258490==--