objects: Update keypairs when saving an instance

The keypair of a server is updated when rebuilding the server with a
keypair. This function has been added since API microversion 2.54.
However the 'keypairs' of the instance object is not saved when saving
the instance object currently.

Make the instance object update the 'keypairs' field when saving the
instance object.

Change-Id: I8a2726b39d0444de8c35480024078a97430f5d0c
Closes-Bug: #1843708
Co-authored-by: Stephen Finucane <stephenfin@redhat.com>
(cherry picked from commit 086796021b189c3ac64805ed8f6bde833906d284)
(cherry picked from commit aed86ee5d6289edf1baf9fe0b2a9e509031fdd25)
(cherry picked from commit b971dc82cb524fe86284c95ec671e2bad1c2874f)
(cherry picked from commit 0bc5a4ecb524a73aacb5d0dd2887799885bdbb14)
(cherry picked from commit aa7a6939d5177c0dd8c9f5a7bf7975264d2f5a2a)

4 files changed, 116 insertions, 8 deletions

diff --git a/nova/objects/instance.py b/nova/objects/instance.py
index 08b60593fd..1884604eca 100644
--- a/nova/objects/instance.py
+++ b/nova/objects/instance.py
@@ -695,8 +695,9 @@ class Instance(base.NovaPersistentObject, base.NovaObject,
         pass
 
     def _save_keypairs(self, context):
-        # NOTE(danms): Read-only so no need to save this.
-        pass
+        if 'keypairs' in self.obj_what_changed():
+            self._save_extra_generic('keypairs')
+            self.obj_reset_changes(['keypairs'], recursive=True)
 
     def _save_extra_generic(self, field):
         if field in self.obj_what_changed():
diff --git a/nova/tests/functional/regressions/test_bug_1843708.py b/nova/tests/functional/regressions/test_bug_1843708.py
new file mode 100644
index 0000000000..8db10285f7
--- /dev/null
+++ b/nova/tests/functional/regressions/test_bug_1843708.py
@@ -0,0 +1,92 @@
+# Copyright 2019 NTT Corporation
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+from nova import context
+from nova import objects
+from nova import test
+from nova.tests import fixtures as nova_fixtures
+from nova.tests.functional import integrated_helpers
+from nova.tests.unit import fake_notifier
+from nova.tests.unit.image import fake as fake_image
+
+
+class RebuildWithKeypairTestCase(
+    test.TestCase, integrated_helpers.InstanceHelperMixin,
+):
+    """Regression test for bug 1843708.
+
+    This tests a rebuild scenario with new key pairs.
+    """
+
+    def setUp(self):
+        super(RebuildWithKeypairTestCase, self).setUp()
+        # Start standard fixtures.
+        self.useFixture(nova_fixtures.PlacementFixture())
+        self.useFixture(nova_fixtures.NeutronFixture(self))
+        fake_image.stub_out_image_service(self)
+        self.addCleanup(fake_image.FakeImageService_reset)
+        fake_notifier.stub_notifier(self)
+        self.addCleanup(fake_notifier.reset)
+        self.api = self.useFixture(nova_fixtures.OSAPIFixture(
+            api_version='v2.1')).admin_api
+        self.api.microversion = 'latest'
+        # Start nova services.
+        self.start_service('conductor')
+        self.start_service('scheduler')
+        self.start_service('compute')
+
+    def test_rebuild_with_keypair(self):
+        keypair_req = {
+            'keypair': {
+                'name': 'test-key1',
+                'type': 'ssh',
+            },
+        }
+        keypair1 = self.api.post_keypair(keypair_req)
+        keypair_req['keypair']['name'] = 'test-key2'
+        keypair2 = self.api.post_keypair(keypair_req)
+
+        server = self._build_minimal_create_server_request(
+            self.api, 'test-rebuild-with-keypair',
+            image_uuid=fake_image.get_valid_image_id(),
+            networks='none')
+        server.update({'key_name': 'test-key1'})
+
+        # Create a server with keypair 'test-key1'
+        server = self.api.post_server({'server': server})
+        self._wait_for_state_change(self.api, server, 'ACTIVE')
+
+        # Check keypairs
+        ctxt = context.get_admin_context()
+        instance = objects.Instance.get_by_uuid(
+            ctxt, server['id'], expected_attrs=['keypairs'])
+        self.assertEqual(
+            keypair1['public_key'], instance.keypairs[0].public_key)
+
+        # Rebuild a server with keypair 'test-key2'
+        body = {
+            'rebuild': {
+                'imageRef': fake_image.get_valid_image_id(),
+                'key_name': 'test-key2',
+            },
+        }
+        self.api.api_post('servers/%s/action' % server['id'], body)
+        fake_notifier.wait_for_versioned_notifications('instance.rebuild.end')
+        self._wait_for_state_change(self.api, server, 'ACTIVE')
+
+        # Check keypairs changed
+        instance = objects.Instance.get_by_uuid(
+            ctxt, server['id'], expected_attrs=['keypairs'])
+        self.assertEqual(
+            keypair2['public_key'], instance.keypairs[0].public_key)
diff --git a/nova/tests/unit/fake_instance.py b/nova/tests/unit/fake_instance.py
index 934fdd2441..680b5148b0 100644
--- a/nova/tests/unit/fake_instance.py
+++ b/nova/tests/unit/fake_instance.py
@@ -117,7 +117,6 @@ def fake_instance_obj(context, obj_instance_class=None, **updates):
                                 is_public=True,
                                 extra_specs={},
                                 projects=[])
