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

[Yingzhen Qu <[e-mail]>]

Hi Dhananjaya,

Thanks for updating the draft and addressing my comments. I've read version
-05, the overall quality and readability of the document have been improved
substantially. All my major concerns have been answered, especially about
why type 2 NLRI is needed.

One nits I noticed is that on the front page, it says co-authors are listed
in section 11, which should be section 13.

Looking forward to your update presentation in the upcoming interim meeting.

Thanks,
Yingzhen





On Wed, Nov 15, 2023 at 1:03 PM Yingzhen Qu <[e-mail]> wrote:

> 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
>>
>>