Re: [lmap] AD evaluation: draft-ietf-lmap-yang-10

[Juergen Schoenwaelder <[e-mail]>]

Alissa,

thanks for your review comments. Let me see what can be done to
address them...

On Tue, Jan 24, 2017 at 11:04:47AM -0500, Alissa Cooper wrote:
> I have reviewed this document in preparation for IETF last call. I have a few substantive comments and questions that I’d like to discuss before issuing the LC. I’ve also included some nits and minor comments that should be resolved together with any IETF LC comments.
> 
> Substantive comments and questions:
> 
> = Section 3 =
> 
> "Configuration Information: This is modeled in the /lmap/agent
>       subtree, the /lmap/schedules subtree, and the /lmap/tasks subtree
>       described below.  Some items have been left out because they are
>       expected to be dealt with by the underlying protocol."
> 
> I think this needs some further elaboration about the items that have been left out. As far as I can tell the only items from the information model that this data model does not define are ma-config-credentials and ma-config-control-channels. There is already text further below to explain the expectation about how channels will be modeled, which should be referenced here. Is there not something more specific that can be said about how credentials will be handled/modeled, assuming the "underlying protocol" referenced for configuration is NETCONF?
>

I think this is related to the issue you raised for the information
model. Until we are clear what the MA credentials not associated to
a channel were intended to be used for, I can't really answer this
specific question.

> = Section 4.2 =
> 
> (1) "It is generally a good idea to always configure
>             an end time and to refresh the configuration
>             of event object as needed to ensure that agents
>             that loose connectivity to their controller
>             do not continue their tasks forever."
>             
> This is more of a comment for the information model, but if this is worth saying here, shouldn't it (also) be mentioned in the corresponding information model definition in draft-ietf-lmap-information-model? This doesn't seem specific to this particular data model.

I see your point. I think it makes sense to note this in the second
item 4. of section 3 of the information model. (I will change the
numbering of the second list to use letters to make it easier to refer
to things.)

       [...] For Event objects specifying a
       series of events, it is generally a good idea to configure an end
       time and to refresh the end time as needed to ensure that MAs
       that loose connectivity to their controller do not continue
       executing Schedules forever.

> (2) For all the counters defined here, I think you need to specify the time period over which the counts are to be calculated.

We use counters as defined in RFC 6991 and I think the definition in
RFC 6991 takes care of things.

> (3) 'list action {
>              key name;
>              description
>                "An action describes a task that is invoked by the
>                 schedule. Multiple actions are invoked sequentially.";'
> 
> I don't get this -- doesn't the execution mode define whether the actions are executed sequentially, in parallel, or pipelined?

Good catch. Changed this to:

           list action {
             key name;
             description
               "An action describes a task that is invoked by the
		schedule. Multiple actions are invoked according to
                the execution-mode of the schedule.";

> (4) "A queue is internally used to pass
>                   results to another schedule."
> 
> I thought it was up to the implementation to decide how to implement this?

OK. I meanwhile understand that the word 'queue' has too many
connotations. Is 'buffer' a less problematic term? The key here is
that the data producer and the data consumer are in general not
running at the time and hence data needs to be stored temporarily
somewhere. I will replace queue with buffer for now.

> (5) I don't understand why the 'program' elements are included in the task configurations and capabilities. It doesn't seem wise to allow the controller to tell the MA which program it needs to use to complete a task, and I don't understand why the MA would need to communicate that information to the controller either. I thought the recent list discussion indicated that if an MA was not capable of performing an action, that action would simply fail, which seems like all that is needed.

There are multiple things:

(a) How do I tell which task I want to have executed? The information
    model assumes that this can be done with the help of a registry.
    The YANG data model, in addition, allows to use a simple 'program
    name'. Note that this is a choice, i.e., you either use a registry
    or a program name, but not both.

    The registry at the end is just some level of indirection - but
    this indirection also requires to have tasks registered in a
    registry. Right now, the implementation I know of only supports
    program names.

(b) The task list in the capabilities branch serves as an inventory,
    i.e., it tells the controller which tasks are supported by a given
    implementation. The other task list defined options that are used
    when a certain task is invoked (the Task Configuration in the
    information model). If a controller configures a task that the
    agent does support (i.e., it is not listed in the capability
    tasks), it will not be executed.

Note that a capability task name 'traceroute' exposed by the LMAP
agent does not necessarily mean that there is a program called
traceroute at the operating system level. In fact, an implementation
could choose to run traceroute internally without an explicit system
level process (like RIPE Atlas did everything in a big event loop, not
sure whether this is still the case).
 
> Nits and minor comments:
> 
> = Section 4.1 =
> 
> Please provide a reference for ISO 8601.

added

> = Section 4.2 =
> 
> s/loose connectivity/lose connectivity/
> 
> s/schedule determins/schedule determines/
> 
> s/A set of month/A set of months/
> 
> s/A set of second/A set of seconds/

fixed
 
> = Section 5 =
> 
> (1) Just a suggestion, but I think this is worth calling out specifically:
> 
> OLD
> Unauthorized access may leak measurement results.
> 
> NEW
> Unauthorized access may leak measurement results, including from passive measurements.

added

> (2) "Implementers MUST taken care that
>    option names and values are passed literally to programs.  In
>    particular, it MUST be avoided that any shell expansions are
>    performed that may alter the option names and values."
> 
> This text strikes me as a bit odd. Surely there are a whole selection of good programming practices that are necessary to ensure that things don't go haywire when implementing LMAP -- why call out these two with normative recommendations? Why does this guidance only apply to options? I would recommend having this text be non-normative, but if you do keep it I would suggest the following:
> 
> Implementers MUST take care that
>    option names and values are passed literally to programs.  In
>    particular, shell expansions that may alter the option names and values MUST NOT be performed.

I have no strong opinion on MUST vs must here and I usually happily
follow the advice of IESG members on RFC 2119 keywords. ;-)

Yes, there are many ways to write broken code but this is such a
common problem that I think can't hurt to remind implementors.

A diff of -10 to my working copy (of -11) can be found here:

  http://beadg.de/lmap/

/js

-- 
Juergen Schoenwaelder           Jacobs University Bremen gGmbH
Phone: [phone]         Campus Ring 1 | 28759 Bremen | Germany
Fax:   [phone]         <http://www.jacobs-university.de/>