Subject: [CCAMP] Balazs Review of alarm-module-04

From: Balázs Lengyel <[email-masked]>
Date: Fri, 12 Oct 2018 13:52:01 +0000

Hello,

I have reviewed draft-ietf-ccamp-alarm-module-04

General:

IMHO the draft is in a fairly good shape.

The draft
https://tools.ietf.org/html/draft-lengyel-netmod-yang-instance-data-04
describes how to document YANG instance data. One of the main use
case for documenting YANG instance data in a file is to provide
information about the YANG server's capabilities. The
alarm-inventory is exactly such a set of server capabilities that
could, that SHOULD be documented in a YANG Instance Data file
already in design-time to help users integrate management systems.
The draft is in the process of being adopted by the Netmod WG.

It should be mentioned somewhere that a cleared alarm can be
raised again thus become is-cleared=false; potentially many times
during its life.

IMHO the counters defined are not the best. For me the two most
important statistics would be:

How many active (is-cleared=false) alarms do I have in my
system?   total-not-cleared
This could be added both to the list alarm and the list
alarm-summary. I know this can be derived from the total minus
total-cleared but I feel that this is the primary information,
so this should be available just by a read operation without any
maths.

How many alarms did my system generate in the last
hour/day/week ?   total-raised,
this should be a yang:counter32 that increments whenever a new
alarm is added or set to is-cleared=false. (types, severity,
shelving could be considered)

We have neither counter in the module.

I would generally prefer using YANG actions instead of rpcs. I
would place all RPCs as actions under the /alarms container.

These are not general system-wide operations, they are
specific to alarm-handling. They belong under /alarms

It makes setting up access control simpler. As the access
control for the /alarms container already controls access to the
RPCs too. Otherwise you need separate rules for each RPC.

On a CLI if you ask for help, all top level RPCs are listed as
options. If there are many RPCs the list gets really long. With
actions you do not have this problem.

4.4) I don't understand the first sentence.

5)

type resource)

required-instance false; Is not needed as that is the default
meaning for instance-identifiers. Specifying default statements
is not recommended by RFC6087bis

IMHO Distinguished names are more common then UUIDs. I would
list them as more preferred then UUIDs.

typedef resource-match) The following two lines are redundant and contradictory (choose one :-) )

The function library is the core function library and the
functions defined in Section 10
of RFC 7950.

The function library is the core function library

The context node is the root node in the data tree.
AFAIK YANG has many root nodes. Please clarify.

list-alarm)

I am missing a leaf: time-alarm-raised.  As an alarm
can be raised then cleared, then raised again I have no way of
knowing when the current alarm condition started. If an interface
is down I can't check how long has it been down. We have the leaf time-created,
however if the alarm is repeatedly raised/cleared/raised but never
purged this leaf will tell me when this alarm was first raised in
the history of the system, which is not very interesting.

container shelved-alarms)

alarm-shelf-last-changed) The name of the leaf seems to indicate
that this refers to the time when one specific alarm-shelf
changed. However as I understand this is the time when any of the
shelves changed. Please clarify / rename

list alarm-profile)

Why is this user-ordered ? I see no meaning of the order.

rpc compress-alarms)  leaf resource) IMHO this could be of type
resource-match

6)  module-description)

Mention that shelved-alarms are also augmented

Appendix F.1)

Clarify whether this annex is normative or informative. To me
this seems, this should be normative text.

regards Balazs

P.S. Please send any replies to me personally too as I am not
always reading the cccamp list.

--
Balazs Lengyel Ericsson Hungary Ltd.
Senior Specialist
Mobile: +36-70-330-7909 email: [email-masked]

---

From: stefan vallin <[email-masked]>
Date: Mon, 22 Oct 2018 14:14:11 +0200

Hi Balazs!
Thanks for your comments, see inline
br stefan and martin

> On 12 Oct 2018, at 15:52, Balázs Lengyel <[email-masked] <mailto:[email-masked]>> wrote:
> 
> Hello,
> 
> The draft https://tools.ietf.org/html/draft-lengyel-netmod-yang-instance-data-04 <https://tools.ietf.org/html/draft-lengyel-netmod-yang-instance-data-04> describes how to document YANG instance data. One of the main use case for documenting YANG instance data in a file is to provide information about the YANG server's capabilities. The alarm-inventory is exactly such a set of server capabilities that could, that SHOULD be documented in a YANG Instance Data file already in design-time to help users integrate management systems. The draft is in the process of being adopted by the Netmod WG.
> 
> It should be mentioned somewhere that a cleared alarm can be raised again thus become is-cleared=false; potentially many times during its life.
> 
This is stated in 3.5.1.  Resource Alarm Life-Cycle

  From a resource perspective, an alarm can for example have the
  following life-cycle: raise, change severity, change severity, clear,
  being raised again etc.  

