Early Review of draft-ietf-lmap-yang-05
review-ietf-lmap-yang-05-yangdoctors-early-bjorklund-2017-03-17-00

• Status
• Email expansions
• History

• Versions
• 00

Review
review-ietf-lmap-yang-05-yangdoctors-early-bjorklund-2017-03-17

Martin,

thanks for your review. See my response inline. I am also adding
the LMAP WG to this thread.

On Tue, Aug 23, 2016 at 02:46:36PM +0200, Martin Bjorklund wrote:
> Hi,
> 
> I am the assigned YANG doctor for this document.  I have reviewed this
> document from a pure YANG standpoint.  I have not checked that the
> YANG model matches the information model etc.
> 
> As expected, this model is in good shape.  Here are my comments:
> 
> 
> o  The typedef glob-pattern use \ in a double qouted string.
>    Specifically, it uses the characters \\ which actually expand to a
>    single '\'.  Also in YANG 1.1 the sequences \* and \?  are illegal.
> 
>    I suggest this text is re-written to avoid backslahses, or that a
>    single quote string is used.

Changed the description statement to use a single quoted string.
 
> o  The device-id is of type inet:uri.  I see that this is specified in
>    the information model as well.  But why is it a URI?  As an
>    operator, which URI should I choose?

The information model says this is configurable but frankly this does
not seem to make much sense. I think this really should be a read-only
state object and not a configurable object. Need to check this with the
LMAP WG.

The URI value itself does not really matter, any URI that provides a
reasonably stable and unique identification is good enough. Note that
the entity (aka hardware) YANG model has a similar URI. How to
implement this? It could be a urn:uuid or urn:clei or something else.
There once was an I-D proposing a URN namespace for device identifiers
<draft-arkko-core-dev-urn-03> but this seems to have stalled
(unfortunately).

I think the entity (aka hardware) model also leaves it open what kind
of URIs are used to identify hardware components so I think we are fine
to leave this open as well. (See also below for some further comments.)

> o  lmap/agent/device-id's description says:
> 
>            The device-id identifies a property of the
>            device running the measurement agent.
> 
>    Should this be s/a property of//?  Otherwise, which property is
>    being referred to?

Yes, removed the property text.
 
> o  lmap-state/agent/device-id is marked as 'mandatory true', but its
>    description says that is optional.

This is a good catch. I removed mandatory for now.

The bigger issue, however, is that in the information model certain
elements are marked as optional because of security concerns while in
the YANG module I would find it more natural to make these objects
mandatory and leave it to an access control model to suppress access
to sensitive leafs. Right now, the design strictly follows the
information model but it kind of feels wrong to say in the definition
of a YANG leaf that it is _optional to implement_ given the security
requirements different _deployments_ may have.

I would propose to the WG to make these objects mandatory in YANG
and to move text concerning their sensitivity to the security
considerations section. Need to check this with the LMAP WG.
 
> o  lmap/agent/controller-timeout says that "an event is raised".  What
>    kind of event?  Which protocol?  [later] Ok, reading the events
>    subtree, I see that there is an event 'controller-lost' there.
>    Presumably the text refers to this event.  If so, this text could
>    maybe be clarified.

I have changed this to 'an event (controller-lost) is raised' so that
readers can easily search for the event definition.
 
> o  The descriptions in several nodes in
>    /lmap/events/event/event-type/calendar are a bit confusing:
> 
>               leaf-list month {
>                 type lmap:month-or-all;
>                 min-elements 1;
>                 description
>                   "A month at which this calendar timing will
>                    trigger. The wildcard means all months.";
> 
>    The description says "_A_ month", but the node is a leaf-list.
>    Maybe change to "A set of months".  (same for other nodes in this
>    case as well)

Yes, I could not make up my mind whether the description describes an
element of the set or the set as a whole - and I think I have not been
consistent with this either. I will change all leaf-list descriptions
so that they describe the set as a whole.
 
> o  /lmap/tasks/task/program is optional (and marked as
>    nacm:default-deny-write).
> 
>    What happens if this leaf is not set?

The idea is that you can do without identifying a program if the
agent can map the registry URIs to a suitable program. In a pure
registry-driven implementation, you would configure the measurement
you want to run by pointing to a registry entry and then the system
decides locally which program to execute (i.e., a controller does
not need to know the internal software structure on the device).
I will add:

    [...] If this leaf is not set, then the system
    will try to identify a suitable program based on
    the registry information present."
 
> o  Are /lmap-state/agent/{hardware,firmware} different than
>    /system-state/platform/{machine,os-release/os-version} in
>    ietf-system?

They overlap. One option is to remove these two objects and instead
add language to section 3 pointing to the relevant portions of the
system model. If we go this route (and also conclude that the
device-id is indeed read-only), we might drop the device-id as well
(since it will be covered by the entity (aka hardware) model. Need to
check this with the LMAP WG.
 
> o  So the MA is supposed to invoke RPCs on a controller.  Is this
>    described somewhere?  Are the details left to implementations?

This is actually stated in section 3

   o  Reporting Information: This is modeled by the report data model to
      be implemented by the Collector.  Measurement Agents send results
      to the Collector via an RPC operation.

and the ietf-lmap-report module description

       "This module defines a data model for reporting results from
        measurement agents, which are part of a Large-Scale Measurement
        expected to be implemented by a collector.";

but perhaps this just does not stand out enough. Perhaps add a more
explicit overview figure?

                                    +------------------------+
                                    | LMAP Controller        |
                                    |                        |
                                    | Client:                |
                                    | ietf-lmap-comman.yang  |
                                    | ietf-lmap-control.yang |
                                    +------------------------+
+------------------------+                      |
| LMAP Measurement Agent |                      |
|                        |     <- request       |
| Server:                |<---------------------'
| ietf-lmap-comman.yang  |     response ->
| ietf-lmap-control.yang |
|                        |
|                        |     request ->
| Client:                |----------------------.
| ietf-lmap-comman.yang  |     <- response      |
| ietf-lmap-report.yang  |                      |
+------------------------+                      v
                                    +------------------------+
                                    | LMAP Collector         |
                                    |                        |
                                    | Server:                |
                                    | ietf-lmap-comman.yang  |
                                    | ietf-lmap-report.yang  |
                                    +------------------------+

I tend to agree that the Introduction section is perhaps a bit terse.

/js

-- 
Juergen Schoenwaelder           Jacobs University Bremen gGmbH
Phone: +49 421 200 3587         Campus Ring 1 | 28759 Bremen | Germany
Fax:   +49 421 200 3103         <http://www.jacobs-university.de/>