Subject: [OPSAWG] Yangdoctors early review of draft-ietf-opsawg-sap-02

From: Martin Björklund via Datatracker <[email-masked]>
Date: Tue, 08 Mar 2022 00:42:59 -0800

Reviewer: Martin Björklund
Review result: Ready with Nits

This is my YANG doctors review of draft-ietf-opsawg-sap-02.  I have reviewed the YANG module only.

The YANG module has the following structure:

    +--rw service* [sap-type]
       +--rw sap-type                    identityref
       +--rw service-attachment-point* [attachment-id]
          +--rw attachment-id               string
          +--rw description?                string
          +--rw attachment-interface?       string
          +--rw parent-termination-point?   nt:tp-id
          +--rw interface-type?             identityref
          +--rw encapsulation-type?         identityref
          +--rw role?                       identityref
          +--rw peer-customer-sap-id?       string

o  "service"

  The model defines a list "service", with the key "sap-type".  This
  looks a bit weird - is the term "service" the same thing as "sap"?
  I don't think so, and the text in the document talks about "service
  type" in section 5.

  I don't know which term is correct.  If the correct term "service",
  I suggest you change "sap-type" to "service-type".  If the correct
  term is "sap", I suggest you change the list name.

  In any case, there can obviously only be one "service" of a given
  type, but I assume this is intentional.

o  "service-attachment-point"

  In other places in the model you use "sap" ("peer-customer-sap-id",
  "sap-type").  Perhaps simple call this "sap".

o  "attachment-id"

  In the description of the leaf "attachment-id", it suggests that
  this is the identifier of the "service-attachment-point".  Normally,
  I would suggest to not repeat the name of the list in the names of
  the nodes in the list, and thus I would suggest simply "id".
  However, in this case it seems the model that this module augments
  use this convention, so perhaps call this leaf
  "service-attachment-point-id" - which is quite long, perhaps
  "sap-id".

  Also note the name mismatch "peer-customer-sap-id" and
  "attachement-id".

  Also, I note that the type of the id in this document is a string,
  whereas in the other models it is an inet:uri.  See section 4.4.11
  of RFC 8345.  Note that all examples in RFC 8345 violates the "uri"
  type, so it it is not clear to me that they really meant uri.  My
  suggestion is to stick to string for this type.  (BTW, the example
  in this document also violates the "uri" type, e.g.:
             "network-id": "an-id"
  is not a valid uri.)

o  "attachment-interface" vs "interface-type"

  These two leafs define properties of the same object; the name and
  type of the "interface to which the SAP is bound".

  This should be reflected in the names of these leafs.  Perhaps:
  "interface-name" and "interface-type".

  Also, I suggest you put these two leafs together in the data model, giving:
  
          ...
          +--rw description?                string
          +--rw parent-termination-point?   nt:tp-id
          +--rw interface-name?             string
          +--rw interface-type?             identityref

o  "interface-type"

  It is not clear to me how this relates to the
  "/interfaces/interface/type" from RFC 8343.  How are the interface
  types defined in iana-if-type mapped to the types defined in this
  document?  Perhaps they are not?

o  "role"

  The description of "role" says: 

           description
             "Indicates whether the SAP is an UNI or a NNI.";

  Now, the role is an identity, which is an "extensible enumeration".
  The description seems to indicate that this leaf really can have
  just two values.  If the description is right, perhaps change this
  type to an enumeration.  Otherwise, change the description.

o  "peer-customer-sap-id"

  The description says:

             Indicates an identifier of the peer's termination
             identifier (e.g., Customer Edge (CE)). This

  Is the peer always a "customer sap"?  If so, it could be explained
  better in the description.  If not, the leaf's name is not correct.

/martin

---

From: <[email-masked]>
Date: Tue, 8 Mar 2022 10:39:52 +0000

Hi Martin,

Many thanks for the review. 

All good points. Please see inline for more context. 

The changes can also be tracked at: https://www.ietf.org/rfcdiff?url1=draft-ietf-opsawg-sap&url2=https://raw.githubusercontent.com/IETF-OPSAWG-WG/lxnm/master/I-D-sap/draft-ietf-opsawg-sap.txt

Cheers,
Med

> -----Message d'origine-----
> De : OPSAWG <[email-masked]> De la part de Martin Björklund via
> Datatracker
> Envoyé : mardi 8 mars 2022 09:43
> À : [email-masked]
> Cc : [email-masked]; [email-masked]
> Objet : [OPSAWG] Yangdoctors early review of draft-ietf-opsawg-sap-02
> 

