Hi Mike,

Thanks for the quick reply. I think my comments are almost addressed. Please 
see my reply inline.

Thanks,
Cheng


From: Mike Koldychev (mkoldych) <mkoldych=40cisco....@dmarc.ietf.org>
Sent: Tuesday, January 16, 2024 6:05 PM
To: Cheng Li <c...@huawei.com>; Dhruv Dhody <d...@dhruvdhody.com>; pce@ietf.org
Cc: pce-chairs <pce-cha...@ietf.org>; 
draft-ietf-pce-segment-routing-policy...@ietf.org; mkold...@proton.me
Subject: RE: [Pce] WGLC for draft-ietf-pce-segment-routing-policy-cp-12

Hi Cheng,

Thanks for your review! Comments inline with <MK></MK>.

Thanks,
Mike.


From: Cheng Li 
<c.l=40huawei....@dmarc.ietf.org<mailto:c.l=40huawei....@dmarc.ietf.org>>
Sent: Tuesday, January 16, 2024 11:09 AM
To: Dhruv Dhody <d...@dhruvdhody.com<mailto:d...@dhruvdhody.com>>; 
pce@ietf.org<mailto:pce@ietf.org>
Cc: pce-chairs <pce-cha...@ietf.org<mailto:pce-cha...@ietf.org>>; 
draft-ietf-pce-segment-routing-policy...@ietf.org<mailto:draft-ietf-pce-segment-routing-policy...@ietf.org>
Subject: RE: [Pce] WGLC for draft-ietf-pce-segment-routing-policy-cp-12

Hi WG,

I read the document and support the WGLC.
However, I also have some minor comments below.


1.
Abstract

