Re: [Pce] Benjamin Kaduk's Discuss on draft-ietf-pce-stateful-path-protection-10: (with DISCUSS and COMMENT)

[Benjamin Kaduk <[e-mail]>]

On Tue, Sep 24, 2019 at 11:38:13AM +0530, Dhruv Dhody wrote:
> Hi Ben,
> 
> Apologies for the delay. I am replying this email where we tackle
> points that needed some more discussion.
> 
> On Fri, Sep 20, 2019 at 2:32 AM Benjamin Kaduk <[e-mail]> wrote:
> >
> > Hi Dhruv,
> >
> > On Wed, Sep 18, 2019 at 11:45:23AM +0530, Dhruv Dhody wrote:
> > > Hi Ben,
> > >
> > > Thanks for your review, let me start the discussion and the authors
> > > can interject as needed.
> >
> > Thanks; it's a great start.
> >
> > > On Wed, Sep 18, 2019 at 5:14 AM Benjamin Kaduk via Datatracker
> > > <[e-mail]> wrote:
> > > >
> > > > Benjamin Kaduk has entered the following ballot position for
> > > > draft-ietf-pce-stateful-path-protection-10: Discuss
> > > >
> > > > When responding, please keep the subject line intact and reply to all
> > > > email addresses included in the To and CC lines. (Feel free to cut this
> > > > introductory paragraph, however.)
> > > >
> > > >
> > > > Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
> > > > for more information about IESG DISCUSS and COMMENT positions.
> > > >
> > > >
> > > > The document, along with other ballot positions, can be found here:
> > > > https://datatracker.ietf.org/doc/draft-ietf-pce-stateful-path-protection/
> > > >
> > > >
> > > >
> > > > ----------------------------------------------------------------------
> > > > DISCUSS:
> > > > ----------------------------------------------------------------------
> > > >
> > > > (1) draft-ietf-pce-association-group notes that "PCEP extensions that define
> > > > a new association type should clarify the relationship between the SVEC
> > > > object and the association type, if any".  Where do we do so for the
> > > > path protection association type?
> > > >
> > >
> > > This is discussed in
> > > https://tools.ietf.org/html/draft-ietf-pce-association-diversity-10#section-5.3,
> > > where it is more relevant. This draft includes the relationship with
> > > diversity association in Section 5.
> >
> > A naive reading of the linked section would be that it applies only to the
> > "Disjointness Association Type" defined in that document.  I'm not in a
> > position to claim that the reasoning in that document does not apply to
> > this one, but I think we should make a more clear link between the Path
> > Protection ASsociation Type and the discussion in that document.
> >
> 
> The direct relationship is between the 'Disjointness Association Type'
> and SVEC. But it is natural to include both Protection association and
> disjoint association together. If SVEC is also present the
> relationship is as per the Section 5.3 of
> [I-D.ietf-pce-association-diversity]. Suppose if only protection
> association is present and SVEC object, there is no direct
> relationship and both object would be processed.
> 
> I suggest we add this text to make sure reader is aware of SVEC - "The
> relationship between the Synchronization VECtor (SVEC) object and the
> Disjointness Association is described in section 5.3 of
> [I-D.ietf-pce-association-diversity]."

Thanks!

> 
> > > > (2) Section 3.2 says:
> > > >
> > > >       Protection Type (PT): 6 bits - This field is as defined in
> > > >       Section 14.1 of [RFC4872] to indicate the LSP protection type in
> > > >       use.
> > > >
> > > > There doesn't seem to be a registry created by RFC 4872 to track these
> > > > PT values, so I assume that the way to allocate a new value is
> > > > "standards-track RFC that Updates: RFC 4872".  Is that also the way to
> > > > allocate new PT values for PPAG usage?  How would someone updating RFC
> > > > 4872 to allocate a new type know to consider/document whether it applies
> > > > for PPAG usage?
> > > >
> > >
> > > That's true. Maybe we can add this text - "Any type already defined or
> > > that could be defined in the future for use in the RSVP-TE PROTECTION
> > > object is acceptable in this TLV unless explicitly stated otherwise."
> >
> > To be honest, that's probably good enough in practice, since it seems
> > somewhat unlikely that the last bit is ever going to get allocated (though,
> > I guess technically the field could be converted to an integer instead of
> > flags).  I do still have to wonder how the reader would know *where* it
> > would be stated otherwise, and how the author of a document defining a new
> > type would know to consider our use case as well as the RFC 4872 use case.
> >
> 
> I guess such an update would happen in TEAS; and historically PCE and
> TEAS work very closely, as well as share the same AD. I think we are
> quite safe here.
> 
> In case you were wondering why flags - we chose PT as flags as per RFC
> 4872 where this is called "LSP (Protection Type) Flags".
> 
> > > > (3) In Section 4.3:
> > > >
> > > >    A PCE can create/update working and protection LSPs independently.
> > > >    As specified in [I-D.ietf-pce-association-group], Association Groups
> > > >    can be created by both the PCE and the PCC.  Further, a PCE can
> > > >
> > > > The requirement that source, destination, and tunnel ID of all LSPs
> > > > within a PPAG MUST be the same is new to this document, so I think we
> > > > need to specify error handling for when attempts to update LSPs
> > > > independently would violate that invariant (presumably in Section 4.5).
> > > >
> > >
> > > In the existing error case in Section 4.5 we could make it "add or update".
> >
> > Wording it right might be a little finicky, but that's fine in general.
> >
> > > > (4) In Section 6.3:
> > > >
> > > >                                       IANA is requested to allocate new
> > > >    error values within the "PCEP-ERROR Object Error Types and Values"
> > > >    sub-registry of the PCEP Numbers registry, as follows:
> > > >
> > > > The following table only lists Error-values.  What Error-type(s) should
> > > > they be associated with?
> > > >
> > >
> > > It is mentioned in the text, but worth making it explicit in the table as well.
> > >
> 
> Lets update the table as well. Request authors to update.

