Re: [yang-doctors] Yangdoctors early review of draft-ietf-rtgwg-policy-model-09

[Yingzhen Qu <[e-mail]>]

Hi Mahesh,

Thank you for your thorough review. I’ve uploaded version -12: removed “source-protocol” from the example and added “Routing Policy” definition.

Thanks,
Yingzhen

From: Mahesh Jethanandani <[e-mail]>
Date: Wednesday, May 27, 2020 at 3:17 PM
To: Yingzhen Qu <[e-mail]>
Cc: "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>
Subject: Re: Yangdoctors early review of draft-ietf-rtgwg-policy-model-09

Hi Yingzhen,

Thank you for addressing my comments. I think we are down to the last few.

Thanks for including the example. It really helps in implementation. However, I believe we have a bug. On the command line below, all the YANG models published by IETF reside in my ~/git/iana/yang-parameters directory and the example you included in the draft is in the file called example-routing-policy.xml. On running the command, I get the following error:

% yanglint -s -i -t auto -p ~/git/iana/yang-parameters/ [e-mail]<mailto:[e-mail]> example-routing-policy.xml
err : Failed to resolve identityref "bgp". (/ietf-routing-policy:routing-policy/policy-definitions/policy-definition[name='export-tagged-BGP']/statements/statement[name='term-0']/conditions/source-protocol)

That is because you do not have “bgp” defined anywhere in your module. The only definition for that exists in ietf-bgp module. But you cannot include that module, because it creates a circular dependency (ietf-bgp depends on ietf-routing-policy).

Luckily for you, ‘source-protocol’ is not a mandatory field, so if you remove the following line in the example, the example is validated.

              <source-protocol>bgp</source-protocol>

For the remaining point see inline.


On May 27, 2020, at 1:36 PM, Yingzhen Qu <[e-mail]<mailto:[e-mail]>> wrote:

Hi Mahesh,

Thanks for your nice and patient review. I’ve uploaded version -11 to address your comments.

Major changes:

  *   Removed groupings only used in the model and not meant to be used later by augmentations
  *   Added an xml example in section 11.
  *   Fixed the cross-reference issue.
  *   Changed the “mailto” in the model to “Email”

Looking at the terms we have so far in section 2, I don’t think it’s necessary to add a definition for “routing policy”. But this is open for discussion.

It might be obvious to folks who know and understand routing what "routing policy" means. But it is not obvious to anyone who knows YANG modeling what it means. I am not going to hold up the review because of this, but I would highly encourage the authors to come up with a definition and include it in the draft.

Cheers.



Thanks,
Yingzhen

From: Mahesh Jethanandani <[e-mail]<mailto:[e-mail]>>
Date: Sunday, May 24, 2020 at 11:39 AM
To: Yingzhen Qu <[e-mail]<mailto:[e-mail]>>
Cc: "[e-mail]<mailto:[e-mail]>" <[e-mail]<mailto:[e-mail]>>, "[e-mail]<mailto:[e-mail]>" <[e-mail]<mailto:[e-mail]>>, "[e-mail]<mailto:[e-mail]>" <[e-mail]<mailto:[e-mail]>>
Subject: Re: Yangdoctors early review of draft-ietf-rtgwg-policy-model-09

[Trimming the list to open issues]

Hi Yingzhen,



On May 23, 2020, at 1:21 PM, Yingzhen Qu <[e-mail]<mailto:[e-mail]>> wrote:

Hi Mahesh,

Thanks for the review, very much appreciated.

I submitted -10 version to address your comments: https://datatracker.ietf.org/doc/draft-ietf-rtgwg-policy-model/<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdatatracker.ietf.org%2Fdoc%2Fdraft-ietf-rtgwg-policy-model%2F&data=02%7C01%7Cyingzhen.qu%40futurewei.com%7C6ec8621912f64640bd2208d8028baff0%7C0fee8ff2a3b240189c753a1d5591fedc%7C1%7C0%7C637262146270083465&sdata=g07tQ8sUtiRs7rQqf3tCDBTfp6Ya0q6j681QIxZ3duc%3D&reserved=0>

Note: There are YANG validation errors because yanglint can't find model "ietf-if-extensions", not sure why.
yanglint SO 1.6.7: yanglint --verbose -p {rfclib} -p {draftlib} -p {tmplib} {model} -i:
err : Data model "ietf-if-extensions" not found.
err : Importing "ietf-if-extensions" module into "ietf-routing-policy" failed.
err : Module "ietf-routing-policy" parsing failed.

