Subject: [dhcwg] Review comments for draft-ietf-dhc-dhcpv6-failover-protocol-00.txt

From: Shawn Routhier <[email-masked]>
Date: Thu, 3 Dec 2015 11:29:57 -0800

During the last IETF there was a request to review the failover protocol draft.
I’ve now had a chance to review it and here are my comments.

regards,
Shawn

**

1) Glossary
Page 5, Delegable Prefix
The use of this term seems to be inconsistent.  In the glossary
it seems to be something like XXXX:0:1 from which a set of
prefixes to be delegated can be generated (say a set of /64s).
In 4.2.2 it seems to be treating the term as the prefix that would
be delegated for example XXXX:0:1:0/64.

2) Page 5, Failover endpoint - This mentions "prefixes associated with
that relationship".  I assume this means prefixes from which addresses
or prefixes may be leased but it could be read as simply the prefixes
that can be delegated.

3) Page 6, Resource - It might be useful to mention why temporary addresses
are not included in failover - either in this paragraph or maybe somewhere
else.  (Or maybe it was in the requirements document and I missed it?)

4) page 8,
4.2.1 Independent Allocation
I wonder if handing out addresses from only one partner's pool during
a PARTNER-DOWN state will have any odd effects.  In v4 the addresses could
sort of be re-mixed over time.  In v6 that won't happen.  Offhand I can't
think of any issues aside from the obvious pool imbalance, but it may
be worth spending some time to think if odd things could happen.

Paragraph 4 mentions that a partner MUST NOT lease Ipv6 addresses that
were assigned to its downed partner and later released by a client.
Should it also mention expired leases? declines?

5) 4.2.1.1 Independent allocation Algorithm
I am confused, shouldn't the low-order bit be 127?  Or is this text trying
to state that low-order bit within a prefix is set or cleared?  

Also while it is most likely that address ranges will be described via
a prefix it is also possible that they are described via start & end
addresses.  For example XXXX::100 to XXXX::200 (or if you prefer to
have a larger range XXXX::100:0:0:0 to XXXX::200:0:0:0).  When you
mention "prefix" are you intending to restrict the description? or
are you including something like turning the above ranges into a
set of prefixes for processing?

6) 4.2.2 Proportional Allocation
What happens if I have a large number of possible prefixes?  For example
the two servers are configured with a /32 and a prefix length of /64?
Does the spec need to either limit this in some fashion or supply some
guidelines or checks?

Are we expecting that the configuration will include the length of the
prefixes a server can delegate?

I don't see any option for the rewind optimization to avoid a secondary
in communications interrupted from running out of prefixes.

7) 5.3.4 POOLRESP
This definition isn't quite in line with 4.2.2.  This text has
"to indicate that it has responded to the secondary's request"
The use of "has responded to" suggests that the primary has completed
the response (as in has sent all the BNDUPDs and recevied all the BNDACKs)
4.2.2 states that the POOLRESP make be sent before, during or after any
BINDUPDs that need to be sent.

8) 5.4.2 OPTION_F_DNS_REMOVAL_INFO
I'm imagining the sub-options encapsulated within the option which
I think means the length depends on the sub-options.  Are you thinking
of some other arrangement?

9) 5.4.12 OPTION_F_PROTOCOL_VERSION
Does the document actually specify the version number somewhere?  I didn't
see it but maybe I missed it.  (Not the option code but the actual version
number.)

10) 5.4.20 OPTION_F_STATE_EXPIRATION_TIME
I believe this is the actual lifetime from 4.4?  If so it would be useful
to make that clear - either by changing one of the names to the other
or simply adding some text.

11) 6.1.1 Sending a CONNECT message
From the description of Failover Endpoint (page 5) it appears that one
server can be in failover relationships with multiple other servers and
one server could even be in multiple relationships with one other server.
The text in 6.2 seems to reinforce this as an option.  If this is true then
doesn't the CONNECT message need to contain information to specify which
relationship this CONNECT message is for?  (Note: I'm not arguing for
having multiple endpoints on the same pair of servers, I am wondering
how one of them will determine which of those endpoints the other is
trying to reach.)

12) 6.1.2 Receiving a CONNECT message
It sounds like the the two serves can use different versions of the protocol,
receive time, and unacked bind updates.  The latter two seem okay but the
text may want to mention that as a possibility.  The first one seems like
it might be difficult to deal with.

13) 6.3 Sending a STATE message
In the bullet list two of them use "this failover endpoint" while the
other two use "the failover endpoint".  I think they are also referring
to the failover endpoint sending the STATE message and find "this failover
endpoint" clearer.

