Remove some unnecessary usages of verify()

The general usage of verify() is if you are doing something like:

 ports = row.ports
 ports.append('newport')
 row.ports = ports

where you read a value from the db, modify it, then overwrite it.
The setkey/addvalue/delvalue mutate functions make it largely
unnecessary to use verify(), so this patch removes those.

This cherry pick required a small change to address the following
pep8 error:
./neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py:717:48:
H703  Multiple positional placeholders
              "Can't update external IDs") % (self.table, self.record)
                                         ^

Change-Id: Idd42491dcaa10ec36c963b477438eaa2336ef3a0
(cherry picked from commit b11dd3836e)

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py

@@ -696,54 +696,40 @@ class DelAddrSetCommand(command.BaseCommand):
         self.api._tables['Address_Set'].rows[addrset.uuid].delete()
 
 
-class UpdateChassisExtIdsCommand(command.BaseCommand):
+class UpdateObjectExtIdsCommand(command.BaseCommand):
-    def __init__(self, api, name, external_ids, if_exists):
+    table = None
-        super(UpdateChassisExtIdsCommand, self).__init__(api)
+    field = 'name'
-        self.name = name
+
+    def __init__(self, api, record, external_ids, if_exists):
+        super(UpdateObjectExtIdsCommand, self).__init__(api)
+        self.record = record
         self.external_ids = external_ids
         self.if_exists = if_exists
 
     def run_idl(self, txn):
         try:
-            chassis = idlutils.row_by_value(self.api.idl, 'Chassis',
+            # api.lookup() would be better as it doesn't rely on hardcoded col
-                                            'name', self.name)
+            obj = idlutils.row_by_value(self.api.idl, self.table, self.field,
+                                        self.record)
         except idlutils.RowNotFound:
             if self.if_exists:
                 return
-            msg = _("Chassis %s does not exist. "
+            msg = _("%(tbl)s %(rec)s does not exist. "
-                    "Can't update external IDs") % self.name
+                    "Can't update external IDs") % {
+                'tbl': self.table, 'rec': self.record}
             raise RuntimeError(msg)
 
-        chassis.verify('external_ids')
-        chassis_external_ids = getattr(chassis, 'external_ids', {})
         for ext_id_key, ext_id_value in self.external_ids.items():
-            chassis_external_ids[ext_id_key] = ext_id_value
+            obj.setkey('external_ids', ext_id_key, ext_id_value)
-        chassis.external_ids = chassis_external_ids
 
 
-class UpdatePortBindingExtIdsCommand(command.BaseCommand):
+class UpdateChassisExtIdsCommand(UpdateObjectExtIdsCommand):
-    def __init__(self, api, name, external_ids, if_exists):
+    table = 'Chassis'
-        super(UpdatePortBindingExtIdsCommand, self).__init__(api)
-        self.name = name
-        self.external_ids = external_ids
-        self.if_exists = if_exists
 
-    def run_idl(self, txn):
-        try:
-            port = idlutils.row_by_value(self.api.idl, 'Port_Binding',
-                                         'logical_port', self.name)
-        except idlutils.RowNotFound:
-            if self.if_exists:
-                return
-            msg = _("Port %s does not exist. "
-                    "Can't update external IDs") % self.name
-            raise RuntimeError(msg)
 
-        port.verify('external_ids')
+class UpdatePortBindingExtIdsCommand(UpdateObjectExtIdsCommand):
-        port_external_ids = getattr(port, 'external_ids', {})
+    table = 'Port_Binding'
-        for ext_id_key, ext_id_value in self.external_ids.items():
+    field = 'logical_port'
-            port_external_ids[ext_id_key] = ext_id_value
-        port.external_ids = port_external_ids
 
 
 class AddDHCPOptionsCommand(command.BaseCommand):
@@ -817,11 +803,7 @@ class AddNATRuleInLRouterCommand(command.BaseCommand):
         row = txn.insert(self.api._tables['NAT'])
         for col, val in self.columns.items():
             setattr(row, col, val)
-        # TODO(chandrav): convert this to ovs transaction mutate
+        lrouter.addvalue('nat', row.uuid)
-        lrouter.verify('nat')
-        nat = getattr(lrouter, 'nat', [])
-        nat.append(row.uuid)
-        setattr(lrouter, 'nat', nat)
 
 
 class DeleteNATRuleInLRouterCommand(command.BaseCommand):
@@ -845,20 +827,13 @@ class DeleteNATRuleInLRouterCommand(command.BaseCommand):
             msg = _("Logical Router %s does not exist") % self.lrouter
             raise RuntimeError(msg)
 
-        lrouter.verify('nat')
+        for nat in lrouter.nat:
-        # TODO(chandrav): convert this to ovs transaction mutate
+            if (self.type == nat.type and
-        nats = getattr(lrouter, 'nat', [])
+                    self.external_ip == nat.external_ip and
-        for nat in nats:
+                    self.logical_ip == nat.logical_ip):
-            type = getattr(nat, 'type', '')
+                lrouter.delvalue('nat', nat)
-            external_ip = getattr(nat, 'external_ip', '')
-            logical_ip = getattr(nat, 'logical_ip', '')
-            if (self.type == type and
-                    self.external_ip == external_ip and
-                    self.logical_ip == logical_ip):
-                nats.remove(nat)
                 nat.delete()
                 break
-        setattr(lrouter, 'nat', nats)
 
 
 class SetNATRuleInLRouterCommand(command.BaseCommand):
@@ -876,9 +851,7 @@ class SetNATRuleInLRouterCommand(command.BaseCommand):
             msg = _("Logical Router %s does not exist") % self.lrouter
             raise RuntimeError(msg)
 
