Fast datapath #1438

dpw · 2015-09-15T16:23:44Z

Smoke tests pass (modulo the one that is broken on master), so I think this is fit for a review pass.

Things still to do:

The weave script changes are still a bit grotty. But...
I think there needs to be a "disable it all" option to weave, which disables fastdp, --net=host, and the MTU change.
I haven't modified architecture.txt. To be honest, most of the contents are more like implementation notes that belong as comments in the code. If the fast datapath code requires additional notes of that nature, I'd be happy to add them as comments. A high-level overview of fast datapath would be a good thing, but I think I'll need to re-write the whole of architecture.txt to match.
1.1 compatibility support. That will be a separate issue. Short ids is the tricky part here.
I need to check out a kernel oops I saw when testing with ubuntu 12.04 a few days ago.

router/peers.go

+
+type ShortIDPeers struct {
+	// If we know about a single peer with the short id, this is
+	// that peer.  If there is a coliision, this is the peer with


awh · 2015-09-16T14:56:25Z

Regarding the create/delete datapath & add datapath interface commands added to weaver:

Previously, the proxy would run the weave script in ways that did not run the weaver executable. Now they can.

We've started (for better or worse) to develop a stable of little helper executables under prog/* - might these functions be moved there? If they were, would it ameliorate the proxy coverage issue?

dpw · 2015-09-16T15:10:18Z

We've started (for better or worse) to develop a stable of little helper executables under prog/* - might these functions be moved there? If they were, would it ameliorate the proxy coverage issue?

Where there is no functional overlap, I think having a separate helper program is natural. That's not the case here. And we probably do want coverage on those helper programs, too.

awh · 2015-09-17T11:26:57Z

I have a couple of thoughts regarding the short peer ID allocation code:

Rather than the two fields in ShortIDPeers, could you maintain a single sorted slice holding the peers in order of ascending name? You might save some complexity in addByShortID/deleteByShortID by leveraging the sort package, as well as reducing the amount of test code required to check the invariants associated with the existing peer/others design. (Initially I wondered if you could use container/heap for this but you need to be able to remove peers by name)
At the cost of pre-allocating 4096 ShortIDPeers structs and storing them in a free entries map, you could drastically simplify chooseShortID; even more so if you rely on Go's map iteration randomisation:

func (peers *Peers) chooseShortID() (PeerShortID, bool) {
    for shortID, _ := range peers.freeShortIDs {
        return shortID, true
    }
    return 0, false
}

add/delete then move entries between the free/in-use maps; hopefully this would make the mechanism more 'obviously correct' and obviate the need for the complex unit tests.

router/peer.go

+const PeerShortIDBits = 12
+
+func randomPeerShortID() PeerShortID {
+	return PeerShortID(randUint64() & (1<<PeerShortIDBits - 1))


awh · 2015-09-17T15:15:16Z

Things still to do

Add smoke tests for FDP?

dpw · 2015-09-17T15:23:05Z

Add smoke tests for FDP?

Such as? The existing smoke tests exercise FDP. What functional additions are there to test?

awh · 2015-09-17T15:26:35Z

What functional additions are there to test?

Fallback behaviour.

dpw · 2015-09-17T15:32:36Z

What functional additions are there to test?

Fallback behaviour.

Already there: test/105_frag_2_test.sh

router/overlay_switch.go

+	// less preferred forwarders are no longer needed
+	fwd.stopFrom(index + 1)
+
+	if !fwd.alreadyEstablished {


awh · 2015-09-18T11:45:16Z

What functional additions are there to test?

Fallback behaviour.

Already there: test/105_frag_2_test.sh

You're going to have to help me out - that test isn't modified in the PR, and I can't see how the current version tests fallback. Here's what I think will happen:

Both forwarders will race to establish. They will both succeed, but because fdp is preferred it will end up as the 'best' forwarder and the overlay forwarder will be stopped (probably before it completes PMTU discovery)
The large ping will get fragmented according to the overlay MTU, and as the VXLAN path is unaffected by the iptables rule it will succeed

What am I missing?

dpw · 2015-09-18T11:53:41Z

What am I missing?

The test introduces a iptables rule to discard packets > 700 bytes. fastdp sends a heartbeat packet sized to the overlay MTU (i.e. 1410 bytes). This won't get through, and so fastdp will never report that it is established (and ultimately it will time out and report an error). But sleeve will report that it is established, and so be selected as the active overlay. The test then tests sleeve fragmentation behaviour as before.

awh · 2015-09-18T11:57:50Z

fastdp sends a heartbeat packet sized to the overlay MTU

Argh I did check for that but I took the comment

 587     // the heartbeat payload consists of the 64-bit connection uid
 588     // followed by the 16-bit packet size.

literally and missed the +fwd.fastdp.mtu in the buffer allocation 😄

Thanks!

router/fastdp.go

+
+// The router needs to know which flows are active in order to
+// maintain its MAC->peer table.  We do this by quering the router
+// without an actual packet being involed.  Maybe it's


dpw · 2015-09-20T17:47:27Z

I need to check out a kernel oops I saw when testing with ubuntu 12.04 a few days ago.

Now fixed.

router/fastdp.go

+	fwd := op.fastdp.forwarders[op.srcPeer.Name]
+	op.fastdp.lock.Unlock()
+
+	if fwd != nil || dec.IsSpecial() {


rade · 2015-10-12T16:03:48Z

There seem to be some quite big coverage holes, e.g.

{fastdp,overlap_switch,sleeve} InvalidateShortIDs
fastdp expireMACs,expireFlows,touchFlow

awh · 2015-10-12T16:13:35Z

There seem to be some quite big coverage holes

That's not the problem - in fact, only overlay.go has reduced in coverage due to the addition of eight untested lines. The report is still counting all the lines of the source files in odp, except now it is reporting zero test coverage for them all...

dpw · 2015-10-12T16:15:30Z

There seem to be some quite big coverage holes, e.g.

{fastdp,overlap_switch,sleeve} InvalidateShortIDs

Because none of the smoke tests have nearly enough peers to cause short ID collisions.

fastdp expireMACs,expireFlows,touchFlow

Because we don't run the smoke tests for long enough for flows to be expired (in the same way we never see MAC cache expiries).

At 74% and 78%, the coverage of the two big new files (fastdp.go and overlay_switch.go) is about average.

rade · 2015-10-12T16:19:24Z

Can you think of an easy way to test InvalidateShortIDs?

dpw · 2015-10-12T16:25:59Z

Can you think of an easy way to test InvalidateShortIDs?

No, nothing that isn't a case of Volkswagening.

dpw · 2015-10-12T16:31:52Z

Can you think of an easy way to test InvalidateShortIDs?

No, nothing that isn't a case of Volkswagening.

And note that:

InvalidateShortIDs is just logging and locking around a call to deleteFlows, which is covered.
The unit tests verify that InvalidateShortIDs notifications occur when appropriate.

So it seems reasonable to check the code in InvalidateShortIDs by inspection.

rade · 2015-10-12T17:28:34Z

coverage sorted.

These are needed to support vxlan encapsulation. In a vxlan packet, the only place we can straightforwardly hold the source and destination peer identifiers used by the weave router is in the 24-bit VNI (vxlan network identifier) field. This gives us 12 bits to indicate a peer. The resulting limit of 4096 peers on a weave network does not seem like a problem in the near future. But it does mean that decentralized allocation makes collisions fairly likely even in weave networks of sizes that are common today. Fortunately, the problem is much more tractable than IPAM because we can detect when collisions occur and take action to resolve them.

It's more robust to signal the conditions asynchronously via channels rather than callbacks.

This means: * The 'weave' netdev is an OVS datapath * The weave router container runs with --net=host in order to access the datapath. * The MTU is set to a smaller value, to allow vxlan encapsulation. Setting the WEAVE_NO_FASTDP environment variable disables these things, effectively running the router without fast datapath. It is an environment variable rather than an option to 'weave launch' because it takes effect in create_bridge, which is called from many subcommands, and it doesn't seem practical to add option processing to all of them.

Previously, the proxy would run the weave script in ways that did not run the weaver executable. Now they can. This is a problem because of the contortions involved in using golang's code coverage support in the integration tests, which require the weaver command line args to be slightly mangled.

Partly for the side effect of restoring coverage of router/ethernet_decoder.go

awh · 2015-10-13T09:29:22Z

@dpw @rade I don't think the current short ID discussion in #1522 has any bearing on this PR - any final comments before I press merge?

rade · 2015-10-13T09:32:41Z

merge it

Fast datapath

awh assigned dpw and awh and unassigned dpw Sep 15, 2015

awh reviewed Sep 16, 2015
View reviewed changes

router/peers.go

type ShortIDPeers struct {

// If we know about a single peer with the short id, this is

// that peer. If there is a coliision, this is the peer with

This comment was marked as abuse.

Sign in to view

awh reviewed Sep 17, 2015
View reviewed changes

router/peer.go

const PeerShortIDBits = 12

func randomPeerShortID() PeerShortID {

return PeerShortID(randUint64() & (1<<PeerShortIDBits - 1))

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

awh reviewed Sep 18, 2015
View reviewed changes

dpw mentioned this pull request Sep 18, 2015

fastdp compatibility with 1.1 #1448

Closed

awh reviewed Sep 18, 2015
View reviewed changes

dpw mentioned this pull request Sep 21, 2015

fix sleeve wire compatibility with 1.1 #1396

Closed

awh reviewed Sep 24, 2015
View reviewed changes

router/fastdp.go

fwd := op.fastdp.forwarders[op.srcPeer.Name]

op.fastdp.lock.Unlock()

if fwd != nil || dec.IsSpecial() {

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

dpw added 15 commits October 12, 2015 21:27

Always include local peer in Peers

97fb90c

Handle the not-found case implicitly in Broadcast and BroadcastAll

c6edf84

Tidy broadcastPeerUpdate

0841f4a

applyUpdate can return a PeerNameSet

67bfa5b

Rename Peers "table" field to "byName"

91dda08

Introduce PeersPendingNotifications

51317dc

Replace OverlayForwarderListener with channels

bc5250c

It's more robust to signal the conditions asynchronously via channels rather than callbacks.

OverlaySwitch

f564572

Show the overlay name in sleeve log messsages

bc026ab

Introduce checkFatal in prog/weaver/main.go

e8f975e

Fast datapath

295876e

A smoke test for WEAVE_NO_FASTDP

0093eb1

Partly for the side effect of restoring coverage of router/ethernet_decoder.go

dpw force-pushed the fdp branch from b8187f4 to 0093eb1 Compare October 12, 2015 20:27

awh added a commit that referenced this pull request Oct 13, 2015

Merge pull request #1438 from weaveworks/fdp

827bcd8

Fast datapath

awh merged commit 827bcd8 into master Oct 13, 2015

rade mentioned this pull request Oct 15, 2015

use ovs as an alternative to pcap #36

Closed

rade added this to the 1.2.0 milestone Oct 15, 2015

awh deleted the fdp branch November 9, 2015 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fast datapath #1438

Fast datapath #1438

dpw commented Sep 15, 2015

This comment was marked as abuse.

awh commented Sep 16, 2015

dpw commented Sep 16, 2015

awh commented Sep 17, 2015

This comment was marked as abuse.

This comment was marked as abuse.

awh commented Sep 17, 2015

dpw commented Sep 17, 2015

awh commented Sep 17, 2015

dpw commented Sep 17, 2015

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

awh commented Sep 18, 2015

dpw commented Sep 18, 2015

awh commented Sep 18, 2015

This comment was marked as abuse.

This comment was marked as abuse.

dpw commented Sep 20, 2015

This comment was marked as abuse.

This comment was marked as abuse.

rade commented Oct 12, 2015

awh commented Oct 12, 2015

dpw commented Oct 12, 2015

rade commented Oct 12, 2015

dpw commented Oct 12, 2015

dpw commented Oct 12, 2015

rade commented Oct 12, 2015

awh commented Oct 13, 2015

rade commented Oct 13, 2015

Fast datapath #1438

Fast datapath #1438

Conversation

dpw commented Sep 15, 2015

This comment was marked as abuse.

awh commented Sep 16, 2015

dpw commented Sep 16, 2015

awh commented Sep 17, 2015

This comment was marked as abuse.

This comment was marked as abuse.

awh commented Sep 17, 2015

dpw commented Sep 17, 2015

awh commented Sep 17, 2015

dpw commented Sep 17, 2015

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

awh commented Sep 18, 2015

dpw commented Sep 18, 2015

awh commented Sep 18, 2015

This comment was marked as abuse.

This comment was marked as abuse.

dpw commented Sep 20, 2015

This comment was marked as abuse.

This comment was marked as abuse.

rade commented Oct 12, 2015

awh commented Oct 12, 2015

dpw commented Oct 12, 2015

rade commented Oct 12, 2015

dpw commented Oct 12, 2015

dpw commented Oct 12, 2015

rade commented Oct 12, 2015

awh commented Oct 13, 2015

rade commented Oct 13, 2015