From e96b68f0aaba399dab56da88c26918bf5b3fb519 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 3 May 2016 11:36:08 +0100 Subject: [PATCH 1/5] Don't use persisted data until we are sure it matches --- ipam/allocator.go | 8 ++++++-- test/380_ipam_seed_2_test.sh | 8 ++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/ipam/allocator.go b/ipam/allocator.go index 741390fa..46fcf4e3 100644 --- a/ipam/allocator.go +++ b/ipam/allocator.go @@ -876,11 +876,13 @@ func (alloc *Allocator) loadPersistedData() { if err != nil { alloc.fatalf("Error loading persisted peer name: %s", err) } - ringFound, err := alloc.db.Load(ringIdent, &alloc.ring) + var persistedRing *ring.Ring + ringFound, err := alloc.db.Load(ringIdent, &persistedRing) if err != nil { alloc.fatalf("Error loading persisted IPAM data: %s", err) } - ownedFound, err := alloc.db.Load(ownedIdent, &alloc.owned) + var persistedOwned map[string]ownedData + ownedFound, err := alloc.db.Load(ownedIdent, &persistedOwned) if err != nil { alloc.fatalf("Error loading persisted address data: %s", err) } @@ -891,9 +893,11 @@ func (alloc *Allocator) loadPersistedData() { if len(alloc.seed) != 0 { alloc.infof("Found persisted IPAM data, ignoring supplied IPAM seed") } + alloc.ring = persistedRing alloc.space.UpdateRanges(alloc.ring.OwnedRanges()) } if ownedFound { + alloc.owned = persistedOwned for _, d := range alloc.owned { for _, cidr := range d.Cidrs { alloc.space.Claim(cidr.Addr) diff --git a/test/380_ipam_seed_2_test.sh b/test/380_ipam_seed_2_test.sh index 3f18b41b..82184a8e 100755 --- a/test/380_ipam_seed_2_test.sh +++ b/test/380_ipam_seed_2_test.sh @@ -19,4 +19,12 @@ weave_on $HOST2 connect $HOST1 assert_raises "exec_on $HOST1 c1 $PING c2" assert_raises "exec_on $HOST2 c2 $PING c1" +# Now restart one router with a different peername +docker_on $HOST2 rm -f c2 >/dev/null +weave_on $HOST2 forget $HOST1 +weave_on $HOST2 stop +weave_on $HOST2 launch --name ::3 +# Check that this host has not retained the previous IPAM data +assert "weave_on $HOST2 status ipam" "" + end_suite From 61fc5d56fb75bc22772946d97bf0dc156a4ee204 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 3 May 2016 11:59:59 +0100 Subject: [PATCH 2/5] Refactor: move initialisation in the case that we don't use persisted data out from loadPersistedData(). --- ipam/allocator.go | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/ipam/allocator.go b/ipam/allocator.go index 46fcf4e3..a8b14fe8 100644 --- a/ipam/allocator.go +++ b/ipam/allocator.go @@ -113,7 +113,20 @@ func NewAllocator(config Config) *Allocator { // Start runs the allocator goroutine func (alloc *Allocator) Start() { - alloc.loadPersistedData() + if alloc.loadPersistedData() { + if len(alloc.seed) != 0 { + alloc.infof("Found persisted IPAM data, ignoring supplied IPAM seed") + } + } else { + if len(alloc.seed) != 0 { + alloc.infof("Initialising with supplied IPAM seed") + alloc.createRing(alloc.seed) + } else if alloc.paxos.IsElector() { + alloc.infof("Initialising via deferred consensus") + } else { + alloc.infof("Initialising as observer - awaiting IPAM data from another peer") + } + } actionChan := make(chan func(), mesh.ChannelSize) stopChan := make(chan struct{}) alloc.actionChan = actionChan @@ -870,7 +883,8 @@ func (alloc *Allocator) persistRing() { } } -func (alloc *Allocator) loadPersistedData() { +// Returns true if persisted data is to be used, otherwise false +func (alloc *Allocator) loadPersistedData() bool { var checkPeerName mesh.PeerName nameFound, err := alloc.db.Load(nameIdent, &checkPeerName) if err != nil { @@ -890,9 +904,6 @@ func (alloc *Allocator) loadPersistedData() { if nameFound { if checkPeerName == alloc.ourName { if ringFound { - if len(alloc.seed) != 0 { - alloc.infof("Found persisted IPAM data, ignoring supplied IPAM seed") - } alloc.ring = persistedRing alloc.space.UpdateRanges(alloc.ring.OwnedRanges()) } @@ -904,22 +915,14 @@ func (alloc *Allocator) loadPersistedData() { } } } - return + return true } alloc.infof("Deleting persisted data for peername %s", checkPeerName) alloc.persistRing() alloc.persistOwned() } - if len(alloc.seed) != 0 { - alloc.infof("Initialising with supplied IPAM seed") - alloc.createRing(alloc.seed) - } else if alloc.paxos.IsElector() { - alloc.infof("Initialising via deferred consensus") - } else { - alloc.infof("Initialising as observer - awaiting IPAM data from another peer") - } - + return false } func (alloc *Allocator) persistOwned() { From fb3ed9c1a684c05b73bb5fc7a399e162f45c559a Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 3 May 2016 12:26:17 +0100 Subject: [PATCH 3/5] Re-organise checking of persisted data so the failure paths bail out of the function first. Also bail out if there is no persisted ring, since persisted 'owned' data isn't meaningful in that state. --- ipam/allocator.go | 42 ++++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/ipam/allocator.go b/ipam/allocator.go index a8b14fe8..d708702a 100644 --- a/ipam/allocator.go +++ b/ipam/allocator.go @@ -901,28 +901,34 @@ func (alloc *Allocator) loadPersistedData() bool { alloc.fatalf("Error loading persisted address data: %s", err) } - if nameFound { - if checkPeerName == alloc.ourName { - if ringFound { - alloc.ring = persistedRing - alloc.space.UpdateRanges(alloc.ring.OwnedRanges()) - } - if ownedFound { - alloc.owned = persistedOwned - for _, d := range alloc.owned { - for _, cidr := range d.Cidrs { - alloc.space.Claim(cidr.Addr) - } - } - } - return true - } - alloc.infof("Deleting persisted data for peername %s", checkPeerName) + overwritePersisted := func(fmt string, args ...interface{}) { + alloc.infof(fmt, args...) alloc.persistRing() alloc.persistOwned() } - return false + if !nameFound || !ringFound { + overwritePersisted("No valid persisted data") + return false + } + + if checkPeerName != alloc.ourName { + overwritePersisted("Deleting persisted data for peername %s", checkPeerName) + return false + } + + alloc.ring = persistedRing + alloc.space.UpdateRanges(alloc.ring.OwnedRanges()) + + if ownedFound { + alloc.owned = persistedOwned + for _, d := range alloc.owned { + for _, cidr := range d.Cidrs { + alloc.space.Claim(cidr.Addr) + } + } + } + return true } func (alloc *Allocator) persistOwned() { From f6966548ebcc2faf3bf7c46665c1dfbdc5421f21 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 3 May 2016 12:34:32 +0100 Subject: [PATCH 4/5] Delete persisted data when IPAM range changes --- ipam/allocator.go | 5 +++++ test/362_ipalloc_range_change.sh | 14 ++++++++++++++ 2 files changed, 19 insertions(+) create mode 100755 test/362_ipalloc_range_change.sh diff --git a/ipam/allocator.go b/ipam/allocator.go index d708702a..e95e01bb 100644 --- a/ipam/allocator.go +++ b/ipam/allocator.go @@ -917,6 +917,11 @@ func (alloc *Allocator) loadPersistedData() bool { return false } + if persistedRing.Range() != alloc.universe { + overwritePersisted("Deleting persisted data for IPAM range %s; our range is %s", persistedRing.Range(), alloc.universe) + return false + } + alloc.ring = persistedRing alloc.space.UpdateRanges(alloc.ring.OwnedRanges()) diff --git a/test/362_ipalloc_range_change.sh b/test/362_ipalloc_range_change.sh new file mode 100755 index 00000000..d7fab65a --- /dev/null +++ b/test/362_ipalloc_range_change.sh @@ -0,0 +1,14 @@ +#! /bin/bash + +. ./config.sh + +start_suite "IPAM alloc range change" + +weave_on $HOST1 launch --ipalloc-range 10.2.0.0/16 +weave_on $HOST1 prime +weave_on $HOST1 stop +weave_on $HOST1 launch --ipalloc-range 10.3.0.0/16 +# Ensure allocations can proceed +assert_raises "timeout 10 cat <( start_container $HOST1 )" + +end_suite From 5ac8cc74d3799680ef1a88cc475866adecbf3c94 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 3 May 2016 13:51:22 +0100 Subject: [PATCH 5/5] Refactor nested if-statements as switch --- ipam/allocator.go | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/ipam/allocator.go b/ipam/allocator.go index e95e01bb..dfa8a951 100644 --- a/ipam/allocator.go +++ b/ipam/allocator.go @@ -113,19 +113,19 @@ func NewAllocator(config Config) *Allocator { // Start runs the allocator goroutine func (alloc *Allocator) Start() { - if alloc.loadPersistedData() { - if len(alloc.seed) != 0 { - alloc.infof("Found persisted IPAM data, ignoring supplied IPAM seed") - } - } else { - if len(alloc.seed) != 0 { - alloc.infof("Initialising with supplied IPAM seed") - alloc.createRing(alloc.seed) - } else if alloc.paxos.IsElector() { - alloc.infof("Initialising via deferred consensus") - } else { - alloc.infof("Initialising as observer - awaiting IPAM data from another peer") - } + loadedPersistedData := alloc.loadPersistedData() + switch { + case loadedPersistedData && len(alloc.seed) != 0: + alloc.infof("Found persisted IPAM data, ignoring supplied IPAM seed") + case loadedPersistedData: + alloc.infof("Initialising with persisted data") + case len(alloc.seed) != 0: + alloc.infof("Initialising with supplied IPAM seed") + alloc.createRing(alloc.seed) + case alloc.paxos.IsElector(): + alloc.infof("Initialising via deferred consensus") + default: + alloc.infof("Initialising as observer - awaiting IPAM data from another peer") } actionChan := make(chan func(), mesh.ChannelSize) stopChan := make(chan struct{})