Browse Source

Check for floating ips correctness

This patch is updating networking-ovn to check for correctness when
creating, updating or deleting floating ips.

Partial-Bug: #1605089
Change-Id: I377007c955809b8d56af93e24f0914e446f56bb2
changes/46/528746/19
Lucas Alvares Gomes 4 years ago
parent
commit
8fd9411b8a
  1. 1
      networking_ovn/common/constants.py
  2. 34
      networking_ovn/common/maintenance.py
  3. 44
      networking_ovn/common/ovn_client.py
  4. 3
      networking_ovn/common/utils.py
  5. 14
      networking_ovn/l3/l3_ovn.py
  6. 5
      networking_ovn/ovn_db_sync.py
  7. 23
      networking_ovn/ovsdb/commands.py
  8. 3
      networking_ovn/tests/functional/test_revision_numbers.py
  9. 26
      networking_ovn/tests/unit/l3/test_l3_ovn.py
  10. 12
      networking_ovn/tests/unit/test_ovn_db_sync.py

1
networking_ovn/common/constants.py

@ -100,3 +100,4 @@ TYPE_PORTS = 'ports'
TYPE_SECURITY_GROUP_RULES = 'security_group_rules'
TYPE_ROUTERS = 'routers'
TYPE_SECURITY_GROUPS = 'security_groups'
TYPE_FLOATINGIPS = 'floatingips'

34
networking_ovn/common/maintenance.py

