Re: [Gen-art] Genart last call review of draft-ietf-pce-stateful-pce-lsp-scheduling-13

[Robert Sparks <[e-mail]>]

On 6/11/20 9:12 AM, Robert Sparks wrote:
>
> Thanks! Some initial replies inline (I haven't looked at the updated 
> draft yet, but will do so soon):
>
I've looked at the diffs between -13 and -15 and my comments have been 
addressed except for one point.


         Only one of them (i.e., elastic range and grace periods) is 
used for an LSP.


Isn't strong enough. I think you need normative language here.


I suggest

     A TLV can configure a non-zero grace period or elastic bound, but 
it MUST NOT provide both.

> On 6/10/20 7:35 PM, Huaimo Chen wrote:
>> Hi Robert,
>>
>>     Thank you very much for your time and valuable comments.
>>
>>     My answers/explanations are inline below with prefix [HC].
>>
>>     Your comments have been addressed in the updated document
>> https://datatracker.ietf.org/doc/draft-ietf-pce-stateful-pce-lsp-scheduling/
>>
>> Best Regards,
>> Huaimo
>> ------------------------------------------------------------------------
>> *From:* Robert Sparks via Datatracker <[e-mail]>
>> *Sent:* Tuesday, June 9, 2020 5:41 PM
>> *To:* [e-mail] <[e-mail]>
>> *Cc:* [e-mail] <[e-mail]>; [e-mail] 
>> <[e-mail]>; 
>> [e-mail] 
>> <[e-mail]>
>> *Subject:* Genart last call review of 
>> draft-ietf-pce-stateful-pce-lsp-scheduling-13
>> Reviewer: Robert Sparks
>> Review result: Ready with Issues
>>
>> I am the assigned Gen-ART reviewer for this draft. The General Area
>> Review Team (Gen-ART) reviews all IETF documents being processed
>> by the IESG for the IETF Chair.  Please treat these comments just
>> like any other last call comments.
>>
>> For more information, please see the FAQ at
>>
>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftrac.ietf.org%2Ftrac%2Fgen%2Fwiki%2FGenArtfaq&amp;data=02%7C01%7Chuaimo.chen%40futurewei.com%7C009e6885956a4c1957c908d80cbde38d%7C0fee8ff2a3b240189c753a1d5591fedc%7C1%7C0%7C637273356999321440&amp;sdata=x1%2BeFVCwSKtx16TqY5ycJgjrXK%2Bb%2Be4ttO0SuBrPStY%3D&amp;reserved=0>.
>>
>> Document: draft-ietf-pce-stateful-pce-lsp-scheduling-13
>> Reviewer: Robert Sparks
>> Review Date: 2020-06-09
>> IETF LC End Date: 2020-06-12
>> IESG Telechat date: Not scheduled for a telechat
>>
>> Summary: Essentially ready for publication as a Proposed Standard 
>> RFC, but with
>> issues to consider before progressing
>>
>> Minor Issues:
>>
>> Section 4.2.2: It's not clear when the computation for a path 
>> satisfying the
>> constraint happens. Is this done once when the periodical LSP 
>> arrives, or at
>> each scheduled interval? If the former, what happens if there is no path
>> solution for only one of the multiple intervals?
>>
>> [HC]: We have made it clear in the document through revising the texts.
>> When a periodical LSP is configured, the paths for all the scheduled
>> intervals of the LSP is computed once.
>> If there is no path for some (e.g., one) of the intervals,
>> the constraints for the periodical LSP is not satisfied and
>> the LSP will not be set up at all.
>>
>> Section 4.4, second paragraph, last sentence: If the path cannot be 
>> set, is the
>> previous LSP left in effect? Or does the failure result in no there 
>> being no
>> scheduled LSPs in effect?
>>
>> [HC]: We have added the texts explicitly stating that
>> the previous LSP will not be impacted if the path for new
>> requirements cannot be set.
>>
>> Section 5.1 first paragraph: Why is TCP called out here?
>>
>> [HC]: We have removed TCP.
>>
>> You should be explicit about whether it's ok to have both grace 
>> periods and
>> elastic bounds at the same time. The TLV allows that to happen. I'm 
>> not sure
>> what it would mean, and I suspect it's unlikely that you would have two
>> implementers compute the consequences the same way.
>>
>> [HC]: We have added some texts to say that only one of them is used.
>>
>> Section 5.2.1, definition of the R bit: You should call out that 
>> relative time
>> is in seconds (I think?) when the R bit is 1, and you need a discussion
>> somewhere about the necessity of synchronized clocks and potential 
>> problems
>> with transmission delay when the R bit is 1.
>>
>> [HC]: You are right. The relative time is in seconds.
>> We have explicitly stated this and had some discussions about
>> synchronizing clocks and potential problems with transmission delay.
>>
>> Section 5.2.1, definition of Start-Time: Why must a value of 0 not be 
>> used? Is
>> this true for both R=1 and R=0? For R=1, a start time value of 1 vs a 
>> start
>> time value of 0 may, in practice, be indistinguishable (because of 
>> transmission
>> or processing delay).
>>
>> [HC]: When R=1, Start-Time=0 means right now.
>> Because of transmission and processing delay, this cannot be achieved.
>
> And you don't have the same problem (or at least a race condition) for 
> R=1, Start-Time=1?
>
> (And perhaps other small values of Start-Time?). This isn't a big 
> deal, but it looks like a rough
>
> edge, and I want to make sure it's been thought through.
>
>> For R=0, Start-Time=0 means the epoch (i.e., 1 January 1970 at 00:00 
>> UTC).
>> So for both R=1 and R=0, a value of 0 must not be used in Start-Time.
>>
>> In section 5.2.2 at the definition of Repeat-time-length: Please be 
>> explicit
>> about whether this is the interval between the start time of two 
>> repetitions or
>> the interval between the end-time of one repetition and the start of 
>> the next
>> repetition. I think you mean the former.
>>
>> [HC]: It is the interval between the end-time of one repetition and
>> the start of the next repetition. We had added some texts to
>> state this explicitly.
>
> That surprises me. All the other values are start-start and not end-start.
>
> With REPEAT-EVERY-DAY, you're aiming for the same time every day.
>
> Suppose you didn't have REPEAT-EVERY-DAY and had to express that with 
> REPEAT-EVERY-TIME-LENGTH.
>
> If it were start-start, you could set the interval to 86400 (as you 
> compute in 9.1) and be done.
>
> With it being end-start you have to calculate the value by subtracting 
> the Duration field value from
>
> 86400.
>
> I think that's likely to surprise implementors trying to achieve 
> something like "every other day".
>
>
>>
>> At section 5.2.1 you say this TLV SHOULD NOT be included unless both 
>> PCEP peers
>> have set the B bit. But in section 6.6, you say MUST NOT. Please 
>> choose one. I
>> think you want both places to say MUST NOT.
>>
>> [HC]: We have changed SHOULD NOT to MUST NOT.
>>
>> Nits:
>>
>> Introduction, paragraph 3, second sentence: This is hard to read. I 
>> suggest
>> trying to break it into more than one sentence.
>>
>> [HC]: We have split and rephrased it.
>>
>> Introduction, paragraph 4, third sentence: This is hard to read. Please
>> simplify.
>>
>> [HC]: We have rephrased it.
>>
>> The document uses both "database" and "data base". Please pick one.
>>
>> [HC]: We have changed "data base" to database.
>>
>> Top of page 7: "In case of former" does not parse. Please clarify.
>>
>> [HC]: We have rephrased it.
>>
>> Section 4.2.2, second paragraph, first sentence: Does not parse. I 
>> think it is
>> missing more than articles.
>>
>> [HC]: We have revised it.
>>
>> Section 4.3 at "In both modes": It's not clear what "modes" means here.
>>
>> [HC]: We have added some details.
>>
>> It would be worth calling out in section 5.1 that setting PD requires 
>> setting B
>> as specified in 5.2.2.
>>
>> [HC]: We have added some texts to state this.
>>
>> It would be helpful in 5.2.2 at the definition of Opt: to point 
>> forward to the
>> registry you are creating for its values. It would also be good to be 
>> explicit
>> about what to do if an element receives a TLV with a value here it 
>> does not
>> understand yet.
>>
>> [HC]: We have added some descriptions about these.
>>
>> Section 9.1 ignores leap-years and leap-seconds. It's worth 
>> explicitly noting
>> that.
>>
>> [HC]: We have added some texts about this.
>>
>
> _______________________________________________
> Gen-art mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/gen-art