Handling concurrent updates to configmap and IP reclamation

[mmerrill3]

Handling concurrent updates to configmap and IP reclamation by multiple weave instances to fix #3722
Signed-off-by: mmerrill3 michael.merrill@vonage.com

[bboreham]

I've pushed your branch to the weaveworks/weave repo so the smoke tests will run.

[bboreham]

Thanks for the PR.
I had an alternative idea, which is to re-check the peer list in the annotation before rmpeer.
That way we will see that someone else has removed the entry, and it does not rely on the sleep being "long enough", which is difficult to get right.

[mmerrill3]

@bboreham, I agree that the sleep is not deterministic. The solution you have come up with makes sense, but I just have one point. The determination that a peer is to be removed was already make within the reclaimRemovedPeers() function, which is called after the sleep. At that time, cml.getPeerList() is called, and the peer is within the annotation for the existing peer list. So, chances are, if we call cml.getPeerList() again shortly after, the annotation will probably still be there.
I think maybe we do what you suggest, but also wrap the new call to cml.getPeerList() and weave.RmPeer() inside a separate cml.LoopUpdate(), so we make sure we have the most recent cml config map version.
Either way is not bullet proof, but you're idea is simpler.
I can work on that...

[mmerrill3]

Hi @bboreham,
I pushed my changes. I don't have permission to the weaveworks/weave branch feature/fix-3722 to push my changes there as well.

[bboreham]

make sure we have the most recent cml config map version

Yes, that's what I meant - re-fetch from the api-server.

Either way is not bullet proof

Here's what I think the failure at #3722 looks like - time goes from top to bottom:

Node A	Node B
write annotation "X:A"
rmpeer X
remove X from peer list
update peer list
remove annotation "X:A"
.	write annotation "X:B"
.	rmpeer X
inconsistent data

The reason why I think it is more bullet-proof is we get this instead:

Node A	Node B
write annotation "X:A"
rmpeer X
remove X from peer list
update peer list
remove annotation "X:A"
.	write annotation "X:B"
.	re-fetch peer list
.	X no longer there - do nothing
.	remove annotation "X:B"

Can you show in a similar notation the sequence(s) where we still get an error?

[bboreham]

Some nits

[bboreham]

Can you think of a sequence where this is false?

[mmerrill3]

if it's false, there's nothing to do from an annotation perspective, so maybe just log it and then proceed to remove the locking annotation.
But, the remove will be a no/op if there's nothing in the list, so the check for contains() is unnecessary, unless we want to catch an unthought of edge case where the peer is really not in the list.
I'd prefer to keep this, but log if the peer is not in the list so we have evidence that this edge case occurred.

[mmerrill3]

Just pushed changed from your feedback @bboreham , thanks for that. I agree with your diagram about what happened.

[bboreham]

Looks good, thanks!