@ -236,6 +236,36 @@ class DBInconsistenciesPeriodics(object):
else:
self._ovn_client.delete_security_group(row.resource_uuid)
def _fix_create_update_floatingip(self, row):
# Get the latest version of the resource in Neutron DB
admin_context = n_context.get_admin_context()
fip_db_obj = self._ovn_client._l3_plugin.get_floatingip(
admin_context, row.resource_uuid)
ovn_fip = self._nb_idl.get_floatingip(row.resource_uuid)
if not ovn_fip:
# If the resource doesn't exist in the OVN DB, create it.
self._ovn_client.create_floatingip(fip_db_obj)
else:
ovn_revision = int(ovn_fip['external_ids'].get(
ovn_const.OVN_REV_NUM_EXT_ID_KEY, -1))
# If the resource exist in the OVN DB but the revision
# number is different from Neutron DB, updated it.
if ovn_revision != fip_db_obj['revision_number']:
self._ovn_client.update_floatingip(fip_db_obj)
else:
# If the resource exist and the revision number
# is equal on both databases just bump the revision on
# the cache table.
db_rev.bump_revision(fip_db_obj, ovn_const.TYPE_FLOATINGIPS)
def _fix_delete_floatingip(self, row):
ovn_fip = self._nb_idl.get_floatingip(row.resource_uuid)
if not ovn_fip:
db_rev.delete_revision(row.resource_uuid)
else:
self._ovn_client.delete_floatingip(row.resource_uuid)
@periodics.periodic(spacing=DB_CONSISTENCY_CHECK_INTERVAL,
run_immediately=True)
def check_for_inconsistencies(self):
@ -263,6 +293,8 @@ class DBInconsistenciesPeriodics(object):
self._fix_create_update_routers(row)
elif row.resource_type == ovn_const.TYPE_SECURITY_GROUPS:
self._fix_create_security_group(row)
elif row.resource_type == ovn_const.TYPE_FLOATINGIPS:
self._fix_create_update_floatingip(row)
except Exception:
LOG.exception('Failed to fix resource %(res_uuid)s '
'(type: %(res_type)s)',
@ -282,6 +314,8 @@ class DBInconsistenciesPeriodics(object):
self._fix_delete_router(row)
elif row.resource_type == ovn_const.TYPE_SECURITY_GROUPS:
self._fix_delete_security_group(row)
elif row.resource_type == ovn_const.TYPE_FLOATINGIPS:
self._fix_delete_floatingip(row)
except Exception:
LOG.exception('Failed to fix deleted resource %(res_uuid)s '
'(type: %(res_type)s)',

44
networking_ovn/common/ovn_client.py

@ -568,6 +568,8 @@ class OVNClient(object):
ext_ids = {
ovn_const.OVN_FIP_EXT_ID_KEY: floatingip['id'],
ovn_const.OVN_REV_NUM_EXT_ID_KEY: str(utils.get_revision_number(
floatingip, ovn_const.TYPE_FLOATINGIPS)),
ovn_const.OVN_FIP_PORT_EXT_ID_KEY: floatingip['port_id'],
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: gw_lrouter_name}
columns = {'type': 'dnat_and_snat',
@ -594,6 +596,16 @@ class OVNClient(object):
external_ip=fip['external_ip'])]
self._transaction(commands, txn=txn)
def update_floatingip_status(self, floatingip):
# NOTE(lucasagomes): OVN doesn't care about the floating ip
# status, this method just bumps the revision number
check_rev_cmd = self._nb_idl.check_revision_number(
floatingip['id'], floatingip, ovn_const.TYPE_FLOATINGIPS)
with self._nb_idl.transaction(check_error=True) as txn:
txn.add(check_rev_cmd)
if check_rev_cmd.result == ovn_const.TXN_COMMITTED:
db_rev.bump_revision(floatingip, ovn_const.TYPE_FLOATINGIPS)
def create_floatingip(self, floatingip):
try:
self._create_or_update_floatingip(floatingip)
@ -601,6 +613,9 @@ class OVNClient(object):
with excutils.save_and_reraise_exception():
LOG.error('Unable to create floating ip in gateway '
'router. Error: %s', e)
db_rev.bump_revision(floatingip, ovn_const.TYPE_FLOATINGIPS)
# NOTE(lucasagomes): Revise the expected status
# of floating ips, setting it to ACTIVE here doesn't
# see consistent with other drivers (ODL here), see:
@ -625,7 +640,10 @@ class OVNClient(object):
router_id, fip_object['fixed_ip_address'],
fip_object['floating_ip_address'])
check_rev_cmd = self._nb_idl.check_revision_number(
floatingip['id'], floatingip, ovn_const.TYPE_FLOATINGIPS)
with self._nb_idl.transaction(check_error=True) as txn:
txn.add(check_rev_cmd)
if (ovn_fip and
(floatingip['fixed_ip_address'] != ovn_fip['logical_ip'] or
floatingip['port_id'] != ovn_fip['external_ids'].get(
@ -642,6 +660,9 @@ class OVNClient(object):
self._create_or_update_floatingip(floatingip, txn=txn)
fip_status = const.FLOATINGIP_STATUS_ACTIVE
if check_rev_cmd.result == ovn_const.TXN_COMMITTED:
db_rev.bump_revision(floatingip, ovn_const.TYPE_FLOATINGIPS)
if fip_status:
self._l3_plugin.update_floatingip_status(
n_context.get_admin_context(), floatingip['id'], fip_status)
@ -660,18 +681,17 @@ class OVNClient(object):
router_id, fip_object['fixed_ip_address'],
fip_object['floating_ip_address'])
if not ovn_fip:
return
lrouter = ovn_fip['external_ids'].get(
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY, utils.ovn_name(router_id))
try:
self._delete_floatingip(ovn_fip, lrouter)
except Exception as e:
with excutils.save_and_reraise_exception():
LOG.error('Unable to delete floating ip in gateway '
'router. Error: %s', e)
if ovn_fip:
lrouter = ovn_fip['external_ids'].get(
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY,
utils.ovn_name(router_id))
try:
self._delete_floatingip(ovn_fip, lrouter)
except Exception as e:
with excutils.save_and_reraise_exception():
LOG.error('Unable to delete floating ip in gateway '
'router. Error: %s', e)
db_rev.delete_revision(fip_id)
def disassociate_floatingip(self, floatingip, router_id):
lrouter = utils.ovn_name(router_id)

3
networking_ovn/common/utils.py

@ -207,7 +207,8 @@ def get_revision_number(resource, resource_type):
constants.TYPE_PORTS,
constants.TYPE_SECURITY_GROUP_RULES,
constants.TYPE_ROUTERS,
constants.TYPE_SECURITY_GROUPS):
constants.TYPE_SECURITY_GROUPS,
constants.TYPE_FLOATINGIPS):
return resource['revision_number']
else:
raise ovn_exc.UnknownResourceType(resource_type=resource_type)