14) 6.6 Unreachability detection
Does the document need to specify the messages are on this connection?
"...any message is transmitted on this connection...".

15) The second paragraph states that if the option OPTION_F_RECEIVE_TIME is
in the CONNECT or CONNECTACK message it is used.  But the descriptions of
CONNECT and CONNECTACK messages make it sound like that option is required.
One of the two sections of text should be clarified (I would vote for
always requiring the option and disconnecting if it isn't in the CONNECT
or CONNECTACK).

16) Figure 1, note 4
This has the following text:
...and the MCLT has passed since the entry in RELEASED, EXPIRED, or RESET states.
Should "in" be "into"?

17) Figure 1, note 7
This states that Addresses belonging to the primary server can be assigned to
the secondary server pool (or the reverse) but elsewhere it appears that
addresses are always handled via Independent Allocation rather than
Proportional Allocation.

18) 7.3 Tiems Required for Exchanging Binding Updates
partner-raw-clt-time
This mentions time context which is also mentioned later on but is never defined.
Perhaps adding an entry in the definitions would be useful?

state-expiration-time
As mentioned abouve for 5.4.20 I believe this is the actual lifetime as
handed to the client?  If so some text to clarify that would be useful.

19) 7.4 Sending Binding Updates
The double asterisk (**) is defined as "only if the value in the OPTION_F_BINDING_STATUS
is associated with a timeout".  I don't see a description of which status vaues
are associated with a timeout.  Does this mean that a timeout happened? or could
happen?  From the use I think this means status == ACTIVE and therefore there is
a time at which this entry would expire, but I don't think that is clear from
the text.

20) 7.5.2 Procesing Binding Updates, para 4
This paragraph seems to indicate that a BNDACK doesn't need to include
all of the OPTION_CLIENT_DATA options from the BNDUPD.  From the other
paragraphs it sounds like the BNDACK should contain all of them indicating
either success or failure.  In further reading 7.6 does state that the BNDACK
contains an option for each option in the BNDUPD so perhaps update the phrase
"each of which corresponds to one of the OPTION_CLIENT_DATA options"
to something like:
"one for each of the OPTION_CLIENT_DATA options"

21) Bullet item 6:
This states that the partner-lifetime can be updated from a BNDUPD.  In 7.3
the partner-lifetime is described as "The time that we will send (or have
sent) the partner...".  I think the text in 7.3 may need to update a bit
to indicate that the partner can update us as well.

22) 7.6 Sending Binding Acks
It occurs to me that this text and the previous text about IA_NAs and
IA_PDs doesn't say if multiple options for a single client  MUST or SHOULD
be in a single OPTION_CLIENT_DATA or if the server is free to put them
in multiple OPTION_CLIENT_DATA options (or even in separate BNDUPDs).
I'm not sure if the protocol should require any behavior but it might
be useful to at least point out the possibility.

23) 7.7 Receiving Binding Acks
Is there any processing for receving a NAK?  or should there be
text saying if a NAK is recieved don't do anything?

I also don't understand why the partner-lifetime is set to 0.  Given
the acked-partner-lifetime is set to it, retaining partner-lifetime doesn't
seem necessary but it doesn't seem necessary to 0 it either.  Is this
to provide a way to know if / when the partner will need to be updated if
it isn't 0?

24) 7.9 BNDUPD/ BNDACK Data Flow, Figure 4
At the bottom of the figure is the state expiration line in BNDACK.
I assume that there is nothing under storage on receiving side as
that server doesn't update the state-expiration-time.  Perhaps
explicitly mentioning that would be clearer?  Something like 
"no change" or "no update"?

25) 8.1 State Machine Operation
The last paragraph before the diagram mentions "+" and "-".  It would
be best to specify what each of them mean (I assume "-" means no
communication and "+" means communication.  I would also add a third
to mean both.  Some of the boxes include "-|+" while others simply
use "+-", having a single character would probably be clearer.

26) 8.3.2
This section makes use of PREVIOUS-STATE and CURRENT-STATE without
a definition of them.  I believe they are intended to be the variables
for those items? 

There seems to be some inconsistency in the use of PREVIOUS-STATE &
CURRENT-STATE and previous state & current state.  See step 2 para 2,
step 5 para2, step 5 para 4

