[bmwg] draft-ietf-bmwg-network-tester-cfg-06 early Yangdoctors review

[Andy Bierman via Datatracker <[e-mail]>]

Document: draft-ietf-bmwg-network-tester-cfg
Title: A YANG Data Model for Network Tester Management
Reviewer: Andy Bierman
Review result: Not Ready



# General comments

- Could be a very useful YANG module. The 'augmented choice' approach
  allows for ongoing work improving the standard features.
  It is customary to provide guidelines and examples of
  a YANG module that would augment the base modules.

- Not yet clear enough to implementors how to code many leaves
  in each module. This applies to server developers and
  client developers configuring them.

- No XML or JSON examples of the data structures

- The detailed interaction between the analyzer or generator
  and the interface it augments is unclear.  There seems to be
  an assumption that the 'interface-type' is used to determine
  the details of the frames sent or received, but this is not defined.

  E.g. from idle-octets description:

         for example layer 1 framing octets - for Ethernet interfaces
         7+1 preamble octets per packet.";

- The interface-specific details are important to implement or use
  several objects where the frame contents are specified by the client.
  It seems for each testframe-filter identity and each interface-type,
  the processing will be different.

- No server capabilities provided wrt/ which interfaces
  support creation of a traffic-generator or traffic-analyzer
  container.

- Ad-hoc type for binary data is used instead of YANG 'binary' type.
  This should be binary but could be 'yang:hex-string' if Base16
  is really desired instead of Base64.

          type string {
            pattern '([0-9A-F]{2})*';
          }

- The 'units' should be specified for each leaf where
  uint32 or uint64 is the base type.

- Many configurable data nodes do not have any range definitions.
  This is OK because of the generic config objects, but
  not that interoperable.

- Reviews needed from NMOP WG for operator usability

- Reviews needed from OPSAREA WG regarding PCAPNG work


# [e-mail]

## grouping common-data

- leaf realtime-epoch: the name is confusing. Seems like 'start-time'
  would be more clear. What happens if the timestamp is in the past?

- what if both realtime-epoch and total-frames are both present?
  (assume AND-expression in this case but it should be clear).

## grouping burst-data

- The 'dynamic' testframe-type is not clear wrt/ impact on all
  data nodes in this grouping.  Definitions for Ethernet frames
  are spread out in the document.

-  leaf frame-data: seems very complex for implementors to
   figure out the exact usage, even for Ethernet.

-  leaf interframe-gap: not clear what units are being used

-  leaf interburst-gap: should be a complete description,
   not 'similar' reference to previous leaf.

## grouping modifier-data

-  Examples and use-cases should be included in the document
   how this feature works

-  It is unclear how the action (e.g., increment, decrement)
   is applied to the mask and offset.  How does a server
   increment only the masked bits?  This design is very complex.

-  leaf repetitions: this is very unclear and requires much more
   detailed documentation.

## augment "/if:interfaces/if:interface"

- container traffic-generator should be a presence container
  like the traffic-analyzer container.  This makes it clear
  to clients which interfaces have been configured. An NP container
  can be implemented different ways in the server, but not a
  presence container.


# [e-mail]

## identity bit-field-match

This is the only testframe-filter defined.
The description is unclear:

      "Bit field matching filter according to template data and mask.";

The term 'template' is not used anywhere else in the draft.
The filter operation is not really explained.
Details or a reference is needed.

## grouping statistics-data

- leaf pkts: description should be consistent.
  Missing 'since the analyzer was created'.

- leaf octets: not sure why text about RFC 8343 is there.
  This counter should be consistent with the pkts counter

- idle-octets: it is not clear what the units are here.
  It seems to be some unit of time, but the name suggests octets.

- container testframe-stats:
  It is not clear that these are the analyzed packets and the
  previous 3 counters are totals for the analyzer on that interface.

- leaf sequence-errors: Is there a reference or more specific
  description of this error condition?

- leaf payload-errors: Is there a reference or more specific
  description of this error condition?

- container latency: do the leaves in this container exist
  if ../pkts == 0?

- leaf latency/samples: ../testframe-stats is a container.
  Probably mean ../testframe-stats/pkts

- leaf last-sequence-error/expected: this type should be uint64
  not counter64

- leaf last-sequence-error/received: this type should be uint64
  not counter64

## grouping capture-config-data

- container capture: seems like this should be a presence container
  since it is configured by the client.

- choice start-trigger: It is not clear where 'frame index' is defined
  or testframe-index. The difference between these cases is
  not clear.  It is also customary to define a proper default case
  instead of using an ad-hoc description instead.


- choice stop-trigger: It is not clear how 'when-full' is
  different from the no-case-set condition.
  It is also customary to define a proper default case
  instead of using an ad-hoc description instead.

## grouping capture-data

- leaf capture/frame/data:  The 'nacm:default-deny-all' extension
  should be used here since this leaf exposes frame contents


##  augment "/if:interfaces/if:interface/ntta:traffic-analyzer/"
        + "ntta:testframe-filter" {

- mask and data leaves represent binary data, and should use
  binary or hex-string data types.

- The procedures for using this filter are not that clear.
  E.g. how these 2 fields are applied to test frames.