Last Call Review of draft-ietf-i2nsf-nsf-monitoring-data-model-06
review-ietf-i2nsf-nsf-monitoring-data-model-06-yangdoctors-lc-bierman-2021-03-23-00

Status: Ready with Issues

Most of the issues raised in the review of draft-04 have been
addressed.

Major Issues:

 - None

Moderate Issues:

1) too many YANG features

There are 13 YANG features, one for each of the 13 notification-stmts
defined.  There should be as few YANG features defined as possible.
They should only be used if it is an unreasonable burden (compared
to the feature value) for all servers to support the functionality.

2) list /i2nsf-monitoring-configuration/system-alarm

This is yet another alarm management system created in the IETF.
I guess the WG decided that RFC 8632 was not suitable.

It is not clear how this system prevents excessive notifications
sent to a client.

What happens when the CPU, memory, or disk usage crosses back and
forth over the threshold? Seems like an alarm is generated for each
upward crossing of the threshold leaf.

The precise behavior for triggering and then re-arming an alarm
needs to be specified in the YANG module.

RMON Alarms (RFC 2819) defines one way to prevent bursts of
SNMP notifications, using an alarm reset threshold.

YANG Push (RFC 8641) uses a dampening-period approach to prevent
flooding the receiver with notifications.

Also, it is not clear what use-case is served by "threshold = 0".
The range is 0..100 instead of 1..100.


Minor Issues:

3) too many notifications

This module creates a lot of notifications to manage, and they are
all optional to implement. This increases complexity in both
the client implementation and operations.

If you really need all 13 notifications then OK, but
13 notification events is a lot for one YANG module,
especially if this set will get even larger over time.

Here is one way to reduce the number of event definitions.
The example below has 1 event and 13 sub-event types, but it could
also apply to N event types each with some sub-event types.

This design template adds one more layer in the notification message,
but it is probably easier for the client and operator to manage.
The deployment may require filters and access control rules that become
more complex for a large number of notifications.


    notification i2nsf-event {
      description
        "Wrapper for all I2NSF events";

      choice sub-event-type {
        description
          "This choice must be augmented with cases for each allowed
           sub-event.  Only 1 sub-event will be instantiated in each
           i2nsf-event message. Each case is expected to define one
           container with all the sub-event fields.";

         // could put sub-events inline
         case i2nsf-system-detection-alarm {
           if-feature "i2nsf-system-detection-alarm";
           container i2nsf-system-detection-alarm {
             // contents of i2nsf-system-detection-alarm data
           }
         }

       }
     }


     // could add sub-events via augments at any time
     augment "/i2nsf-event/sub-event-type" {
       case i2nsf-system-detection-event {
         if-feature "i2nsf-system-detection-event";
         container i2nsf-system-detection-event {
           // contents of i2nsf-system-detection-event data
         }
       }
     }


Nits:

4) underscore vs. hyphen

There are many field names in sec. 7 that are incorrect
because they use an underscore instead of a hyphen char
(e.g. req_cookies but leaf name is req-cookies)

5) verbose SNMP-style names

The term -configuration in the object names is unusual.
Repeating the parent name (like SMIv2) is not usually done in YANG.
(e.g., i2nsf-system-detection-event-configuration)

6) identifiers should use well-known abbreviations or spell
out the word if not too long.  E.g "ave" -> "average"

7) Is there a reason some identities are ALL-CAPS and others
are all-lower-case? This should be explained in the YANG module.