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

[Robert Sparks <[e-mail]>]

Thanks! Some initial replies inline (I haven't looked at the updated 
draft yet, but will do so soon):


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