Re: [Idr] Opsdir early review of draft-ietf-idr-bgp-car-02

[Yingzhen Qu <[e-mail]>]

HI Dhananjaya,

Ack. Thanks for addressing my comments. I'll review the updated draft and
get back to you.

Thanks,
Yingzhen

On Wed, Nov 15, 2023 at 2:35 AM Dhananjaya Rao (dhrao) <[e-mail]>
wrote:

> Hi Yingzhen,
>
>
>
> Thank you once again for your thoughtful review.
>
>
>
> We’ve made a couple of updates to the draft. I’m hoping they address most
> of the comments and feedback you had provided.
>
>
>
> I’ve also included some clarifications inline (please check for DR#).
>
>
>
> *From: *Yingzhen Qu via Datatracker <[e-mail]>
> *Date: *Wednesday, August 23, 2023 at 1:18 AM
> *To: *[e-mail] <[e-mail]>
> *Cc: *[e-mail] <
> [e-mail]>, [e-mail] <[e-mail]>
> *Subject: *Opsdir early review of draft-ietf-idr-bgp-car-02
>
> Reviewer: Yingzhen Qu
> Review result: Has Issues
>
> Hi,
>
> Thanks for the draft.
>
> I am the assigned OPSDIR reviewer to conduct an "early" review of this
> draft.
>
> General comments:
>
> There are lots of abbreviations in the draft. I'd suggest to add them
> in the the terminology section. For example, I'd assume BR means Border
> Router, but there might be different guessing.
>
> DR# Ack, we have expanded on abbreviations.
>
>
> In this draft, it says E is globally unique, which makes E-C in that
> order unique. Can you please explain a bit more about the second unique?
> I suppose it's possible to have two different source nodes, E1 and E2,
> all reach destination E with color C, correct?
>
> DR# Uniqueness is for the destination (E)’s BGP route. The data model of
> E,C keeps the route NLRI unique end to end even though color in NLRI may
> map to another intent in a different color (administrative) domain during
> propagation, since E is globally unique in provider network and scopes the
> color. Section 12 discusses the operational/manageability considerations.
>
> DR# I assume by source you mean an ingress node. Both source nodes above
> E1 and E2 will receive the (E,C) route without any change in the NLRI, and
> can use it to send traffic to the destination node E.
>
>
> The draft has an informative reference to
> [I-D.hr-spring-intentaware-routing-using-color], which is an important
> problem
> statement for this solution. Will the problem statement draft progress as
> well?
> Even so, to improve the readability of the bgp-car draft, I'd suggest
> adding
> some text for a brief introduction of the problem.
>
> DR# Yes, the problem statement is awaiting adoption call in spring.
>
> DR# We have also expanded the Introduction section.
>
>
> IP Prefix NLRI was added in version -02. The use case is where a unique
> routable IP prefix is assigned a given intent or color. In other words,
> the IP is overloaded with a color. The same can be achieved using an IP
> with a color. I'm not totally convinced that this type 2 NLRI is needed.
> Please clearly specify when it should be used.
>
> DR# We have tried to clarify the usage of Type-2 for different cases in
> the updated versions, specifically in Section 8.
>
> DR# The expanded intro section also describes the use-case/motivation a
> bit more.
>
>
> Please consider adding a section for operation considerations. There are
> pieces of information about operation and deployment scattered in the
> document, please consider group them together.
>
> DR# We did have a Manageability Considerations section 12, renamed it.
> SRv6 specific operational considerations are in Section 8.3, since the SRv6
> specific items are in Section 8.
>
>
> There are quite some sentences missing "." at the end. Please do an
> editorial pass and fix them.
>
> DR# Done, thanks.
>
>
> Detailed comments with line # from idnits:
>
> 478        The value set (or appropriately incremented) in the AIGP TLV
> 479        corresponds to the metric associated with the underlying intent
> of
> 480        the color.  For example, when the color is associated with a
> low-
> 481        latency path, the metric value is set based on the delay metric.
>
> 483        Information regarding the metric type used by the underlying
> intra-
> 484        domain mechanism can also be set.
>
> comment: This statement lacks a clear definition how the metric should be
> set.
>
> DR# Updated to indicate it’s the metric value which is set based on the
> path/metric type.
>
>
> 486        If BGP CAR routes traverse across a discontinuity in the
> transport
> 487        path for a given intent, add a penalty in accumulated IGP metric
> 488        (value by user policy).  For instance, when color C1 path is not
> 489        available, and route resolves via color C2 path (e.g., Appendix
> A.3).
>
> How about the case where encapsulations are different? For example, SR
> policy in one AS and IGP-FlexAlgo in the other AS vs. SR Policy in both
> ASes.
>
> DR# metric is independent of encapsulation type. For example delay metric
> can be provided by IGP FA or SR policy.
>
> DR# That said, the metric value/penalty can be updated differently by user
> depending on the path type.
> Section 2.7
> 504        The (E, C) route inherently provides availability of redundant
> paths
> 505        at every hop, identical to BGP-LU or BGP IP.
>
> "every hop" is a bit confusing here since it may mean an IGP hop within
> an AS. To my understanding, this section means ECMP or backup paths can
> provide protection in case of failure within an AS domain without impact
> other ASes.
>
> DR# Clarified to indicate it’s every BGP hop. And yes.
>
>
> "Path Availability" as the section title is not very clear. How about
> something
> like "Inherent Path Protection"?
>
> DR# Made it “Native Multipath Capability”, as path protection could be
> misinterpreted.
>
>
> 513        BGP ADD-PATH should be enabled for BGP CAR to signal multiple
> next
> 514        hops through a transport RR.
>
> I'd suggest to change to "SHOULD be enabled".
>
> DR# Done
> 526        The BGP CAR solution seamlessly supports this (rare) scenario
> while
>
> I'd suggest adding a small paragraph explaining why this is a rare but
> useful case. I would guess the tow domains used to belong to different
> administrators, now they're trying to merge under one admin domain.
> nits: personally I don't like how "(rare)" with parentheses is used
> here, but I'd leave this to the authors.
>
> DR#. Section 10 (manageability) describes some considerations. The
> use-case/requirement is described in the problem statement draft.
>
> DR# Removed parenthesis.
>
>
> 806        NLRI instead of the BGP Prefix SID attribute.  The BGP Prefix
> SID
> 807        Attribute SHOULD be omitted from the labeled color-aware routes
> when
> 808        the attribute is being used to only convey the Label Index TLV.
> Add a reference to Appendix D?
> DR# Done.
>
>
> 848        BGP CAR SRv6 SID TLV definitions provide the following benefits:
>
> 850        *  Native encoding of SIDs avoids robustness issue caused by
> 851           overloading of MPLS label fields.
>
> 853        *  Simple encoding to signal Unique SIDs (non-transposition),
> 854           maintaining BGP update prefix packing
>
> 856        *  Highly efficient transposition scheme (12-14 bytes saved per
> 857           NLRI), also maintaining BGP update prefix packing
> minor: I don't think the text belongs to the encoding section. Maybe
> part of "Operation Considerations"?
>
> DR# This text describes the reasoning for the CAR encoding design, and was
> added based on comments during adoption. Hence, we’ve kept it inline to
> provide context.
>
>
> 1019       *  If multiple instances of same type are encountered, all but
> the
> 1020          first instance MUST be ignored.
>
> 1022       *  If multiple instances of same type are encountered, all but
> the
> 1023          first instance MUST be ignored.
> nits: please remove the repetition.
>
> DR# Done, thanks.
>
>
> 1025       *  A TLV is not considered malformed because of failing any
> semantic
> 1026          validation of its Value field.
> Q: When should a TLV be considered malformed? and how should it be handled?
>
> DR# TLV validation and its handling is already specified above in this
> section for type-specific Non-key TLVs.
>
>
> 1033    3.  Service route Automated Steering on Color-Aware path
> nits: Service Route Automated Steering on Color-Aware Path
> Please check to make sure all section titles are consistent.
>
> DR# Ack, done.
>
> 1044       destination, per-flow, CO-only.  For brevity, in this revision,
> we
> 1045       refer the reader to the [RFC9256] text.
> nits: maybe change to something like "For brevity, please refer to
> [RFC9256]
> section X for detail."?
>
> DR# Ack, done.
>
>
> 1047       Salient property: Seamless integration of BGP CAR and SR Policy.
> minor: personally I don't think this sentence belong to this section.
>
> DR# Removed.
>
>
> 1055    4.  Intents
> The section title and content don't seem to match. I don't quite understand
> the purpose of this section.
>
> DR# It’s meant to include illustrations for various intent use-cases.
> We’ve removed this section as some cases are already included in Appendix.
>
>
> 1085       A separate document will analyze the BGP CAR supports for 3, 5
> and 6.
> Any reference?
>
> DR# Not yet.
>
>
> 1097    5.1.  (E, C) Subscription and Filtering
> Q: how is this subscription sent between routers?
>
> DR# It would be via a BGP route, but the specification/details are out of
> scope for this document.
>
> 1115       *  If A does not have (E2, C1), it will advertise F (E2, C1) to
> its
> 1116          peer B
> I suppose it meant to be "If A does not have subscription of (E2, C1)"
>
> DR# It’s if A doesn’t have the route.
> 1124       On-demand filtering procedures are outside the scope of this
> 1125       document.
> what's "on-demand filtering"?
>
> DR# It’s on-demand subscription and filtering – fixed.
>
>
> 1138       Two key principles used to address the scaling requirements are
> a
> 1139       hierarchical network and routing design, and on-demand route
> 1140       subscription and filtering.
> Q: on-demand filtering is claimed to be out of the scope. (line #1124)
>
>
> DR# The hierarchical design is the primary technique which is described.
> The subscription is optional for control plane optimization. it’s included
> for completeness of the framework.
>
>
> 1342       Note: E1 does not need the BGP CAR (451, C1) route
> Q: what's the benefit?
>
> DR# The benefit is explained in Section 6.3 last bullet where it’s the
> case of “E1 needs BGP CAR (451, C1).”
>
> “the ingress PE needs an additional BGP transport level recursion and
> pushes a BGP VPN label and two BGP transport labels.  It may also need to
> handle load-balancing for the egress ABRs.  This is the most complex
> dataplane option for the ingress PE.”
>
>
> 1545    7.  Routing Convergence
> comments: Maybe section 2.7 and 7 should be put together somehow, but I'll
> leave this to the authors.
>
> DR# We’ve kept it as is for now to avoid reorganizing the section.
>
>
> 1602
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 1603       |  NLRI Length  |  Key Length   |   NLRI Type   |Prefix Length
> |
> 1604
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> Q: what's value of the NLRI Type here?
>
> DR# Same as for CAR SAFI.
>
>
> 1664       service route is advertised by the egress PE with a Color
> Ext-Comm C.
> nits: "Color Ext-Comm" is only used here once while "Color
> Extended-Community"
> elsewhere.
>
> DR# Done.
>
>
> Nits: this section 9.2.1 and 9.2.2 needs some editorial work. For
> example, each bullet point should be a sentence finished with a ".".
> DR# Done.
>
>
> 1796       CAR SAFI may also be used for best-effort routes in addition to
> 1797       intent-aware routes.
> Q: Should a color be specified here? or use the IP Prefix NLRI Type 2?
>
> DR# Its Type-2 without color. Clarified in Section 8.
>
>
> 2008       This extension defines a new SAFI within a BGP and therefore
> does not
> it should be two new SAFIs: BGP CAR/83 and BGP VPN CAR/84
>
> 2307       The examples use MPLS/SR for the transport data plane.
> Scenarios
> 2308       specific to other encapsulations will be added in subsequent
> 2309       versions.
> nits: this should be removed.
>
> DR# Ack, fixed.
>
> Regards,
>
> -Dhananjaya
>
>
> Thanks,
> Yingzhen
>
>