See also the is-cleared leaf:
      leaf is-cleared {
        type boolean;
        mandatory true;
        description
          "Indicates the current clearance state of the alarm.  An
           alarm might toggle from active alarm to cleared alarm and
           back to active again.”;

> IMHO the counters defined are not the best. For me the two most important statistics would be:
> 
> 	• How many active (is-cleared=false) alarms do I have in my system?   total-not-cleared This could be added both to the list alarm and the list alarm-summary. I know this can be derived from the total minus total-cleared but I feel that this is the primary information, so this should be available just by a read operation without any maths.
Good comment, we should have a counter for number of active alarms. Will add in next revision
> 	• How many alarms did my system generate in the last hour/day/week ?   total-raised, this should be a yang:counter32 that increments whenever a new alarm is added or set to is-cleared=false. (types, severity, shelving could be considered)
I think this is outside the scope of the alarm module, the management application can do alarm statistics
> 
> I would generally prefer using YANG actions instead of rpcs. I would place all RPCs as actions under the /alarms container.
> 	• These are not general system-wide operations, they are specific to alarm-handling. They belong under /alarms
> 	• It makes setting up access control simpler. As the access control for the /alarms container already controls access to the RPCs too. Otherwise you need separate rules for each RPC.
> 	• On a CLI if you ask for help, all top level RPCs are listed as options. If there are many RPCs the list gets really long. With actions you do not have this problem.

We see your point, actions vs rpc can always be discussed.
How RPCs are presented in a CLI is a totally different thing though, that is up to the CLI engine
However, we prefer to keep these as RPCs at this point.

> 4.4) I don't understand the first sentence.
" The alarm list (/alarms/alarm-list) is a function from (resource,
  alarm type, alarm type qualifier) to the current composite alarm
  state.  The composite state includes states for the resource life-
  cycle such as severity, clearance flag and operator states such as
  acknowledgment.”

This states by using the keys (resource, alarm type, alarm type qualifier) as input you get
the current resource (severity, is-cleared, text) and operator state (e.g. ack) of the alarm as output.

The essence of the alarm list is that function, not for example, to be a list of time-stamped notifications.
Will improve this paragraph in next revision

> 
> 5) 
> type resource)
> 
> 	• required-instance false; Is not needed as that is the default meaning for instance-identifiers. Specifying default statements is not recommended by RFC6087bis
No, the default for require-intance is "true".  See 7950, section 9.9.3.

> 	• IMHO Distinguished names are more common then UUIDs. I would list them as more preferred then UUIDs.
Agree, will change in next revision
> 	• The function library is the core function library and the functions defined in Section 10 of RFC 7950. 
> 	• The function library is the core function library
Thanks for catching this, typo...
> 	•
No, a tree has always exactly one root, and the root has all top-leve
nodes from all modules as children.  See 7950 section 6.4.1.
> 
> list-alarm)
> 
> I am missing a leaf: time-alarm-raised.  As an alarm can be raised then cleared, then raised again I have no way of knowing when the current alarm condition started. If an interface is down I can't check how long has it been down. We have the leaf time-created, however if the alarm is repeatedly raised/cleared/raised but never purged this leaf will tell me when this alarm was first raised in the history of the system, which is not very interesting.
This can be done by inspecting:
       +--ro status-change* [time]
          +--ro time                    yang:date-and-time
          +--ro perceived-severity      severity-with-clear
          +--ro alarm-text              alarm-text

> alarm-shelf-last-changed) The name of the leaf seems to indicate that this refers to the time when one specific alarm-shelf changed. However as I understand this is the time when any of the shelves changed. Please clarify / rename 
Naming can be improved, agree

> Why is this user-ordered ? I see no meaning of the order.
If a user configures overlapping criterias, the first match is used.

> 
> rpc compress-alarms)  leaf resource) IMHO this could be of type resource-match
Good suggestion, will consider a change.

> Mention that shelved-alarms are also augmented
Ok!

> 
> Appendix F.1)
> 
> Clarify whether this annex is normative or informative. To me this seems, this should be normative text.
This is informative
> regards Balazs
> 
> P.S. Please send any replies to me personally too as I am not always reading the cccamp list.
> -- 
> Mobile: +36-70-330-7909              email: 
> [email-masked] <mailto:[email-masked]>
> 
> 
> _______________________________________________
> CCAMP mailing list
> [email-masked] <mailto:[email-masked]>
> https://www.ietf.org/mailman/listinfo/ccamp <https://www.ietf.org/mailman/listinfo/ccamp>