Use association_proxy for port groups node_uuid

This change adds 'node_uuid' to:
  ironic.objects.portgroup.Portgroup

'node_uuid' is a relationship using association_proxy in
models.Portgroup. Using the association_proxy removes the
need to do the node lookup to populate node uuid for
portgroups in the api controller.

NOTE:
 On portgroup create a read is added to read the port from
 the database, this ensures node_uuid is loaded and solves
 the DetachedInstanceError which is otherwise raised.

The test test_list_with_deleted_port_group was deleted, if
the portgroup does not exist porgroup_uuid on the port will
be None, no need for extra handling of that case.

Bumps Portgroup object version to 1.5

Change-Id: I4317d034b6661da4248935cb0b9cb095982cc052

9 files changed, 41 insertions, 47 deletions

diff --git a/ironic/api/controllers/v1/port.py b/ironic/api/controllers/v1/port.py
index d70c13f27..2c9c3dab7 100644
--- a/ironic/api/controllers/v1/port.py
+++ b/ironic/api/controllers/v1/port.py
@@ -163,22 +163,13 @@ def port_sanitize(port, fields=None):
 def list_convert_with_links(rpc_ports, limit, url, fields=None, **kwargs):
     ports = []
     for rpc_port in rpc_ports:
-        try:
-            port = convert_with_links(rpc_port, fields=fields,
-                                      sanitize=False)
-            # NOTE(dtantsur): node was deleted after we fetched the port
-            # list, meaning that the port was also deleted. Skip it.
-            if port['node_uuid'] is None:
-                continue
-        except exception.PortgroupNotFound:
-            # NOTE(dtantsur): port group was deleted after we fetched the
-            # port list, it may mean that the port was deleted too, but
-            # we don't know it. Pretend that the port group was removed.
-            LOG.debug('Removing port group UUID from port %s as the port '
-                      'group was deleted', rpc_port.uuid)
-            rpc_port.portgroup_id = None
-            port = convert_with_links(rpc_port, fields=fields,
-                                      sanitize=False)
+        port = convert_with_links(rpc_port, fields=fields,
+                                  sanitize=False)
+        # NOTE(dtantsur): node was deleted after we fetched the port
+        # list, meaning that the port was also deleted. Skip it.
+        if port['node_uuid'] is None:
+            continue
+
         ports.append(port)
     return collection.list_convert_with_links(
         items=ports,
diff --git a/ironic/api/controllers/v1/portgroup.py b/ironic/api/controllers/v1/portgroup.py
index 6c68e07ba..91740d3c7 100644
--- a/ironic/api/controllers/v1/portgroup.py
+++ b/ironic/api/controllers/v1/portgroup.py
@@ -90,10 +90,10 @@ def convert_with_links(rpc_portgroup, fields=None, sanitize=True):
             'mode',
             'name',
             'properties',
-            'standalone_ports_supported'
+            'standalone_ports_supported',
+            'node_uuid'
         )
     )