ietf-if-extensions is part of the draft-ietf-netmod-inft-ext-yang draft, which is still a draft. It should ideally be in the {draftlib} path variable. I would suggest a open an issue with the tools team to have them track it.

I am assuming of course that you are able to validate the model locally on your machine.
…



   The document includes a section on Terminology and Notation. However it does
   not define what is a "routing policy" or provide a reference to it.
[YQ]: I'm not sure there is a standard definition of "routing policy". I'll do some
research on this and see whether we should add it to the terminology.

Looking at how the draft describes it, how about something like this?

“A routing policy defines whether a routing protocol imports a route into the routing table, modifies it, or exports a route from the routing table."

…



   Section 10.1 - Routing policy model

   The model uses a lot of groupings, most of them used only once in the model.
   That makes the model difficult to understand. Unless these groupings are meant
   for use by other models, they should be folded into the main container. Take
   this grouping for example:

        grouping policy-definitions {
          description
            "This grouping provides policy definitions";

          leaf name {
            type string;
            description
              "Name of the top-level policy definition -- this name
              is used in references to the current policy";
          }
        }

   For one the description of the grouping is not correct. Secondly, it is used
   only once in the model and that too to define a key of a list.

[YQ]: This model was initiated by OpenConfig, we've removed some groupings
Since this become an IETF draft. I removed "policy-definitions" and "policy-statements"
groups.

The general rule is that if the grouping is used only once in the model, and not meant for use outside the module, that it should not be a grouping.

For example, ‘grouping neighbor-set’ is used only once in the model, and I doubt it is meant for use outside of this module. That grouping should be collapsed into the model where it gets used. Same for ‘grouping tag-set’. These are just two examples. There are many more.




   The YANG model imports multiple models from other drafts. It provides normative
   references to the imported models in Section 2.2. (A nit here is that it
   missing the cross-reference in the table for if-l3-vlan),

A nit. if-l3-vlan is still missing the cross-reference (and the hyperlink) to the draft. Also why idnits is giving you an error at the bottom.



but does not provide
   references to them in the model. See Section 3.9 of RFC 8407. Please add
   something like this in the model.

   import ietf-inet-types {
     prefix "inet";
     reference
       "RFC 6991: Common YANG Data Types.";
   }
[YQ]: fixed.

   Please drop <mailto:....> and just keep the e-mail address. That tag works only
   when embedded within a HTML document.
[YQ]: do you mean the "editor" in the model?

I meant to say you should drop the word “mailto:" from the list of e-mail addresses of the editors. That tag works inside a HTML document, not inside a YANG model.




   A pyang compilation of the model with —ietf and —lint option was clean.
   However, there seems to be an issue with the example in Section 11. Instead of
   actually including an example, it is showing the path to what should have been
   the example. I was therefore not able to validate the example against the model.
[YQ]: I can't find this example in OpenConfig Github anymore, so removed this section.

Hmm. That leaves the model without any examples, and examples are important.

I was able to find the example in an earlier version of your own draft (01). I would suggest that you take that example and modify it to make it compatible with the latest version of the model. If you need help, let me know.




     Checking references for intended status: Proposed Standard
     ----------------------------------------------------------------------------

        (See RFCs 3967 and 4897 for information about using normative references
        to lower-maturity documents in RFCs)

     == Unused Reference: 'I-D.ietf-netmod-sub-intf-vlan-model' is defined on
        line 1620, but no explicit reference was found in the text

The reason you are getting this error is because you are lacking a normative reference to I-D.ietf-netmod-sub-intf-vlan-model in the document. Goes back to the nit I point up earlier.

I would suggest that once you have fixed all the issues in the draft, that you re-run idnits locally on the machine.




     -- No information found for draft-ietf-netmod-intf-ext-yang - is the name
        correct?

     -- Possible downref: Normative reference to a draft: ref.
        'I-D.ietf-netmod-intf-ext-yang'

     -- No information found for draft-ietf-netmod-sub-intf-vlan-model - is the
        name correct?

     -- Possible downref: Normative reference to a draft: ref.
        'I-D.ietf-netmod-sub-intf-vlan-model'

[YQ]: there is dependency on both 'I-D.ietf-netmod-intf-ext-yang' and 'I-D.ietf-netmod-sub-intf-vlan-model'.

        Summary: 0 errors (**), 0 flaws (~~), 9 warnings (==), 5 comments (--).