Hi Samuel,
Thanks for your detailed review and comments! Sorry for the delay due to the
holiday.LoL. Happy Chinese New Year!
Please see inline with [Quan].
Abstract:
“…Label Indicator (ELI)/EL pairs SHOULD be inserted in the SR-MPLS label stack
as per RFC8662…”.
Is it good to start using requirements in draft abstract like this (I consider
it a bit non-standard looking into abstracts used in other RFCs)? I would use
more generic wording to just say that RFC8662 is already explaining how Els how
are supposed to be used in SR-MPLS label stack. Requirements language
(including “SHOULD” is really defined in Section 2.2 only).
[Quan]: Thanks for your remind. I checked the RFC8662 section 7.1, it mentioned
"In order for the EL to occur within the ERLD of LSRs along the path
corresponding to a SPRING label stack, multiple <ELI, EL> pairs MAY be inserted
in this label stack." Would it be better to change it to "multiple Entropy
Label Indicator (ELI)/EL pairs may be inserted in the SR-MPLS label stack.
stack as per RFC8662." without using Requirements language?
Introduction:
“[RFC5440] describes the Path Computation Element Computation Protocol (PCEP)
which is …“
Do you need to expand “PCEP” acronym again even if you expanded it already in
draft title and abstract? Same potentially applies to EL/ELI/ELP
[Quan]: I think this is required when processing RFC. (And I also checked the
existing RFC.)
“In some cases, the the controller(e.g. PCE)”
Double “the” and maybe add space after “controller”.
[Quan]:Thanks, I will fix it in next version.
Section 4.2:
“A PCE would also set this bit to 1 to indicate that the ELP information is
included by PCE and encoded in the Path Computation Reply (PCRep) message as
per [RFC5440]. And in a stateful PCE model, it also can be carried in Path
Computation Update Request (PCUpd) message as per [RFC8231] or LSP Initiate
Request (PCInitiate) message as per [RFC8281].”
It seems that for stateless it is “MUST” requirement and for stateful, it is
“CAN” requirement. Isn’t it better to explicitly indicate for both whether PCE
MUST do it or it is just optional and PCE may decide to do not set that flag at
all?
[Quan]: Thanks for your comment. Would it be better to add that " The PCE
SHOULD set this bit to 1 ... And in a stateful PCE model, it also CAN be
carried in .... "
Section 4.3:
“E (ELP Configuration) : If this flag is set, the PCC SHOULD insert <ELI, EL>
into the position after this SR-ERO subobject, otherwise it SHOULD not insert
<ELI, EL> after this segment.”
You are indicating what PCC should do if flag is set (so I assume that PCE
should set it, but it may be better to be explicit and say if “If this flag is
set by PCE,…”). I can see that you are explaining briefly in next section, but
it is still better to be clear. Maybe explain also whether PCC can set it by
itself (e.g. if it wants to report state of existing LSP).
[Quan]: Thanks for your comment. I think the ELP can only be computed by PCE,
so the flag can only be set by PCE.
Section 5:
“And the SR path can also be initiated by PCE with PCInitiate or PCUpd message
in stateful PCE mode.”
It seems a bit misleading to say that SR path can be initiated by PCUpd.
“The SR path being received by PCC with SR-ERO segment list…”
Maybe “…received by PCC encoded in SR-ERO…” (without segment-list as you are
already talking about SR path)
[Quan]:Thanks, I will fix it in next version.
You defined 3 new flags, but what I’m missing a bit is any details about
interaction between those flags. E.g. what will happen if capability was not
advertised by E flag in LSP object is set? Is that allowed? Is it valid to
include E flag in SR-ERO if it was not requested by PCC? If any of those will
happen, should we have some PCError to reject them?
Is E flag from SR-ERO applicable to any SID type or are there cases, when it is
not valid to include it (e.g. L2 Adj SID?) Should PCC just ignore that flag in
such case?
Is E flag from SR-ERO applicable to RRO as well?
(I’m not pushing you to solve everything before adoption, just throwing ideas
for improving 😊 )
[Quan]:Yes, thanks for your great point. So far we only consider the normal
use of the E bit and will consider other abnomal case in the future.
Section 7:
You should explicitly mention registry name from which you want to allocate
Consider if you need to add “Manageability Considerations” Section.
[Quan]: Thanks, will consider to add in next version.
Best Regards,
Quan
Original
From: SamuelSidor(ssidor) <ssi...@cisco.com>
To: 熊泉00091065;
Cc: pce-chairs <pce-cha...@ietf.org>;Dhruv Dhody
<d...@dhruvdhody.com>;draft-peng-pce-entropy-label-posit...@ietf.org
<draft-peng-pce-entropy-label-posit...@ietf.org>;pce@ietf.org <pce@ietf.org>;
Date: 2024年02月07日 18:38
Subject: RE: [Pce] WG Adoption of draft-peng-pce-entropy-label-position-10
Hi Quan and PCE WG,
I support adoption of this draft with a few minor/not blocking comments (I
reviewed v11 as a lot of comments were addressed, so to avoid commenting same
thing again even if v10 is being adoption).
Abstract:
“…Label Indicator (ELI)/EL pairs SHOULD be inserted in the SR-MPLS label stack
as per RFC8662…”.
Is it good to start using requirements in draft abstract like this (I consider
it a bit non-standard looking into abstracts used in other RFCs)? I would use
more generic wording to just say that RFC8662 is already explaining how Els how
are supposed to be used in SR-MPLS label stack. Requirements language
(including “SHOULD” is really defined in Section 2.2 only).
Introduction:
“[RFC5440] describes the Path Computation Element Computation Protocol (PCEP)
which is …“
Do you need to expand “PCEP” acronym again even if you expanded it already in
draft title and abstract? Same potentially applies to EL/ELI/ELP
“In some cases, the the controller(e.g. PCE)”
Double “the” and maybe add space after “controller”.
Terminology
You are just pointing to terminology in other RFCs, but in the same time you
are introducing a lot of new acronyms and expanding them directly in the text.
For example in section 3:
“The Entropy Readable Label Depth (ERLD) is defined as the number of labels
which m…”
“… MSD (e.g. Base MPLS Imposition MSD (BMI-MSD) or ERLD-MSD) through Interior
Gateway Protocol (IGP) and…”
Why not use this section to explain them?
Section 3:
“The PCEs could get the information of all nodes such as … through Interior
Gateway Protocol (IGP)”
Isn’t it better to refer to BGP-LS as well? At least my understanding is that
usage of BGP-LS as a source for topology for PCE is more standard than directly
learning topology from IGP (but maybe that is me).
Section 4.1:
“…multiple ELI/EL pairs and and supports the results of SR path with ELP from
PCE”
Double “and”. I also assume that PCC really “supports the results of SR
path-computation” or it “supports SR path with ELP received from PCE”.
Section 4.2:
“A PCE would also set this bit to 1 to indicate that the ELP information is
included by PCE and encoded in the Path Computation Reply (PCRep) message as
per [RFC5440]. And in a stateful PCE model, it also can be carried in Path
Computation Update Request (PCUpd) message as per [RFC8231] or LSP Initiate
Request (PCInitiate) message as per [RFC8281].”
It seems that for stateless it is “MUST” requirement and for stateful, it is
“CAN” requirement. Isn’t it better to explicitly indicate for both whether PCE
MUST do it or it is just optional and PCE may decide to do not set that flag at
all?
Section 4.3:
“E (ELP Configuration) : If this flag is set, the PCC SHOULD insert <ELI, EL>
into the position after this SR-ERO subobject, otherwise it SHOULD not insert
<ELI, EL> after this segment.”
You are indicating what PCC should do if flag is set (so I assume that PCE
should set it, but it may be better to be explicit and say if “If this flag is
set by PCE,…”). I can see that you are explaining briefly in next section, but
it is still better to be clear. Maybe explain also whether PCC can set it by
itself (e.g. if it wants to report state of existing LSP).
Section 5:
“And the SR path can also be initiated by PCE with PCInitiate or PCUpd message
in stateful PCE mode.”
It seems a bit misleading to say that SR path can be initiated by PCUpd.
“The SR path being received by PCC with SR-ERO segment list…”
Maybe “…received by PCC encoded in SR-ERO…” (without segment-list as you are
already talking about SR path)
You defined 3 new flags, but what I’m missing a bit is any details about
interaction between those flags. E.g. what will happen if capability was not
advertised by E flag in LSP object is set? Is that allowed? Is it valid to
include E flag in SR-ERO if it was not requested by PCC? If any of those will
happen, should we have some PCError to reject them?
Is E flag from SR-ERO applicable to any SID type or are there cases, when it is
not valid to include it (e.g. L2 Adj SID?) Should PCC just ignore that flag in
such case?
Is E flag from SR-ERO applicable to RRO as well?
(I’m not pushing you to solve everything before adoption, just throwing ideas
for improving 😊 )
Section 7:
You should explicitly mention registry name from which you want to allocate
Consider if you need to add “Manageability Considerations” Section.
Thanks,
Samuel
From: Pce <pce-boun...@ietf.org> On Behalf Of Dhruv Dhody
Sent: Friday, January 26, 2024 5:49 PM
To: pce@ietf.org
Cc: pce-chairs <pce-cha...@ietf.org>;
draft-peng-pce-entropy-label-posit...@ietf.org
Subject: [Pce] WG Adoption of draft-peng-pce-entropy-label-position-10
Hi WG,
This email begins the WG adoption poll for
draft-peng-pce-entropy-label-position-10
https://datatracker.ietf.org/doc/draft-peng-pce-entropy-label-position/
Should this draft be adopted by the PCE WG? Please state your reasons - Why /
Why not? What needs to be fixed before or after adoption? Are you willing to
work on this draft? Review comments should be posted to the list.
Please respond by Monday 12th Feb 2024.
Please be more vocal during WG polls!
Thanks!
Dhruv & Julien
_______________________________________________
Pce mailing list
Pce@ietf.org
https://www.ietf.org/mailman/listinfo/pce
