Re: [OPSAWG] Éric Vyncke's Discuss on draft-ietf-opsawg-service-assurance-yang-10: (with DISCUSS and COMMENT)

["[e-mail]" <[e-mail]>]

Hi,

I have looked at the changes and they seem fine, I could not find any 
major problems except for the file revisions not matching the YANG 
revisions, for all the modules, so please remember to fix that when you 
do the next update.

file"[e-mail]"
...
   revision 2022-08-10 {
     description
       "Initial version.";
     reference
       "RFC xxxx: YANG Modules for Service Assurance";
   }

Also, I have found some restrictions in descriptions of some nodes that 
can be expressed in YANG (were already in draft v6 but I have missed it 
then), but it is up to you to decide whether that makes sense or not. In 
general, it is always better because validating tools are then able to 
check the constraint automatically. The affected nodes are:

leaf assurance-graph-last-change (depends on last-change)
leaf health-score-weight (depends on health-score or rather the other way around)
leaf stop-date-time (depends on start-date-time)

Regarding the comments and proposed changes, I have nothing to add, all 
my potential comments have already been mentioned.

Regards,
Michal

On 15. 12. 2022 1:54, Jean Quilbeuf wrote:
> Hi Eric,
> Thanks for your comment.
>
> No new draft revision yet, but you’ll find below answers to discuss and comment, including modification that we plan to do.
>
> Michal Vaško  did the early YANG doctor review. Since there are modifications planned to the YANG module in this mail, I added him in Cc.
>
> Best,
> Jean
>> -----Original Message-----
>> From: Éric Vyncke via Datatracker [mailto:[e-mail]]
>> Sent: Tuesday 13 December 2022 08:13
>> To: The IESG<[e-mail]>
>> Cc:[e-mail]; opsawg-
>> [e-mail];[e-mail];[e-mail];[e-mail];
>> [e-mail]
>> Subject: Éric Vyncke's Discuss on draft-ietf-opsawg-service-assurance-yang-
>> 10: (with DISCUSS and COMMENT)
>>
>> Éric Vyncke has entered the following ballot position for
>> draft-ietf-opsawg-service-assurance-yang-10: Discuss
>>
>> When responding, please keep the subject line intact and reply to all email
>> addresses included in the To and CC lines. (Feel free to cut this introductory
>> paragraph, however.)
>>
>>
>> Please refer to
>> https://www.ietf.org/about/groups/iesg/statements/handling-ballot-
>> positions/
>> for more information about how to handle DISCUSS and COMMENT
>> positions.
>>
>>
>> The document, along with other ballot positions, can be found here:
>> https://datatracker.ietf.org/doc/draft-ietf-opsawg-service-assurance-yang/
>>
>>
>>
>> ----------------------------------------------------------------------
>> DISCUSS:
>> ----------------------------------------------------------------------
>>
>>
>> # Éric Vyncke, INT AD, comments for draft-ietf-opsawg-service-assurance-
>> yang-10
>> CC @evyncke
>>
>> Thank you for the work put into this document. A very interesting piece of
>> work
>> and a well-written piece of text (even if I am balloting DISCUSS). The
>> examples
>> are also helping.
>>
>> Please find below some DISCUSS points (+ suggestions), some non-blocking
>> COMMENT points (but replies would be appreciated even if only for my own
>> education).
>>
>> Special thanks to Michael Richardson for the shepherd's detailed write-up
>> including the WG consensus *but* it lacks the justification of the intended
>> status. It would have been nice to list the implementations (even if I know
>> one).
>>
>> Please note that Tommy Pauly is the Internet directorate reviewer (at my
>> request) and you may want to consider this int-dir reviews as well when
>> Tommy
>> will complete the review (no need to wait for it though):
>> https://datatracker.ietf.org/doc/draft-ietf-opsawg-service-assurance-
>> yang/reviewrequest/16806/
>>
>> Also, thanks to the WG chairs and the responsible AD to bundle this I-D and
>> its
>> companion to the same IESG telechat: it helps a lot!
>>
>> I hope that this review helps to improve the document,
>>
>> Regards,
>>
>> -éric
>>
>> ## DISCUSS
>>
>> As noted inhttps://www.ietf.org/blog/handling-iesg-ballot-positions/, a
>> DISCUSS ballot is a request to have a discussion on the following topics:
>>
>> ### BCP14 template
>>
>> As noted by
>> https://author-
>> tools.ietf.org/api/idnits?url=https://www.ietf.org/archive/id/draft-ietf-
>> opsawg-service-assurance-yang-10.txt
>> and Lars, the BCP14 template is not correct even if it is required for a
>> proposed standard (it mentions BCP13 ;-) ).
>>
>> As I have further DISCUSS issues below, I am raising the trivial BCP14 issue to
>> a blocking DISCUSS.
> Thanks to Lars and you for pointing that out. We will fix it.
>
>> ### Section 3.3
>>
>> To my SQL eyes, it hurts to use a -1 value for health-score when there is no
>> value. There is no "mandatory true" statement for this leaf, i.e., it can be
>> absent in the telemetry. Is there a semantic difference between the absence
>> of
>> health-score and the value of -1 ? Is the SAIN collector expected to process
>> those 2 cases differently ?
>>
>> Suggest to either remove the -1 sentinel value, or add "mandatory true"
>> attribute, or be specific about the difference (if any).
> We propose to change to:
>
> leaf health-score {
>          type int8 {
>              range "-1 .. 100";
>          }
>          config false;
>          mandatory true;
>          default -1;
>          description
>            "Score value of the subservice health. A value of 100 means
>             that subservice is healthy. A value of 0 means that the
>             subservice is broken. A value between 0 and 100 means that
>             the subservice is degraded. A value of -1 means that the
>             health-score could not be computed.";
>        }
>
> We need the -1  to be able to state that the health-status is not available anymore, notably for some model-driven telemetry protocol that do not support stating the fact that a node does not exist anymore, but only that the value of that node has changed.
>
>> ### Section 4
>>
>> It is unclear from this section whether it applies to IETF-specified YANG
>> modules only? I.e., may a vendor augment this IETF YANG module in its own
>> namespace ? I guess so, but worth writing it (or restrict this section to
>> future IETF work only).
>>
> 1) Yes, we need to specify that we focus in IETF-specified modules only in this section. We propose to change:
>
> The base YANG module defined in Section 3.3 only defines a single type of subservices that represent service instances. As explained above, this model is meant to be augmented so that a variety of subservices can be used in the assurance graph. In this section, we propose some guidelines in order to build theses extensions.
>
> To
>
> The base YANG module defined in Section 3.3 only defines a single type of subservices that represent service instances. As explained above, this model is meant to be augmented so that a variety of subservices can be used in the assurance graph. In this section, we propose some guidelines for specifying such extensions at IETF.
>
> 2) Yes, we need to specify that a vendor can define subservices using they own namespace. We propose to add the following as last paragraph for section 4.
>
> Vendors can specify their own subservices types by defining the corresponding modules in their own namespace. An example of such a vendor-specific module is specified in Appendix A. Vendors can also augment existing IETF-specified subservices to add their own vendor-specific information.
>
>> ----------------------------------------------------------------------
>> COMMENT:
>> ----------------------------------------------------------------------
>>
>>
>> ## COMMENTS
>>
>> ### Downref
>>
>> As noted by
>> https://author-
>> tools.ietf.org/api/idnits?url=https://www.ietf.org/archive/id/draft-ietf-
>> opsawg-service-assurance-yang-10.txt
>> and Lars, there is a downward reference that was not mentioned in the IETF
>> LC.
> I will leave that one to Benoît in his answer to Lars.
>
>> ### Section 1
>>
>> ```
>> [I-D.ietf-opsawg-service-assurance-architecture] specifies an
>>     architecture and a set of involved components for service assurance,
>>     called Service Assurance for Intent-Based Networking (SAIN).
>> ```
>> The SAIN architecture draft is informational, so it cannot "specify". Please
>> use "describes".
> Will change
>
>> ### Section 3.1
>>
>> ```
>>     The two subservices presented above need different sets of parameters
>>     to fully characterize one of their instance.  An instance of the
>>     device subservice is fully characterized by a single parameter
>>     allowing to identify the device to monitor.  For ip-connectivity
>>     subservice, at least the device and IP address for both ends of the
>>     link are needed to fully characterize an instance.
>> ```
>> s/two subservices presented/two example subservices presented/
> Will change
>
>> While I am not English native speaker, I wonder whether the plural form
>> should
>> be used for "IP address for both ends" ?
> Will check and adapt
>
>> `The dependencies are modelled as an adjacency list,` or simply `The
>> dependencies are modelled as a list,` ?
>>
> I would like to keep "adjacency list" because ofhttps://en.wikipedia.org/wiki/Adjacency_list  
>   
>> ### Section 3.2
>>
>> ```
>>     The date of last change "assurance-graph-last-change" is read only.
>>     It must be updated each time the graph structure is changed by
>>     addition or deletion of subservices, dependencies or modification of
>>     their configurable attributes.
>> ```
>> Is 'under-maintenance' a triggering change to update the last change time ?
>> Perhaps worth mentioning
> Yes it is, I will add it.
>
>> ### Section 3.3
>>
>> Should the under-maintenance.contact be more specified? I.e., as a URI like
>> phone:[phone] ormailto:[e-mail]  ? One goal of this
>> document
>> (section 1) is to be 'machine readable' ;-)
> We had many discussions about it, maybe URI is the good solution. We did not find any existing yang type to model a contact.
>
>> leaf health-score-weight, the use of this leaf is rather under specified.
>> Should it rather be a float between 0.0 and 1.0 ? I also wonder whether the
>> last sentence of the description applies to this leaf ?
> We use 0..100 to be aligned with the health-score.
>
>> I will let to my fellow ART AD to check whether the waiver `Therefore, no
>> mechanism for language tagging is needed.` is acceptable for lack of i18n
>> support (for me: it is ok).
>>
>> ### Section 5
>>
>> Is a device always a 'physical' device or can it be virtual as well ?
>>
>> Should there be a link to the miscellaneous 'inventory models' in OPSAWG ?
>> If
>> not, should there be more information about the device, e.g., its location.
> There should be an inventory mapping the "device" parameter to other information such as location. I don’t think we have an answer yet in OPSAWG for such a model .
>
>> ### Section 6
>>
>> I am not familiar enough with YANG, but should there be some text or YANG
>> statement to declare the 'device' leaf in this module as a foreign key to the
>> device module ?
> Not in this case, since the ACME device subservice is totally independent from the IETF device subservice. However, another option would have been for ACME to extend the IETF device subservice, in which case the "device" leaf would have been the same in both models.
>
>> Should the interface model handle the sub-interface (e.g., a specific VLAN on
>> a
>> GigEthernet interface) ?
> Following ietf-interfaces YANG module, the name of the interface (a string) is sufficient to be the key identifying an  interface on a given device, including virtual interfaces stacked on top of other interfaces. So I don’t think that we need to model it.
>
> I will add this information in the in the text.
>
>> ### Appendix C
>>
>> Thanks for using an IPv6 example ;)
>>
>> ## Notes
>>
>> This review is in the ["IETF Comments" Markdown format][ICMF], You can
>> use the
>> [`ietf-comments` tool][ICT] to automatically convert this review into
>> individual GitHub issues.
>>
>> [ICMF]:https://github.com/mnot/ietf-comments/blob/main/format.md
>> [ICT]:https://github.com/mnot/ietf-comments
>>
>>
>>