Subject: [tsvwg] Comments on SCTP draft-ietf-tsvwg-sctp-failover-05

From: [email-masked]
Date: Fri, 3 Oct 2014 10:26:07 +0100

https://tools.ietf.org/html/draft-ietf-tsvwg-sctp-failover-05

As a Chair, I think the above draft seems in good shape from a protocol
perspective, if people see other ways to improve the protocol - it would
be good to know at this stage.

The authors have requested that this be considered as a PS. I have
therefore given this a more critical read - especially carefully looking
at the language, and suggest they consider the comments below and re-issue
the draft.  If we are to consider the draft as PS, then  I think a section
needs to be added to motivate this change (which *WILL* be confirmed on
this list after this revision).

Comments follow on the document (with no intended change to thee
protocol), please consider these - I'd love to hear from others on the
list if they also have any comments on this version!

Gorry
(tsvwg chair)

—

Section 4: (NiT in language)

/In order to address the issues described in Section 3, We propose to
   update SCTP path management scheme as follows./
to
/To address the issues described in Section 3, this section
   updates SCTP path management:/
-If this is to be considered as a PS, it /specifies/ the update.

Section 4.1: (language, there is no ordering so avoid this usage):
/In order to minimize performance impact during failover/
to
/To minimise the performance impact during failover/

Section 4.1 (structure)

- If I understand correctly, the first two paras, including the 2 bullets
are actually informative discussion to introduce the method, if this is
the case could these be moved to section 4, so thats action 4.1 includes
the RFC2119 protocol update.

Section 4.1: (NiT in language)
/tweaking/tuning/

/This proposal introduces/This new method introduces/

Requirement  1 (RFC2119)
/The recommended value of PFMR = 0  when quick failover is used.  /
- is this /RECOMMENDED/ ?

Requirement 3: (NiT in language)
/PF state with least error count/
/PF state with the lowest error count/

/In case of multiple PF/
/When there are multiple PF/

Requirement 4: (Clarity)

/This
        specification recommends that heartbeats be send to PF
        destinations independently from whether the Path Heartbeat
        function (Section 8.3 of [RFC4960]) is enabled for the
        destination address or not./
- This appears at the end of the requirement. I'm not sure what is
intended, could it be that this
RECOMMENDS the use of heartbeats with PF, in which case I'd have expected
this requirement perhaps to be stated first?

Requirement 7: (NiT in language)

/state with least error count/
/state with the lowest error count/

/In  case of multiple destinations with same error count in inactive
        state/
/When there are multiple destinations with the same error count in the
inactive state/

Requirement 4: (RFC2119)

/In order to support this
        prescription a sender SHOULD allow for increment of the
        destination error counters up to some reasonable limit above
        PMR+1/
/Therefore, a sender SHOULD allow for incrementing the
        destination error counters up to some reasonable limit above
        PMR+1/
- I can see the goal of reasonable limit, but is this actually a "limit" 
- that stops further incrementing or "permits" the value to be incremented
beyond PMR+1?

/A  sender MAY choose to deploy other strategies than the above/
- good to state this, but this is the second part of a SHOULD statement
explaining options, as such I do not think it needs an upper case /MAY/
and suspect that /may/ will be sufficient.

Requirement 8: (NiT in language)

/ACKs for chunks which have been transmitted to multiple
        destinations (i.e., a chunk which has been retransmitted to a
        different destination than the destination to which the chunk
        was first transmitted) /
/ACKs for chunks that have been transmitted to multiple
        destinations (i.e., a chunk that has been retransmitted to a
        different destination than the destination to which the chunk
        was first transmitted) /
- change /which/ to /that/

Use upper case for /ack/?

/In this respect then it is specified
        that bytes of a newly acknowledged chunk which has been
        transmitted to multiple destinations SHOULD, when the conditions
        for such contribution is fulfilled following the prescriptions
        of Section 7.2 of [RFC4960], contribute to the congestion window
        growth towards the destination to which the chunk was last
        transmitted. /
- This clause is rather complex and long, please rephrase!

/A SCTP sender MAY apply a different approach for
        both the error count handling as well as the congestion control
        growth handling does it have unequivocally information as to
        which destination (including multiple destinations) the chunk
        reached.  /

- This clause is rather complex, and hard to parse, please rephrase!

Requirement 10: (Clarity)

/SCTP SHOULD provide the means to expose the PF state /
- Is this the SCTP stack should expose to the ULP, please clarify, and use
language similar to other SCTP RFCs.

/Active state ./ - remove space before stop,

/When  doing such SCTP MUST /
/When  this, the SCTP stack MUST /

/suppress exposure of PF state to ULP.  /
/suppress exposure of PF state to the ULP.  /

- I found this requirement not so clearly explained, and rather hard to
comprehend from the text.

Section 4.2: (RFC2119, Clarity)

/Post failover then, by [RFC4960] behavior, an SCTP sender migrates
   the traffic back to the original primary destination once this
   destination becomes active anew.  /
- This doesn’t seem to make sense.

No 1

/Any setting of PSMR < PFMR
       MUST be rejected by the implementation./