-        flavor.obj_reset_changes()
     inst = obj_instance_class._from_db_object(context,
                obj_instance_class(), fake_db_instance(**updates),
                expected_attrs=expected_attrs)
@@ -133,7 +132,7 @@ def fake_instance_obj(context, obj_instance_class=None, **updates):
             inst.memory_mb = flavor.memory_mb
     inst.old_flavor = None
     inst.new_flavor = None
-    inst.obj_reset_changes()
+    inst.obj_reset_changes(recursive=True)
     return inst
 
 
diff --git a/nova/tests/unit/objects/test_instance.py b/nova/tests/unit/objects/test_instance.py
index 14798e36de..45d54f6201 100644
--- a/nova/tests/unit/objects/test_instance.py
+++ b/nova/tests/unit/objects/test_instance.py
@@ -683,14 +683,30 @@ class _TestInstanceObject(object):
         inst.numa_topology = None
         inst.migration_context = None
         inst.vcpu_model = test_vcpu_model.fake_vcpumodel
-        inst.save()
+        inst.keypairs = objects.KeyPairList(
+            objects=[objects.KeyPair(name='foo')])
+
         json_vcpu_model = jsonutils.dumps(
             test_vcpu_model.fake_vcpumodel.obj_to_primitive())
-        expected_vals = {'numa_topology': None,
-                         'migration_context': None,
-                         'vcpu_model': json_vcpu_model}
+        json_keypairs = jsonutils.dumps(inst.keypairs.obj_to_primitive())
+
+        # Check changed fields in the instance object
+        self.assertIn('keypairs', inst.obj_what_changed())
+        self.assertEqual({'objects'}, inst.keypairs.obj_what_changed())
+
+        inst.save()
+
+        expected_vals = {
+            'numa_topology': None,
+            'migration_context': None,
+            'vcpu_model': json_vcpu_model,
+            'keypairs': json_keypairs,
+        }
         mock_update.assert_called_once_with(self.context, inst.uuid,
                                             expected_vals)
+        # Verify that the record of changed fields has been cleared
+        self.assertNotIn('keypairs', inst.obj_what_changed())
+        self.assertEqual(set(), inst.keypairs.obj_what_changed())
 
     @mock.patch.object(notifications, 'send_update')
     @mock.patch.object(cells_rpcapi.CellsAPI, 'instance_update_from_api')