Re: [OPSAWG] Francesca Palombini's Discuss on draft-ietf-opsawg-l3sm-l3nm-16: (with DISCUSS and COMMENT)

[Francesca Palombini <[e-mail]>]

Hi Med!

Amazing reaction time, as always :) Comments inline, but it all looks good! As soon as you submit the new version I’ll remove my discuss.

Thanks,
Francesca

From: [e-mail] <[e-mail]>
Date: Monday, 4 October 2021 at 13:34
To: Francesca Palombini <[e-mail]>, The IESG <[e-mail]>
Cc: [e-mail] <[e-mail]>, [e-mail] <[e-mail]>, [e-mail] <[e-mail]>, [e-mail] <[e-mail]>
Subject: RE: Francesca Palombini's Discuss on draft-ietf-opsawg-l3sm-l3nm-16: (with DISCUSS and COMMENT)
Hi Francesca,

Thank you for the review.

Please see inline.

Cheers,
Med

> -----Message d'origine-----
> De : Francesca Palombini via Datatracker <[e-mail]>
> Envoyé : lundi 4 octobre 2021 12:52
> À : The IESG <[e-mail]>
> Cc : [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Objet : Francesca Palombini's Discuss on draft-ietf-opsawg-l3sm-l3nm-16:
> (with DISCUSS and COMMENT)
>
> Francesca Palombini has entered the following ballot position for
> draft-ietf-opsawg-l3sm-l3nm-16: 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/blog/handling-iesg-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-l3sm-l3nm/
>
>
>
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
>
> Thank you for the work on this document, and apologies for the delayed
> review.
> I have one DISCUSS point, a couple of comments.
>
> As noted in https://www.ietf.org/blog/handling-iesg-ballot-positions/, a
> DISCUSS ballot is a request to have a discussion; I really think that the
> document would be improved with a change here, but can be convinced
> otherwise.
>

[Med] Cool to have this reminder in the preamble :-)
FP: Right? All kudos for this text goes to Warren.


> I have divided comments into "minor" (including the questions) and "nits".
> Neither require replies strictly speaking, please feel free to address as
> you see fit. I will appreciate answers to my questions, to improve my
> understanding. If any clarification comes out of it, I hope it will help
> improve the document.
>
> Francesca
>
> 1. -----
>
>                        leaf holdtime {
>                          type uint32;
>                          units "msec";
>
> FP: This might be me not finding the right reference (or little knowledge
> of YANG), but I was wondering if "msec" was defined somewhere as a unit

[Med] There is no formal list of units. RFC7520 does not make any constraint on the string used in the "units" statement:

   The "units" statement, which is optional, takes as an argument a
   string that contains a textual definition of the units associated
   with the type.

Because I interpret your comment as "msec" is confusing as a unit and for consistency with 'microseconds" already in use in the module, I made this change: s/msec/milliseconds

FP: Thank you for the explanation, I was missing that there was no formal list of units. And as you saw, given that, consistency is exactly what I was hoping for. Thank you for making this change!


> (note that the description does not mention that the unit is milliseconds
> either).

[Med] The description of the YANG module does not include such mention because this is redundant with the "units" statement. However, I updated Section 7.6.4 to mention that the timer is expressed in milliseconds. Thanks.

>
> While doing my due diligence to see if I missed or misunderstood
> something, I researched the RFCs mentioned in the beginning of the YANG
> module:
>
>    This module uses types defined in [RFC6991] and [RFC8343].  It also
>    uses groupings defined in [RFC8519], [RFC8177], and [RFC8294].
>
> And found no use of the "msec" unit. A quick google search shows that RFC
> 8299 uses it

[Med] Bingo! We are actually inheriting that leaf (including the unit) from the L3VPN service module.


, so there is precedence for it, but I couldn't find its
> definition from that document either. All the other leaves use
> "milliseconds" (which is defined in RFC 8294), so my preference would be
> to have consistency, if "msec" was defined and I just missed it.

[Med] I think this is fixed with the proposed change above.
FP: Indeed.


>
> (Note that a similar remark could be made for "bps" used, which does not
> appear in the description text, and is also used in RFC 8466, however
> there is no issue about consistency there).

[Med] Indeed. Made this change to avoid any confusion:

OLD:
 Indicates the inbound bandwidth of the connection

NEW:
 Indicates, in bytes per second (bps), the inbound bandwidth of the connection

And a similar for the outbound bw.

FP: Thank you! I see that was not strictly necessary, but the addition is appreciated anyway.


>
>
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
>
> ## Minor
>
> 2. -----
>
>    'status':  Indicates the status of the OSPF routing instance.
>
> FP: Most likely a copy-paste leftover in section 7.6.3.4 should be IS-IS
> instead of OSPF.

[Med] Fixed. Thank you for catching this.
FP: Happy to have helped!


>
> 3. -----
>
> Section 7.6.3.5, re timers
>
> FP: shouldn't the units be explicitly stated in the timers description
> text, or are they defined somewhere else? Actually, I can see the unit is
> specified later on in the YANG module - so my suggestion is to add some
> simple text in
> 7.6.3.5 to explicitly say that the timers are in seconds.

[Med] Agree. Fixed.

>
> 4. -----
>
>                        leaf required-min-rx-interval {
>
> FP: I see that RFC 5880 does not specify a default value for this; is
> there really no default that can be specified here?

[Med] I thought we had already one! Updated the text to use 1000000 as default to align with the BFD device module (draft-ietf-bfd-yang). Thank you for pointing to this.
FP: Great, thank you.


>
> ## Nits
>
> 5. -----
>
>                              "It is included only when enryption
>                               is enabled.";
>                          }
>
> FP: typo s/enryption/encryption
>
>

[Med] Fixed. FYI, all the changes to address your review can be tracked at: https://protect2.fireeye.com/v1/url?k=4aa8d067-1533e965-4aa890fc-869a14f4b08c-2d8a4280e1e2daab&q=1&e=727a2a92-9a8f-4ac9-a806-37e28ad13c45&u=https%3A%2F%2Fgithub.com%2FIETF-OPSAWG-WG%2Flxnm%2Fcommit%2Fb8904b4cfbd545f2128848207898f53dffc9e83d

Please let me know if any further change is needed. Many thanks again for the review.



_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.