A YANG Network Data Model for Layer 2 VPNs
draft-ietf-opsawg-l2nm-19

# ART AD Review of draft-ietf-opsawg-l2nm-17

cc @fpalombini

Thank you for the work on this document.

I have a couple of comments, hopefully easy to fix.

I have not finished reviewing the examples in appendix, I might update this ballot if I have additional comments.

Francesca

## Comments

### boolean for enabled/disabled

Section 8.4:
```
             "Controls whether loss measurement is enabled/disabled.";
```
```
                               "Controls whether ingress replication is
                                enabled/disabled.";
```
```
                               "Controles whether P2MP replication is
                                enabled/disabled.";
```
Suggest rephrasing for clarity as other boolean fields: "Controls whether X is enabled ('true') or disabled ('false').";

### needs a language tag

Section 8.4:
```
                   leaf description {
                     type string;
                     description
                       "A textual description of the VPN network
                        access.";
```
```
               leaf description {
                 type string;
                 description
                   "Textual description of a VPN node.";
               }
```
As these fields contain human-readable text, I believe it might need a language tag, or specify why a tag is not needed, as specified by RFC 5646. I think that such a tag is not necessary for pw-description and vpn-description as, if I read them correctly, that is covered by the docs where those are initially defined (for example, for pw-description, this is covered by the last paragraph of section 5.5 of RFC 4447). Do let me know if I missed these vpn-network-access description and vpn-node description, and their language are also described here or inherited from another doc.

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues.

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments

Thank you to Chris Lonvick for the SECDIR review.

** Section 7.3. vpn-services tracks two different last-change timestamps.  Would it be useful to also track a “created-on” timestamp?

Thanks for addressing my discuss comments. I think I got what I wanted to achieve with the Discuss.. that is to see whether those references are conscious choicee or oversights. Hence, I am clearing  by discuss.

I, however, think it will be great if the authors ( along with AD) can go through the document again and make sure references are correctly categorized.

# Éric Vyncke, INT AD, review of # Éric Vyncke, INT AD, review of draft-ietf-opsawg-l2nm-18
CC @evyncke

Thank you for the work put into this document. It solves a common and important issue while keeping backward compatibility.

Please find below some non-blocking COMMENT points (but replies would be appreciated even if only for my own education), and some nits.

Special thanks to Adrian Farrel for the shepherd's write-up including the WG consensus and the intended status. 

The use of IANA-maintained YANG modules looks attractive to me.

I hope that this helps to improve the document,

Regards,

-éric

## COMMENTS

### Set of L2VPN technologies

Just wondering how extensible this model is ? I.e., the L2 cross-connect of RFC 8986 is not included, any reason why ? How easy would it be to extend this model to also include RFC 8986 ?

### Section 2
```
      The corresponding YANG module can be used by
      a service orchestrator to request a VPN service to a network
      controller or to expose the list of active L2VPN services.
```
Does this mean that state information (e.g., counters) are not included ? Actually, sections 7.3 & 7.6.3 mention some status & OAM support so suggest adding status & OAM to the above text.

### Section 6

While I understand that "ethernet" is used in a broad concept (i.e., also covering Wi-Fi), I find the use of 'ethernet' a little restrictive as layer-2 VPN could exist in a near future with technologies that are not IEEE 802.3 based (e.g., some IoT networks or the good old frame relay). Alas, probably too late to change anything.

### Section 7.4

```
  'svc-mtu':  Is the service MTU for an L2VPN service (i.e., Layer 2
      MTU including L2 frame header/tail).  It is also known as the
      maximum transmission unit or maximum frame size.
```
Does it include CRC and/or preamble ? It would be nice also to mention the unit of this metric.

Same question in the 'mtu' in section 7.6.4.

### Section 8.4

Missing "units" in "svc-mtu'.

Is 300 msec a valid default aging timer for a MAC address ? This seems really short.

### Sections A.2 & A.3

Thanks for providing an IPv6 example ;-)

## NITS  

### MAC is uppercase

I noticed at least one occurence of 'mac' in lower case.

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. 

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments

# GEN AD review of draft-ietf-opsawg-l2nm-17

CC @larseggert

Thanks to Dale Worley for the General Area Review Team (Gen-ART) review
(https://mailarchive.ietf.org/arch/msg/gen-art/iTZYyhVQ7nygCX_KIedl1RhPBE4).

## Nits

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.

### Outdated references

Reference `[RFC5143]` to `RFC5143`, which was obsoleted by `RFC4842` (this may
be on purpose).

### Grammar/style

#### Section 4, paragraph 9
```
ider does not assume, nor does it precludes exposing the VPN service via the 
                                  ^^^^^^^^^
```
Did you mean "preclude"? As "do" is already inflected, the verb cannot also be
inflected.

#### Section 8.4, paragraph 54
```
 MAC address' situation has occurred and the duplicate MAC address has been 
                                    ^^^^
```
Use a comma before "and" if it connects two independent clauses (unless they
are closely connected and short).

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. Review generated by the [`ietf-reviewtool`][IRT].

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments
[IRT]: https://github.com/larseggert/ietf-reviewtool