From 4dca7fae87d9bea01e01f786465fdb4d8231dc69 Mon Sep 17 00:00:00 2001 From: elajkat Date: Tue, 6 Oct 2020 13:20:06 +0200 Subject: [PATCH] Keepalived version check [0] introduced keepalived_use_no_track config option which defaults to True. Here I add and extra runtime check to check keepalived version, and only if that version is bigger than 2.0.0 add the no_track option to the keepalived config file. [0] https://review.opendev.org/745641 Change-Id: I16dce5b3faf62ce02535170a1b0e1ad22ee8ac30 Closes-Bug: #1896506 --- neutron/agent/linux/keepalived.py | 24 ++++++- neutron/cmd/runtime_checks.py | 19 +++++ .../tests/unit/agent/linux/test_keepalived.py | 70 +++++++++++++++---- 3 files changed, 99 insertions(+), 14 deletions(-) diff --git a/neutron/agent/linux/keepalived.py b/neutron/agent/linux/keepalived.py index 121cf550886..d428857cc5c 100644 --- a/neutron/agent/linux/keepalived.py +++ b/neutron/agent/linux/keepalived.py @@ -26,6 +26,7 @@ from oslo_utils import fileutils from neutron._i18n import _ from neutron.agent.linux import external_process +from neutron.cmd import runtime_checks as checks from neutron.common import utils VALID_STATES = ['MASTER', 'BACKUP'] @@ -37,10 +38,19 @@ KEEPALIVED_EMAIL_FROM = 'neutron@openstack.local' KEEPALIVED_ROUTER_ID = 'neutron' GARP_PRIMARY_DELAY = 60 HEALTH_CHECK_NAME = 'ha_health_check' +_IS_NO_TRACK_SUPPORTED = None LOG = logging.getLogger(__name__) +def _is_keepalived_use_no_track_supported(): + global _IS_NO_TRACK_SUPPORTED + if _IS_NO_TRACK_SUPPORTED is None: + _IS_NO_TRACK_SUPPORTED = ( + checks.keepalived_use_no_track_support()) + return _IS_NO_TRACK_SUPPORTED + + def get_free_range(parent_range, excluded_ranges, size=PRIMARY_VIP_RANGE_SIZE): """Get a free IP range, from parent_range, of the specified size. @@ -106,7 +116,12 @@ class KeepalivedVipAddress(object): if self.scope: result += ' scope %s' % self.scope if cfg.CONF.keepalived_use_no_track and not self.track: - result += ' no_track' + if _is_keepalived_use_no_track_supported(): + result += ' no_track' + else: + LOG.warning("keepalived_use_no_track cfg option is True but " + "keepalived on host seems to not support this " + "option") return result @@ -129,7 +144,12 @@ class KeepalivedVirtualRoute(object): if self.scope: output += ' scope %s' % self.scope if cfg.CONF.keepalived_use_no_track: - output += ' no_track' + if _is_keepalived_use_no_track_supported(): + output += ' no_track' + else: + LOG.warning("keepalived_use_no_track cfg option is True but " + "keepalived on host seems to not support this " + "option") return output diff --git a/neutron/cmd/runtime_checks.py b/neutron/cmd/runtime_checks.py index af10ba7f7d7..12a9d869972 100644 --- a/neutron/cmd/runtime_checks.py +++ b/neutron/cmd/runtime_checks.py @@ -13,6 +13,8 @@ # License for the specific language governing permissions and limitations # under the License. +import packaging + from neutron_lib import exceptions from oslo_log import log as logging @@ -33,3 +35,20 @@ def dnsmasq_host_tag_support(): except exceptions.ProcessExecutionError: return False return True + + +def keepalived_use_no_track_support(): + cmd = ['keepalived', '--version'] + env = {'LC_ALL': 'C', 'PATH': '/sbin:/usr/sbin'} + + keepalived_with_track = packaging.version.parse("2.0.0") + try: + # keepalived --version returns with stderr only + res = agent_utils.execute(cmd, addl_env=env, log_fail_as_error=False, + return_stderr=True) + # First line is the interesting one here from stderr + version_line = res[1].split('\n')[0] + keepalived_version = packaging.version.parse(version_line.split()[1]) + return keepalived_version >= keepalived_with_track + except exceptions.ProcessExecutionError: + return False diff --git a/neutron/tests/unit/agent/linux/test_keepalived.py b/neutron/tests/unit/agent/linux/test_keepalived.py index 4a46901dc20..2b618208633 100644 --- a/neutron/tests/unit/agent/linux/test_keepalived.py +++ b/neutron/tests/unit/agent/linux/test_keepalived.py @@ -183,12 +183,23 @@ class KeepalivedConfTestCase(KeepalivedBaseTestCase, }""") def test_config_generation(self): + with mock.patch.object( + keepalived, '_is_keepalived_use_no_track_supported', + return_value=True): + config = self._get_config() + self.assertEqual(self.expected, config.get_config_str()) + + def test_config_generation_no_track_not_supported(self): config = self._get_config() - self.assertEqual(self.expected, config.get_config_str()) + self.assertEqual(self.expected.replace(' no_track', ''), + config.get_config_str()) def test_config_with_reset(self): - config = self._get_config() - self.assertEqual(self.expected, config.get_config_str()) + with mock.patch.object( + keepalived, '_is_keepalived_use_no_track_supported', + return_value=True): + config = self._get_config() + self.assertEqual(self.expected, config.get_config_str()) config.reset() self.assertEqual(KEEPALIVED_GLOBAL_CONFIG, config.get_config_str()) @@ -309,10 +320,13 @@ class KeepalivedInstanceRoutesTestCase(KeepalivedBaseTestCase): 20.0.0.0/8 via 2.0.0.2 no_track 30.0.0.0/8 dev eth0 scope link no_track }""" - routes = self._get_instance_routes() - self.assertEqual(expected, '\n'.join(routes.build_config())) + with mock.patch.object( + keepalived, '_is_keepalived_use_no_track_supported', + return_value=True): + routes = self._get_instance_routes() + self.assertEqual(expected, '\n'.join(routes.build_config())) - def test_build_config_without_no_track_option(self): + def _get_no_track_less_expected_config(self): expected = """ virtual_routes { 0.0.0.0/0 via 1.0.0.254 dev eth0 ::/0 via fe80::3e97:eff:fe26:3bfa/64 dev eth1 @@ -320,9 +334,18 @@ class KeepalivedInstanceRoutesTestCase(KeepalivedBaseTestCase): 20.0.0.0/8 via 2.0.0.2 30.0.0.0/8 dev eth0 scope link }""" + return expected + + def test_build_config_no_track_not_supported(self): + routes = self._get_instance_routes() + self.assertEqual(self._get_no_track_less_expected_config(), + '\n'.join(routes.build_config())) + + def test_build_config_without_no_track_option(self): cfg.CONF.set_override('keepalived_use_no_track', False) routes = self._get_instance_routes() - self.assertEqual(expected, '\n'.join(routes.build_config())) + self.assertEqual(self._get_no_track_less_expected_config(), + '\n'.join(routes.build_config())) class KeepalivedInstanceTestCase(KeepalivedBaseTestCase, @@ -386,7 +409,13 @@ class KeepalivedInstanceTestCase(KeepalivedBaseTestCase, self.assertEqual(expected, config.get_config_str()) def test_remove_addresses_by_interface(self): - self._test_remove_addresses_by_interface(" no_track") + with mock.patch.object( + keepalived, '_is_keepalived_use_no_track_supported', + return_value=True): + self._test_remove_addresses_by_interface(" no_track") + + def test_remove_address_by_interface_no_track_not_supported(self): + self._test_remove_addresses_by_interface("") def test_remove_addresses_by_interface_without_no_track(self): cfg.CONF.set_override('keepalived_use_no_track', False) @@ -452,9 +481,18 @@ class KeepalivedVipAddressTestCase(KeepalivedBaseTestCase): class KeepalivedVirtualRouteTestCase(KeepalivedBaseTestCase): def test_virtual_route_with_dev(self): - route = keepalived.KeepalivedVirtualRoute(n_consts.IPv4_ANY, '1.2.3.4', - 'eth0') - self.assertEqual('0.0.0.0/0 via 1.2.3.4 dev eth0 no_track', + with mock.patch.object( + keepalived, '_is_keepalived_use_no_track_supported', + return_value=True): + route = keepalived.KeepalivedVirtualRoute( + n_consts.IPv4_ANY, '1.2.3.4', 'eth0') + self.assertEqual('0.0.0.0/0 via 1.2.3.4 dev eth0 no_track', + route.build_config()) + + def test_virtual_route_with_dev_no_track_not_supported(self): + route = keepalived.KeepalivedVirtualRoute( + n_consts.IPv4_ANY, '1.2.3.4', 'eth0') + self.assertEqual('0.0.0.0/0 via 1.2.3.4 dev eth0', route.build_config()) def test_virtual_route_with_dev_without_no_track(self): @@ -465,8 +503,16 @@ class KeepalivedVirtualRouteTestCase(KeepalivedBaseTestCase): route.build_config()) def test_virtual_route_without_dev(self): + with mock.patch.object( + keepalived, '_is_keepalived_use_no_track_supported', + return_value=True): + route = keepalived.KeepalivedVirtualRoute('50.0.0.0/8', '1.2.3.4') + self.assertEqual('50.0.0.0/8 via 1.2.3.4 no_track', + route.build_config()) + + def test_virtual_route_without_dev_no_track_not_supported(self): route = keepalived.KeepalivedVirtualRoute('50.0.0.0/8', '1.2.3.4') - self.assertEqual('50.0.0.0/8 via 1.2.3.4 no_track', + self.assertEqual('50.0.0.0/8 via 1.2.3.4', route.build_config()) def test_virtual_route_without_dev_without_no_track(self):