Re: [Gen-art] Gen-ART Last Call review of draft-ietf-anima-grasp-api-07

Brian E Carpenter <brian.e.carpenter@gmail.com> Wed, 28 October 2020 20:48 UTC

Return-Path: <brian.e.carpenter@gmail.com>
X-Original-To: gen-art@ietfa.amsl.com
Delivered-To: gen-art@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id DD9243A09CC; Wed, 28 Oct 2020 13:48:09 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.344
X-Spam-Level:
X-Spam-Status: No, score=-2.344 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, NICE_REPLY_A=-0.247, RCVD_IN_DNSWL_BLOCKED=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 6X7NokuGFOz8; Wed, 28 Oct 2020 13:48:08 -0700 (PDT)
Received: from mail-pl1-x630.google.com (mail-pl1-x630.google.com [IPv6:2607:f8b0:4864:20::630]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id E919F3A09C5; Wed, 28 Oct 2020 13:48:07 -0700 (PDT)
Received: by mail-pl1-x630.google.com with SMTP id r3so224602plo.1; Wed, 28 Oct 2020 13:48:07 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:organization:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=Ci/NYt13+HQHOJ6Rg4U4jUrWXEfVUd/r2R+EgyyXvss=; b=HA5WLvn3J6hbU0Fh+tCVfnYxjUnuralGndgTsWQMeDuVhaukKFepPBFGvCJe9i+iit yL6XSdXj9G5NKiRhR5lUJVtEW/BnXPGs20/2MWuVX9/Hv7u7QhTPIDnCRaqLpK4F+x/1 oksg4vPoKM8cbYSNYvVd4Z8RbQfSyMrPta6huYdXUJQ+cAjQsmWYIczRfMx0bN5xWn1y /ym31iwxpzm9+0a8muSN1GYGrdZg0NqMd3nU67tvleUX+bbJEaoBgEm3KQPojbsN2lEj Poy8ZzPutA3Z2xWJYBAGa3R3Lgknu3gb/TD348NiGZfDiqK9p7RzVBzRCx33uGRXmbJ8 tmeA==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=Ci/NYt13+HQHOJ6Rg4U4jUrWXEfVUd/r2R+EgyyXvss=; b=W8W7bPRW534MyUXcIReF6fiuOvv8X5+Xa0KsIkLFpNxJ+1bDDOGDqrJgVKwnhh/qRd yDTqhOGj5/L4Ic0txuFKdOIm4s7hGbt9PnS7IbVDbDa0NewYeCtJo791sdXxOE8JXMul KcDrknkIq1AKDvS97uesxIViTz5qb8HPmz2z+9dPt8aPRhZ9FYVWnazqNPNcpoQCHUup +VQrSxdFpBWynaIXcN1YoJkAfV4IrzwdlTnce0EUHbkRJp7WII85t3olYppj2V/49pIS a+maTJyYtzc277MCjgDO6yCZ5URxfIddwjDnDu/ItDZPMsM3MiUn9KNkJVbTwo1QO9w5 63jw==
X-Gm-Message-State: AOAM531peMdggvOARGcQRgYidmChBJrQRVc77U7PsNRW/2u0sNJgHkeu mUz70H/0DAX4fpbSx6qXOQ7LG+807xk=
X-Google-Smtp-Source: ABdhPJweE0X8WhLwFjj/9rCSIkhToMFseyhnxHA8LLLHSetOMl4gNWJk/4n1xqQ9dtzhYo8ATllgtw==
X-Received: by 2002:a17:902:8c8a:b029:d6:42d5:1cb7 with SMTP id t10-20020a1709028c8ab02900d642d51cb7mr768251plo.73.1603918086752; Wed, 28 Oct 2020 13:48:06 -0700 (PDT)
Received: from [192.168.178.20] ([151.210.130.0]) by smtp.gmail.com with ESMTPSA id t5sm281739pjq.7.2020.10.28.13.48.04 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Wed, 28 Oct 2020 13:48:05 -0700 (PDT)
To: Paul Kyzivat <pkyzivat@alum.mit.edu>, draft-ietf-anima-grasp-api.all@ietf.org
Cc: General Area Review Team <gen-art@ietf.org>
References: <d71097bc-c37a-c322-d478-7435c35cc5c4@alum.mit.edu>
From: Brian E Carpenter <brian.e.carpenter@gmail.com>
Organization: University of Auckland
Message-ID: <4e179b77-2874-a016-f461-dd7ba5749575@gmail.com>
Date: Thu, 29 Oct 2020 09:48:02 +1300
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.9.1
MIME-Version: 1.0
In-Reply-To: <d71097bc-c37a-c322-d478-7435c35cc5c4@alum.mit.edu>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Archived-At: <https://mailarchive.ietf.org/arch/msg/gen-art/stkmNDX0AuAM9cJH69hzJq1PVNE>
Subject: Re: [Gen-art] Gen-ART Last Call review of draft-ietf-anima-grasp-api-07
X-BeenThere: gen-art@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: "GEN-ART: General Area Review Team" <gen-art.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/gen-art>, <mailto:gen-art-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/gen-art/>
List-Post: <mailto:gen-art@ietf.org>
List-Help: <mailto:gen-art-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/gen-art>, <mailto:gen-art-request@ietf.org?subject=subscribe>
X-List-Received-Date: Wed, 28 Oct 2020 20:48:10 -0000

Thanks Paul. We'll try to clarify those points in the text, so I
won't comment in detail on your comments. I can see how your questions
arise for a newcomer to GRASP, so they do need to be answered. It will
take a little while to do this, so the AD will defer the draft to a
later telechat.

Regards
   Brian Carpenter

On 29-Oct-20 04:50, Paul Kyzivat wrote:
> I am the assigned Gen-ART reviewer for this draft. The General Area
> Review Team (Gen-ART) reviews all IETF documents being processed
> by the IESG for the IETF Chair.  Please treat these comments just
> like any other last call comments.
> 
> For more information, please see the FAQ at
> 
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
> 
> Document: draft-ietf-anima-grasp-api-07
> Reviewer: Paul Kyzivat
> Review Date: 2020-10-28
> IETF LC End Date: 2020-10-28
> IESG Telechat date: ?
> 
> This draft is on the right track but has open issues, described in the 
> review.
> 
> General:
> 
> I began this review without any knowledge of Anima. I did read several 
> of the related documents for context, but my overall understanding 
> remains somewhat sketchy. So take my comments with with that in mind. 
> (At least you get a fresh, untainted perspective.)
> 
> Issues:
> 
> Major: 5
> Minor: 6
> Nits:  2
> 
> 1) MAJOR: Unclear model for API function usage
> 
> As I read through sections 2.3.2 through 2.3.6 these I got progressively 
> more confused. I finally concluded that a big picture of how API 
> functions interact is lacking.
> 
> For one thing, there isn't a clear delineation between those calls used 
> by requesters and those used by responders. And then valid sequencing of 
> calls is hard to tease out.
> 
> It would be helpful to have one state model for requesters and another 
> for responders. It may also be helpful to break this out for threaded 
> and event loop variants.
> 
> My subsequent issues regarding these sections are reflective of this 
> confusion.
> 
> 2) MAJOR: What state does GRASP retain for Objectives?
> 
> It is clear that the GRASP must retain the names of registered 
> objectives. There are implications that it must keep more. Seemingly it 
> must retain received flooded objectives, on a per-source basis. It is 
> unclear if it is only for registered objectives, or also for 
> unregistered objectives. This could potentially become a large resource 
> drain if there are lots of nodes flooding values.
> 
> Negotiated objectives seem to be treated differently. It isn't clear to 
> me if GRASP needs to retain copies of their values.
> 
> 3) MAJOR: Consistency of Objective definitions
> 
> In section 2.3.1.2 and elsewhere, presumably all parties that use a 
> particular objective must agree on the values of synch, neg, dry, and 
> the format of the value.
> 
> Apparently you are relying on each caller getting this right. What 
> happens when this isn't the case? How can blame be ascribed so that the 
> problem can be fixed?
> 
> 4) MAJOR: Confusing semantics of 'request_negotiate'
> 
> In section 2.3.4 I don't understand the following:
> 
>           1.  The 'session_nonce' parameter is null.  In this case the
>               negotiation has succeeded immediately (the peer has
>               accepted the request).  The returned 'proffered_objective'
>               contains the value accepted by the peer.
> 
> IIUC this requires a network exchange with the peer. I don't see how 
> this can complete *immediately*. ISTM that this could only complete 
> immediately if it were satisfied from a local cache. That doesn't seem 
> appropriate for this function.
> 
> Similarly, in bullet 2 I don't see how the proffered_objective would be 
> available in the initial call.
> 
> Or is this text only applicable to a threaded implementation with 
> synchronous calls?
> 
> Bullet 2 also says:
> 
>               ... The
>               returned 'proffered_objective' contains the first value
>               proffered by the negotiation peer.  The contents of this
>               instance of the objective must be used in the subsequent
>               negotiation call because it contains the updated loop
>               count, sent by the negotiation peer.
> 
> Do you mean this must be used in the call to negotiate_step? But that 
> would mean asking for what has already been granted. For the the 
> negotiation to be useful I would expect that the next round would ask 
> for a value somewhere between what was originally requested and what was 
> initially offered.
> 
> I think you need to say more about the whole concept of negotiation and 
> how it is expected to work. Also, additional discussion of the purpose 
> and semantics of dry run negotiation.
> 
> One more thing: you don't explain the semantics of 'timeout'. Is it only 
> used locally, to terminate the synchronous form of the call? Or is it 
> transmitted and used by the responder to control how long it might spend 
> trying to fulfill the request before giving up?
> 
> 5) MAJOR: Confusing semantics of 'negotiate_step'
> 
> In section 2.3.4, I'm confused by:
> 
>              Called in the same thread as the
>              preceding 'request_negotiate' or 'listen_negotiate'
> 
> I understand use of this following request_negotiate, but not with 
> listen_negotiate. A negotiation should consist of an ordered sequence of 
> "rounds" of bidding. Allowing both requester and responder to initiate a 
> next step can lead to a disruption of the ordering. I would expect this 
> to only be used by the caller of request_negotiate. What am I missing?
> 
> 6) MINOR(MAJOR?): Confusing semantics of 'negotiate_wait'
> 
> Regarding section 2.3.4: I understand that the GRASP Confirm Waiting 
> message (from [I-D.ietf-anima-grasp]) and hence 'negotiate_wait' is only 
> used by the responder. How is the effect manifested in the requester? Is 
> a synchronous requester just silently extended? What about an event loop 
> requester?
> 
> 7) MINOR(MAJOR?): Role of 'peer' in 'synchronize'
> 
> The description of 'synchronize' (in section 2.3.5) says:
> 
>        -  If the objective was already flooded, the flooded value is
>           returned immediately in the 'result' parameter.  In this case,
>           the 'peer' and 'timeout' are ignored.
> 
> Do you really want to ignore 'peer' in this case?
> 
> I infer from the description of get_flood that the GRASP retains flooded 
> values separately for each flood source. So you should have the 
> capability to return only a value flooded by the requested peer.
> 
> 8) MINOR(MAJOR?): Confusing semantics of 'flood'
> 
> The descriptions of 'flood' in section 2.3.5 seem to imply that the 
> GRASP in each of the nodes receiving the flood saves the value, for use 
> in get_flood and synchronize. Is that right? (Actually, the description 
> of get_flood implies that flooded values are saved separately for every 
> flood source!)
> 
> Are only registered objectives saved? Or must the GRASP save all the 
> values from every flood that it receives?
> 
> What if one flood contains X, Y, and Z. Then a later flood from the same 
> source contains only X and Y. Presumably the new values of X and Y 
> replace the old ones. Does GRASP retain the value of Z, or discard it?
> 
> ISTM (based on my limited understanding) that it would make more sense 
> for the GRASP to simply cache all objectives that have been registered 
> and/or received as a result of synchronization, together with their TTL. 
> And then subsequent requests would be satisfied from the cache as long 
> as the TTL hasn't expired.
> 
> 9) MINOR: Clarify Session
> 
> Section 2.2.3 implies that Session is a full fledged entity. It would be 
> helpful to flesh this out in more detail. E.g., describe it as a 
> distinct entity in the API and include a state model.
> 
> 10) MINOR: Representing mutually exclusive items with booleans
> 
> In section 2.3.1.3, 'synch' and 'neg' are mutually exclusive. By 
> representing them as independent booleans you introduce the possibility 
> of invalidly setting them both or neither. ISTM it would be better to 
> represent these as a single value (enumeration).
> 
> A similar situation occurs in section 2.3.1.4.
> 
> 11) MINOR: Managing thread for 'listen_negotiate'
> 
> In section 2.3.4, suppose the ASA wants try to ensure that it always has 
> a listener ready for an incoming request. IIUC the only way to achieve 
> that is to create the maximum number of threads it is willing to devote 
> and set them all to listening. That is sub-optimal. It would be better 
> if it could somehow learn that another one is needed and create it. Some 
> additional API mechanisms would be needed to support that.
> 
> 12) NIT: Idempotence
> 
> Bullet #2 of section 2.2.1 says:
> 
>         To facilitate this, the API implementation would provide non-
>         blocking versions of all the functions that otherwise involve
>         blocking and queueing.  In these calls, a 'noReply' code will be
>         returned by each call instead of blocking, until such time as the
>         event for which it is waiting (or a failure) has occurred.  Thus,
>         for example, discover() would return 'noReply' instead of waiting
>         until discovery has succeeded or timed out.  The discover() call
>         would be repeated in every cycle of the main loop until it
>         completes.  Effectively, it becomes a polling call.
> 
> This seems to contradict the earlier statement (in 2.2) that the calls 
> are not idempotent. Section 2.2.2 tries to clarify this, but it is not 
> very clear.
> 
> 13) NIT: Confusing terminology in 'discover'
> 
> In the description of 'discover()' (in section 2.3.3) the 'age_limit' 
> parameter is confusing. At the least the name seems wrong for the 
> described semantics. The implication is that an expiration date/time is 
> what GRASP stores, and that remaining time until expiration is what is 
> being used here. It is hard to see how it is appropriate to call that an 
> "age". ISTM that would better be called a TTL, and that is parameter 
> might then be called 'minimum_TTL'.
> .
>