27) In Step 6
Should the server proceed to transition from PREVIOUS-STATE to the
next state due to communications interrupted?  Or perhaps the text
from step 2 should be moved to here?  I don't see what having step
2 is doing.  In step 5 there is a mention of taking the communications
OK transition, so isn't step 6 the only other case?

28) 8.7.2 Transition out of RECOVER-DONE State
The text doesn't seem to match the diagram.  The text only mentions
transitioning when the partner is NORMAL or RECOVER-DONE.  The diagram
also (seems to) has (have) transitions for: RECOVER & RECOVER-WAIT to
COMMUNICATIONS-INTERRUPTED, POTENTIAL-CONFLICT to POTENTIAL-CONFLICT.

29) 8.8.1 Operation in NORMAL State
This note is for the NORMAL box in the diagram.  The box has (responsive)
which would generally indicate that both servers are responsive while
only the primary is.

30) 8.9 COMMUNICATIONS-INTERRUPTED State
Para 2 mentions starting a timer if configured for auto-partner down.
I assume this is the "Safe Period Timer" from the diagram?  If so
mentioning that name in the text would be clearer.  (Something like
"...then a timer (the Safe Period Timer from the Figure 5) is started...")

31) 9.3 Adding RRs to the DNS
There is a use case for doing DDNS updates on all lease renewals.
If there is a concern that the DNS might be damaged in some way
then having the DHCP server update it on each renew will tend
to heal it of breakages.

32) 9.4 Deleting RRs from the DNS
Is there any concern about the server making the resource FREE* crashing
after receiving the BNDACK but before doing the DDNS update?  I think
the end result of that would be the AAAA and PTR records still in the
DNS until the address is reused (for the PTR) and the client gets
a new address (for the AAAA) or possibly the server recovers and finishes
the cleanup.

---

From: Kim Kinnear <[email-masked]>
Date: Fri, 4 Dec 2015 17:32:28 -0500

Shawn,

First, let me thank you for this truly high quality review.  You have
a taken the time to understand what we are trying to say and have
found a number of places where the draft is inconsistent or misleading
or missing something important.  This takes a tremendous amount of
effort, and yet without several reviews of this quality we stood
little chance of pulling this draft together.  Between your review and
Bernie's my confidence in this document has gone up considerably.
Thank you again!!  We really needed this review!

More comments inline...

> On Dec 3, 2015, at 2:29 PM, Shawn Routhier <[email-masked]> wrote:
> 

	I'm not in love with the term delegable prefix, for what that
	is worth, but it is the best term I've yet found.

	I was using delegable prefix to refer both to a prefix from
	which another prefix might be delegated, as well as a prefix
	which was itself delegated.  I didn't think this was
	particularly confusing, though as I said above, the entire
	term is a bit awkward for me.

> or prefixes may be leased but it could be read as simply the prefixes

	Thanks, I'll clear this up.