-        lrouter.verify('nat')
+        for nat_rule in lrouter.nat:
-        nat_rules = getattr(lrouter, 'nat', [])
-        for nat_rule in nat_rules:
             if nat_rule.uuid == self.nat_rule_uuid:
                 for col, val in self.columns.items():
                     setattr(nat_rule, col, val)
@@ -986,21 +959,17 @@ class DeleteLRouterExtGwCommand(command.BaseCommand):
             msg = _("Logical Router %s does not exist") % self.lrouter
             raise RuntimeError(msg)
 
-        lrouter.verify('static_routes')
+        for route in lrouter.static_routes:
-        static_routes = getattr(lrouter, 'static_routes', [])
-        for route in static_routes:
             external_ids = getattr(route, 'external_ids', {})
             if ovn_const.OVN_ROUTER_IS_EXT_GW in external_ids:
-                _delvalue_from_list(lrouter, 'static_routes', route)
+                lrouter.delvalue('static_routes', route)
                 route.delete()
                 break
 
-        lrouter.verify('nat')
+        for nat in lrouter.nat:
-        nats = getattr(lrouter, 'nat', [])
-        for nat in nats:
             if nat.type != 'snat':
                 continue
-            _delvalue_from_list(lrouter, 'nat', nat)
+            lrouter.delvalue('nat', nat)
             nat.delete()
 
         lrouter_ext_ids = getattr(lrouter, 'external_ids', {})
@@ -1015,7 +984,7 @@ class DeleteLRouterExtGwCommand(command.BaseCommand):
         except idlutils.RowNotFound:
             return
 
-        _delvalue_from_list(lrouter, 'ports', lrouter_port)
+        lrouter.delvalue('ports', lrouter_port)
 
 
 class SetLSwitchPortToVirtualTypeCommand(command.BaseCommand):

neutron/tests/unit/fake_resources.py

@@ -360,9 +360,29 @@ class FakeOvsdbRow(FakeResource):
         ovsdb_row_attrs.update(attrs)
         ovsdb_row_methods.update(methods)
 
-        return FakeResource(info=copy.deepcopy(ovsdb_row_attrs),
+        result = FakeResource(info=copy.deepcopy(ovsdb_row_attrs),
-                            loaded=True,
+                              loaded=True,
-                            methods=copy.deepcopy(ovsdb_row_methods))
+                              methods=copy.deepcopy(ovsdb_row_methods))
+        result.setkey.side_effect = lambda col, k, v: (
+                getattr(result, col).__setitem__(k, v))
+
+        def fake_addvalue(col, val):
+            try:
+                getattr(result, col).append(val)
+            except AttributeError:
+                # Not all tests set up fake rows to have all used cols
+                pass
+
+        def fake_delvalue(col, val):
+            try:
+                getattr(result, col).remove(val)
+            except (AttributeError, ValueError):
+                # Some tests also fake adding values
+                pass
+
+        result.addvalue.side_effect = fake_addvalue
+        result.delvalue.side_effect = fake_delvalue
+        return result
 
 
 class FakeOvsdbTable(FakeResource):

neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_commands.py

@@ -1319,7 +1319,8 @@ class TestDeleteLRouterExtGwCommand(TestBaseCommand):
         fake_route_2 = fakes.FakeOvsdbRow.create_one_ovsdb_row(
             attrs={'ip_prefix': '50.0.0.0/24', 'nexthop': '40.0.0.101'})
         fake_lrouter = fakes.FakeOvsdbRow.create_one_ovsdb_row(
-            attrs={'static_routes': [fake_route_1, fake_route_2]})
+            attrs={'static_routes': [fake_route_1, fake_route_2],
+                   'nat': []})
         with mock.patch.object(self.ovn_api, "is_col_present",
                                return_value=True):
             with mock.patch.object(idlutils, 'row_by_value',
@@ -1338,7 +1339,8 @@ class TestDeleteLRouterExtGwCommand(TestBaseCommand):
             attrs={'external_ip': '192.168.1.8',
                    'logical_ip': '10.0.0.5', 'type': 'badtype'})
         fake_lrouter = fakes.FakeOvsdbRow.create_one_ovsdb_row(
-            attrs={'nat': [fake_nat_1, fake_nat_2]})
+            attrs={'nat': [fake_nat_1, fake_nat_2],
+                   'static_routes': []})
         with mock.patch.object(self.ovn_api, "is_col_present",
                                return_value=True):
             with mock.patch.object(idlutils, 'row_by_value',
@@ -1353,7 +1355,8 @@ class TestDeleteLRouterExtGwCommand(TestBaseCommand):
         port_id = 'fake-port-id'
         fake_lrouter = fakes.FakeOvsdbRow.create_one_ovsdb_row(
             attrs={'external_ids':
-                   {ovn_const.OVN_GW_PORT_EXT_ID_KEY: port_id}})
+                   {ovn_const.OVN_GW_PORT_EXT_ID_KEY: port_id},
+                   'static_routes': [], 'nat': []})
         with mock.patch.object(self.ovn_api, "is_col_present",
                                return_value=True):
             with mock.patch.object(idlutils, 'row_by_value',
@@ -1368,7 +1371,8 @@ class TestDeleteLRouterExtGwCommand(TestBaseCommand):
         port_id = 'fake-port-id'
         fake_lrouter = fakes.FakeOvsdbRow.create_one_ovsdb_row(
             attrs={'external_ids':
-                   {ovn_const.OVN_GW_PORT_EXT_ID_KEY: port_id}})
+                   {ovn_const.OVN_GW_PORT_EXT_ID_KEY: port_id},
+                   'static_routes': [], 'nat': []})
         with mock.patch.object(self.ovn_api, "is_col_present",
                                return_value=True):
             with mock.patch.object(idlutils, 'row_by_value',