Subject: [Teas] Yangdoctors early review of draft-ietf-teas-yang-rsvp-10

From: Ebben Aries via Datatracker <[email-masked]>
Date: Fri, 03 May 2019 17:10:32 -0700

Reviewer: Ebben Aries
Review result: On the Right Track

2 modules in this draft:
- [email-masked]
- [email-masked]

No YANG compiler errors or warnings (pyang 2.0, yanglint 1.1.16, confdc 6.6.1)

Module [email-masked]:
- Remove WG Chairs from contact information per
  https://tools.ietf.org/html/rfc8407#appendix-B
- Use of 'state' containers.  It is stated in Section 2.3 that 'Derived state
  data is contained under a "state" container...'.  My only comments here are:
  a) Should use caution when using 'state' containers in NMDA compliant
  modules.  Rob put together a nice doc here that I won't reiterate:
  https://github.com/netmod-wg/FAQ/wiki/NMDA-Modelling-FAQ
  Using such nomenclature locks you into r/o nodes only.
  b) In some cases, the hierarchy is a bit redundant (statistics/state).
  Other NMDA compliant modules will not introduce another 'state' hierarchy
  for instance (see ietf-interfaces)
- leaf 'packet-len' is abbreviated while most other leafs are not
- authentication-key is of type string.  Is this leaf mean to be clear-text?
  There is nothing associated with this type that would imply special
  treatment in handling.
- crypto-algorithm: Are all identities from ietf-key-chain supported?
- Pluralization on counters:  e.g. 'in-error' vs. 'in-errors'. Maintain
  consistency with other modules (see ietf-interfaces)
- Normative references are missing for some of the modules imported.  These
  must be added per https://tools.ietf.org/html/rfc8407#section-3.9

Module [email-masked]:
- Remove WG Chairs from contact information per
  https://tools.ietf.org/html/rfc8407#appendix-B
- Use of 'state' containers.  It is stated in Section 2.3 that 'Derived state
  data is contained under a "state" container...'.  My only comments here are:
  a) Should use caution when using 'state' containers in NMDA compliant
  modules.  Rob put together a nice doc here that I won't reiterate:
  https://github.com/netmod-wg/FAQ/wiki/NMDA-Modelling-FAQ
  Using such nomenclature locks you into r/o nodes only.
  b) In some cases, the hierarchy is a bit redundant (statistics/state).
  Other NMDA compliant modules will not introduce another 'state' hierarchy
  for instance (see ietf-interfaces)
- Pluralization on counters:  e.g. 'in-error' vs. 'in-errors'. Maintain
  consistency with other modules (see ietf-interfaces)
- 9 features are defined with no 'if-feature' statements.  Where are these
  feature conditions meant to be used?
- Normative references are missing for some of the modules imported.  These
  must be added per https://tools.ietf.org/html/rfc8407#section-3.9

General comments on the draft/modules:
- The state container and list key designs appear very familiar to that of
  OpenConfig modules however not consistent with IETF module design.
  Consistency is key from each producing entity (e.g. IETF in this case)
- The draft mentions RPCs and Notifications however none are defined within
  the modules
- Section 2.3: Has examples of RPCs and Notifications that are non-existant in
  the modules
- Abstract: s/RVSP/RSVP/
- Abstract: s/remote procedural/remote procedure/
- Section 2: s/extensiion/extension/
- Section 4: Update the security considerations section to adhere to
  https://tools.ietf.org/html/rfc8407#section-3.7 and
  https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines
- Various (missing) wording/pluralization cleanup throughout that I'll defer
  to the RFC Editor.  A once over proofread should solve this.

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Mon, 13 May 2019 18:08:42 +0000

Hi,

Question to the authors (I haven't followed this draft so apologies if I'm trying to revive a dead horse): why is there an artificial index (leaf local-index) for the sessions list, why not uniquely identify the session with destination address etc? Is it because there are different types of sessions?

Regards,
Reshad.

