Re: [sfc] Regarding last call for draft-ietf-sfc-multi-layer-oam

[Greg Mirsky <[e-mail]>]

Hi Donald,
many thanks for your detailed review and thoughtful suggestions. Your help
in improving this document is much appreciated. Please find my responses
in-lined below under the GIM>> tag.
Attached, please find the copy of the new working version of the draft and
its diff to -16.

Regards,
Greg

On Thu, Nov 4, 2021 at 10:38 PM Donald Eastlake <[e-mail]> wrote:

> I support publication of this draft. However, I have a few
> suggestions/questions below. I also have a bunch of other suggestions,
> mostly editorial, in the attached.
>
> Section 6: I found the following text and nearby material confusing:
>    "TLV is a variable-length field.  Multiple TLVs MAY be placed in an
>    SFC Echo Request/Reply packet.  Additional TLVs may be enclosed
>    within a given TLV, subject to the semantics of the (outer) TLV in
>    question.  If more than one TLV is to be included, the value of the
>    Type field of the outmost outer TLV MUST be set to "Multiple TLVs
>    Used" (TBA12), ..."
> Starting at the beginning, The field I think that is being referred to
> is labeled "TLVs", not "TLV".
>
GIM>> Thank you for pointing this out. I think that the intention was to
refer to the Value field. Then the text will read like this:
"TLV is a variable-length construct."
Would it be better?

> Then it says "Multiple TLVs MAY be placed in an SFC Echo Request/Reply
> packet." which sounds fine. The usual thing when TLVs are allowed is
> to allow multiple of them in sequence. But then there is the later
> text that seems to imply that if there is more than on TLV, there has
> to be a single(?) top level "Multiple TLVs" type TLV that encloses
> all(?) the others (presumably in its Value field). Why? Is the nesting
> only one level? (Also, "outmost" should be "outermost".)
>
GIM>> Great point, thanks. I think that this construct was put in "in-case"
and never used. We have a close case, Errored TLVs TLV, but nothing that
uses Multiple TLVs type. I propose the following update:
OLD TEXT:
   Multiple TLVs MAY be placed in an
   SFC Echo Request/Reply packet.  Additional TLVs may be enclosed
   within a given TLV, subject to the semantics of the (outer) TLV in
   question.  If more than one TLV is to be included, the value of the
   Type field of the outmost outer TLV MUST be set to "Multiple TLVs
   Used" (TBA12), as assigned by IANA according to Section 9.4.
NEW TEXT:
   Multiple TLVs MAY be placed in an
   SFC Echo Request/Reply packet. None, one or more sub-TLVs may be enclosed
   in a TLV, subject to the semantics of the (outer) TLV.

And I remove the Multiple TLVs type from the IANA section. What do you
think about these updates?

>
> I also think that Figure 4 should be moved down a little. The "TLV"
> (should be "TLVs"?) field described right after Figure 4 is actually
> from Figure 3, I think. Maybe you could take the paragraph after
> Figure 4, split it just before the last sentence in that paragraph,
> put Figure 4 there, and adjust wording appropriately.
>
GIM>> I've moved Figure 4, please let me know if it is better connected to
the text. I think that the name Value reflects is the part of the TLV
format and would like not to change it. Would that be acceptable?

>
> Section 6.4: What exactly is "NSH Service Index (SI) expiration" and
> how is this different from being the terminal SFF of an SFP?
>
GIM>> I normal operation, I agree, these SI == 0 should not happen. But as
TTL, SI is a loop-prevention mechanism for SFs. And, as TTL expiration, SI
expiration can be used as OAM exception mechanism based on the last
sentence in Section 2.3 RFC 8300:
   In addition, an SFF that is not the
   terminal SFF for an SFP will discard any NSH packet with an SI of 0,
   as there will be no valid next SF information.

>
> Section 6.4.1: TLVs inside the "Errored TLVs" TLV are here called
> "sub-TLVs" but when nested TLVs were being talked about earlier,
> "sub-TLVs" was not used
>
GIM>> Yes, thank you for pointing out this inconsistency in terminology. As
proposed above, the updated text now is as follows:
   TLV is a variable-length consruct.  Multiple TLVs MAY be placed in an
   SFC Echo Request/Reply packet.  None, one or more sub-TLVs may be
   enclosed in a TLV, subject to the semantics of the (outer) TLV.