Thanks!

> > > > (5) We don't say which objects the PPAG TLV can appear in.  (Section 3.2
> > > > says "[t]he Path Protection Association TLV is an optional TLV for use
> > > > with the Path Protection Association Type", but it's hard to interpret
> > > > that as meaning "for use [only] with the ASSOCIATION object defined in
> > > > draft-ietf-pce-association-group", especially since there is a "path
> > > > protection association type" already (and it's a codepoint in the
> > > > "ASSOCIATION Type Field" registry).
> > > >
> > >
> > > We can update to - "The Path Protection Association TLV is an optional
> > > TLV for use in the ASSOCIATION Object with the Path Protection
> > > Association Type."
> >
> > Thanks!
> >
> > > > (6) I'm not sure if a change to the document is needed here, but perhaps
> > > > some discussion is in order: we say that a given LSP can belong to more
> > > > than one PPAG, but only allow one PPAG TLV per [some context that
> > > > remains unclear; see (5)].  I don't have a good handle for whether these
> > > > two requirements are potentially in conflict: that is, whether a single
> > > > PPAG TLV would have to specify the flags that apply for both PPAGs that
> > > > a given LSP is a member of, or if the containing objects serve to scope
> > > > the PPAG TLV flags' interpretation.
> > > >
> > >
> > > When a given LSP belong to more than one PPAG, we expect to have
> > > multiple ASSOCIATION Object for each PPAG with each of them having a
> > > single PPAG TLV.
> >
> > Sounds good, and quite natural given (5).
> >
> > > > (7) Do the Protection Type fields of the PPAG TLV in the various LSPs
> > > > that are members of the same PPAG need to match, in the same way that
> > > > the source/destination/tunnel-ID do?
> > > >
> > >
> > > Yes. Authors should make that explicit.
> >
> 
> Sure. Request authors to add.

Thanks!