On 2019-05-03, 8:11 PM, "yang-doctors on behalf of Ebben Aries via Datatracker" <[email-masked] on behalf of [email-masked]> wrote:

    Reviewer: Ebben Aries
    
    2 modules in this draft:

    

    _______________________________________________
    yang-doctors mailing list
    [email-masked]
    https://www.ietf.org/mailman/listinfo/yang-doctors

---

From: Tarek Saad <[email-masked]>
Date: Tue, 14 May 2019 14:45:59 +0000

Hi Reshad,

Base RSVP RFC2205 defines the session as. "An RSVP session is defined by the triple: (DestAddress, ProtocolId      [, DstPort])." RFC3209 defines a session object for RSVP-TE LSP(s) as tuple (tunnel endpoint, tunnel ID, extended tunnel ID). Since keys are mandatory leafs for lists, and to make it generic, we thought of a having an independent index key for the list.

Thinking more of it now, since the list is state/read-only, the key is optional and omitting it may be better. We can consider this in the next update to the document, thanks for pointing it out.

Regards,

Tarek

On 5/13/19, 2:09 PM, "Teas on behalf of Reshad Rahman (rrahman)" <[email-masked] on behalf of [email-masked]> wrote:

    Hi,

    Regards,

    Reshad.

        Reviewer: Ebben Aries

        2 modules in this draft:

          treatment in handling.

          the modules

          the modules

        - Abstract: s/RVSP/RSVP/

        [email-masked]

    Teas mailing list

    [email-masked]

    https://www.ietf.org/mailman/listinfo/teas

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Tue, 14 May 2019 16:31:46 +0000

Hi Tarek,

If a user wants to have information for a specific 2205 session or a 3209 session, an artificial key or no key doesn’t help for that situation: the whole list has to be read until the session is found. Why not have separate lists (under a container) for the different session types, that way you can have meaningful keys for the 2205 and 3209 sessions.

Regards,
Reshad.

From: Tarek Saad <[email-masked]>
Date: Tuesday, May 14, 2019 at 10:46 AM
To: "Reshad Rahman (rrahman)" <[email-masked]>, "[email-masked]" <[email-masked]>
Cc: "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, Ebben Aries <[email-masked]>
Subject: Re: [Teas] [yang-doctors] Yangdoctors early review of draft-ietf-teas-yang-rsvp-10

Hi Reshad,

Regards,

Tarek

    Hi,

    Regards,

    Reshad.

        Reviewer: Ebben Aries

        2 modules in this draft:

          treatment in handling.

          the modules

          the modules

        - Abstract: s/RVSP/RSVP/

        [email-masked]

    Teas mailing list

    [email-masked]

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Tue, 14 May 2019 17:27:40 +0000

Hi Tarek,

I stand corrected. As you’ve pointed out this is a read-only list and Xpath filter can be used, no need for “meaningful” keys.

Regards,
Reshad.

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Tuesday, May 14, 2019 at 12:31 PM
To: Tarek Saad <[email-masked]>, "[email-masked]" <[email-masked]>
Cc: "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, Ebben Aries <[email-masked]>
Subject: Re: [Teas] [yang-doctors] Yangdoctors early review of draft-ietf-teas-yang-rsvp-10

Hi Tarek,

Regards,
Reshad.

Hi Reshad,

Regards,

Tarek

    Hi,

    Regards,

    Reshad.

        Reviewer: Ebben Aries

        2 modules in this draft:

          treatment in handling.

          the modules

          the modules

        - Abstract: s/RVSP/RSVP/

        [email-masked]

    Teas mailing list

    [email-masked]

---

From: Jan Lindblad <[email-masked]>
Date: Tue, 14 May 2019 19:45:20 +0200

Reshad, all,

Even with an XPath search (on servers that support XPath), a full linear search is required. The linear search would be performed by the server, but still takes the same amount of search work. If there can be a lot of sessions, it would be wise to consider using separate lists and natural keys.

Keyless lists are only good when the number of entries is guaranteed to be small (now and forever), as there is no other way to read such lists than reading the entire list at once. Usually not optimal for clients if there may be many entries.

/jan