> else.  (Or maybe it was in the requirements document and I missed it?)

	Good idea, except that recently Bernie suggested that perhaps they
	*should* be included, because of the direction of some of the
	privacy drafts (or maybe RFC's). So I think we may well be 
	including temporary addresses.
> be worth spending some time to think if odd things could happen.

	I will do so.  I think the mixing in v4 was more likely to be
	an issue than the way we are doing it in this draft, and I
	haven't seen any issues in the v4 space over this.
> Should it also mention expired leases? declines?

	The term "released" isn't well used here -- it most certainly
	means expired and declined.  I'll fix this.
> to state that low-order bit within a prefix is set or cleared? 

	For sure, yes, 127.  Thanks!
> set of prefixes for processing?

	I'm not sure where you are going with this.  I see a prefix as
	either including the entire set of IP addresses which are
	possible with that prefix, or perhaps a prefix has a range or
	perhaps a set of ranges of addresses associated with it.
	Delegated prefixes come with *all* of the addresses possible
	with them.  Configured prefixes get whatever your
	configuration approach specifies.

	I don't see that we need to specify the way that prefixes are
	configured, but perhaps I'm missing something.

> Does the spec need to either limit this in some fashion or supply some

	I don't see why it would need to say anything about it.  We
	are trying to stay out of the configuration space with this
	document, for a variety of reasons.
> prefixes a server can delegate?

	I would expect so, and it could be fixed or it could have maximum
	and minimum lengths.
> in communications interrupted from running out of prefixes.

	You've lost me here.  If the server isn't in partner-down
	then the secondary *could* run out of prefixes to delegate,
	but I'd think that is going to be pretty unlikely given the
	way people through around DHCPv6 prefixes, but it is possible.
> BINDUPDs that need to be sent.

	Good catch, I'll fix the text to make it clear that it isn't
	associated with completing the request.
> of some other arrangement?

	No, thanks for catching that!
> see it but maybe I missed it.  (Not the option code but the actual version

	Er, no.  I should ensure that it does.
> or simply adding some text.

	This comes up again.  Actually, there are some states other
	than ACTIVE that can have a lifetime.  We have found over
	the years that ABANDONED also needs to have a lifetime, though
	we didn't start out thinking so.  Experience in the real world
	has led us down that path, and expired also has a lifetime in
	our world.  As these might not have a lifetime in some implementations,
	I'm thinking about how to allow sufficient flexibility for
	implementations while still having a standard failover protocol.
	I don't have a good answer to this as yet.
> how one of them will determine which of those endpoints the other is

	What a great catch!  Thanks!  I used to think multiple
	relationships were a great way to let people do whatever they
	wanted, but experience has shown that you get headaches and
	ultimately, less availability, when you allow more than one
	relationship per server.  At least with our v4 failover
	customers.  We moved away from multiple relationships years
	ago and have never looked back, so I don't typically think
	about multiple relationships.  And here is where I really left
	out the key to having multiple relationships -- some way to
	define them.  I'll fix this.  Wow, I can't believe I missed
	this!
> It sounds like the the two servers can use different versions of the protocol,

	I was planning to leave the version negotiation until we had
	a second version.  Once there was a second version, then we could
	see if it would make sense to work cross-version or what.  But
	as long as we *have* a version, it should be possible to defer
	that decision.  But I'm open to discussion on that.
> to the failover endpoint sending the STATE message and find "this failover

	I'll fix this.
> "...any message is transmitted on this connection...".

	I don't see this as necessary.  If you care, could you motivate
	this a bit more?
> always requiring the option and disconnecting if it isn't in the CONNECT

	I will make them required, nice catch!
> ...and the MCLT has passed since the entry in RELEASED, EXPIRED, or RESET states.

	For sure.
> addresses are always handled via Independent Allocation rather than

	Leftovers from previous drafts.  I'll fix this.
> 
> 18) 7.3 Times Required for Exchanging Binding Updates
> partner-raw-clt-time

	Will do.
> handed to the client?  If so some text to clarify that would be useful.
	
	Above, I discussed that active isn't the only timed state.
> a time at which this entry would expire, but I don't think that is clear from

	This is the crux of the "which states are timed states" issue.
	I will drive a stake in the ground when I update the draft and
	say that these states are timed.  This is exactly what I hoped
	people would find -- the things missing altogether from the
	draft.
> "one for each of the OPTION_CLIENT_DATA options"

	Will do.  Of course, Bernie has noted that not all information 
	appears in OPTION_CLIENT_DATA options because of prefix delegation
	and the need to exchange information on prefixes that are not
	associated with any client.  So that needs to be figured out.
> to indicate that the partner can update us as well.

	Ok.
> be useful to at least point out the possibility.

	I'm going to defer a definitive response here.  I think I need to
	do a little research on the bulk leasequery draft and see where
	it goes with this.  But I will say *something* more in this area.
> text saying if a NAK is recieved don't do anything?

	Interesting question.  I will have to think on this too.  I
	don't see anything in the draft that we do on a NAK, but I
	will give it some more thought.  In either case, I'll add text
	with the answer to your question!
> to provide a way to know if / when the partner will need to be updated if

	In some cases, partner-lifetime is used as a flag that you need
	to send something to the partner.  Once you have done that and
	they got it, it makes sense to clear it.
> "no change" or "no update"?

	Sure.
> use "+-", having a single character would probably be clearer.

	Maybe "*"?
> a definition of them.  I believe they are intended to be the variables

	I'll discuss these locals/variables.
> step 5 para2, step 5 para 4

	Nice catch, I'll fix this.
> OK transition, so isn't step 6 the only other case?

	Hmmm.  I think the point of this discussion is that step
	2 sets the value of PREVIOUS-STATE, but the transition to
	that state is in (well, could be in) step 6.
> COMMUNICATIONS-INTERRUPTED, POTENTIAL-CONFLICT to POTENTIAL-CONFLICT.

	Wow, yes.  I will fix the text.
> which would generally indicate that both servers are responsive while

	Good catch.  I'll fix that.
> "...then a timer (the Safe Period Timer from the Figure 5) is started...")

	Thanks. I thought I had expunged "Safe Period".  I'll
	fix this.