- MUST be rejected seems odd. Can you phrase this the other way around?
The PSMR MUST be set so that PSMAR>=PFMR.
- This seems to contradict the previous SHOULD.

No 2 and 3 do not introduce RFC 2119 requirements. Is this intentional, if
so why?

No 4
/support set of PSMR = PFMR/
/support the setting of PSMR = PFMR/

/ We recommend that SCTP-PF sticks to the standard RFC4960 switchback
   behavior as default/
- is this intended to be:
/This specifications RECOMMENDS a default configuration that uses standard
RFC4960 switchback./

/However in order to support//However, to support/

- does this section need the MAYs in capital letters, since it follows a
SHOULD?

—

Section 6:

I’m happy with this clause, if it is correct. For clarity I’d prefer the
section is prefixed by something like:
“Secutity considerations for the use of SCTP are discussed in RFCxxx and
RFCyyyy.”

Appendices: (Structure)

Two appendices are provided, but seem not to be described in the body of
the specification. - please either add references were appropriate to the
appendices or remove them.

Proposed change of status

I would expect such a section to b marked for removal before publication
by RFC-Ed (since this is ONLY an AD/IESG decision - that provides a couple
of short paras that document the evolution of the draft from EXP to PS,
and motivate that the experimental considerations are no longer an issue.
I believe the authors have clearly put this argument in the past, and hope
that this is not a difficult section to add.

---

From: Yoshifumi Nishida <[email-masked]>
Date: Sun, 5 Oct 2014 01:00:02 -0700

Hi Gorry,

Thanks for the detailed reviews. I agree with your comments.
We will reflect them in our upcoming new version after checking if there's
any inconsistency.

Regards,
--
Yoshi

---

From: Yoshifumi Nishida <[email-masked]>
Date: Thu, 23 Oct 2014 21:59:04 -0700

Hi Gorry,

Thanks for the detailed reviews (again!)
We've updated our draft based on your suggestions and submitted it as -07
draft. (Sorry, I've made a mistake in 06 and resubmitted 07)
We believe there's no discrepancy, but just in case, I've inserted some
comments in lines.

Thank you so much!
--
Yoshi

> /To minimise the performance impact during failover/

Done.

> the RFC2119 protocol update.

We've split Section 4.1 into two sections: "concept" and "algorithm
detail". Section 4.2 includes 2119 update.
We've also changed the title of Section 4.3 to clarify this is an optional
feature.

> /This proposal introduces/This new method introduces/

Done.

> - is this /RECOMMENDED/ ?

We've changed to RECOMMENDED.

> /When there are multiple PF/

Done.

> this requirement perhaps to be stated first?

We've changed the sentence to:

        It is RECOMMENDED that heartbeats be send to PF destinations
        regardless of whether the Path Heartbeat function
        (Section 8.3 of [RFC4960]) is enabled for the destination
        address or not.

Since this is not strong requirement, I think it doesn't have to be stated
first.

> /When there are multiple destinations with the same error count in the

Done.

> - that stops further incrementing or "permits" the value to be incremented

Done. We've changed this to:

Therefore, a sender SHOULD allow for incrementing the
destination error counters up to some reasonable limit larger than PMR+1.

> and suspect that /may/ will be sufficient.

Agree. changed to may.

> Use upper case for /ack/?

Done.

> - This clause is rather complex and long, please rephrase!

We've changed it to:
     The bytes of a newly acknowledged chunk which has been transmitted

     to multiple destinations SHOULD be considered for contribution to

     the congestion window growth towards the destination where the chunk
was last sent.
     The contribution of the acked bytes to the window growth is subject to

     the prescriptions described in Section 7.2 of [RFC4960] is fulfilled.

> - This clause is rather complex, and hard to parse, please rephrase!

We've changed this to:

        A SCTP sender MAY apply a different approach for
        both the error count handling and the congestion control growth
        handling based on unequivocally information on which destination
        (including multiple destinations) the chunk reached.

> /suppress exposure of PF state to the ULP.  /

Done.

> comprehend from the text.

We've changed this to:

            SCTP stack SHOULD provide the ULP with the means to expose the
PF state of its
            destinations as well as the means to notify the state
transitions from Active to PF and vice-versa.
            When doing this, such SCTP stack MUST provide the ULP with the
means to suppress exposure of PF
            state and association state transitions as well.

> - This doesn’t seem to make sense.

We've changed this to:

   In [RFC4960], an SCTP sender migrates the traffic back to the
   original primary destination once this destination becomes active
   again.

> - This seems to contradict the previous SHOULD.

We've changed this to:
            The PSMR MUST be set greater or equal to the PFMR value.

            Implementations MUST reject any other values of PSMR.

> /However in order to support//However, to support/

done

> - does this section need the MAYs in capital letters, since it follows a

Yes. changed to MAY

> “Secutity considerations for the use of SCTP are discussed in RFCxxx and

Yes. We've added the sentences in the section.

> appendices or remove them.

We've add some texts to refer them at the end of Section 1.

> We've added Section 8 for this.