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 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