Hi,
Here are my review comments on YANG 1.1.
IMO the document is almost ready for publication.
I plan to implement YANG 1.1 in YumaPro tools soon.
Andy
Review of draft-ietf-netmod-rfc6020bis-08.txt
Andy Bierman
2015-10-27
Sec 1:
- text about 'proposed to be used' should be 'going to be used';
RESTCONF and JSON drafts part of same WG charter as YANG 1.1
- RESTCONF needs to be in scope
"Other protocols and encodings are possible, but out of scope for this
specification."
Sec. 1.1
o Made "when" and "if-feature" illegal on list keys, unless the
parent is also conditional, and the condition matches the parent's
condition.
- This contradicts text in
7.20.2:
A leaf that is a list key MUST NOT have any "if-feature" statements.
7.21.5:
A leaf that is a list key MUST NOT have a "when" statement.
- Which is correct?
- The 7.* MUST NOT text is not backward compatible with YANG 1.0
I think the WG agreed on what the bullet in 1.1 says, not the
new restrictions in 7.*
Sec. 4.2.1, para 2:
A server may implement a number of modules, allowing multiple views
of the same data, or multiple views of disjoint subsections of the
server's data. Alternatively, the server may implement only one
module that defines all available data.
- What does 'multiple views of the same data' really mean?
- Should 'may' be 'MAY'?
Sec. 5.6.4: para 2:
A server MUST NOT implement more than one revision of a module.
- IMO this should a SHOULD NOT or 'MUST NOT advertise' instead
of MUST NOT implement
Example of possible exception:
- rev N-1 has object needed by application
- rev N has that object set to status 'obsolete' (and removed)
- rev N also has other objects unrelated to the obsolete object
but desired for the product feature set
Sec. 5.6.4:
- para 4: the rules for populating the YANG library should probably be
in the yang-library draft
- the example on page 35 - 37 should probably be in the
yang-library draft
Sec. 5.6.5. Announcing Conformance Information in NETCONF
- This should really be in a NETCONF 1.2 spec
- RESTCONF draft defines ietf-yang-library requirements for RESTCONF.
Why should YANG 1.1 define NETCONF ietf-yang-library requirements?
Sec. 5.7: Datastore Modification
- This section is NETCONF specific for no apparent reason
Sec. 6.4.1: bullet 5
o The set of variable bindings is empty.
- This is not true. The NACM data model defines the USER variable.
This text should say that YANG modules MAY define variable bindings
associated with conformance for that module (like NACM does).
Sec. 6.4.1: XPath accessible tree
- I object to expanding the context of XPath evaluation for
notification and rpc/input, rpc/output. This is too complicated
to implement and under-specified, resulting in poor interoperability.
- What is the justification for this expansion of complexity?
An tool cannot reproduce the same result anymore for
<rpc>, <rpc-reply> or <notification> offline validation.
- Notification context expanded from notification message to message +
running config + operational state; When is the server supposed
to gather and check all the data nodes? Event time? Buffer time?
Send time? What if operational state or config changes while the
notification is in the replay buffer?
- RPC input context expanded from input message to message +
running config + operational state; When is the server supposed
to gather and check all the data nodes? Parse time? Buffer time?
Invocation time?
- RPC output context expanded from input message to message +
running config + operational state; Is the server supposed to
wait for operational state that may change as a result of the
action or rpc being invoked?
- The expanded contexts for notification and rpc statements
require coupling of the notification and RPC processing
implementation components to the datastore and operational data.
This is a big change from current NETCONF requirements.
YANG should not be making any changes to NETCONF, but it does
in several places, effectively creating a new undeclared
version of the NETCONF protocol
- It is not clear if "action" and "notification" statements
nested in the data tree are visible at all for XPath evaluation.
(They MUST NOT be visible)
- NETCONF <get> and <get-config> "filter" element needs clarification
wrt/ action and notification within the data tree
- NETCONF and RESTCONF notifications "filter" element needs clarification
wrt/ action and notification within the data tree
Sec. 7.5.3: must: last para
Also note that the XPath expression is conceptually evaluated. This
means that an implementation does not have to use an XPath evaluator
in the server. How the evaluation is done in practice is an
implementation decision.
Sec. 7.5.4.2. The error-app-tag Statement
The "error-app-tag" statement, which is optional, takes a string as
an argument. If the constraint evaluates to false, the string is
passed as <error-app-tag> in the <rpc-error> in NETCONF.
- all the error-message and error-app-tag sub-sections apply to
RESTCONF as well as NETCONF. They share the same error reporting
structure
Sec. 7.5.8:
Sec. 7.6.7:
Sec. 7.7.9:
Sec. 7.9.6:
Sec. 7.10.3: NETCONF <edit-config> Operations
- Can these sections be changed to 'Configuration Datastore Edit
Operations' and be generic so they focus on the contents of
the datastore before and after the edit, without specifying
<edit-config> as the edit operation
- RESTCONF needs to be supported, or all this should be moved
to NETCONF 1.2
Sec. 7.6.1. The leaf's default value
- this section should mention that a false if-feature
or false when-stmt removes the schema node from the
tree, which overrides all this text. There is no default
value if there is no schema node
- It is confusing that 'must' and 'when' work very
differently wrt/ default. must-stmt does not ignore defaults
but 'when' overrides and removes the 'when', so it needs to
be evaluated before the 'must' stmt:
leaf confusing {
must "/some/val=4";
when "/some/other/value/test";
default 42;
type int32;
}
Sec. 7.7.2. The leaf-list's default values
- this section represents 2 new requirements to compilers
that should be more apparent:
1) remembering the order of "default" statements
2) the default only applies if 'min-elements 0' is in effect
- what is the rationale for not applying the default if
min-elements is > 0? It looks like the leaf/container rule
that mandatory and default are not allowed together is
being applied here. This should be more clear here
Sec. 7.11. The anyxml Statement
- The 'anyxml' statement should be deprecated.
If not, this section needs to provide rationale for
its 'current' status, given that 'anydata' has been added
No use cases are provided for anyxml in the draft.
- There are few if any servers that support anyxml the way
it is defined here, and that is not likely to change.
The draft does not specify how the XML must be parsed,
stored, or rendered later, if it is part of configuration
Sec. 7.15. The action Statement
- an action must appear directly within a container or a
list, an 'augment' or a 'uses' statement.
Why is 'case' statement left out of direct usage and
only allowed via 'uses' or 'augment'?
- para 2: groupings are not reusable if they contain actions,
since they are not allowed in rpc, notification, or action
This should be pointed out in the section on groupings
- the reason for the restriction on top-level action is not
given. The syntax would support this, and the identifier
namespace is shared (same as rpc).
- the context nodes for action input and output are very different
this could be more clear
- is the server required to process XPath or constraints on the
output at some point in the execution of the action?
- there is no rationale given why the ping operation should
be done as an action instead of an RPC
list interface {
key "name";
leaf name {
type string;
}
action ping {
input {
leaf destination {
type inet:ip-address;
}
}
output {
leaf packet-loss {
type uint8;
}
}
}
}
Why not use an RPC?
No rationale for both ways to implement an operation
like 'ping' is given.
rpc ping {
input {
leaf itf-name {
mandatory true;
type if:interface-ref;
}
leaf destination {
type inet:ip-address;
}
}
output {
leaf packet-loss {
type uint8;
}
}
}
Sec. 7.16. The notification Statement
If a leaf in the notification tree has a "mandatory" statement with
the value "true", the leaf MUST be present in a notification
instance.
- Why is this apply to leaf but not container or choice?
They should be mentioned here as well.
- No rationale is given for defining notifications within data nodes
vs. top-level notification-stmt
- It is not clear how RFC 5277 <filter> can be applied to the
notification message that is nested within data, given the
location of the 'notification' root within the data tree is
not static, and different for each notification.
Sec. 7.17. The augment Statement
- can augment path-stmts contain nested 'action' or 'notification'
nodes and subnodes, like for rpc-stmt? If, so then the
sub-statements are restricted by context, e.g., action-stmt
cannot be added to /foo/bar/action/input/baz.
Sec. 7.18.2. The base Statement
- There are no examples or use-cases given for multiple base
statements. An example should be added.
- Para 1 says:
If multiple "base" statements are present, the identity is derived
from all of them.
The draft does not say how this is done. It is not clear how
a particular identity-stmt has multiple bases and how a server
determines an identify is derived from multiple bases. Examples
of a 'pass' and 'fail' for this test should be given.
Sec. 7.19. The extension Statement
"YANG statements in extensions MUST follow the syntactical rules
in Section 14."
- What is the rationale for this rule? Extension statements
should be outside the scope of YANG. They should just follow the
syntax rules in sec. 6.
- This says YANG syntax MUST be preserved, but YANG semantics
can be ignored or changed. Is that the intent?
"An extension can allow refinement (see Section 7.13.2) and deviations
(Section 7.20.3.2), but the mechanism for how this is defined is
outside the scope of this specification."
- This text about refinement and deviations is not clear.
Is it MUST, SHOULD, or MAY? It is normative, but unspecified.
Sec. 7.20.2. The if-feature Statement
- This section does not define how "(" if-feature-expr ")" is
evaluated, or how the precedence is evaluated if parens are used.
Evaluated of nested parens is not defined either.
- Are operators of the same type evaluated left to right?
- should be some examples to show precedence
"not foo or bar and baz"
"(not foo) or (bar and baz)"
"not (foo or bar) and baz"
Sec. 7.21.2. The status Statement
If a definition is "current", it MUST NOT reference a "deprecated" or
"obsolete" definition within the same module.
If a definition is "deprecated", it MUST NOT reference an "obsolete"
definition within the same module.
- It is not clear what it means for a definition to reference
another definition.
- Why is this restriction limited to the same module and not
include imported modules?
- Don't the 'deprecated' and 'obsolete' values ripple up through
the data tree, all the way to the top-level node? Otherwise
a current list can have deprecated or obsolete children.
It should be explained how status is contained (or not)
for nodes within other statements.
- What does it mean to use a current grouping that contains deprecated
or obsolete nodes? Is this allowed?
Sec. 7.21.5. The when Statement
bullet 3:
- Altering the tree changes means that each when-stmt processes
a different XML document. IMO it is a bad idea to replace a
node that is in the tree with a dummy node. It is OK to create
a temp dummy node as an implementation detail to decide if
a when-stmt is true or not.
"If the XPath expression references any node that also has associated
"when" statements, these "when" expressions MUST be evaluated first.
There MUST NOT be any circular dependencies in these "when"
expressions."
- This requirement is not implementable and needs to be removed.
The draft should just say what the result is supposed to be
(and it does -- all nodes with false when-stmts are removed)
"Note that the XPath expression is conceptually evaluated. This means
that an implementation does not have to use an XPath evaluator in the
server. The "when" statement can very well be implemented with
specially written code."
- The rules for how to process a when-stmt contradict this
text at the end of the section that when-stmt is only
conceptual. No implementation details should be mandated.
Sec. 8.1: Constraints on Data
- bullets say constraint MUST be true, but this info should really
be part of NETCONF, and should say what a NETCONF server should
so when a constraint is false
Sec. 8.2.1. Payload Parsing
- The NETCONF-specific error handling text should be made more generic
Sec. 8.2.2. NETCONF <edit-config> Processing
- This text should be more generic if possible.
- Why do all the NETCONF implementation requirements apply only
to <edit-config> and not also <copy-config>?
Sec. 9.6.4.1. The enum's Substatements
Sec. 9.7.4.1. The bit's Substatements
- example of if-feature should be given
- It should be explicitly stated that if-feature has no effect
on auto-numbering
- what happens if a feature value is changed at boot-time or run-time,
causing valid enumeration or bits values within a datastore to
suddenly become invalid? Changing the value set of a leaf or leaf-list
with if-feature is very complicated and under-specified
Sec. 9.9.1. (Leafref) Restrictions
- can leafref require-instance be changed via typedef?
typedef foo-ptr {
type leafref {
path "/some/node/foo";
}
}
typedef foo-ptr2 {
type foo-ptr {
require-instance false;
}
}
Sec. 9.10.5. (Identityref) Usage Example
- Add an example and use-case showing multiple base-stmts
for an identityref
Sec. 9.12. The union Built-In Type
- What happens to a union type when a feature is enabled or disabled,
and there are 1 or more bits or enumeration member types that are
affected, such that the member type changes? Is this just an
implementation detail?
Sec. 10.1.1. current()
- an example for current() should be given
Sec. 10.3.1. deref()
- why is this XPath function needed?
No use-cases are explained.
The example given shows that deref(.) saves some extra typing
from the previous line. Not very interesting new feature.
- what if the first node in the parameter node-set is another
leafref? Does this function keep de-referencing until it
gets to a plain leaf?
Sec. 10.4.1. derived-from()
Sec. 10.4.2. derived-from-or-self()
- example for derived from in wrong section
- no example given for derived-from-or-self()
- no explanation why 1 function would be used instead of the other
- these functions do not actually define what it means
to be "derived from". What does it mean in terms of base-stmt matching
like an identityref?
- What if there are multiple bases? These functions only support 1 base.
Sec. 11. Updating a Module
- seems all instances of 'may' need to be changed to 'MAY'
- last para:
In statements that have any data definition statements as
substatements, those data definition substatements MUST NOT be
reordered.
Why is this requirement in here? Seems like changing the
order of case-stmts within a choice-stmt does not matter.
Changing the order of augment-stmts does not matter.
Why does changing order of any data-def statement break an old client?
Sec 12. Coexistence with YANG version 1
- para 4: :"server MAY implement both..." contradicts this text in 5.6.4:
"A server MUST NOT implement more than one revision of a module."
- this section does not address what happens if protocol-accessible
objects have changed between the implemented YANG 1.0 module revision
and the implemented YANG 1.1 module revision. Since NETCONF and
RESTCONF do not specify YANG language version or YANG module revision
in edit payloads or retrieval filters, how does the server even
know if a client is asking for the YANG 1.0 vs. YANG 1.1 implementation
of a particular data node?
Sec. 18. Contributors
OLD:
- Andy Bierman (Brocade)
NEW:
- Andy Bierman (YumaWorks)
Nits:
p8: para 1: s/propsed/proposed/
p10: bullet 3: s/defintions/definitions/
p37, para 4: s/enconding/encoding/
p43, para 3: s/SHOULD not/SHOULD NOT/
_______________________________________________
netmod mailing list
netmod@ietf.org
https://www.ietf.org/mailman/listinfo/netmod
