From: Mahesh Jethanandani <mjethanandani@gmail.com>
Content-Type: text/plain;
	charset=utf-8
Content-Transfer-Encoding: quoted-printable
Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.120.0.1.15\))
Message-Id: <223E635F-124D-4F6A-9949-FF54A34710CF@gmail.com>
Date: Fri, 25 Oct 2024 22:10:01 -0700
To: draft-ietf-opsawg-ntw-attachment-circuit@ietf.org
Message-ID-Hash: 7O34TI26OSS4FDJRGAFUFQH5SZ4SFIHZ
CC: opsawg <opsawg@ietf.org>
Precedence: list
Subject: =?utf-8?q?=5BOPSAWG=5DAD_review_of_draft-ietf-opsawg-ntw-attachment-circuit?=
Archived-At: 
 <https://mailarchive.ietf.org/arch/msg/opsawg/DOZ_WtVI8BEN_6jPH7Js6y9OUSY>

Dear authors of draft-ietf-opsawg-ntw-attachment-circuit,

Here is the last of the four documents that form this cluster. And just =
like the other three, please respond to the COMMENTs provided here, =
while you decide how you want to handle the NITs.

Thanks again for a comprehensive document.=20

=
--------------------------------------------------------------------------=
-----
COMMENT
=
--------------------------------------------------------------------------=
-----

Paragraph 1
>            A Network YANG Data Model for Attachment Circuits
>               draft-ietf-opsawg-ntw-attachment-circuit-12

Just a note. This review covers -12 version of the document. I see there =
is a -13 verion that is out.

Section 1, paragraph 6
>                      .---.
>                      |CE6|
>                      '-+-'
>                     ac |         .---.             .---.
>                        |         |CE5+------+------+CE2|
>                  .-----+-----.   '---'      |      '---'
>                  |           |              |ac
>                  |           |              |
>                .-+-.       .-+-.          .-+-.
>              .-+sap+-------+sap+-.      .-+sap+-------------.
>              | '---'       '---' |      | '---'             |
>              |        PE1        |      |        PE2        |
>     .---.  .-+-.                 |      |                   |
>     |CE1+--+sap|                 |      |                   |
>     '---'ac'-+-'                 |      |                   |
>              '-------------------'      '-------------------'
>=20
>              .-------------------.      .-------------------.
>              |                   |      |                 .-+-.ac.---.
>              |         PE3       |      |        PE4      |sap+--+CE5|
>              |                   |      |                 '---'  '---'
>              |                   |      |                   |
>              |             .---. |      | .---. .---. .---. |
>              '-------------+sap+-'      '-+sap+-+sap+-+sap+-'
>                            '-+-'          '-+-' '-+-' '-+-'
>                              |ac            |     |ac   |ac
>                            .-+-.            |   .-+-.   |
>                            |CE3+-----ac-----'   |CE4+---'
>                            '---'                '---'
>=20
>                    Figure 1: Attachment Circuits Examples

It would be helpful if the cluster of these four documents would use a =
single diagram across the drafts, even as they define different parts of =
the diagram. Specifically, in draft-ietf-opsawg-ac-lxsm-lxnm-glue, the =
diagram was updated to differentiate between CE having two ACs, one with =
the same bearer id, and one with two separate bearer ids. That is not =
reflected in this document.

Section 1, paragraph 7
>    The YANG data model in this document conforms to the Network
>    Management Datastore Architecture (NMDA) defined in [RFC8342].

Please state explicitly that this is a YANG 1.1 mode, and providing a =
normative reference to RFC7950.

Section 4, paragraph 1
>    Figure 3 shows the positioning of the AC network model in the =
overall
>    service delivery process.  The 'ietf-ac-ntw' module is a network
>    model which augments the SAP with a comprehensive set of parameters
>    to reflect the attachment circuits that are in place in a network.
>    The model also maintains the mapping with the service references =
that
>    are used to expose these ACs to customers
>    [I-D.ietf-opsawg-teas-attachment-circuit].  Whether the same naming
>    conventions to reference an AC are used in the service and network
>    layers is deployment-specific.

Would it help to specify a little more context here? For example, "The =
model also maintains the mapping with the service references that are =
used to expose those ACs to customer using the 'ietf-ac-sv' module =
defined in [I-D.ietf-opsawg-teas-attachment-circuti].

Section 5, paragraph 1
>    The full tree diagram of the 'ietf-ac-ntw' module can be generated
>    using the "pyang" tool [PYANG].  That tree is not included here
>    because it is too long (Section 3.4 of [RFC8340]).  Instead, =
subtrees
>    are provided in the following subsections for the reader's
>    convenience.