[Med] Made this change s/sap-type/service-type. 

>   type, but I assume this is intentional.

[Med] Yes. 

>   "sap-type").  Perhaps simple call this "sap".

[Med] Works for me. 

>   "service-attachment-point-id" - which is quite long, perhaps

[Med] went with this one and peer-sap-id. 

>   in this document also violates the "uri" type, e.g.:

[Med] Good catch. Fixed the example. Thanks. 

>   "interface-name" and "interface-type".

[Med] I prefer to maintain the OLD name. 

>           +--rw interface-type?             identityref

[Med]  OK.

>   document?  Perhaps they are not?

[Med] We have considered in the past the use of 8343/7224, but we went finally with the current approach as we want to cover a limited set of things such as bridge domains, bearer references, IRBs, etc. that we are already using in other service modules. The mapping to the detailed interface types as per 7224 will be made by an orchestrator when it translates this network module into devices modules. 

>   type to an enumeration.  Otherwise, change the description.

[Med] Fair. Changed the description. 

>   better in the description.  If not, the leaf's name is not correct.

[Med] Changed the name. Thanks. 

> 
> /martin
> 
> 
> 
> _______________________________________________
> OPSAWG mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/opsawg

_________________________________________________________________________________________________________________________

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.

---

From: Martin Björklund <[email-masked]>
Date: Tue, 08 Mar 2022 16:23:11 +0100 (CET)

Hi,

Trimming...

<[email-masked]> wrote:

[...]

> > Reviewer: Martin Björklund

[...]

> [Med] We have considered in the past the use of 8343/7224, but we went
> finally with the current approach as we want to cover a limited set of
> things such as bridge domains, bearer references, IRBs, etc. that we
> are already using in other service modules. The mapping to the
> detailed interface types as per 7224 will be made by an orchestrator
> when it translates this network module into devices modules.

Ok.  Perhaps you can add a few lines of text that explains this in the
document.

/martin

---

From: <[email-masked]>
Date: Tue, 8 Mar 2022 16:10:59 +0000

Re-,

Sure. We will add a note about that. 

Thank you for the follow-up. 

Cheers,
Med

> De : Martin Björklund <[email-masked]>
> Envoyé : mardi 8 mars 2022 16:23
> À : BOUCADAIR Mohamed INNOV/NET <[email-masked]>
> Cc : [email-masked]; [email-masked];
> [email-masked]
> Objet : Re: [OPSAWG] Yangdoctors early review of draft-ietf-opsawg-sap-
> 02
> 
> Hi,
> 
> Trimming...
> 
> 
> > > De : OPSAWG <[email-masked]> De la part de Martin Björklund
> > > via Datatracker Envoyé : mardi 8 mars 2022 09:43 À :
> > > [email-masked] Cc : [email-masked];
> > > [email-masked] Objet : [OPSAWG] Yangdoctors early review of
> > > draft-ietf-opsawg-sap-02
> > >

---

From: <[email-masked]>
Date: Fri, 25 Mar 2022 06:54:00 +0000

Hi Martin, 

FWIW, the changes discussed in this thread are now implemented in the draft: https://datatracker.ietf.org/doc/html/draft-ietf-opsawg-sap-03

Cheers,
Med

> De : [email-masked] <[email-masked]>
> Envoyé : mardi 8 mars 2022 17:11
> À : Martin Björklund <[email-masked]>
> Objet : RE: [OPSAWG] Yangdoctors early review of draft-ietf-opsawg-sap-
> 02
> 
> Re-,
> 
> > De : Martin Björklund <[email-masked]> Envoyé : mardi 8 mars 2022
> > 16:23 À : BOUCADAIR Mohamed INNOV/NET <[email-masked]>
> > > > De : OPSAWG <[email-masked]> De la part de Martin
> > > > Björklund via Datatracker Envoyé : mardi 8 mars 2022 09:43 À :
> > > [Med] We have considered in the past the use of 8343/7224, but we
> > > went finally with the current approach as we want to cover a limited
> > > set of things such as bridge domains, bearer references, IRBs, etc.
> > > that we are already using in other service modules. The mapping to
> > > the detailed interface types as per 7224 will be made by an
> > > orchestrator when it translates this network module into devices
> modules.
> >
> ________________________________________________________________________
> _________________________________________________
> 
> 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.