From 58fc4dadac4a40ab0d2e78897e0c77ffeeacb051 Mon Sep 17 00:00:00 2001 From: Felipe Reyes Date: Tue, 30 Nov 2021 15:49:40 -0300 Subject: [PATCH] Register previously vip set for deletion. When the vip is changed the ones that are no longer present need to be registered for deletion from pacemaker's configuration. This change relies on hookenv.config.changed() to determine what vip(s) are no longer present in the configuration and ask hacluster to remove them. Closes-Bug: #1952363 Change-Id: I1afe987ff26af0e10604dd507daef4ac282d9aab --- charms_openstack/charm/classes.py | 15 ++++++- .../charms_openstack/charm/test_classes.py | 34 ++++++++++++-- unit_tests/charms_openstack/charm/utils.py | 45 +++++++++++++++++++ 3 files changed, 90 insertions(+), 4 deletions(-) diff --git a/charms_openstack/charm/classes.py b/charms_openstack/charm/classes.py index e863c3b..092b684 100644 --- a/charms_openstack/charm/classes.py +++ b/charms_openstack/charm/classes.py @@ -868,7 +868,20 @@ class HAOpenStackCharm(OpenStackAPICharm): """ if not self.config.get(VIP_KEY): return - for vip in self.config[VIP_KEY].split(): + + vips = self.config[VIP_KEY].split() + + # the `vip` config option has changed and there was a value previously + # set, the principle needs to ask hacluster to remove it from + # pacemaker's configuration (LP: #1952363). + if self.config.changed(VIP_KEY) and self.config.previous(VIP_KEY): + old_vips = self.config.previous(VIP_KEY).split() + vips_to_del = set(old_vips) - set(vips) + for vip in vips_to_del: + hookenv.log("Registering %s for deletion" % vip) + hacluster.remove_vip(self.name, vip) + + for vip in vips: iface, netmask, fallback = os_ha_utils.get_vip_settings(vip) if fallback: hacluster.add_vip( diff --git a/unit_tests/charms_openstack/charm/test_classes.py b/unit_tests/charms_openstack/charm/test_classes.py index 1603440..399f131 100644 --- a/unit_tests/charms_openstack/charm/test_classes.py +++ b/unit_tests/charms_openstack/charm/test_classes.py @@ -2,7 +2,10 @@ import base64 import mock import unit_tests.utils as utils -from unit_tests.charms_openstack.charm.utils import BaseOpenStackCharmTest +from unit_tests.charms_openstack.charm.utils import ( + BaseOpenStackCharmTest, + TestConfig, +) from unit_tests.charms_openstack.charm.common import MyOpenStackCharm import charms_openstack.charm.classes as chm @@ -816,6 +819,7 @@ class TestHAOpenStackCharm(BaseOpenStackCharmTest): super(TestHAOpenStackCharm, self).setUp(make_open_stack_charm, TEST_CONFIG) + self.test_config = TestConfig() def test_all_packages(self): self.patch_target('packages', new=['pkg1']) @@ -895,9 +899,10 @@ class TestHAOpenStackCharm(BaseOpenStackCharmTest): nics = { 'vip1': ('eth1', 'netmask1', False), 'vip2': ('eth2', 'netmask2', False)} + self.test_config.set('vip', 'vip1 vip2') interface_mock = mock.Mock() self.patch_target('name', new='myservice') - self.patch_target('config', new={'vip': 'vip1 vip2'}) + self.patch_target('config', new=self.test_config) self.patch_object(chm.os_ha_utils, 'get_vip_settings') self.get_vip_settings.side_effect = lambda x: nics[x] self.target._add_ha_vips_config(interface_mock) @@ -910,13 +915,36 @@ class TestHAOpenStackCharm(BaseOpenStackCharmTest): mock.call('res_myservice_eth2_vip')] interface_mock.delete_resource.assert_has_calls(add_vip_calls) + def test__add_ha_vips_config_changed(self): + nics = { + 'vip1': ('eth1', 'netmask1', False), + 'vip2': ('eth2', 'netmask2', False)} + self.test_config.set('vip', 'vip1 vip2') + self.test_config.set_previous('vip', 'vip1 vip3') + interface_mock = mock.Mock() + self.patch_target('name', new='myservice') + self.patch_target('config', new=self.test_config) + self.patch_object(chm.os_ha_utils, 'get_vip_settings') + self.get_vip_settings.side_effect = lambda x: nics[x] + self.target._add_ha_vips_config(interface_mock) + add_vip_calls = [ + mock.call('myservice', 'vip1'), + mock.call('myservice', 'vip2')] + interface_mock.add_vip.assert_has_calls(add_vip_calls) + interface_mock.remove_vip.assert_called_with('myservice', 'vip3') + add_vip_calls = [ + mock.call('res_myservice_eth1_vip'), + mock.call('res_myservice_eth2_vip')] + interface_mock.delete_resource.assert_has_calls(add_vip_calls) + def test__add_ha_vips_config_fallback(self): nics = { 'vip1': ('eth1', 'netmask1', True), 'vip2': ('eth2', 'netmask2', True)} + self.test_config.set('vip', 'vip1 vip2') interface_mock = mock.Mock() self.patch_target('name', new='myservice') - self.patch_target('config', new={'vip': 'vip1 vip2'}) + self.patch_target('config', new=self.test_config) self.patch_object(chm.os_ha_utils, 'get_vip_settings') self.get_vip_settings.side_effect = lambda x: nics[x] self.target._add_ha_vips_config(interface_mock) diff --git a/unit_tests/charms_openstack/charm/utils.py b/unit_tests/charms_openstack/charm/utils.py index 0d9561b..2647901 100644 --- a/unit_tests/charms_openstack/charm/utils.py +++ b/unit_tests/charms_openstack/charm/utils.py @@ -42,3 +42,48 @@ class BaseOpenStackCharmTest(unit_tests.utils.BaseTestCase): **kwargs): # uses BaseTestCase.patch_object() to patch targer. self.patch_object(self.target, attr, return_value, name, new, **kwargs) + + +class TestConfig(object): + + def __init__(self): + self.config = {} + self.config_prev = {} + + def __call__(self, key=None): + if key: + return self.get(key) + else: + return self + + def previous(self, k): + return self.config_prev[k] if k in self.config_prev else self.config[k] + + def set_previous(self, k, v): + self.config_prev[k] = v + + def unset_previous(self, k): + if k in self.config_prev: + self.config_prev.pop(k) + + def changed(self, k): + if not self.config_prev: + return True + return self.get(k) != self.previous(k) + + def get(self, attr=None): + if not attr: + return self + try: + return self.config[attr] + except KeyError: + return None + + def get_all(self): + return self.config + + def set(self, attr, value): + self.config[attr] = value + + def __getitem__(self, k): + return self.get(k)