Hey Alvaro,
Thanks for the review - we will address your comments in the -22 version. See
inline.
On 6/5/19, 6:26 PM, "Alvaro Retana" <aretana.i...@gmail.com> wrote:
Dear authors:
I just finished reading this document. I have some comments in-line,
including a couple that I consider major -- I'll wait for whose to be
addressed before moving forward.
The datatracker reports 4 errors in the model, mostly related to
ietf-bfd-types. Please take a look.
Thanks!
Alvaro.
[Line numbers from id-nits.]
...
81 1. Overview
...
92 This document defines a YANG data model that can be used to configure
93 and manage OSPF and it is an augmentation to the core routing data
94 model. If fully conforms to the Network Management Datastore
95 Architecture (NDMA) [RFC8342]. A core routing data model is defined
96 in [RFC8349], and it provides the basis for the development of data
97 models for routing protocols. The interface data model is defined in
98 [RFC8343] and is used for referencing interfaces from the routing
99 protocol. The key-chain data model used for OSPF authentication is
100 defined in [RFC8177] and provides both a reference to configured key-
101 chains and an enumeration of cryptographic algorithms.
[nit] s/If fully conforms/It fully conforms
Fixed.
103 Both OSPFv2 [RFC2328] and OSPFv3 [RFC5340] are supported. In
104 addition to the core OSPF protocol, features described in other OSPF
105 RFCs are also supported. These includes demand circuit [RFC1793],
106 traffic engineering [RFC3630], multiple address family [RFC5838],
107 graceful restart [RFC3623] [RFC5187], NSSA [RFC3101], and OSPF(v3) as
108 a PE-CE Protocol [RFC4577], [RFC6565]. These non-core features are
109 optional in the OSPF data model.
[nit] s/OSPF(v3) as a PE-CE Protocol/use as a PE-CE Protocol Using
"OSPF(v3)" is confusing because it is not clear whether OSPFv3-only is
meant or both, but v2 is not mentioned...
Sure "OSPFv2 or OSPFv3"
...
138 2.1. OSPF Operational State
140 The OSPF operational state is included in the same tree as OSPF
141 configuration consistent with Network Management Datastore
142 Architecture [RFC8342]. Consequently, only the routing container in
143 the ietf-routing model [RFC8349] is augmented. The routing-state
144 container is not augmented.
[nit] s/consistent with Network/consistent with the Network
Fixed
146 2.2. Overview
...
154 module: ietf-ospf
155 augment /rt:routing/rt:control-plane-protocols/
156 rt:control-plane-protocol:
157 +--rw ospf
158 .
159 .
160 +--rw operation-mode? identityref
161 +--rw af? identityref
162 .
163 .
164 +--rw areas
165 | +--rw area* [area-id]
166 | +--rw area-id area-id-type
167 | .
168 | .
169 | +--rw virtual-links
170 | | +--rw virtual-link* [transit-area-id router-id]
171 | | .
172 | | .
173 | +--rw sham-links {pe-ce-protocol}?
174 | | +--rw sham-link* [local-id remote-id]
175 | | .
176 | | .
177 | +--rw interfaces
178 | +--rw interface* [name]
179 | .
180 | .
181 +--rw topologies {multi-topology}?
182 +--rw topology* [name]
183 .
184 .
186 The ospf module is intended to match to the vendor specific OSPF
187 configuration construct that is identified by the local identifier
188 'name'. The field 'version' allows support for OSPFv2 and OSPFv3.
[minor] The 'version' field doesn't appear in the tree above. §2.3 says
the same thing with more details. Perhaps take the sentence out of this
paragraph.
Yeah - I removed the sentence. I was thinking of adding it to the tree but it
is not there is the full tree.
190 The ospf container includes one OSPF protocol engine instance. The
191 instance includes OSPF router level configuration and operational
192 state.
[??] One of the things that is confusing me is the instance concept in the
model. I understand instances in OSPF, and I see how the module is
organized around instances ("grouping instance-..."), including even
per-instance Router-ID. But I don't see anywhere the definition of an
instance-id (except attached to an interface (§2.7) and specific to OSPFv3
(§3))...if wanting to configure (or read from) a specific instance, how
does the client differentiate? [YANG is not my thing, so this may be
obvious to everyone else. If it is, a quick pointer would be very
appreciated.]
I have removed all instances of "engine" from the text and model. Also, I've
just added:
The ietf-ospf model defines a single instance of OSPF which may be instantiated
as an OSPFv2 or OSPFv3 instance. Multiple instances are instantiated as
multiple control-plane protocols instances.
At one time, the model did include multiple instances. However, this was the
manifestation of a poor implementation choice for a particular implementation.
...
209 2.4. Optional Features
...
221 3. explicit-router-id: Support explicit per-instance Router-ID
222 specification.
[minor] Is there a reference? Should the document point to rfc5340/rfc6549
here?
No - explicit protocol instance router-id configuration is not standardized.
Apparently, there are implementations that don't allow protocol instance
configuration but rely on global router-id explicit configuration. We got input
from implementations beyond the affiliations of the authors.
...
234 8. ttl-security: Support OSPF Time to Live (TTL) security check
235 suppression [RFC5082].
[minor] s/suppression/support Right?
Right - fixed.
237 9. nsr: Support OSPF Non-Stop Routing (NSR).
[minor] Reference?
No IETF reference for OSPF NSR. However, I have a few patents in this area -
should the model reference these? __
...
242 11. admin-control: Support Administrative control of the protocol
243 state.
[minor] This seems like a "basic" feature: enabling/disabling the
protocol. Why was it decided to make it optional? Are there
implementations that don't support enabling/disabling?
I'm happy to remove it.
...
261 17. ospfv2-authentication-trailer: Support OSPFv2 Authentication
262 trailer as specified in [RFC5709] or [RFC7166].
[minor] s/RFC7166/RFC7474
Yup. Fixed.
...
267 19. ospfv3-authentication-trailer: Support OSPFv3 Authentication
268 trailer as specified in [RFC7474].
[minor] s/RFC7474/RFC7166
How could I missed this... Fixed
...
300 2.5. OSPF Router Configuration/Operational State
...
308 module: ietf-ospf
...
336 +--rw enable? boolean {admin-control}?
[nit] Perhaps the admin-control should be moved closer to the start of this
branch so it is clear what is being enabled/disabled.
Ok
...
464 2.6. OSPF Area Configuration/Operational State
...
570 | | +--rw hello-interval? uint16
571 | | +--rw dead-interval? uint32
572 | | +--rw retransmit-interval? uint16
[minor] Why aren't rt-types:timer-value-* used for the *-timer/interval
definitions? I would have thought this was exactly the reason for the
definitions in rfc8294.
I'd like to use these types universally. However, YANG seems to have a
restriction that I can't further restrict the range. This really sucks. Will do
what I can and make the best trade-off.
...
1054 2.9. OSPF RPC Operations
...
1061 o clear-neighbor: restart a particular set of OSPF neighbor.
[minor] Should the description be something like: "reset a set of OSPF
neighbors on a particular interface"?
Yes. Fixed.
1063 rpcs:
1064 +---x clear-neighbor
1065 | +---w input
1066 | +---w routing-protocol-name
1067 | + -> /rt:routing/control-plane-protocols/
1068 | + control-plane-protocol/name
1069 | +---w interface? if:interface-ref
...
1076 3. OSPF YANG Module
...
1143 Editor: Derek Yeung
1144 <mailto:de...@arrcus.com>
1145 Author: Acee Lindem
1146 <mailto:a...@cisco.com>
1147 Author: Yingzhen Qu
1148 <mailto:yingzhen...@huawei.com>
1149 Author: Jeffrey Zhang
1150 <mailto:zzh...@juniper.net>
1151 Author: Ing-Wher Chen
1152 <mailto:ingwherc...@mitre.org>
1153 Author: Dean Bogdanovic
1154 <mailto:ivand...@gmail.com>
1155 Author: Kiran Agrahara Sreenivasa
1156 <mailto:k...@employees.org";;
[minor] This list is not the same as what is in the front page of this
document.
Sure. Fixed.
...
1685 typedef if-state-type {
1686 type enumeration {
...
1712 enum backup {
1713 value "6";
1714 description
1715 "Interface Backup Designated Router (BDR) state.";
1716 }
[nit] s/backup/bdr To match the other instances where a BDR is referenced
Good catch. Fixed.
Thanks,
Acee
_______________________________________________
Lsr mailing list
Lsr@ietf.org
https://www.ietf.org/mailman/listinfo/lsr
