|
Hi Jürgen,
Reviewer: Jürgen Schönwälder
Review result: Almost Ready
* General YANG Review Remarks
This document depends on a number of other I-Ds. Is it safe to
process this document while the other documents this document
depends on are not yet through the process? Who is tracking things
and making sure that any changes in other documents that may impact
the PIM document are detected and handled appropriately before
publication?
I do look at the YANG module dependencies, thanks to tooling.
Let's look at the draft-ietf-pim-yang-12 dependencies:
https://www.yangcatalog.org/yang-search/impact_analysis.php?modules[]=ietf-pim-dm@xxxxxxxxxxxxxxx&modules[]=ietf-pim-sm@xxxxxxxxxxxxxxx&modules[]=ietf-pim-base@xxxxxxxxxxxxxxx&modules[]=ietf-pim-bidir@xxxxxxxxxxxxxxx&modules[]=ietf-pim-rp@xxxxxxxxxxxxxxx&recurse=2&rfcs=1&show_subm=1&show_dir=dependencies
[I-D.ietf-rtgwg-routing-types] is RFC 8294. We will update the tool.
[I-D.ietf-netmod-rfc7223bis] is on the Jan 11th IESG telechat.
[I-D.ietf-netmod-rfc8022bis] is in IETF LC now and on the Jan 25th
IESG telechat
The only unknown to me is the state of [I-D.ietf-bfd-yang]. Copying
Alia and the BFD chairs.
In terms of I-D normative references,
https://tools.ietf.org/html/draft-ietf-pim-yang-12#section-9.1,
there is an extra related one:
[I-D.ietf-netmod-revised-datastores] is on the Jan 11th IESG
telechat
So we're getting there...
Regards, Benoit
It is generally good style to write complete sentences in
description statements. Some of the description statements are just
fractions of a sentence.
I think we do not recommend anymore to list WG chairs in the contact
statement.
It is sometimes not clear why you define groupings that are only
used once (and sometimes are likely not reusable since they contain
relative paths in must expressions).
* Introduction
- Is there a reason why you refer to RFC 6020 and to RFC 7950 in
sections 1 and 1.2? Why do you need the reference to RFC 6020?
- What are 'wider' management interfaces? If you mention NETCONF,
why not mention RESTCONF?
- s/Protocol-Independent Multicast (PIM) devices
/devices supporting Protocol-Independent Multicast (PIM)/
- So which YANG terminology applies, RFC 6020 or RFC 7950? I
personally think using YANG 1.1 is pretty safe these days.
* Design of Data Model
- I did not really understand Section 2.5. It seems you are
duplicating objects for different address families but on some
systems these duplicate objects must have the same value. If so,
where would the must _expression_ go and how does an implementation
add such a must _expression_? How many of such must expressions
would there have to be? Did you consider having address family
independent objects and optionally (controlled by a feature) per
address family objects that overwrite the independent settings?
* Module Structure
- s/is included/is imported/
- The tree diagrams are rather long. It would likely help readers if
the diagram would be split into meaningful units and additional
text added to describe the units.
Are lists of the form
| +--rw ipv4-rp* [ipv4-address]
| | +--rw ipv4-address inet:ipv4-address
designed for exensibility? Otherwise, this may be collapsed into
a simple leaf-list.
- Since I am not familiar with details of PIM, I can't comment on
the question whether the model makes sense or not.
* PIM base Module
- YANG modules compile cleanly according to the tracker page.
- As said above, consider using YANG 1.1. The ietf-routing module
actually uses YANG 1.1 so you will need a YANG 1.1 compiler
anyway.
- Consider adding reference statements to the feature definitions
in case a feature is described in a protocol specification.
- The value of timer-value is not really clear, you could have used
rt-types:timer-value-seconds16 directly.
- Why is graceful-restart/duration using an ad-hoc type for 16-bit
seconds and not timer-value? Is it because infinity and not-set
does not apply?
- Does a bfd/hello-interval of 'infinity' or 'not-set' make sense?
- More explicit description of bfd/hello-holdtime? Is a choice
really appropriate (hello-holdtime-or-multiplier)? Can I not have
both holdtime and multiplier? Perhaps I am just not clear what
holdtime does...
- Does a bfd/jp-interval of 'infinity' or 'not-set' make sense?
- More explicit description of bfd/jp-holdtime? Is a choice really
appropriate (jp-holdtime-or-multiplier)? Can I not have both
holdtime and multiplier? Perhaps I am just not clear what holdtime
does...
- Please provide more meaningful descriptions:
description "Propagation description";
description "Override interval";
- What is the meaning of the interface augmentation 'oper-status'
relative to 'oper-status' defined by ietf-interfaces? Is this just
a duplicate with fewer states? Or is this somehow more specific to
multicast or PIM packets? In the later case, I think this deserves
to the be explained in the description clause.
- How do the ip4 and ipv6 addresses relate to ip addresses assigned
to an interface in ietf-ip?
- What is the meaning of hello-expiration 'not-set'?
- What is the meaning of expiration 'not-set'?
- Is it useful to return the uptime in seconds (which is changing on
every get that is not in the same second) or could it be an option
to report the time when something transitioned into the up state
(which is not changing)? Well, it could be that we are simply used
to uptime like objects. Anyway, the description of up-time should
make it clear what exactly is defining the state 'up'. If this
says up for 5 seconds, what exactly transitioned into an 'up'
state 5 seconds ago?
- Is the any restriction for dr-priority or is it a full 32-bit
unsigned number space? In some vendor documentation I saw 0..65535
with a default of 1. What do RFCs say?
- I am not sure what the precise meaning of the error statistic
counters are. What turns an received or sent messages into an
error message? The description of grouping statistics-error does
not explain this. Also, if I receive a hello and I later decide
that this hello must have been an error, is this hello counted
twice? And what about messages that could not be parsed because
they were malformed, where are those counted?
- Why is 'pim' a presence container?
- Do comments like 'configuration data nodes' make sense if you
include config false nodes in the same branch of the tree?
* PIM RP Module
- Does the feature bsr-election-state depend on the feature bsr?
- Should there be a default bsr-candidate/priority?
- Do you need the address/hash-mask-length/priority in
bsr-state-attributes in an NMDA implementation?
- I _assume_ the bsr-next-bootstrap value has to be interpreted
relative to the time the value was obtained. What about making
this an absolute timestamp instead? Well, actually I am not sure
what the value represents - is it the remaining time interval
until the next bootstrap will be sent?
- I have no clue what to expect here:
leaf group-policy {
type string;
description "Group policy.";
}
What can I expect the string to contain?
- I am again uncertain how exactly to understand the value of
rp-candidate-next-advertisement, see similar questions above.
- What are the policy values that can go into this:
leaf policy-name {
type string;
description
"Static RP policy.";
}
Is the string just an arbitrary name or does it mean something?
- How is this supposed to be used?
leaf policy {
type string;
description
"ACL (Access Control List) policy used to filter group
addresses.";
}
What is the meaning of <policy>foo</policy>?
* PIM SM Module
- What is the meaning of a policy-name value?
leaf policy-name {
if-feature spt-switch-policy;
type string;
description
"Switch policy.";
}
- What is the meaning of a range-policy value?
leaf range-policy {
type string;
description
"Policy used to define SSM address range.";
}
* PIM DM Module
- I wonder, would you need an identity for dense mode?
* PIM BIDIR Module
- Remove
/*
* Typedefs
*/
- What is the meaning of offer-interval or backoff-interval
'not-set'?
* Implementation Status
It seems the trac page pointed to was used to collect information
about what proprietary implementation support, i.e., it does not
document to what extend the models defined in this document have
been implemented. There are some notable differences. For example,
it seems most counters implemented are 32-bit while most counters in
the YANG models are 64-bit. How chatty is PIM, are 64-bit counters
really needed and are implementors willing to move to 64-bit
counters?
* Security Considerations
I think this needs to include a bit more details. See
https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines
for the latest template that YANG module authors are expected to
use.
_______________________________________________
yang-doctors mailing list
yang-doctors@xxxxxxxx
https://www.ietf.org/mailman/listinfo/yang-doctors
|