14
networking_ovn/l3/l3_ovn.py

@ -77,6 +77,9 @@ class OVNL3RouterPlugin(service_base.ServicePluginBase,
registry.subscribe(
self.create_router_precommit, resources.ROUTER,
events.PRECOMMIT_CREATE)
registry.subscribe(
self.create_floatingip_precommit, resources.FLOATING_IP,
events.PRECOMMIT_CREATE)
@property
def _ovn_client(self):
@ -250,6 +253,11 @@ class OVNL3RouterPlugin(service_base.ServicePluginBase,
return router_interface_info
def create_floatingip_precommit(self, resource, event, trigger, context,
floatingip, floatingip_id, floatingip_db):
db_rev.create_initial_revision(
floatingip_id, ovn_const.TYPE_FLOATINGIPS, context.session)
def create_floatingip(self, context, floatingip,
initial_status=n_const.FLOATINGIP_STATUS_DOWN):
fip = super(OVNL3RouterPlugin, self).create_floatingip(
@ -277,6 +285,12 @@ class OVNL3RouterPlugin(service_base.ServicePluginBase,
self._ovn_client.update_floatingip(fip, fip_object=original_fip)
return fip
def update_floatingip_status(self, context, floatingip_id, status):
fip = super(OVNL3RouterPlugin, self).update_floatingip_status(
context, floatingip_id, status)
self._ovn_client.update_floatingip_status(fip)
return fip
def disassociate_floatingips(self, context, port_id, do_notify=True):
fips = self.get_floatingips(context.elevated(),
filters={'port_id': [port_id]})

5
networking_ovn/ovn_db_sync.py

@ -544,7 +544,7 @@ class OvnNbSynchronizer(OvnDbSynchronizer):
fip['del'])
for nat in fip['del']:
self._ovn_client._delete_floatingip(
nat, utils.ovn_name(fip['id']))
nat, utils.ovn_name(fip['id']), txn=txn)
if fip['add']:
LOG.warning("Router %(id)s floating ips %(fip)s "
"found in Neutron but not in OVN",
@ -553,7 +553,8 @@ class OvnNbSynchronizer(OvnDbSynchronizer):
LOG.warning("Add floating ips %s to OVN NB DB",
fip['add'])
for nat in fip['add']:
self._ovn_client.create_floatingip(nat)
self._ovn_client._create_or_update_floatingip(
nat, txn=txn)
for snat in update_snats_list:
if snat['del']:
LOG.warning("Router %(id)s snat %(snat)s "

23
networking_ovn/ovsdb/commands.py

@ -22,6 +22,7 @@ RESOURCE_TYPE_MAP = {
ovn_const.TYPE_NETWORKS: 'Logical_Switch',
ovn_const.TYPE_PORTS: 'Logical_Switch_Port',
ovn_const.TYPE_ROUTERS: 'Logical_Router',
ovn_const.TYPE_FLOATINGIPS: 'NAT',
}
@ -1021,10 +1022,30 @@ class CheckRevisionNumberCommand(command.BaseCommand):
self.resource_type = resource_type
self.if_exists = if_exists
def _get_floatingip(self):
# TODO(lucasagomes): We can't use self.api.lookup() because that
# method does not introspect map type columns. We could either:
# 1. Enhance it to look into maps or, 2. Add a new ``name`` column
# to the NAT table so that we can use lookup() just like we do
# for other resources
for nat in self.api._tables['NAT'].rows.values():
if nat.type != 'dnat_and_snat':
continue
ext_ids = getattr(nat, 'external_ids', {})
if ext_ids.get(ovn_const.OVN_FIP_EXT_ID_KEY) == self.name:
return nat
raise idlutils.RowNotFound(
table='NAT', col='external_ids', match=self.name)
def run_idl(self, txn):
try:
ovn_table = RESOURCE_TYPE_MAP[self.resource_type]
ovn_resource = self.api.lookup(ovn_table, self.name)
ovn_resource = None
if self.resource_type == ovn_const.TYPE_FLOATINGIPS:
ovn_resource = self._get_floatingip()
else:
ovn_resource = self.api.lookup(ovn_table, self.name)
except idlutils.RowNotFound:
if self.if_exists:
return

3
networking_ovn/tests/functional/test_revision_numbers.py

@ -142,3 +142,6 @@ class TestRevisionNumbers(base.TestOVNFunctionalBase):
self.assertEqual(str(1), ovn_revision)
# Assert it also matches with the newest returned by neutron API
self.assertEqual(str(updated_router['revision_number']), ovn_revision)
# TODO(lucasagomes): Add a test for floating IPs here when we get
# the router stuff done.

26
networking_ovn/tests/unit/l3/test_l3_ovn.py

@ -173,12 +173,23 @@ class OVNL3RouterPlugin(test_mech_driver.OVNMechanismDriverTestCase):
'networking_ovn.l3.l3_ovn_scheduler.'
'OVNGatewayLeastLoadedScheduler._schedule_gateway',
return_value=['hv1'])
# FIXME(lucasagomes): We shouldn't be mocking the creation of
# floating IPs here, that makes the FIP to not be registered in
# the standardattributes table and therefore we also need to mock
# bump_revision.
self._start_mock(
'neutron.db.l3_db.L3_NAT_dbonly_mixin.create_floatingip',
return_value=self.fake_floating_ip)
self._start_mock(
'networking_ovn.db.revision.bump_revision',
return_value=None)
self._start_mock(
'neutron.db.l3_db.L3_NAT_dbonly_mixin._get_floatingip',
return_value=self.fake_floating_ip)
self._start_mock(
'networking_ovn.common.ovn_client.'
'OVNClient.update_floatingip_status',
return_value=None)
@mock.patch('neutron.db.l3_db.L3_NAT_dbonly_mixin.add_router_interface')
def test_add_router_interface(self, func):
@ -713,6 +724,7 @@ class OVNL3RouterPlugin(test_mech_driver.OVNMechanismDriverTestCase):
self.l3_inst.create_floatingip(self.context, 'floatingip')
expected_ext_ids = {
ovn_const.OVN_FIP_EXT_ID_KEY: self.fake_floating_ip['id'],
ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
ovn_const.OVN_FIP_PORT_EXT_ID_KEY:
self.fake_floating_ip['port_id'],
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: utils.ovn_name(
@ -737,6 +749,7 @@ class OVNL3RouterPlugin(test_mech_driver.OVNMechanismDriverTestCase):
self.l3_inst.create_floatingip(self.context, 'floatingip')
expected_ext_ids = {
ovn_const.OVN_FIP_EXT_ID_KEY: self.fake_floating_ip['id'],
ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
ovn_const.OVN_FIP_PORT_EXT_ID_KEY:
self.fake_floating_ip['port_id'],
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: utils.ovn_name(
@ -758,6 +771,7 @@ class OVNL3RouterPlugin(test_mech_driver.OVNMechanismDriverTestCase):
self.l3_inst._ovn.add_nat_rule_in_lrouter.assert_not_called()
expected_ext_ids = {
ovn_const.OVN_FIP_EXT_ID_KEY: self.fake_floating_ip['id'],
ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
ovn_const.OVN_FIP_PORT_EXT_ID_KEY:
self.fake_floating_ip['port_id'],
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: utils.ovn_name(
@ -782,6 +796,7 @@ class OVNL3RouterPlugin(test_mech_driver.OVNMechanismDriverTestCase):
self.l3_inst._ovn.set_nat_rule_in_lrouter.assert_not_called()
expected_ext_ids = {
ovn_const.OVN_FIP_EXT_ID_KEY: self.fake_floating_ip['id'],
ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
ovn_const.OVN_FIP_PORT_EXT_ID_KEY:
self.fake_floating_ip['port_id'],
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: utils.ovn_name(
@ -823,6 +838,7 @@ class OVNL3RouterPlugin(test_mech_driver.OVNMechanismDriverTestCase):
external_ip='192.168.0.10')
expected_ext_ids = {
ovn_const.OVN_FIP_EXT_ID_KEY: self.fake_floating_ip_new['id'],
ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
ovn_const.OVN_FIP_PORT_EXT_ID_KEY:
self.fake_floating_ip_new['port_id'],
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: utils.ovn_name(
@ -846,6 +862,7 @@ class OVNL3RouterPlugin(test_mech_driver.OVNMechanismDriverTestCase):
self.l3_inst._ovn.delete_nat_rule_in_lrouter.assert_not_called()
expected_ext_ids = {
ovn_const.OVN_FIP_EXT_ID_KEY: self.fake_floating_ip_new['id'],
ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
ovn_const.OVN_FIP_PORT_EXT_ID_KEY:
self.fake_floating_ip_new['port_id'],
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: utils.ovn_name(
@ -873,6 +890,7 @@ class OVNL3RouterPlugin(test_mech_driver.OVNMechanismDriverTestCase):
self.l3_inst._ovn.delete_nat_rule_in_lrouter.assert_not_called()
expected_ext_ids = {
ovn_const.OVN_FIP_EXT_ID_KEY: self.fake_floating_ip_new['id'],
ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
ovn_const.OVN_FIP_PORT_EXT_ID_KEY:
self.fake_floating_ip_new['port_id'],
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: utils.ovn_name(
@ -916,6 +934,7 @@ class OVNL3RouterPlugin(test_mech_driver.OVNMechanismDriverTestCase):
external_ip='192.168.0.10')
expected_ext_ids = {
ovn_const.OVN_FIP_EXT_ID_KEY: self.fake_floating_ip_new['id'],
ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
ovn_const.OVN_FIP_PORT_EXT_ID_KEY:
self.fake_floating_ip_new['port_id'],
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: utils.ovn_name(
@ -1001,6 +1020,13 @@ class OVNL3ExtrarouteTests(test_l3_gw.ExtGwModeIntTestCase,
'networking_ovn.common.ovn_client.OVNClient.'
'_get_v4_network_of_all_router_ports',
return_value=[])
self._start_mock(
'networking_ovn.common.ovn_client.'
'OVNClient.update_floatingip_status',
return_value=None)
self._start_mock(
'networking_ovn.common.utils.get_revision_number',
return_value=1)
self.setup_notification_driver()
# Note(dongj): According to bug #1657693, status of an unassociated

12
networking_ovn/tests/unit/test_ovn_db_sync.py

@ -573,12 +573,14 @@ class TestOvnNbSyncML2(test_mech_driver.OVNMechanismDriverTestCase):
self.assertEqual(len(add_snat_list),
ovn_api.add_nat_rule_in_lrouter.call_count)
add_fip_calls = [mock.call(nat) for nat in add_floating_ip_list]
ovn_nb_synchronizer._ovn_client.create_floatingip.assert_has_calls(
add_fip_calls)
add_fip_calls = [mock.call(nat, txn=mock.ANY)
for nat in add_floating_ip_list]
(ovn_nb_synchronizer._ovn_client._create_or_update_floatingip.
assert_has_calls(add_fip_calls))
self.assertEqual(
len(add_floating_ip_list),
ovn_nb_synchronizer._ovn_client.create_floatingip.call_count)
ovn_nb_synchronizer._ovn_client._create_or_update_floatingip.
call_count)
del_nat_calls = [mock.call(mock.ANY, **nat) for nat in del_snat_list]
ovn_api.delete_nat_rule_in_lrouter.assert_has_calls(del_nat_calls,
@ -586,7 +588,7 @@ class TestOvnNbSyncML2(test_mech_driver.OVNMechanismDriverTestCase):
self.assertEqual(len(del_snat_list),
ovn_api.delete_nat_rule_in_lrouter.call_count)
del_fip_calls = [mock.call(nat, mock.ANY) for nat in
del_fip_calls = [mock.call(nat, mock.ANY, txn=mock.ANY) for nat in
del_floating_ip_list]
ovn_nb_synchronizer._ovn_client._delete_floatingip.assert_has_calls(
del_fip_calls, any_order=True)

Loading…
Cancel
Save