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

@@ -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(

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)

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)