From c5b598dd8d4415b014750384325589cb3fe3541c Mon Sep 17 00:00:00 2001 From: Flavio Fernandes Date: Fri, 8 May 2020 17:40:45 -0400 Subject: [PATCH] OVNNB backend: Fix LbDelCommand when using vip param Fix issue in ovn_northbound LbDelCommand where logic for removing vip while using if_exists=True was not doing the right thing. In order to be consistent with as ovn-nbctl [1], this change also introduces a modified behavior. When the last vip is removed, it will also remove the load balancer instance. [1]: https://github.com/ovn-org/ovn/blob/1e2f4aaab67090935bba6347358ad9c56b0868bf/utilities/ovn-nbctl.c#L2870 Closes-Bug: #1877673 Change-Id: Ie8ae8f5efae2afb958ed2010053ff04372cc1c51 (cherry picked from commit 8cb3b56fa9022254d60a15125f86f6e8a7e16041) --- ovsdbapp/schema/ovn_northbound/api.py | 8 ++++-- ovsdbapp/schema/ovn_northbound/commands.py | 17 +++++++----- .../schema/ovn_northbound/test_impl_idl.py | 26 +++++++++++++++++++ 3 files changed, 42 insertions(+), 9 deletions(-) diff --git a/ovsdbapp/schema/ovn_northbound/api.py b/ovsdbapp/schema/ovn_northbound/api.py index 6b9c6a34..62e3ac47 100644 --- a/ovsdbapp/schema/ovn_northbound/api.py +++ b/ovsdbapp/schema/ovn_northbound/api.py @@ -686,11 +686,15 @@ class API(api.API): def lb_del(self, lb, vip=None, if_exists=False): """Remove a load balancer or just the VIP from a load balancer + If all vips of the load balancer are removed, the load balancer + instance will be removed as well. If vip parameter is 'None' + this will cause all vips (and load balancer) to be removed. + :param lb: The name or uuid of a load balancer :type lb: string or uuid.UUID - :param vip: The VIP on the load balancer to match + :param vip: The VIP to match. If None, match all vips :type: string - :param if_exists: If True, don't fail if the port doesn't exist + :param if_exists: If True, don't fail if the lb/vip doesn't exist :type if_exists: boolean """ diff --git a/ovsdbapp/schema/ovn_northbound/commands.py b/ovsdbapp/schema/ovn_northbound/commands.py index fad69cd3..df7b9c44 100644 --- a/ovsdbapp/schema/ovn_northbound/commands.py +++ b/ovsdbapp/schema/ovn_northbound/commands.py @@ -1048,16 +1048,19 @@ class LbDelCommand(cmd.BaseCommand): def run_idl(self, txn): try: lb = self.api.lookup('Load_Balancer', self.lb) - if self.vip: - if self.vip in lb.vips: - if self.if_exists: - return - lb.delkey('vips', self.vip) - else: - lb.delete() except idlutils.RowNotFound: if not self.if_exists: raise + return + if self.vip: + if self.vip in lb.vips: + lb.delkey('vips', self.vip) + elif not self.if_exists: + raise idlutils.RowNotFound(table='Load_Balancer', col=self.vip, + match=self.lb) + # Remove load balancer if vips were not provided or no vips are left. + if not self.vip or not lb.vips: + lb.delete() class LbListCommand(cmd.ReadOnlyCommand): diff --git a/ovsdbapp/tests/functional/schema/ovn_northbound/test_impl_idl.py b/ovsdbapp/tests/functional/schema/ovn_northbound/test_impl_idl.py index d2b19014..826f8c7d 100644 --- a/ovsdbapp/tests/functional/schema/ovn_northbound/test_impl_idl.py +++ b/ovsdbapp/tests/functional/schema/ovn_northbound/test_impl_idl.py @@ -1126,6 +1126,32 @@ class TestLoadBalancerOps(OvnNorthboundTest): self.api.lb_del(utils.get_rand_device_name(), if_exists=True).execute( check_error=True) + def test_lb_del_vip_if_exists(self): + name = utils.get_rand_device_name() + lb = self._lb_add(name, '192.0.2.1', ['10.0.0.1']) + lb2 = self._lb_add(name, '192.0.2.2', ['10.1.0.1']) + self.assertEqual(lb, lb2) + # Remove vip that does not exist. + self.api.lb_del(lb.name, '9.9.9.9', if_exists=True + ).execute(check_error=True) + self.assertIn('192.0.2.1', lb.vips) + self.assertIn('192.0.2.2', lb.vips) + # Remove one vip. + self.api.lb_del(lb.name, '192.0.2.1', if_exists=True + ).execute(check_error=True) + self.assertNotIn('192.0.2.1', lb.vips) + # Attempt to remove same vip a second time with if_exists=True. + self.api.lb_del(lb.name, '192.0.2.1', if_exists=True).execute( + check_error=True) + # Attempt to remove same vip a third time with if_exists=False. + cmd = self.api.lb_del(lb.name, '192.0.2.1', if_exists=False) + self.assertRaises(idlutils.RowNotFound, cmd.execute, check_error=True) + # Remove last vip and make sure that lb is also removed. + self.assertIn('192.0.2.2', lb.vips) + self.api.lb_del(lb.name, '192.0.2.2').execute(check_error=True) + cmd = self.api.lb_del(lb.name) + self.assertRaises(idlutils.RowNotFound, cmd.execute, check_error=True) + def test_lb_list(self): lbs = {self._lb_add(utils.get_rand_device_name(), '192.0.2.1', ['10.0.0.1', '10.0.0.2']) for _ in range(3)}