Subject: [Last-Call] Tsvart last call review of draft-ietf-detnet-tsn-vpn-over-mpls-05

From: Joerg Ott via Datatracker <[email-masked]>
Date: Thu, 04 Feb 2021 11:21:52 -0800

Reviewer: Joerg Ott
Review result: Almost Ready

This document has been reviewed as part of the transport area review team's
ongoing effort to review key IETF documents. These comments were written
primarily for the transport area directors, but are copied to the document's
authors and WG to allow them to address any issues raised and also to the IETF
discussion list for information.

When done at the time of IETF Last Call, the authors should consider this
review as part of the last-call comments they receive. Please always CC
[email-masked] if you reply to or forward this review.

The draft describes interconnecting two TSN networks via a DetNet MPLS data plane
and specifies the interconnecting entities.

Note: I read through RFC8655, RFC8938, and draft-detnet-mpls prior to this review
but I may have missed some document or detail.

>From a transport area perspective, there are no specific issues in this draft.
Possible encapsulation and MTU size issues would be covered generically 
by the underlying services and not be specific to this document.

I have one question, though, concerning flooding, which may be an issue
and deserve clarification: p.9, para 2 states (and para 4 has similar text):

   "If there are no TSN
   Stream specific forwarding configurations the PE MUST flood the
   packet to other locally attached CE(s) and to the DetNet Service
   Proxy.  If the administrative policy on the PE does not allow
   flooding the PE MUST drop the packet."

In case of a misconfiguration, there does not appear to be a rate limiting
or termination criterion defined (maybe the next hop or some other 
entity would respond and a corresponding forwarding configuration
installed, e.g., similar to MAC address mapping in switches), so that it
seems flooding could persist as long as the traffic and other DetNet
services might be impacted. Does it make sense to add some further
explanation here (and for para 4).

Also curious: in 5.2 on page 10, is there no identification validation in
this direction?

The draft has a few general issues:

1. Terminology: Stream Identification vs. Stream ID vs. Stream-ID vs. Stream.
Maybe to a short introduction of the acronym to use, even though this is
almost obvious. 

2. The Requirements Language don't seem to be used in all places where it should be.
Examples: last para of section 4.1? Section 5.1, including 2nd para (has keywords)
vs. 4th para (doesn't use them) on page 9.

3. Does the last para of 5.2 (PREOF and FRER interworking) contradict the 2nd para
of 5.3? While being rightly out of scope: how would sequence number copying work
(as suggested in the last para of 5.2) in case there is an N:1 stream mapping?

4. Does it make sense to provide an example in sect. 6. after the bullet list on page 11?

Nits:
Won't list all since the RFC Editor will do a check on those, but it would be good
to use use consistent capitalisation (e.g., Edge Node vs Edge node, Domain) and
avoid extra plural s. Many articles are missing.
Term "required in 2nd para on p.9
Unclear references: 5.1, 3rd para: "it", "before forwarded"

---

From: Balázs Varga A <[email-masked]>
Date: Mon, 8 Feb 2021 20:45:49 +0000

Hi Joerg,
Many thanks for your review. Please see my reaction inline.
Thanks
Bala'zs

-----Original Message-----
From: Joerg Ott via Datatracker <[email-masked]> 
Sent: Thursday, February 4, 2021 8:22 PM
To: [email-masked]
Cc: [email-masked]; [email-masked]; [email-masked]
Subject: Tsvart last call review of draft-ietf-detnet-tsn-vpn-over-mpls-05

Reviewer: Joerg Ott

This document has been reviewed as part of the transport area review team's ongoing effort to review key IETF documents. These comments were written primarily for the transport area directors, but are copied to the document's authors and WG to allow them to address any issues raised and also to the IETF discussion list for information.

When done at the time of IETF Last Call, the authors should consider this review as part of the last-call comments they receive. Please always CC [email-masked] if you reply to or forward this review.

The draft describes interconnecting two TSN networks via a DetNet MPLS data plane and specifies the interconnecting entities.

Note: I read through RFC8655, RFC8938, and draft-detnet-mpls prior to this review but I may have missed some document or detail.

>From a transport area perspective, there are no specific issues in this draft.
Possible encapsulation and MTU size issues would be covered generically by the underlying services and not be specific to this document.

I have one question, though, concerning flooding, which may be an issue and deserve clarification: p.9, para 2 states (and para 4 has similar text):

   "If there are no TSN

In case of a misconfiguration, there does not appear to be a rate limiting or termination criterion defined (maybe the next hop or some other entity would respond and a corresponding forwarding configuration installed, e.g., similar to MAC address mapping in switches), so that it seems flooding could persist as long as the traffic and other DetNet services might be impacted. Does it make sense to add some further explanation here (and for para 4).

<Balazs> Flooding is a standard behavior of L2 forwarding and IETF's L2VPN service instances. In case of TSN rate limiting and policing of TSN Streams is a mandatory functionality (they are defined in IEEE 802.1 TSN TG, for example in IEEE 802.1Qci (it is now part of 802.1Q-2018)). Therefore the next hop (i.e., the CE) will protect the connected network from the negative impact of a misconfigured PE node. DetNet (and TSN) allocates resources per flow and a flow is restricted to use only its resources.

Also curious: in 5.2 on page 10, is there no identification validation in this direction?

<Balazs> Right, the TSN procedures do the TSN Stream identification as per 802.1CB. The DetNet Service Proxy "only" pairs the already identified TSN Streams and DetNet flows.

1. Terminology: Stream Identification vs. Stream ID vs. Stream-ID vs. Stream.
Maybe to a short introduction of the acronym to use, even though this is almost obvious. 

<Balazs> Right, these are IEEE 802.1 specific terms (also referred in RFC8655). I will add text to the terminology section referring to IEEE 802.1 related terms.
BTW, Stream-ID is a typo, I will fix it.

2. The Requirements Language don't seem to be used in all places where it should be.
Examples: last para of section 4.1? Section 5.1, including 2nd para (has keywords) vs. 4th para (doesn't use them) on page 9.

<Balazs> Right. Section 4.1 is general description. The intention was to have all the details on requirements in chapter 5. 
5.1 is a section dealing with TSN specific procedures defined by IEEE 802.1 TSN TG.

3. Does the last para of 5.2 (PREOF and FRER interworking) contradict the 2nd para of 5.3? While being rightly out of scope: how would sequence number copying work (as suggested in the last para of 5.2) in case there is an N:1 stream mapping?

<Balazs> No, they are not contradicting. TSN specific sequence numbers are encoded in the Ethernet header and forwarded unchanged within the DetNet PW payload. Details for PREOF/FRER interworking are out-of-scope. Regarding the second question, yes You are right. In case of aggregation sequence number copying is not an option in all scenarios, but can work in some special scenes. 

<Balazs> I think that would be a good example for the YANG model. Let's contact the authors.

Nits:
Won't list all since the RFC Editor will do a check on those, but it would be good to use use consistent capitalisation (e.g., Edge Node vs Edge node, Domain) and avoid extra plural s. Many articles are missing.
Term "required in 2nd para on p.9
Unclear references: 5.1, 3rd para: "it", "before forwarded"

<Balazs> Thanks, I will do a check with the co-authors.