>
> Section 6.5.2: I notice that this section has numeric values for many
> Return Codes. Since these are in a new Registry being set up by this
> document, I think that is the way to go and recommend putting all
> values being initially assigned in new Registries numerically in the
> new Registry tables in the IANA Considerations Section.
>
GIM>> Agreed. Please check the updated IANA Considerations section.

>
> Sections 6.6 / 6.6.1: It is a bit confusing that this talks about
> using these CVReq and CVRep messages before they have been specified.
> I suggest putting a pointer to 6.6.1 early in 6.6 or swapping 6.6 and
> 6.6.1.
>
GIM>> I've put the forward reference to Section 6.6.1. Please let me know
if it addresses your concern.

>
> Section 6.6.2: It is very confusing to have "SFF Information Record
> TLV" and "SF Information Record TLV" with such similar names. Suggest
> that the SF Information Record always be called a Sub-TLV as it is in
> the Figure. Suggest making it clear that the SF Information Record
> Sub-TLV only appears inside the SFF Information Record, not directly
> at the top level in the CVRep.
>
GIM>> Agree and updated. Please let me know if it reads more logical.

   In the first sentence after Figure 9, should that be "SFFs" or "SFs"?
>
GIM>> Good catch. Fixed by s/all SFFs mapped to the particular SFF
instance/all SFs mapped to the particular SFF instance/

   Why does the 2nd sentence after Figure 9 say "Figure 9 presents the
> format of an SFC Echo Request/ Reply TLV, ..." when the  caption of
> Figure 9 and Figure 9 itself make it appear to be an "SFF Information
> Record TLV"?
>
GIM>> Sorry for this cut-n-paste error and thank you for another good
catch. Fixed.

>
> Section 6.6.3: I don't understand the first sentence of this section.
>    The "SF Type" field explanation should probably reference the IANA
> registry.
>
GIM>> Agreed and added.

>    How is "SF Identifiers" formatted if it is a list of identifiers?
>
GIM>> Should be singular. Also, removed the reference to a list of SFs.
Each SF mapped to the SFF must be listed in the separate SF Information
sub-TLV.

>
> Section 9: I wouldn't bother to set up either of the Version
> Registries (9.2.1, 9.3.1) in this document. Seems unlikely that a new
> version will be specified anytime soon, if ever. But if you do keep
> the Version Registries, since there are only 4 values available, I
> would make assignment be based on Standards Action.
>
GIM>> Makes sense to remove both.


>
> Section 9: When a document is the first to set up a Registry and
> assign values in that Registry, you should just go ahead and make
> these initial assignments. There is no reason to make more work for
> IANA and the RFC Editor in this case by setting up a table with no
> assignments and then separately asking for the assignments. So you can
> get rid of a bunch of TBAs.
>
GIM>> Agreed, please check the updated version.

>
> Section 9.2.2, Table 4: Only the first and last entries should be
> Reserved. The others should be Unassigned. But also, the "request" to
> IANA verbiage should be dropped and the assigned value in Table 5 just
> put into Table 4. You are creating the registry, you can set the
> initial assignments as desired and approved by the normal document
> approval path. IANA normally has nothing to do with the initial
> assignment decisions for a registry. All values in the Reference
> column should be '[this document]". See attached markup including
> multiple other cases that should have similar treatment.
>
GIM>> Thank you for the suggestion. Accepted.

>
> Thanks,
> Donald
> ===============================
>  Donald E. Eastlake 3rd   [phone] (cell)
>  2386 Panoramic Circle, Apopka, FL 32703 USA
>  [e-mail]
>
> On Mon, Nov 1, 2021 at 2:50 PM Joel Halpern Direct
> <[e-mail]> wrote:
> >
> > I have received a polite request with explanation for delay asking for
> > more time to read and review the subject document.  Given the state of
> > the working group, i want to encourage any and all review.  So I am
> > extending the last call by two additional weeks.
> >
> > Please read and review the document.
> > Also, if you are willing to serve as shepherd for this, please let the
> > chairs know.  (Don't worry if you have not shepherded a document before.
> >   The chairs are more than happy to help you with the process.)
> >
> > Thank you,
> > Joel
> >
> > _______________________________________________
> > sfc mailing list
> > [e-mail]
> > https://www.ietf.org/mailman/listinfo/sfc
>