lxd/bgp: Rework start/stop logic (from Incus) (#15173)

Includes cherry-pick from lxc/incus#1761.

**Changes:**   
- BGP listener is now started after all the networks are set up.
- Predefined network forwards are now added correctly during the BGP
listener start up.
- Extended test/suites/network_forward to test the BGP listener start up
and subsequent restart of LXD daemon.

Author: tomponline

lxd/api_1.0.go

@@ -996,7 +996,7 @@ func doAPI10UpdateTriggers(d *Daemon, nodeChanged, clusterChanged map[string]str
 			return fmt.Errorf("Cannot convert BGP ASN to uint32: Upper bound exceeded")
 		}
 
-		err := s.BGP.Reconfigure(address, uint32(asn), net.ParseIP(routerid))
+		err := s.BGP.Configure(address, uint32(asn), net.ParseIP(routerid))
 		if err != nil {
 			return fmt.Errorf("Failed reconfiguring BGP: %w", err)
 		}

lxd/bgp/server.go

@@ -56,45 +56,21 @@ func NewServer() *Server {
 	return s
 }
 
-func (s *Server) setup() {
-	if s.bgp != nil {
-		return
-	}
-
-	// Spawn the BGP goroutines.
-	s.bgp = bgpServer.NewBgpServer()
-	go s.bgp.Serve()
-
-	// Insert any path that's already defined.
-	if len(s.paths) > 0 {
-		// Reset the path list.
-		paths := s.paths
-		s.paths = map[string]path{}
-
-		for _, path := range paths {
-			err := s.addPrefix(path.prefix, path.nexthop, path.owner)
-			logger.Warn("Unable to add prefix to BGP server", logger.Ctx{"prefix": path.prefix.String(), "err": err})
-		}
-	}
-}
-
 // Start sets up the BGP listener.
-func (s *Server) Start(address string, asn uint32, routerID net.IP) error {
-	// Locking.
-	s.mu.Lock()
-	defer s.mu.Unlock()
-
-	return s.start(address, asn, routerID)
-}
-
 func (s *Server) start(address string, asn uint32, routerID net.IP) error {
 	// If routerID is nil, fill with our best guess.
 	if routerID == nil || routerID.To4() == nil {
 		return ErrBadRouterID
 	}
 
-	// Make sure we have a BGP instance.
-	s.setup()
+	// Check if already running
+	if s.bgp != nil {
+		return fmt.Errorf("BGP listener is already running")
+	}
+
+	// Spawn the BGP goroutines.
+	s.bgp = bgpServer.NewBgpServer()
+	go s.bgp.Serve()
 
 	// Get the address and port.
 	addrHost, addrPort, err := net.SplitHostPort(address)
@@ -131,8 +107,30 @@ func (s *Server) start(address string, asn uint32, routerID net.IP) error {
 		return err
 	}
 
-	// Add any existing peers.
-	for _, peer := range s.peers {
+	// Copy the path list
+	oldPaths := map[string]path{}
+	for pathUUID, path := range s.paths {
+		oldPaths[pathUUID] = path
+	}
+
+	// Add existing paths.
+	s.paths = map[string]path{}
+	for _, path := range oldPaths {
+		err := s.addPrefix(path.prefix, path.nexthop, path.owner)
+		if err != nil {
+			logger.Warn("Unable to add prefix to BGP server", logger.Ctx{"prefix": path.prefix.String(), "err": err})
+		}
+	}
+
+	// Copy the peer list.
+	oldPeers := map[string]peer{}
+	for peerUUID, peer := range s.peers {
+		oldPeers[peerUUID] = peer
+	}
+
+	// Add existing peers.
+	s.peers = map[string]peer{}
+	for _, peer := range oldPeers {
 		err := s.addPeer(peer.address, peer.asn, peer.password, peer.holdtime)
 		if err != nil {
 			return err
@@ -148,20 +146,18 @@ func (s *Server) start(address string, asn uint32, routerID net.IP) error {
 }
 
 // Stop tears down the BGP listener.
-func (s *Server) Stop() error {
-	// Locking.
-	s.mu.Lock()
-	defer s.mu.Unlock()
-
-	return s.stop()
-}
-
 func (s *Server) stop() error {
 	// Skip if no instance.
 	if s.bgp == nil {
 		return nil
 	}
 
+	// Save the peer list.
+	oldPeers := map[string]peer{}
+	for peerUUID, peer := range s.peers {
+		oldPeers[peerUUID] = peer
+	}
+
 	// Remove all the peers (ignore failures).
 	for _, peer := range s.peers {
 		err := s.removePeer(peer.address)
@@ -170,37 +166,38 @@ func (s *Server) stop() error {
 		}
 	}
 
+	// Restore peer list.
+	s.peers = oldPeers
+
 	// Stop the listener.
 	err := s.bgp.StopBgp(context.Background(), &bgpAPI.StopBgpRequest{})
 	if err != nil {
 		return err
 	}
 
-	// Unset the address
+	// Mark the daemon as down.
 	s.address = ""
 	s.asn = 0
 	s.routerID = nil
+	s.bgp = nil
+
 	return nil
 }
 
-// Reconfigure updates the listener with a new configuration..
-func (s *Server) Reconfigure(address string, asn uint32, routerID net.IP) error {
+// Configure updates the listener with a new configuration..
+func (s *Server) Configure(address string, asn uint32, routerID net.IP) error {
 	// Locking.
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
-	return s.reconfigure(address, asn, routerID)
+	return s.configure(address, asn, routerID)
 }
 
-func (s *Server) reconfigure(address string, asn uint32, routerID net.IP) error {
-	// Get the old address.
+func (s *Server) configure(address string, asn uint32, routerID net.IP) error {
+	// Store current configuration for reverting.
 	oldAddress := s.address
 	oldASN := s.asn
 	oldRouterID := s.routerID
-	oldPeers := map[string]peer{}
-	for peerUUID, peer := range s.peers {
-		oldPeers[peerUUID] = peer
-	}
 
 	// Setup reverter.
 	revert := revert.New()
@@ -209,12 +206,9 @@ func (s *Server) reconfigure(address string, asn uint32, routerID net.IP) error
 	// Stop the listener.
 	err := s.stop()
 	if err != nil {
-		return err
+		return fmt.Errorf("Failed to stop current listener: %w", err)
 	}
 
-	// Restore peer list.
-	s.peers = oldPeers
-
 	// Check if we should start.
 	if address != "" && asn > 0 && routerID != nil {
 		// Restore old address on failure.
@@ -223,7 +217,7 @@ func (s *Server) reconfigure(address string, asn uint32, routerID net.IP) error
 		// Start the listener with the new address.
 		err = s.start(address, asn, routerID)
 		if err != nil {
-			return err
+			return fmt.Errorf("Failed to start new listener: %w", err)
 		}
 	}
 

lxd/daemon.go

@@ -1708,18 +1708,6 @@ func (d *Daemon) init() error {
 
 	// Setup BGP listener.
 	d.bgp = bgp.NewServer()
-	if bgpAddress != "" && bgpASN != 0 && bgpRouterID != "" {
-		if bgpASN > math.MaxUint32 {
-			return fmt.Errorf("Cannot convert BGP ASN to uint32: Upper bound exceeded")
-		}
-
-		err := d.bgp.Start(bgpAddress, uint32(bgpASN), net.ParseIP(bgpRouterID))
-		if err != nil {
-			return err
-		}
-
-		logger.Info("Started BGP server")
-	}
 
 	// Setup DNS listener.
 	d.dns = dns.NewServer(d.db.Cluster, func(name string, full bool) (*dns.Zone, error) {
@@ -1769,6 +1757,19 @@ func (d *Daemon) init() error {
 	}
 
 	// Setup tertiary listeners that may use managed network addresses and must be started after networks.
+	if bgpAddress != "" && bgpASN != 0 && bgpRouterID != "" {
+		if bgpASN > math.MaxUint32 {
+			return fmt.Errorf("Cannot convert BGP ASN to uint32: Upper bound exceeded")
+		}
+
+		err := d.bgp.Configure(bgpAddress, uint32(bgpASN), net.ParseIP(bgpRouterID))
+		if err != nil {
+			return err
+		}
+
+		logger.Info("Started BGP server")
+	}
+
 	dnsAddress := d.localConfig.DNSAddress()
 	if dnsAddress != "" {
 		err = d.dns.Start(dnsAddress)

test/suites/network_forward.sh

@@ -5,6 +5,10 @@ test_network_forward() {
   firewallDriver=$(lxc info | awk -F ":" '/firewall:/{gsub(/ /, "", $0); print $2}')
   netName=lxdt$$
 
+  lxc network create bgpbr # Bridge to start BGP listener on.
+
+  bgpIP=$(lxc network get bgpbr ipv4.address | cut -d/ -f1)
+
   lxc network create "${netName}" \
         ipv4.address=192.0.2.1/24 \
         ipv6.address=fd42:4242:4242:1010::1/64
@@ -28,6 +32,15 @@ test_network_forward() {
   # Check forward is exported via BGP prefixes.
   lxc query /internal/testing/bgp | grep "198.51.100.1/32"
 
+  # Enable the BGP listener
+  lxc config set core.bgp_address="${bgpIP}:8874"
+  lxc config set core.bgp_asn=65536
+  lxc config set core.bgp_routerid="${bgpIP}"
+
+  # Check that the listener survives a restart of LXD
+  shutdown_lxd "${LXD_DIR}"
+  respawn_lxd "${LXD_DIR}" true
+
   lxc network forward delete "${netName}" 198.51.100.1
 
   # Check deleting network forward removes forward BGP prefix.