> > Thanks
> >
> > > >
> > > > ----------------------------------------------------------------------
> > > > COMMENT:
> > > > ----------------------------------------------------------------------
> > > >
> > > > Section 1
> > > >
> > > >    This document specifies a stateful PCEP extension to associate two or
> > > >    more LSPs for the purpose of setting up path protection.  The
> > > >    proposed extension covers the following scenarios:
> > > >
> > > > nit: after publication, it doesn't really seem like a *proposed*
> > > > extension anymore.
> > > >
> > > > Section 3.1
> > > >
> > > >    LSPs are not associated by listing the other LSPs with which they
> > > >    interact, but rather by making them belong to an association group
> > > >    referred to as "Path Protection Association Group" (PPAG) in this
> > > >    document.  [...]
> > > >
> > > > The first 2/3 of this sentence is a (true) generic statement about all
> > > > association groups, which exist outside of this document in a generic
> > > > fashion.  I strongly suggest rewording this to make clear that the PPAG
> > > > is the specific association (group) type used for this document's
> > > > functionality but that other association groups are possible.  The
> > > > current wording implies that the PPAG is the only association possible,
> > > > and it is just given a special name for this document's purposes.
> > > >
> > > >    [I-D.ietf-pce-association-group] specifies the mechanism for the
> > > >    capability advertisement of the Association types supported by a PCEP
> > > >    speaker by defining a ASSOC-Type-List TLV to be carried within an
> > > >    OPEN object.  This capability exchange for the Association type
> > > >    described in this document (i.e.  Path Protection Association Type)
> > > >    MAY be done before using the policy association, i.e., the PCEP
> > > >    speaker MAY include the Path Protection Association Type (TBD1) in
> > > >    the ASSOC-Type-List TLV before using the PPAG in the PCEP messages.
> > > >
> > > > Why is this only MAY and not MUST?
> > > >
> > >
> > > In [I-D.ietf-pce-association-group] we explicitly did not mandate the
> > > capability exchange for all association types and let each type decide
> > > for itself. This association type fits into where the capability
> > > exchange is optional. When the capability exchange was added in
> > > [I-D.ietf-pce-association-group], there was a desire from the WG to
> > > keep it optional for already defined/implemented protection
> > > association type. Also this is a basic feature in path computation and
> > > it is expected to be supported universally. In a corner case there is
> > > an error handling for unknown/unsupported association-type to take
> > > care of it.
> >
> > Okay, thanks for giving me the backstory.
> >
> > > >    This Association type is dynamic in nature and created by the PCC or
> > > >
> > > > nit: is it the type that is dynamic or the associations of that type?
> > > >
> > >
> > > It is the type. [I-D.ietf-pce-association-group] has dynamic or
> > > operator-configured association, this one is dynamic.
> >
> > Ah, of course, I had forgotten about that division of configuration.
> >
> > > >       Protecting (P): 1 bit - This bit is as defined in Section 14.1 of
> > > >       [RFC4872] to indicate if the LSP is working or protection LSP.
> > > >
> > > >       Secondary (S): 1 bit - This bit is as defined in Section 14.1 of
> > > >       [RFC4872] to indicate if the LSP is primary or secondary LSP.  The
> > > >       S flag is ignored if the P flag is not set.
> > > >
> > > > Just to check my understanding (no change expected to result): if P is
> > > > zero, the LSP is working, and thus we know that it's primary since it's
> > > > actively carrying traffic?
> > > >
> > >
> > > Yes, Working LSPs are always primary.
> > >
> > >
> > > > Section 3.2
> > > >
> > > > nit(?) The section heading does not match the name of the TLV requested
> > > > in the IANA considerations, nor do we mention here that "PPAG TLV" is
> > > > synonymous with it.
> > > >
> > > >    If the TLV is missing, it is considered that the LSP is the working
> > > >    LSP (i.e. as if P bit is unset).
> > > >
> > > > Is this conditional on the LSP being part of a PPAG, or does it hold
> > > > generically as well?
> > > >
> > >
> > > IMHO when part of a PPAG i.e. association object but no TLV.
> >
> > It may be worth making the text explicit about it.
> >
> > > > Section 4.5
> > > >
> > > >    As per the processing rules specified in section 5.4 of
> > > >    [I-D.ietf-pce-association-group], if a PCEP speaker does not support
> > > >
> > > > There is no Section 5.4 in draft-ietf-pce-association-group-10;
> > > > presumably this was supposed to be Section 6.4?
> > > >
> > > >    A given LSP MAY belong to more than one PPAG.  If there is a conflict
> > > >
> > > > While I'm not arguing to remove this option, I'm also a bit confused at
> > > > how useful having a single LSP be in multiple PPAGs woud be, given the
> > > > need for source/destination/tunnel-ID to match.
> > > >
> > >
> > > One working LSP could have multiple protection LSPs with each
> > > belonging to a different association.
> > >
> > > >    When the protection type is set to 1+1 or 1:N with N=1, there MUST be
> > > >    only one working LSP and one protection LSP within a PPAG.  If a PCEP
> > > >    speaker attempts to add another working/protection LSP, the PCEP peer
> > > >    MUST send PCErr with Error-Type 26 (Association Error)
> > > >    [I-D.ietf-pce-association-group] and Error-Value TBD4 (Attempt to add
> > > >    another working/protection LSP for Path Protection Association).
> > > >
> > > > This seems to prevent a scenario where there's a need to change the
> > > > protection LSP and a desire to do so without removing the protected
> > > > nature of the working LSP.  Unless it's presumed that this could always
> > > > be done by updating the protection LSP and it would never be necessary
> > > > to create a new one?  (Similarly for 1:N, though maybe less severe.)
> > > >
> > >
> > > One can remove the protection LSP and keep the association intact. The
> > > error will not be triggered.
> > > Also, PCEP messages are per LSP, when the first LSP is reported with
> > > association, it is bound to be the only LSP in the association.
> > > So the error in this section are focused only when one adds "another".
> >
> > I don't think that the points you cover above are what I was concerned
> > about -- I was thinking more along the lines of a "make-before-break"
> > scheme for changing what LSP is the protection LSP for a given working LSP.
> > Maybe given the timescales involved it's not an issue in practice, though.
> >
> 
> Maybe worth adding some text here to be safe - "During the
> make-before-break (MBB) procedure, two paths will briefly coexist. The
> error handling related to number of LSPs allowed in a PPAG MUST NOT be
> applied during MBB."
> 
> I hope this message clears all the remaining points.

I think it does; thanks again.

-Ben