Following the discussion on the netmod mailing list, please include a =
complete tree diagram in the Appendix.

Section 5.1, paragraph 3
>    A node can host one or more SAPs.  Per [RFC9408], a SAP is an
>    abstraction of the network reference point (the PE side of an AC, =
in
>    the context of this document) where network services can be =
delivered
>    and/or are delivered to customers.  Each SAP terminates one or
>    multiple ACs.  Each AC in turn may be terminated by one or more =
peer
>    SAPs ('peer-sap').  In order to expose such AC/SAP binding
>    information, the SAP model [RFC9408] is augmented with required AC-
>    related information.

The authors should follow the guidance in rfc8407-bis, Section 4.3.1 for =
naming the identifiers. For example, there is no need to prefix =
'ac-svc-ref', 'ac-profile', or 'ac-parent-ref' with the prefix 'ac', as =
the parent is named 'ac=E2=80=99.=20

Also, not clear on this statement. "where network services can be =
delivered and/or are delivered to customers". I think a clear =
distinction should be added to distinguish between 'sap' and 'peer-sap' =
as it applies to the network or customer side of the network. For =
example, from this modules perspective, and as stated above, a SAP is a =
network reference point on the Provider Edge (PE), whereas the peer-sap =
is remote endpoint of an AC, that is the Customer Edge (CE). It is not =
always clear in the document which end of the AC or SAP is being =
referred to.

Finally, the term 'peer SAP' or 'peer-sap' seems to have been introduced =
here for the first time. It has been defined in RFC9408, so maybe just a =
reference needs to be added. Can the terminology, and maybe one of the =
diagrams indicate where a 'peer-sap' can exist?

Section 5.4, paragraph 1
>    The 'l2-connection' container is used to manage the Layer 2
>    properties of an AC.  The Layer 2 connection tree structure is =
shown
>    in Figure 8.

Which side of the AC. PE or CE? If it is PE, since this is the network =
AC, who is the consumer of this information? If it is the CE, then =
should this be in this model or in the ietf-ac-svc model?

Section 6, paragraph 12
>          description
>            "A type for an absolute reference to an attachment =
circuit.";

Per yangdoctors comment, albeit it was on the typedef statement, but =
because they were both talking about references, this should be modified =
to say, "An absolute reference to an attachment circuit."

Section 6, paragraph 12
>          description
>            "A type for an absolute reference to an attachment =
circuit.";

Same comment as above, and all description statments that start with "A =
type for an absolute reference ...".

Section 7, paragraph 1
>    This section uses the template described in Section 3.7 of
>    [I-D.ietf-netmod-rfc8407bis].

Please consider using the latest text of the template based on the =
discussion on the mailing list.

Possible DOWNREF from this Standards Track doc to [IEEE802.1Qcp]. If so, =
the
IESG needs to approve it.

=
--------------------------------------------------------------------------=
-----
NIT
=
--------------------------------------------------------------------------=
-----

All comments below are about very minor potential issues that you may =
choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so =
there
will likely be some false positives. There is no need to let me know =
what you
did with these suggestions.

Paragraph 1
+ COMMENT:

Section 1, paragraph 4
>    The document leverages [RFC9182] and [RFC9291] by adopting an AC
>    provisioning structure that uses data nodes that are defined in =
these
>    RFCs.  Some refinements were introduced to cover, not only
>    conventional service provider networks, but also specifics of other
>    target deployments (cloud network, for example).

s/in these RFCs/in those RFCs/
s/to cover,/to cover/
s/(cloud network, for example)/e.g., cloud network/

Section 1, paragraph 4
>    The AC network model is designed as augmentations to both the =
'ietf-
>    network' model [RFC8345] and the Service Attachment Point (SAP) =
model
>    [RFC9408].  An attachment circuit can be bound to a single or
>    multiple SAPs.  Likewise, the model is designed to accommodate
>    deployments where a SAP can be bound to one or multiple ACs (e.g., =
a
>    parent AC and its child ACs).

s/augmentations to/augmentations of/

Section 5.3, paragraph 4
>    The exact definition of the specific provisioning profiles profiles
>    is local to each service provider.  The model only includes an
>    identifier for these profiles in order to ease identifying and
>    binding local policies when building an AC.  As shown in Figure 7,
>    the following identifiers can be included:

s/profiles profiles/profiles/

Document references draft-ietf-opsawg-teas-common-ac-11, but -12 is the =
latest
available revision.

Document references draft-ietf-opsawg-teas-attachment-circuit-13, but =
-17 is
the latest available revision.

Document references draft-ietf-netmod-rfc8407bis-14, but -20 is the =
latest
available revision.


Mahesh Jethanandani
mjethanandani@gmail.com