jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/393097 )
Change subject: Support per-service-IP BGP MED values
......................................................................
Support per-service-IP BGP MED values
Add a per-service 'bgp-med' configuation option that allows overriding
the global MED value per service.
Note that as multiple services may share a service IP (e.g. port 80 and
443), the MED values are required to be equal.
These changes also add scaffolding for later support for announcing and
retracting service IPs based on the service health.
Basic test cases are added for the refactored class BGPFailover
Bug: T165764
Change-Id: I08687e8072dd8f2ae88833a1660d154ae28c7add
---
M pybal/bgpfailover.py
M pybal/ipvs.py
M pybal/main.py
A pybal/test/test_bgpfailover.py
4 files changed, 259 insertions(+), 52 deletions(-)
Approvals:
Mark Bergsma: Looks good to me, approved
jenkins-bot: Verified
diff --git a/pybal/bgpfailover.py b/pybal/bgpfailover.py
index 1b6865f..9a50ec9 100755
--- a/pybal/bgpfailover.py
+++ b/pybal/bgpfailover.py
@@ -11,10 +11,7 @@
from twisted.internet.error import CannotListenError
from pybal.util import log
-try:
- from pybal.bgp import bgp
-except ImportError:
- pass
+from pybal.bgp import bgp
class BGPFailover:
@@ -22,51 +19,51 @@
prefixes = {}
peerings = {}
+ ipServices = {}
def __init__(self, globalConfig):
+ # Store globalconfig so setup() can check whether BGP is enabled.
+ self.globalConfig = globalConfig
if not globalConfig.getboolean('bgp', False):
return
- self.globalConfig = globalConfig
- self.setup()
+ self._parseConfig()
+
+ def _parseConfig(self):
+ self.myASN = self.globalConfig.getint('bgp-local-asn')
+ self.asPath = self.globalConfig.get('bgp-as-path', str(self.myASN))
+ self.asPath = [int(asn) for asn in self.asPath.split()]
+
+ self.defaultMED = self.globalConfig.getint('bgp-med', 0)
+
+ try:
+ self.nexthopIPv4 = self.globalConfig['bgp-nexthop-ipv4']
+ except KeyError:
+ if (bgp.AFI_INET, bgp.SAFI_UNICAST) in BGPFailover.prefixes:
+ raise ValueError("IPv4 BGP NextHop (global configuration
variable 'bgp-nexthop-ipv4') not set")
+
+ try:
+ self.nexthopIPv6 = self.globalConfig['bgp-nexthop-ipv6']
+ except KeyError:
+ if (bgp.AFI_INET6, bgp.SAFI_UNICAST) in BGPFailover.prefixes:
+ raise ValueError("IPv6 BGP NextHop (global configuration
variable 'bgp-nexthop-ipv6') not set")
+
+ bgpPeerAddress = self.globalConfig.get('bgp-peer-address', '').strip()
+ if not bgpPeerAddress.startswith('['):
+ bgpPeerAddress = "[ \"{}\" ]".format(bgpPeerAddress)
+ self.peerAddresses = eval(bgpPeerAddress)
+ assert isinstance(self.peerAddresses, list)
def setup(self):
+ if not self.globalConfig.getboolean('bgp', False):
+ return
+
try:
- myASN = self.globalConfig.getint('bgp-local-asn')
- asPath = self.globalConfig.get('bgp-as-path', str(myASN))
- asPath = [int(asn) for asn in asPath.split()]
- med = self.globalConfig.getint('bgp-med', 0)
- baseAttrs = [bgp.OriginAttribute(), bgp.ASPathAttribute(asPath)]
- if med: baseAttrs.append(bgp.MEDAttribute(med))
+ advertisements = self.buildAdvertisements()
- attributes = {}
- try:
- attributes[(bgp.AFI_INET, bgp.SAFI_UNICAST)] =
bgp.FrozenAttributeDict(baseAttrs + [
-
bgp.NextHopAttribute(self.globalConfig['bgp-nexthop-ipv4'])])
- except KeyError:
- if (bgp.AFI_INET, bgp.SAFI_UNICAST) in BGPFailover.prefixes:
- raise ValueError("IPv4 BGP NextHop (global configuration
variable 'bgp-nexthop-ipv4') not set")
-
- try:
- attributes[(bgp.AFI_INET6, bgp.SAFI_UNICAST)] =
bgp.FrozenAttributeDict(baseAttrs + [
- bgp.MPReachNLRIAttribute((bgp.AFI_INET6, bgp.SAFI_UNICAST,
-
bgp.IPv6IP(self.globalConfig['bgp-nexthop-ipv6']), []))])
- except KeyError:
- if (bgp.AFI_INET6, bgp.SAFI_UNICAST) in BGPFailover.prefixes:
- raise ValueError("IPv6 BGP NextHop (global configuration
variable 'bgp-nexthop-ipv6') not set")
-
- advertisements = set([bgp.Advertisement(prefix, attributes[af], af)
- for af in attributes.keys()
- for prefix in BGPFailover.prefixes.get(af,
set())])
-
- bgpPeerAddress = self.globalConfig.get('bgp-peer-address',
'').strip()
- if bgpPeerAddress[0] != '[': bgpPeerAddress = "[ \"{}\"
]".format(bgpPeerAddress)
- peerAddresses = eval(bgpPeerAddress)
- assert isinstance(peerAddresses, list)
-
- for peerAddr in peerAddresses:
- peering = bgp.NaiveBGPPeering(myASN, peerAddr)
- peering.setEnabledAddressFamilies(set(attributes.keys()))
+ for peerAddr in self.peerAddresses:
+ peering = bgp.NaiveBGPPeering(self.myASN, peerAddr)
+ peering.setEnabledAddressFamilies(set(self.prefixes.keys()))
peering.setAdvertisements(advertisements)
log.info("Starting BGP session with peer {}".format(peerAddr))
@@ -102,13 +99,53 @@
peering.setAdvertisements(set())
return peering.manualStop()
- @classmethod
- def addPrefix(cls, prefix):
- try:
- if ':' not in prefix:
- cls.prefixes.setdefault((bgp.AFI_INET, bgp.SAFI_UNICAST),
set()).add(bgp.IPv4IP(prefix))
+ def buildAdvertisements(self):
+ baseAttrs = bgp.AttributeDict([bgp.OriginAttribute(),
bgp.ASPathAttribute(self.asPath)])
+
+ advertisements = set()
+ for af in self.prefixes:
+ afAttrs = bgp.AttributeDict(baseAttrs)
+ if af[0] == (bgp.AFI_INET):
+ afAttrs[bgp.NextHopAttribute] =
bgp.NextHopAttribute(self.nexthopIPv4)
+ elif af[0] == (bgp.AFI_INET6):
+ afAttrs[bgp.MPReachNLRIAttribute] =
bgp.MPReachNLRIAttribute((af, bgp.IPv6IP(self.nexthopIPv6), []))
else:
- cls.prefixes.setdefault((bgp.AFI_INET6, bgp.SAFI_UNICAST),
set()).add(bgp.IPv6IP(prefix))
- except NameError:
- # bgp not imported
- pass
+ raise ValueError("Unsupported address family {}".format(af))
+
+ for prefix in self.prefixes[af]:
+ attributes = bgp.AttributeDict(afAttrs)
+ # This service IP may use a non-default MED
+ med = self.ipServices[prefix][0]['med'] # Guaranteed to exist,
may be None
+ if med is None:
+ attributes[bgp.MEDAttribute] =
bgp.MEDAttribute(self.defaultMED)
+ else:
+ attributes[bgp.MEDAttribute] = bgp.MEDAttribute(med)
+
+ attributes = bgp.FrozenAttributeDict(attributes)
+ advertisements.add(bgp.Advertisement(prefix, attributes, af))
+
+ return advertisements
+
+ @classmethod
+ def associateService(cls, ip, lvsservice, med):
+ if ':' not in ip:
+ af = (bgp.AFI_INET, bgp.SAFI_UNICAST)
+ prefix = bgp.IPv4IP(ip)
+ else:
+ af = (bgp.AFI_INET6, bgp.SAFI_UNICAST)
+ prefix = bgp.IPv6IP(ip)
+
+ # All services need to agree on the same MED for this IP
+ if prefix in cls.ipServices and not med ==
cls.ipServices[prefix][0]['med']:
+ raise ValueError(
+ "LVS service {} MED value {} differs from other MED values for
IP {}".format(
+ lvsservice.name, med, ip))
+
+ service_state = {
+ 'lvsservice': lvsservice,
+ 'af': af,
+ 'med': med
+ }
+
+ cls.ipServices.setdefault(prefix, []).append(service_state)
+ cls.prefixes.setdefault(af, set()).add(prefix)
diff --git a/pybal/ipvs.py b/pybal/ipvs.py
index 8365e50..b8ce182 100644
--- a/pybal/ipvs.py
+++ b/pybal/ipvs.py
@@ -192,9 +192,12 @@
self.ipvsManager.DryRun = configuration.getboolean('dryrun', False)
self.ipvsManager.Debug = configuration.getboolean('debug', False)
- if self.configuration.getboolean('bgp', True):
- # Add service ip to the BGP announcements
- BGPFailover.addPrefix(self.ip)
+ # Per-service BGP is enabled by default but BGP can be disabled
globally
+ if configuration.getboolean('bgp', True):
+ # Pass a per-service(-ip) MED if one is provided
+ med = configuration.get('bgp-med', None)
+ # Associate service ip to this coordinator for BGP announcements
+ BGPFailover.associateService(self.ip, self, med)
self.createService()
diff --git a/pybal/main.py b/pybal/main.py
index 078583b..f204386 100755
--- a/pybal/main.py
+++ b/pybal/main.py
@@ -125,6 +125,7 @@
util.PyBalLogObserver.level = logging.INFO
bgpannouncement = BGPFailover(configdict)
+ bgpannouncement.setup()
# Run the web server for instrumentation
if configdict.getboolean('instrumentation', False):
diff --git a/pybal/test/test_bgpfailover.py b/pybal/test/test_bgpfailover.py
new file mode 100644
index 0000000..e6e19e4
--- /dev/null
+++ b/pybal/test/test_bgpfailover.py
@@ -0,0 +1,166 @@
+# -*- coding: utf-8 -*-
+"""
+ PyBal unit tests
+ ~~~~~~~~~~~~~~~~
+
+ This module contains tests for `pybal.bgpfailover`.
+
+"""
+
+import pybal.bgpfailover
+import pybal.util
+
+from pybal.bgp import bgp
+
+from twisted.internet.error import CannotListenError
+
+import mock
+from .fixtures import PyBalTestCase
+
+class TestBGPFailover(PyBalTestCase):
+ """Test case for `pybal.ipvs.IPVSManager`."""
+
+ def setUp(self):
+ # Reset class attributes
+ pybal.bgpfailover.BGPFailover.prefixes = {}
+ pybal.bgpfailover.BGPFailover.ipServices = {}
+ pybal.bgpfailover.BGPFailover.peerings = {}
+ self.config = pybal.util.ConfigDict({
+ 'bgp': "yes",
+ 'bgp-local-asn': "64666",
+ 'bgp-as-path': "64666 64667",
+ 'bgp-peer-address': "[ \"127.255.255.255\", \"::1\" ]"
+ })
+ self.bgpfailover = pybal.bgpfailover.BGPFailover(self.config)
+
+ def testConstructor(self):
+ config = self.config
+ bgpfailover = self.bgpfailover
+ self.assertEqual(bgpfailover.globalConfig, config)
+
+ # Test config parsing
+ self.assertEqual(bgpfailover.myASN, 64666)
+ self.assertListEqual(bgpfailover.asPath, [64666, 64667])
+
+ self.assertEqual(bgpfailover.defaultMED, 0)
+
+ config['bgp-peer-address'] = "127.255.255.255"
+ bgpfailover = pybal.bgpfailover.BGPFailover(config)
+ self.assertListEqual(bgpfailover.peerAddresses, ["127.255.255.255"])
+ config['bgp-peer-address'] = "[ \"127.255.255.255\" ]"
+ bgpfailover = pybal.bgpfailover.BGPFailover(config)
+ self.assertListEqual(bgpfailover.peerAddresses, ["127.255.255.255"])
+ config['bgp-peer-address'] = "[ \"127.255.255.255\", \"::1\" ]"
+ bgpfailover = pybal.bgpfailover.BGPFailover(config)
+ self.assertListEqual(bgpfailover.peerAddresses, ["127.255.255.255",
"::1"])
+
+ config['bgp-med'] = "10"
+ bgpfailover = pybal.bgpfailover.BGPFailover(config)
+ self.assertEqual(bgpfailover.defaultMED, 10)
+
+ # Test whether missing IPv4/IPv6 nexthops are detected
+ pybal.bgpfailover.BGPFailover.prefixes[(1, 1)] = None
+ self.assertRaises(ValueError, pybal.bgpfailover.BGPFailover, config)
+ config['bgp-nexthop-ipv4'] = "127.1.1.1"
+ pybal.bgpfailover.BGPFailover.prefixes[(2, 1)] = None
+ self.assertRaises(ValueError, pybal.bgpfailover.BGPFailover, config)
+ config['bgp-nexthop-ipv6'] = "::1"
+ pybal.bgpfailover.BGPFailover(config)
+
+ @mock.patch('pybal.bgpfailover.reactor.listenTCP')
+ @mock.patch('pybal.bgpfailover.bgp.NaiveBGPPeering')
+ def testSetup(self, mock_peering, mock_listenTCP):
+ # Test whether setup creates peerings
+ self.bgpfailover.setup()
+ mock_peering.assert_called()
+ mock_listenTCP.assert_called()
+ self.assertSetEqual(set(self.bgpfailover.peerings.keys()),
{"127.255.255.255", "::1"})
+
+ @mock.patch('pybal.bgpfailover.reactor.addSystemEventTrigger')
+ @mock.patch('pybal.bgpfailover.bgp.NaiveBGPPeering')
+ def testSetupStartException(self, mock_peering,
mock_addSystemEventTrigger):
+ # Test exception handling
+ mock_addSystemEventTrigger.side_effect = ValueError("Mock")
+ self.assertRaises(ValueError, self.bgpfailover.setup)
+
+ @mock.patch('pybal.bgpfailover.reactor.listenTCP')
+ @mock.patch('pybal.bgpfailover.bgp.NaiveBGPPeering')
+ def testSetupCannotListen(self, mock_peering, mock_listenTCP):
+ mock_listenTCP.side_effect = CannotListenError(None, None, "Mocked")
+ self.assertRaises(CannotListenError, self.bgpfailover.setup)
+ mock_peering.assert_called()
+
+ def testSetupDisabled(self):
+ # setup should do nothing if bgp is disabled
+ self.config['bgp'] = 'no'
+ bgpfailover = pybal.bgpfailover.BGPFailover(self.config)
+ bgpfailover.setup()
+ self.assertEquals(len(bgpfailover.peerings), 0)
+
+ def testBuildAdvertisements(self):
+ config = self.config
+ config['bgp-nexthop-ipv4'] = "127.1.1.1"
+ config['bgp-nexthop-ipv6'] = "::1"
+ config['bgp-med'] = "10"
+ bgpfailover = pybal.bgpfailover.BGPFailover(config)
+ bgpfailover.associateService('192.168.1.1', None, med=None)
+ bgpfailover.associateService('::1', None, med=20)
+ # bgpfailover.prefixes = {(1, 1): {bgp.IPv4IP('192.168.1.1')},
+ # (2, 1): {bgp.IPv6IP('fe80::1')}}
+ advertisements = bgpfailover.buildAdvertisements()
+ self.assertEquals(len(advertisements), 2)
+ self.assertTrue(all([isinstance(ad, bgp.Advertisement) for ad in
advertisements]))
+ # Check whether the right prefixes are being advertised
+ self.assertSetEqual(
+ {ad.prefix for ad in advertisements},
+ {bgp.IPv4IP('192.168.1.1'), bgp.IPv6IP('::1')})
+ # Test whether the correct MEDs are present in advertisements
+ self.assertSetEqual(
+ {ad.attributes[bgp.MEDAttribute].value for ad in advertisements},
+ {10, 20})
+ # Test whether required attributes are present
+ for ad in advertisements:
+ if ad.addressfamily[0] == bgp.AFI_INET:
+ self.assertTrue(set(ad.attributes.keys()).issuperset(
+ {bgp.OriginAttribute, bgp.ASPathAttribute,
bgp.NextHopAttribute}))
+ elif ad.addressfamily[0] == bgp.AFI_INET6:
+ self.assertTrue(set(ad.attributes.keys()).issuperset(
+ {bgp.OriginAttribute, bgp.ASPathAttribute,
bgp.MPReachNLRIAttribute}))
+
+ bgpfailover.prefixes[(6, 66)] = bgp.IPv6IP('6:66')
+ self.assertRaises(ValueError, bgpfailover.buildAdvertisements)
+
+ def testCloseSession(self):
+ mockPeering = mock.MagicMock(spec=bgp.NaiveBGPPeering)
+ mockPeering.peerAddr = "127.66.66.66"
+ mockPeering.setAdvertisements = mock.MagicMock()
+ mockPeering.manualStop = mock.MagicMock()
+ self.bgpfailover.closeSession(mockPeering)
+ mockPeering.setAdvertisements.assert_called()
+ mockPeering.manualStop.assert_called()
+
+ def testAssociateService(self):
+ bgpfailover = self.bgpfailover
+ mockService = mock.MagicMock()
+
+ ip = bgp.IPv4IP("1.2.3.4")
+ bgpfailover.associateService('1.2.3.4', mockService, None)
+ self.assertSetEqual(bgpfailover.prefixes[(1, 1)], {ip})
+ self.assertEqual(bgpfailover.ipServices[ip][0]['af'], (1, 1))
+ self.assertEqual(bgpfailover.ipServices[ip][0]['med'], None)
+
+ ip = bgp.IPv6IP("1:2:3:4:5:6:7:8")
+ bgpfailover.associateService('1:2:3:4:5:6:7:8', mockService, 66)
+ self.assertSetEqual(bgpfailover.prefixes[(2, 1)], {ip})
+ self.assertEqual(bgpfailover.ipServices[ip][0]['af'], (2, 1))
+ self.assertEqual(bgpfailover.ipServices[ip][0]['med'], 66)
+
+ ip = bgp.IPv4IP("1.2.3.4")
+ bgpfailover.associateService('1.2.3.4', mockService, None)
+ self.assertSetEqual(bgpfailover.prefixes[(1, 1)], {ip})
+ self.assertEqual(bgpfailover.ipServices[ip][1]['af'], (1, 1))
+ self.assertEqual(bgpfailover.ipServices[ip][1]['med'], None)
+
+ # Test whether inequal MED fails
+ self.assertRaises(ValueError,
+ bgpfailover.associateService, "1.2.3.4", mockService, med=99)
--
To view, visit https://gerrit.wikimedia.org/r/393097
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I08687e8072dd8f2ae88833a1660d154ae28c7add
Gerrit-PatchSet: 10
Gerrit-Project: operations/debs/pybal
Gerrit-Branch: master
Gerrit-Owner: Mark Bergsma <m...@wikimedia.org>
Gerrit-Reviewer: Ema <e...@wikimedia.org>
Gerrit-Reviewer: Giuseppe Lavagetto <glavage...@wikimedia.org>
Gerrit-Reviewer: Mark Bergsma <m...@wikimedia.org>
Gerrit-Reviewer: Volans <rcocci...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
