Re: [Anima] AD review of draft-ietf-anima-grasp-api-06

[Brian E Carpenter <[e-mail]>]

Thanks Rob. I think things are clear, so we'll work on an update over the next few days.

Regards
   Brian

On 07-Oct-20 07:15, Rob Wilton (rwilton) wrote:
> Hi Brian,
> 
> Thanks for the comments, please see replies inline.
> 
> 
>> -----Original Message-----
>> From: Brian E Carpenter <[e-mail]>
>> Sent: 30 September 2020 23:28
>> To: Rob Wilton (rwilton) <[e-mail]>; draft-ietf-anima-grasp-
>> [e-mail]
>> Cc: [e-mail]; Sheng Jiang <[e-mail]>
>> Subject: Re: AD review of draft-ietf-anima-grasp-api-06
>>
>> Thanks for this thorough review, Rob. Sorry about the previous partial
>> reply. See responses in line.
>>
>> We'll pause for any feedback before updating the draft. In particular, are
>> there any opnions on the example logic flows issue noted below?
>>
>> On 29-Sep-20 04:19, Rob Wilton (rwilton) wrote:
>>> Hi,
>>>
>>> Apologies for the delay for this review.
>>>
>>> Please see comments below.  I've also attached them in case they get
>> truncated (which has happened on other reviews).
>>>
>>> Thank you for this document.  Overall, it is very well written, easy to
>> read and easy to understand.
>>>
>>>
>>> Comments:
>>>
>>>     1.  Introduction
>>>
>>>        As the following figure shows, a GRASP implementation could
>> contain
>>>        two major sub-layers.  The bottom is the GRASP base protocol
>> module,
>>>        which is only responsible for sending and receiving GRASP
>> messages
>>>        and maintaining shared data structures.  The upper layer contains
>>>
>>> I found the text, relative to the diagram, to be somewhat confusing,
>> although the subsequent paragraphs make its intent clearer.
>>>
>>> I think that the top sub-layer is meant to be "GRASP Extended Function
>> Modules", and the "Grasp Modules" is the bottom sub-layer.  However, the
>> diagram seems to have a sub-layer split in "GRASP Modules", and doesn't
>> necessarily equate to "GRASP base protocol module".
>>
>> Perhaps we have confused two aspects: the view that the app writer has of
>> the API, and the way it has to be implemented. More below...
>>
>>>
>>>     1.  Introduction
>>>
>>>        It is desirable that ASAs can be designed as portable user-space
>>>        programs using a system-independent API.  In many
>> implementations,
>>>        the GRASP module will therefore be split into two layers.  The
>> top
>>>        layer is a library that provides the API and communicates
>> directly
>>>        with ASAs.  The lower layer is a daemon, or a set of sub-
>> services,
>>>        providing GRASP core functions that are independent of specific
>> ASAs,
>>>        such as multicast handling and relaying, and common data
>> structures
>>>        such as the discovery cache.  The GRASP API library would need to
>>>        communicate with the GRASP core via an inter-process
>> communication
>>>        (IPC) mechanism.  The details of this are system-dependent.
>>>
>>> I also find this text confusing relative to the diagram.  More
>> naturally, I would assume that the boxes represent different processes and
>> hence you would have IPC between ASA and say the "GRASP API Library".  If
>> I was to draw this, then I would probably have put the GRASP API library
>> co-located with the ASA boxes.
>>
>> Yes. In fact in my demo implementation, when I added an IPC mechanism,
>> that is essentially what I got: stub functions for all the API calls in
>> the app context, then IPC to the core, then the "real" functions and the
>> various daemons/threads in the core context.
>>
>> I can't instantly provide old/new text to respond to this but we will try
>> to clarify all this.
> [RW] 
> 
> Okay.
> 
> 
>>
>>>
>>>
>>>     1.  Introduction
>>>
>>>        Note that a simple autonomic node might contain very few ASAs in
>>>        addition to the autonomic infrastructure components described in
>>>        [I-D.ietf-anima-bootstrapping-keyinfra] and
>>>        [I-D.ietf-anima-autonomic-control-plane].  Such a node might
>> directly
>>>        integrate GRASP in its code and therefore not require this API to
>> be
>>>        installed.  However, the programmer would then need a deeper
>>>        understanding of the GRASP protocol than is needed to use the
>> API.
>>>
>>> Perhaps change "integrate GRASP in its code" to something like
>> "integrate the full GRASP implementation in its code"?
>>
>> Yes (but actually some cases might choose to implement a GRASP subset).
> [RW] 
> 
> Or possibly something like "integrate a GRASP protocol stack in its code"?  The aim of my comment was to try and differentiate between just linking against a GRASP API library and directly embedding part of the GRASP protocol stack.
> 
> 
> 
>>
>>>
>>>     2.2.1.  Alternative Asynchronous Mechanisms
>>>
>>>        Thus, some ASAs need to support asynchronous operations, and
>>>        therefore the GRASP core must do so.  Depending on both the
>> operating
>>>        system and the programming language in use, there are three main
>>>        techniques for such parallel operations: multi-threading, an
>> event
>>>        loop structure using polling, and an event loop structure using
>>>        callback functions.
>>>
>>> Rather than "there are three main techniques for such parallel
>> operations", perhaps just "three common techniques are considered for
>> parallel operations".
>>
>> OK
>>
>>> Reasoning, there are other approaches for effectively handling
>> concurrent operations, e.g., actors, futures, cooperative threads backed
>> by a OS thread pool ...
>>
>> [Or of course different names for the same thing. Imagine trying to
>> describe all this in terms of ADA rendezvous.]
>>
>>>
>>> Possibly, change "simple function calls" in the multi-threading
>> paragraph to be "simple synchronous function calls" to emphasize the
>> synchronous nature.
>>
>> OK
>>
>>>
>>>
>>>     2.2.2.  Multiple Negotiation Scenario
>>>
>>>        In the callback model, for the scenario just described, the ASAs
>> "A"
>>>        and "B" will each provide two instances of
>> negotiate_step_received(),
>>>        one for each session.  For this reason, each ASA must be able to
>>>        distinguish the two sessions, and the peer's IP address is not
>>>        sufficient for this.  It is also not safe to rely on transport
>> port
>>>        numbers for this, since future variants of GRASP might use shared
>>>        ports rather than a separate port per session.  This is why the
>> GRASP
>>>        design includes a session identifier.  Thus, when necessary, a
>>>        'session_nonce' parameter is used in the API to distinguish
>>>        simultaneous GRASP sessions from each other, so that any number
>> of
>>>        sessions may proceed asynchronously in parallel.
>>>
>>> It wasn't obvious to me why both ASAs "A" and "B" both provide
>> callbacks, I presume that this is because there are multiple asynchronous
>> steps involved.  If so, would it be helpful to clarify this point?
>>
>> Yes, one could build very complicated interlocking negotiations. (That is
>> a whole can of worms, explored in draft-ciavaglia-anima-coordination, but
>> for present purposes we just need to ensure that ASAs can handle any
>> degree of asynchronosity, if that's a word.)
>>
>>> Also in this paragraph suggest "Hence, " rather than "This is wht"
>>
>> OK
>>
>>>
>>>
>>>     2.2.3.  Overlapping Sessions and Operations
>>>
>>>        In calls where it is used, the 'session_nonce' is an opaque read/
>>>        write parameter.  On the first call, it is set to a null value,
>> and
>>>        the API returns a non-null 'session_nonce' value based on the
>> GRASP
>>>        session identifier.  This value must be used in all subsequent
>> calls
>>>        for the same session, and will be provided as a parameter in the
>>>        callback functions.  By this mechanism, multiple overlapping
>> sessions
>>>        can be distinguished, both in the ASA and in the GRASP core.  The
>>>        value of the 'session_nonce" is opaque to the ASA.
>>>
>>> Query the use of a null value, and whether that is too restrictive.
>>
>> Its value on the first call is of no importance, so we should just call it
>> a placeholder.
> [RW] 
> 
> Sorry, I had intended to expand my original comment.  To me this reads quite like a C API, where the session_nonce is a INOUT parameter, but other languages that say allow a tuple to be returned would probably just return a tuple containing whatever other return parameters along with a session_nonce.
> 
> However, I'm also not quite sure whether this text is entirely consistent with how the session_nonce is actually used in the APIs.  E.g. request_negotiate() options returns a session_nonce.  It doesn't appear to be an "in" parameter at all.
> 
> Perhaps the text should just say something like the session_nonce is provided when the session is established and used in all subsequent calls, and detailed semantics to the specific APIs?
> 
> 
> 
> 
> 
>>
>>>
>>>
>>>     2.3.  API definition
>>>
>>> Possibly this API would benefit from a short section (or even pseudo
>> code) explaining the flow or flows of how the APIs are expected to work
>> together.  This could also be in an appendix.  Perhaps the GRASP
>> specification makes this obvious, in which case references back to the
>> GRASP spec would be sufficient.
>>
>> We have some example logic flows in https://tools.ietf.org/id/draft-
>> carpenter-anima-asa-guidelines-09.html#section-appendix.b
>>
>> Would a reference to that help? The authors would like that draft to be
>> adopted by the WG. We could of course move the examples into the API
>> draft, if people prefer that.
> [RW] 
> 
> Yes, this is what I was thinking of.
> 
> I don’t mind whether it is included in an appendix in this draft or as a reference.
> 
> 
> 
>>
>>>
>>>
>>>     2.3.1.3.  Objective
>>>
>>>        *  name (UTF-8 string) - the objective's name
>>>
>>> Should there be a limit on the length of the name?  This question
>> equivalently applies to the other strings/variable sized types.
>>
>> I think that's implementation dependent. Because GRASP is a CBOR protocol,
>> there is no particular limit on string length. There is a limit on the
>> total size of a GRASP message (although I suspect we might decide to raise
>> or remove that limit one day).
> [RW] 
> 
> That sounds reasonable, and I don't feel too strongly on this issue.
> 
> I think that my general concern with arbitrary length strings in APIs, that are implementation dependent, is that clients may not know what (if any) max length is supported by an implementation, which potentially could make interop harder.  In the YANG world, this can be mitigated via deviations to indicate implementation specific restrictions.
> 
> Perhaps what I am thinking of is defining a minimum supported length.  E.g., the length is implementation specific, but all implementations must support, say, at least 64 characters.  Hence, a client would know that if the name was below this length then it would work with all implementations, but implementations may support a much higher limit if they wish.
> 
> 
>>
>>>        *  value - a specific data structure expressing the value of the
>>>           objective.  The format is language dependent, with the
>> constraint
>>>           that it can be validly represented in CBOR (default integer =
>> 0).
>>>
>>> I didn't understand the "default integer = 0" part.  If this is a byte
>> string then I would thought that the default might be an empty length
>> bytestring, or perhaps a CBOR Null.
>>
>> I'm not even sure now why we need to specify a default in a generic API.
>>
>>>           An essential requirement for all language mappings and all
>>>           implementations is that, regardless of what other options
>> exist
>>>           for a language-specific represenation of the value, there is
>>>           always an option to use a CBOR byte string as the value.  The
>> API
>>>           will then wrap this byte string in CBOR Tag 24 for
>> transmission
>>>           via GRASP, and unwrap it after reception.
>>>
>>> It was unclear whether you just meant an opaque CBOR byte string here
>> (CBOR major type 2), or a CBOR byte string that itself contains CBOR
>> encoded data (i.e. CBOR Tag 24).  Is there any limit on the length of the
>> CBOR byte string that is allowed?
>>
>> It's not intended to be restricted to major type 2; I'll have to check the
>> CBOR RFC to find the correct phrase to use.
> [RW] 
> 
> Perhaps something along the lines of "An encoded CBOR data item, wrapped in CBOR Tag 24, as per section 3.4.5.1 of 7049bis"
> 
> 
> 
>>
>> As above, the only size limit we have is the total GRASP message size.
>>
>>>
>>>
>>>     2.3.2.  Registration
>>>
>>> Some APIs list "Asynchronous Mechanisms:" and others don't.  It might be
>> helpful to clarify that those APIs that don't list asynchronous mechanisms
>> are implicitly synchronous in their behaviour.
>>
>> OK
>>
>>>
>>>
>>>         *  deregister_asa()
>>>
>>>           -  Note - the ASA name is strictly speaking redundant in this
>>>              call, but is present for clarity.
>>>
>>> Presumably it also makes it significantly harder for a rogue client to
>> try and deregister all ASAs by looping through all asa_nonces.
>>
>> True, but there is so much entropy in the nonce that this would be a
>> challenge anyway.
> [RW] 
> 
> Okay.  When I wrote this, I also realized that the other APIs are just relying on the nonce anyway.
> 
> 
>>
>>>
>>>
>>>     2.3.4.  Negotiation
>>>
>>>              2.  The 'session_nonce' parameter is not null.  In this
>> case
>>>                  negotiation must continue.  The returned
>>>                  'proffered_objective' contains the first value
>> proffered by
>>>                  the negotiation peer.  Note that this instance of the
>>>                  objective must be used in the subsequent negotiation
>> call
>>>                  because it also contains the current loop count.  The
>>>                  'session_nonce' must be presented in all subsequent
>>>                  negotiation steps.
>>>
>>> Presumably the loop count in the proffered_objective must be one greater
>> than the loop count in the objective structure?  Would it be helpful to
>> mention this?
>>
>> Right... I'm going to look in my GRASP code before I try to rephrase that.
>> I recall that it was a bit tricky to get it right.
> [RW] 
> 
> Okay.
> 
>>
>>>
>>> I also wasn't sure whether it must be "this instance of the objective
>> must be used", or just that the loop counter must be copied across.  E.g.,
>> functional languages would encourage these structures to be immutable.
>>
>> And because of Python's vagueness about whether it's call-by-reference or
>> call-by-value, there are cases where I've had to explicitly clone a copy
>> of an objective to be sure that I have the right to decrement the loop
>> counter. So yes, it's "this instance or a clone of this instance". Will
>> rephrase.
> [RW] 
> 
> Okay.
> 
>>
>>>
>>>
>>>          -  Asynchronous Mechanisms:
>>>
>>>              o  Event loop implementation: The 'session_nonce' parameter
>> is
>>>                 used in read/write mode.
>>>
>>> It is unclear to me, exactly what you mean by this, particularly because
>> this is a return parameter.  Perhaps this could be further explained in
>> the text, or refer back the previous text.
>>
>> Yes. I think we were trying to express something that will look a bit
>> different in C compared to high-level languages.
> [RW] 
> 
> Possibly even referring it an in/out parameter would be clearer.
> 
>>
>>>
>>>
>>>     2.3.4.  Negotiation
>>>
>>>         *  listen_negotiate()
>>>
>>> I was surprised that the listen_negotate didn't include any timeout,
>> e.g. to allow it to be used in polling fashion.  Same comment applies to
>> listen_synchronize().
>>
>> Well, in a polling solution won't the listen state just go into the event
>> loop? But I'm a bit out of my depth there.
> [RW] 
> 
> Okay, possibly I was thrown by the "Unless there is an unexpected failure,
> this call only returns after an incoming negotiation request."  I.e., it seems to suggest that it always blocks.  But perhaps the description needs to be refined depending on whether it is a synchronous or asynchronous API.
> 
> In a pure event loop scenario, I would expect a registration API to register an event (e.g. an objective) that I am interested in, and optionally a callback function to be invoked when that event is received.  Normally, the thread would then have a loop waiting (blocking) for an event to be received that might either be returned back in the loop (or plausibly dispatched directly).  In the code that I am familiar with there are separate calls to wait (block) for a new event, and then a separate function is invoked to dispatch the event to the registered call back handler.  In this scenario is it sometimes useful for the event block() call to have a timeout, allowing the thread to do other periodic housekeeping work and then make a new event_block() call.
> 
> I'm not particular familiar with the Linux epoll, but this gives an example of a timeout whilst waiting for an event: https://man7.org/linux/man-pages/man2/epoll_wait.2.html
> 
> So, I was interpreting this API as effectively having a GRASP specific event loop, and that listen_negotiate() was acting like epoll_wait() call.  But I might be misunderstanding the intent here.
> 
> 
> 
> 
>>
>>>
>>>
>>>     2.3.4.  Negotiation
>>>
>>>         *  negotiate_wait()
>>>
>>> It wasn't clear to me why you would want to do this, although that may
>> be apparent in the GRASP draft.  Perhaps a cross reference might be
>> helpful?
>>
>> Yes, the idea is simply that one party in the negotiation wants to tell
>> the other party to be patient for a while. It is described in the GRASP
>> spec so a reference will help.
> [RW] 
> 
> Okay.
> 
> 
>>
>>>
>>>
>>>     2.3.4.  Negotiation
>>>
>>>         *  end_negotiate()
>>>
>>> Possibly "successful" might be a better parameter name than "reply".  I
>> think that you reply both if the negotiation is successful and also if it
>> failed.
>>
>> Yes, I don't know where that came from. In the Python implementation it's
>> called 'result' (True for accept, False for decline).
>>
>>>
>>>
>>>     2.3.5.  Synchronization and Flooding
>>>
>>>        *  synchronize()
>>>
>>>           -  If the objective was already flooded, the flooded value is
>>>              returned immediately in the 'result' parameter.  In this
>> case,
>>>              the 'source' and 'timeout' are ignored.
>>>
>>> Should source be quoted here?  It is unclear what it is referring to.
>>
>> Good catch. That should be 'peer'.
>>
>>>
>>>
>>>           -  Otherwise, synchronization with a discovered ASA is
>> performed.
>>>              The 'peer' parameter is an 'ASA_locator' as returned by
>>>              discover().  If 'peer' is null, GRASP discovery is
>> performed
>>>              first.
>>>
>>> How does the GRASP discovery work in this scenario?  Does this require
>> use of the discovery APIs?  This may require further explanation.
>>
>> OK, will fix. The intention is that discovery is launched automatically.
> [RW] 
> 
> Okay.
> 
> 
>>
>>>
>>>
>>>     3.  Implementation Status [RFC Editor: please remove]
>>>
>>>        A prototype open source Python implementation of GRASP, including
>> an
>>>        API similar to this document, has been used to verify the
>> concepts
>>>        for the threaded model.  It may be found at
>>>        https://github.com/becarpenter/graspy with associated
>> documentation
>>>        and demonstration ASAs.
>>>
>>> This section will be removed anyway, but I thought that the GIL (global
>> interpreter lock) effectively prevented Python threads from running
>> concurrently.
>>
>> It's not that simple, because you can get true concurrency during I/O and
>> clock waits. In any case, the Python queue and lock mechanism is valid
>> regardless of this.
>> https://wiki.python.org/moin/GlobalInterpreterLock is informative.
> [RW] 
> 
> Ah, okay.  My knowledge of Python is fairly superficial.
> 
>>
>>>
>>>
>>>
>>> Nits:
>>>
>>> I've noted that you have chosen not to use RFC2119 language.  No issues
>> with this, but this may come up in IESG review ...
>>
>> Well, it's Informational.
> [RW] 
> Ah, yes.
> 
>>
>>>
>>>
>>> Use of the term "nonce".  Note in UK English this word sometimes takes
>> an alternative meaning, depending on who you ask and which dictionary you
>> look at: https://dictionary.cambridge.org/dictionary/english/nonce
>>>
>>> You may wish to consider an alternative term, but please note that this
>> is up to the authors.  I'm not objecting to this term, just making you
>> aware, if you weren't already.
>>
>> I'm not aware of a viable alternative.
> [RW] 
> 
> That's fine.
> 
> 
>>
>>>     Abstract:
>>> Maybe languages => programming languages.
>>
>> OK
>>
>>>
>>>
>>>
>>>     1.  Introduction
>>>         As the following figure shows,
>>>
>>> Given that the figure is a few paragraphs away (and on the next page in
>> the txt version), perhaps give the figure a name and reference it by name.
>>
>> Yes.
>>>
>>>
>>>
>>>     2.3.1.3.  Objective.
>>> For me, I would outdent the "} objective" in the C struct example.
>>
>> Yes.
>>
>>>
>>> represenation -> representation
>>
>> Yes.
>>
>> Thanks much,
>>
>>     Brian
> [RW] 
> 
> Thanks,
> Rob
> 
> 
>>>
>>> Thanks,
>>> Rob
>>>