> then having the DHCP server update it on each renew will tend

	For sure, I'll mention that.
> a new address (for the AAAA) or possibly the server recovers and finishes

	That is an issue, and I think it depends on the implementation
	and how it records whether a DNS removal need to be done.  I need
	to think about this one a bit more before I have a good answer 
	for you.

	Thanks again for a great review!

	Kim

> _______________________________________________
> dhcwg mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/dhcwg

---

From: Shawn Routhier <[email-masked]>
Date: Mon, 7 Dec 2015 21:54:41 -0800

> On Dec 4, 2015, at 2:32 PM, Kim Kinnear <[email-masked]> wrote:
> 
> Shawn,
> 
> First, let me thank you for this truly high quality review.  

You’re welcome.  I should also point out that, while I had a number of comments
the draft seems to be in pretty good shape to start with.

> More comments inline…

I’ve snipped out things that seemed clear.

> 	term is a bit awkward for me.

Where it might get confusing is in 4.2.2 or in the BNDUPD.  I think in those
areas the text is referring to a single prefix and the intention is to have the
servers pass a number of single prefixes back and forth 1 per BNDUPD.
By using an unqualified delegable prefix in the text it could be read to mean
that the primary can send a number of prefixes by aggregating them.  For example
the primary could send 1 update
XXXX:0:1:0/60
or it could send 16 updates
XXXX:0:1:0/64, XXXX:0:1:1/64, … XXXX:0:1:F/64

> 	including temporary addresses.

Either including them or a note as to why they aren’ t included works for me.

> 	configured, but perhaps I'm missing something.

It may be a nit - what I was thinking about is that the term “prefix” tends to mean
a single block of addresses as in XXXX::/64.  So there could be some confusion
that the protocol requires configuration based on that style.  From your reply that
isn’t your intent so using something like 
  “For each address that can be allocated, the primary server MUST allocate…”
Which avoids any discussion about how the addresses are arranged.  

> 	document, for a variety of reasons.

That should be okay, it will simply be up to the product documentation to
point out that having a really large number of prefixes might suck up a lot
of memory.

> 	way people through around DHCPv6 prefixes, but it is possible.

There is a rewind optimization in some versions of the v4 failover protocol. 
 It basically allows a secondary server that is in communications-interrupted
to “rewind” a lease to whatever the primary last knew.  So if the lease was
in backup the secondary could hand the lease out, then it could expire and
the secondary could reuse it.  This assumes that the secondary is in
communications-interrupted for a while.  

The v6 failover seems to be going more towards the auto-partner down style
where a secondary will move to partner-down more quickly, so this may not
be a useful optimization.  After thinking about it more, the right thing is probably
to leave the rewind optimization out and see how the protocol goes and how
it gets used.  If it becomes useful in the future it can be added then and we can
avoid extra complications that might not be necessary now.

> 	I don't have a good answer to this as yet.

Yes I can see that abandoned at least might have a lifetime.  By expired
having a lifetime do you mean that the previous lease information (for
example client id) is still associated with the address?

I haven’t thought about what it might mean for the protocol to handle
that but adding a bit of text to 5.4.20 would to explain that  bit would
probably be useful.

> 	define them.  I'll fix this.  Wow, I can't believe I missed

I understand that some of our customers and the customers of our customers do 
use the multiple relationship feature.  Unfortunately I have no way of knowing how 
many (or what percentage) of our users do so, so I can’t really say how required it
might be.

> 	that decision.  But I'm open to discussion on that.

You’re probably correct that we can defer the decision.  I think we would
probably want to be somewhat compatible so version X and version X+1 
can run for long enough to sync at least but that’s a discussion for later.
(At least some of our users upgrade the product versions by stopping one
server and restarting it with the new version and then doing the same for
the second.)
> 	I don't see this as necessary.  If you care, could you motivate

The way the text is currently written the requirement that the message be on
this connection is not explicit.  It could be read that “every time any message 
is transmitted” means any message on any connection.  From the context this
doesn’t make much sense but adding the extra words makes it clear.  I don’t
feel strongly about this, I’m just trying to remove any items that may cause
people to trip.

> 	it goes with this.  But I will say *something* more in this area.

That’s fine, I’m not sure what I think the “correct answer” is either.

> 	they got it, it makes sense to clear it.

Perhaps a brief (1 line at most) note?

> 	Maybe "*”?

that would work for me.  
>> 27) In Step 6 (8.3.2)

I think I was mis-reading step 2 & 5

Shawn