A Segment Routing (SR) Policy 
[RFC9256<https://www.rfc-editor.org/info/rfc9256>] is a non-empty set of SR 
Candidate Paths, that share the same <headend, color, endpoint> tuple.
1.Nits: s/that/which.
<MK>
Sure.
</MK>

2.share the same <> tuple?
<MK>
Sorry, not clear what your comment is here? The text is referring to the 
3-tuple that identifies the SR Policy, from here: 
https://www.rfc-editor.org/rfc/rfc9256.html#name-identification-of-an-sr-pol
</MK>
<C>well, I see your point. But recommend to rephrase the text. But this is not 
a big deal, you can ignore it.

3.This document extends [RFC8664<https://www.rfc-editor.org/info/rfc8664>] to 
fully support the SR Policy construct.
fully? suggest to remove this world. The SR policy is developing, so it can not 
be fully supported now I guess.
<MK>
Sure, I will remove the “fully”.
</MK>


4.
Introduction

PCEP Extensions for Segment Routing 
[RFC8664<https://www.rfc-editor.org/info/rfc8664>] specifies extensions that 
allow PCEP to work with basic SR-TE 
paths.¶<https://datatracker.ietf.org/doc/html/draft-ietf-pce-segment-routing-policy-cp-12#section-1-1>
s/specifies/specify
<MK>
Sure, I can just use the RFC Number, instead of the full name.
</MK>
<C>it works to me.

PCEP Extensions for Establishing Relationships Between Sets of LSPs 
[RFC8697<https://www.rfc-editor.org/info/rfc8697>] introduces
s/introduces/introduce because of extensions?  or you only wanto to list the 
name of the RFC here? anyway, all are nits. Normally, we use RFCXXXX as the 
subject directly.
<MK>
Sure, I can just use the RFC Number, instead of the full name.
</MK>
<C>great

5. again. Suggest to delete 'fully' in the last paragraph of Introduction.
<MK>
Will do.
</MK>

6.
SR Policy Association. PCEP ASSOCATION that describes the SR Policy. Can refer 
to the PCEP object or to the group of LSPs that belong to the Association. This 
should be clear from the 
context.¶<https://datatracker.ietf.org/doc/html/draft-ietf-pce-segment-routing-policy-cp-12#section-2-2.4>

suggest to rephrase this description to be more formal? , too casual to me.
<MK>
Sure, will rephrase. There was a prior comment about this as well.
</MK>

7.When these rules are not satisfied, the PCE MUST send a PCErr message with 
Error-Type = 26 "Association Error", Error Value = TBD8

Only the PCE sends? do we have any case that a PCC may send this error?
<MK>
True, it should be “PCEP speaker”, not “PCE”. Thanks.
</MK>
<C>nice
8.

This Association Type is dynamic in nature, thus operator-configured 
Association Range MUST NOT be set for this Association type and MUST be 
ignored.¶<https://datatracker.ietf.org/doc/html/draft-ietf-pce-segment-routing-policy-cp-12#section-4-2>

Sorry I do not understand this paragraph. What do you mean this association 
type is dynamic in nature?
<MK>
It’s referring to this: https://www.rfc-editor.org/rfc/rfc8697.html#section-3.4 
. The text is basically saying that this Association Type is fully dynamic. I’m 
not sure if this is necessary to say, or if we can rephase it to be clearer? 
Any suggestions?
</MK>
<C> All right, I see your point. If you decided to use 6, then the text works 
to me. But I will suggest to add a reference to explain that, and also, it is 
not dynamic in nature. Some of them can be configured so it is dynamic. We 
chosen 6, so it is not dynamic anymore, isn’t it?
9.
·        Association ID (16-bit): set to 
"1".¶<https://datatracker.ietf.org/doc/html/draft-ietf-pce-segment-routing-policy-cp-12#section-4.1-3.3>
why the ID must be set as 1? do you mean a association is identified by 
association source, color, and endpoint in extended association ID TLV? so we 
do not need Association ID?
or you like to use This ID 1 to avoid the race case between multiple PCE?
<MK>
Yes, the association is identified by <Source, Color, Endpoint>, so this 16-bit 
field is not useful. We set it to “1” to avoid using “0”, which is a reserved 
value for that field. I can put a sentence to clarify this in the text.
</MK>
<C>All right

10.

   0                   1                   2                   3
   0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  |             Type              |             Length            |
  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  |                                                               |
  ~                       SR Policy Name                          ~
  |                                                               |
  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
Figure 
2<https://datatracker.ietf.org/doc/html/draft-ietf-pce-segment-routing-policy-cp-12#figure-2>:
 The SRPOLICY-POL-NAME TLV 
format<https://datatracker.ietf.org/doc/html/draft-ietf-pce-segment-routing-policy-cp-12#name-the-srpolicy-pol-name-tlv-f>

Though I can not image what may be added to this TLV now. But I think reserving 
0 bits for a new TLV is not a good decision.
same for "SRPOLICY-CPATH-NAME" TLV
<MK>
It’s not reserving 0 bits. If you read the description of the TLV, it encodes a 
policy name string.
</MK>
<C>I see. Even for encoding a name string, it may reserve some bits for future 
extension? But this is not vital now. I can accept it now, though from a 
general protocol design angle, I prefer to reserve some bits.


11. section 8

This document defines one new type for association, which do not add any new 
security concerns beyond those discussed
s/association/ASSOCIATION Object
s/do/does
<MK>
Ack, thanks.
</MK>

Thanks,
Cheng



From: Pce <pce-boun...@ietf.org<mailto:pce-boun...@ietf.org>> On Behalf Of 
Dhruv Dhody
Sent: Monday, January 8, 2024 11:29 AM
To: pce@ietf.org<mailto:pce@ietf.org>
Cc: pce-chairs <pce-cha...@ietf.org<mailto:pce-cha...@ietf.org>>; 
draft-ietf-pce-segment-routing-policy...@ietf.org<mailto:draft-ietf-pce-segment-routing-policy...@ietf.org>
Subject: [Pce] WGLC for draft-ietf-pce-segment-routing-policy-cp-12

Hi WG,

This email starts a 2-weeks working group last call for 
draft-ietf-pce-segment-routing-policy-cp-12.

https://datatracker.ietf.org/doc/draft-ietf-pce-segment-routing-policy-cp/

Please indicate your support or concern for this draft. If you are opposed to 
the progression of the draft to RFC, please articulate your concern. If you 
support it, please indicate that you have read the latest version and it is 
ready for publication in your opinion. As always, review comments and nits are 
most welcome.

The WG LC will end on Monday 22nd January 2024.

A general reminder to the WG to be more vocal during the last-call/adoption.

Thanks,
Dhruv & Julien

_______________________________________________
Pce mailing list
Pce@ietf.org
https://www.ietf.org/mailman/listinfo/pce

Reply via email to