-    api_utils.populate_node_uuid(rpc_portgroup, portgroup)
     url = api.request.public_url
     portgroup['ports'] = [
         link.make_link('self', url, 'portgroups',
diff --git a/ironic/common/release_mappings.py b/ironic/common/release_mappings.py
index 76c40fc2f..993ac2b77 100644
--- a/ironic/common/release_mappings.py
+++ b/ironic/common/release_mappings.py
@@ -524,7 +524,7 @@ RELEASE_MAPPING = {
             'Deployment': ['1.0'],
             'DeployTemplate': ['1.1'],
             'Port': ['1.11'],
-            'Portgroup': ['1.4'],
+            'Portgroup': ['1.5'],
             'Trait': ['1.0'],
             'TraitList': ['1.0'],
             'VolumeConnector': ['1.0'],
diff --git a/ironic/db/sqlalchemy/models.py b/ironic/db/sqlalchemy/models.py
index 6fc8e21ab..0dbfabddd 100644
--- a/ironic/db/sqlalchemy/models.py
+++ b/ironic/db/sqlalchemy/models.py
@@ -293,6 +293,15 @@ class Portgroup(Base):
     mode = Column(String(255))
     properties = Column(db_types.JsonEncodedDict)
 
+    _node_uuid = orm.relationship(
+        "Node",
+        viewonly=True,
+        primaryjoin="(Node.id == Portgroup.node_id)",
+        lazy="selectin",
+    )
+    node_uuid = association_proxy(
+        "_node_uuid", "uuid", creator=lambda _i: Node(uuid=_i))
+
 
 class NodeTag(Base):
     """Represents a tag of a bare metal node."""
diff --git a/ironic/objects/portgroup.py b/ironic/objects/portgroup.py
index 8628df731..ef21a5f90 100644
--- a/ironic/objects/portgroup.py
+++ b/ironic/objects/portgroup.py
@@ -36,7 +36,8 @@ class Portgroup(base.IronicObject, object_base.VersionedObjectDictCompat):
     # Version 1.4: Migrate/copy extra['vif_port_id'] to
     #              internal_info['tenant_vif_port_id'] (not an explicit db
     #              change)
-    VERSION = '1.4'
+    # Version 1.5: Add node_uuid field
+    VERSION = '1.5'
 
     dbapi = dbapi.get_instance()
 
@@ -45,6 +46,7 @@ class Portgroup(base.IronicObject, object_base.VersionedObjectDictCompat):
         'uuid': object_fields.UUIDField(nullable=True),
         'name': object_fields.StringField(nullable=True),
         'node_id': object_fields.IntegerField(nullable=True),
+        'node_uuid': object_fields.UUIDField(nullable=True),
         'address': object_fields.MACAddressField(nullable=True),
         'extra': object_fields.FlexibleDictField(nullable=True),
         'internal_info': object_fields.FlexibleDictField(nullable=True),
@@ -261,6 +263,10 @@ class Portgroup(base.IronicObject, object_base.VersionedObjectDictCompat):
         """
         values = self.do_version_changes_for_db()
         db_portgroup = self.dbapi.create_portgroup(values)
+        # NOTE(hjensas): To avoid lazy load issue (DetachedInstanceError) in
+        # sqlalchemy, get new port the port from the DB to ensure the node_uuid
+        # via association_proxy relationship is loaded.
+        db_portgroup = self.dbapi.get_portgroup_by_id(db_portgroup['id'])
         self._from_db_object(self._context, self, db_portgroup)
 
     # NOTE(xek): We don't want to enable RPC on this call just yet. Remotable
diff --git a/ironic/tests/unit/api/controllers/v1/test_port.py b/ironic/tests/unit/api/controllers/v1/test_port.py
index 7092a4012..7b9e0e9d1 100644
--- a/ironic/tests/unit/api/controllers/v1/test_port.py
+++ b/ironic/tests/unit/api/controllers/v1/test_port.py
@@ -15,7 +15,6 @@ Tests for the API /ports/ methods.
 
 import datetime
 from http import client as http_client
-import types
 from unittest import mock
 from urllib import parse as urlparse
 
@@ -236,24 +235,6 @@ class TestListPorts(test_api_base.BaseApiTest):
         # never expose the node_id
         self.assertNotIn('node_id', data['ports'][0])
 
-    # NOTE(jlvillal): autospec=True doesn't work on staticmethods:
-    # https://bugs.python.org/issue23078
-    @mock.patch.object(objects.Portgroup, 'get', spec_set=types.FunctionType)
-    def test_list_with_deleted_port_group(self, mock_get_pg):
-        # check that we don't end up with HTTP 400 when port group deletion
-        # races with listing ports - see https://launchpad.net/bugs/1748893
-        portgroup = obj_utils.create_test_portgroup(self.context,
-                                                    node_id=self.node.id)
-        port = obj_utils.create_test_port(self.context, node_id=self.node.id,
-                                          portgroup_id=portgroup.id)
-        mock_get_pg.side_effect = exception.PortgroupNotFound('boom')
-        data = self.get_json(
-            '/ports/detail',
-            headers={api_base.Version.string: str(api_v1.max_version())}
-        )
-        self.assertEqual(port.uuid, data['ports'][0]["uuid"])
-        self.assertIsNone(data['ports'][0]["portgroup_uuid"])
-
     @mock.patch.object(policy, 'authorize', spec=True)
     def test_list_non_admin_forbidden(self, mock_authorize):
         def mock_authorize_function(rule, target, creds):
diff --git a/ironic/tests/unit/db/utils.py b/ironic/tests/unit/db/utils.py
index 87ba4b514..f75eea06d 100644
--- a/ironic/tests/unit/db/utils.py
+++ b/ironic/tests/unit/db/utils.py
@@ -448,6 +448,8 @@ def get_test_portgroup(**kw):
         'uuid': kw.get('uuid', '6eb02b44-18a3-4659-8c0b-8d2802581ae4'),
         'name': kw.get('name', 'fooname'),
         'node_id': kw.get('node_id', 123),
+        'node_uuid': kw.get('node_uuid',
+                            '40481b96-306b-4a33-901f-795a3dc2f397'),
         'address': kw.get('address', '52:54:00:cf:2d:31'),
         'extra': kw.get('extra', {}),
         'created_at': kw.get('created_at'),
diff --git a/ironic/tests/unit/objects/test_objects.py b/ironic/tests/unit/objects/test_objects.py
index 017a0d210..9b4adfcfe 100644
--- a/ironic/tests/unit/objects/test_objects.py
+++ b/ironic/tests/unit/objects/test_objects.py
@@ -680,7 +680,7 @@ expected_object_fingerprints = {
     'MyObj': '1.5-9459d30d6954bffc7a9afd347a807ca6',
     'Chassis': '1.3-d656e039fd8ae9f34efc232ab3980905',
     'Port': '1.11-97bf15b61224f26c65e90f007d78bfd2',
-    'Portgroup': '1.4-71923a81a86743b313b190f5c675e258',
+    'Portgroup': '1.5-df4dc15967f67114d51176a98a901a83',
     'Conductor': '1.3-d3f53e853b4d58cae5bfbd9a8341af4a',
     'EventType': '1.1-aa2ba1afd38553e3880c267404e8d370',
     'NotificationPublisher': '1.0-51a09397d6c0687771fb5be9a999605d',
diff --git a/ironic/tests/unit/objects/test_portgroup.py b/ironic/tests/unit/objects/test_portgroup.py
index 81b68437b..7e844dac7 100644
--- a/ironic/tests/unit/objects/test_portgroup.py
+++ b/ironic/tests/unit/objects/test_portgroup.py
@@ -80,13 +80,18 @@ class TestPortgroupObject(db_base.DbTestCase, obj_utils.SchemasTestMixIn):
     def test_create(self):
         portgroup = objects.Portgroup(self.context, **self.fake_portgroup)
         with mock.patch.object(self.dbapi, 'create_portgroup',
-                               autospec=True) as mock_create_portgroup:
-            mock_create_portgroup.return_value = db_utils.get_test_portgroup()
-
-            portgroup.create()
-
-            args, _kwargs = mock_create_portgroup.call_args
-            self.assertEqual(objects.Portgroup.VERSION, args[0]['version'])
+                               autospec=True) as mock_create_pg:
+            with mock.patch.object(self.dbapi, 'get_portgroup_by_id',
+                                   autospec=True) as mock_get_pg:
+                test_pg = db_utils.get_test_portgroup()
+                mock_create_pg.return_value = test_pg
+                mock_get_pg.return_value = test_pg
+                mock_create_pg.return_value = db_utils.get_test_portgroup()
+
+                portgroup.create()
+
+                args, _kwargs = mock_create_pg.call_args
+                self.assertEqual(objects.Portgroup.VERSION, args[0]['version'])
 
     def test_save(self):
         uuid = self.fake_portgroup['uuid']