Fix agent token and URL handling during fast-track deployment

We wipe these fields on some conditions, most notable - on starting
the deployment. Make the removal of these fields to always go through
the helpers in conductor/utils (and remove an unused one).

Change-Id: Idb952588bb8a6d5131764f29c6225762ba5d55cc

ironic/conductor/cleaning.py

@@ -213,19 +213,7 @@ def do_next_clean_step(task, step_index):
 
     # Clear clean_step
     node.clean_step = None
-    driver_internal_info = node.driver_internal_info
+    utils.wipe_cleaning_internal_info(task)
-    driver_internal_info['clean_steps'] = None
-    driver_internal_info.pop('clean_step_index', None)
-    driver_internal_info.pop('cleaning_reboot', None)
-    driver_internal_info.pop('cleaning_polling', None)
-
-    # Remove agent_url
-    if not utils.fast_track_able(task):
-        driver_internal_info.pop('agent_url', None)
-        driver_internal_info.pop('agent_secret_token', None)
-        driver_internal_info.pop('agent_secret_token_pregenerated', None)
-
-    node.driver_internal_info = driver_internal_info
     node.save()
     try:
         task.driver.deploy.tear_down_cleaning(task)
@@ -273,15 +261,6 @@ def do_node_clean_abort(task, step_name=None):
 
     node.last_error = last_error
     node.clean_step = None
-    info = node.driver_internal_info
+    utils.wipe_cleaning_internal_info(task)
-    # Clear any leftover metadata about cleaning
-    info.pop('clean_step_index', None)
-    info.pop('cleaning_reboot', None)
-    info.pop('cleaning_polling', None)
-    info.pop('skip_current_clean_step', None)
-    info.pop('agent_url', None)
-    info.pop('agent_secret_token', None)
-    info.pop('agent_secret_token_pregenerated', None)
-    node.driver_internal_info = info
     node.save()
     LOG.info(info_message)

ironic/conductor/deployments.py

@@ -70,10 +70,6 @@ def start_deploy(task, manager, configdrive=None, event='deploy'):
     :param event: event to process: deploy or rebuild.
     """
     node = task.node
-    # Record of any pre-existing agent_url should be removed
-    # except when we are in fast track conditions.
-    if not utils.is_fast_track(task):
-        utils.remove_agent_url(node)
 
     if event == 'rebuild':
         # Note(gilliard) Clear these to force the driver to
@@ -127,8 +123,7 @@ def start_deploy(task, manager, configdrive=None, event='deploy'):
 def do_node_deploy(task, conductor_id=None, configdrive=None):
     """Prepare the environment and deploy a node."""
     node = task.node
-    utils.wipe_deploy_internal_info(node)
+    utils.wipe_deploy_internal_info(task)
-    utils.del_secret_token(node)
     try:
         if configdrive:
             if isinstance(configdrive, dict):
@@ -308,7 +303,7 @@ def do_next_deploy_step(task, step_index, conductor_id):
 
     # Finished executing the steps. Clear deploy_step.
     node.deploy_step = None
-    utils.wipe_deploy_internal_info(node)
+    utils.wipe_deploy_internal_info(task)
     node.save()
 
     _start_console_in_deploy(task)

ironic/conductor/manager.py

@@ -1025,11 +1025,9 @@ class ConductorManager(base_manager.BaseConductorManager):
             # But we do need to clear the instance-related fields.
             node.instance_info = {}
             node.instance_uuid = None
+            utils.wipe_deploy_internal_info(task)
             driver_internal_info = node.driver_internal_info
-            driver_internal_info.pop('agent_secret_token', None)
-            driver_internal_info.pop('agent_secret_token_pregenerated', None)
             driver_internal_info.pop('instance', None)
-            driver_internal_info.pop('clean_steps', None)
             driver_internal_info.pop('root_uuid_or_disk_id', None)
             driver_internal_info.pop('is_whole_disk_image', None)
             driver_internal_info.pop('deploy_boot_mode', None)

ironic/conductor/utils.py

@@ -444,11 +444,15 @@ def cleaning_error_handler(task, msg, tear_down_cleaning=True,
         task.process_event('fail', target_state=target_state)
 
 
-def wipe_deploy_internal_info(node):
+def wipe_deploy_internal_info(task):
     """Remove temporary deployment fields from driver_internal_info."""
-    info = node.driver_internal_info
+    info = task.node.driver_internal_info
-    info.pop('agent_secret_token', None)
+    if not fast_track_able(task):
-    info.pop('agent_secret_token_pregenerated', None)
+        info.pop('agent_secret_token', None)
+        info.pop('agent_secret_token_pregenerated', None)
+        # Remove agent_url since it will be re-asserted
+        # upon the next deployment attempt.
+        info.pop('agent_url', None)
     # Clear any leftover metadata about deployment.
     info['deploy_steps'] = None
     info.pop('agent_cached_deploy_steps', None)
@@ -457,10 +461,24 @@ def wipe_deploy_internal_info(node):
     info.pop('deployment_polling', None)
     info.pop('skip_current_deploy_step', None)
     info.pop('steps_validated', None)
-    # Remove agent_url since it will be re-asserted
+    task.node.driver_internal_info = info
-    # upon the next deployment attempt.
+
-    info.pop('agent_url', None)
+
-    node.driver_internal_info = info
+def wipe_cleaning_internal_info(task):
+    """Remove temporary cleaning fields from driver_internal_info."""
+    info = task.node.driver_internal_info
+    if not fast_track_able(task):
+        info.pop('agent_url', None)
+        info.pop('agent_secret_token', None)
+        info.pop('agent_secret_token_pregenerated', None)
+    info['clean_steps'] = None
+    info.pop('agent_cached_clean_steps', None)
+    info.pop('clean_step_index', None)
+    info.pop('cleaning_reboot', None)
+    info.pop('cleaning_polling', None)
+    info.pop('skip_current_clean_step', None)
+    info.pop('steps_validated', None)
+    task.node.driver_internal_info = info
 
 
 def deploying_error_handler(task, logmsg, errmsg=None, traceback=False,
@@ -503,7 +521,7 @@ def deploying_error_handler(task, logmsg, errmsg=None, traceback=False,
         # Clear deploy step; we leave the list of deploy steps
         # in node.driver_internal_info for debugging purposes.
         node.deploy_step = {}
-        wipe_deploy_internal_info(node)
+        wipe_deploy_internal_info(task)
 
     if cleanup_err:
         node.last_error = cleanup_err
@@ -1047,19 +1065,6 @@ def add_secret_token(node, pregenerated=False):
     node.driver_internal_info = i_info
 
 
-def del_secret_token(node):
-    """Deletes the IPA agent secret token.
-
-    Removes the agent token secret from the driver_internal_info field
-    from the Node object.
-
-    :param node: Node object
-    """
-    i_info = node.driver_internal_info
-    i_info.pop('agent_secret_token', None)
-    node.driver_internal_info = i_info
-
-
 def is_agent_token_present(node):
     """Determines if an agent token is present upon a node.
 

ironic/tests/unit/conductor/test_cleaning.py

@@ -531,11 +531,15 @@ class DoNodeCleanTestCase(db_base.DbTestCase):
     @mock.patch('ironic.drivers.modules.fake.FakeDeploy.execute_clean_step',
                 autospec=True)
     def _do_next_clean_step_last_step_noop(self, mock_execute, manual=False,
-                                           retired=False):
+                                           retired=False, fast_track=False):
+        if fast_track:
+            self.config(fast_track=True, group='deploy')
         # Resume where last_step is the last cleaning step, should be noop
         tgt_prov_state = states.MANAGEABLE if manual else states.AVAILABLE
         info = {'clean_steps': self.clean_steps,
-                'clean_step_index': len(self.clean_steps) - 1}
+                'clean_step_index': len(self.clean_steps) - 1,
+                'agent_url': 'test-url',
+                'agent_secret_token': 'token'}
 
         node = obj_utils.create_test_node(
             self.context, driver='fake-hardware',
@@ -563,6 +567,15 @@ class DoNodeCleanTestCase(db_base.DbTestCase):
         self.assertNotIn('clean_step_index', node.driver_internal_info)
         self.assertIsNone(node.driver_internal_info['clean_steps'])
         self.assertFalse(mock_execute.called)
+        if fast_track:
+            self.assertEqual('test-url',
+                             node.driver_internal_info.get('agent_url'))
+            self.assertIsNotNone(
+                node.driver_internal_info.get('agent_secret_token'))
+        else:
+            self.assertNotIn('agent_url', node.driver_internal_info)
+            self.assertNotIn('agent_secret_token',
+                             node.driver_internal_info)
 
     def test__do_next_clean_step_automated_last_step_noop(self):
         self._do_next_clean_step_last_step_noop()
@@ -573,6 +586,9 @@ class DoNodeCleanTestCase(db_base.DbTestCase):
     def test__do_next_clean_step_retired_last_step_change_tgt_state(self):
         self._do_next_clean_step_last_step_noop(retired=True)
 
+    def test__do_next_clean_step_last_step_noop_fast_track(self):
+        self._do_next_clean_step_last_step_noop(fast_track=True)
+
     @mock.patch('ironic.drivers.utils.collect_ramdisk_logs', autospec=True)
     @mock.patch('ironic.drivers.modules.fake.FakePower.execute_clean_step',
                 autospec=True)
@@ -989,6 +1005,8 @@ class DoNodeCleanAbortTestCase(db_base.DbTestCase):
             target_provision_state=states.AVAILABLE,
             clean_step={'step': 'foo', 'abortable': True},
             driver_internal_info={
+                'agent_url': 'some url',
+                'agent_secret_token': 'token',
                 'clean_step_index': 2,
                 'cleaning_reboot': True,
                 'cleaning_polling': True,
@@ -1010,6 +1028,10 @@ class DoNodeCleanAbortTestCase(db_base.DbTestCase):
                              task.node.driver_internal_info)
             self.assertNotIn('skip_current_clean_step',
                              task.node.driver_internal_info)
+            self.assertNotIn('agent_url',
+                             task.node.driver_internal_info)
+            self.assertNotIn('agent_secret_token',
+                             task.node.driver_internal_info)
 
     def test__do_node_clean_abort(self):
         self._test__do_node_clean_abort(None)

ironic/tests/unit/conductor/test_deployments.py

@@ -124,7 +124,9 @@ class DoNodeDeployTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
 
     @mock.patch.object(deployments, '_store_configdrive', autospec=True)
     def _test__do_node_deploy_ok(self, mock_store, configdrive=None,
-                                 expected_configdrive=None):
+                                 expected_configdrive=None, fast_track=False):
+        if fast_track:
+            self.config(fast_track=True, group='deploy')
         expected_configdrive = expected_configdrive or configdrive
         self._start_service()
         with mock.patch.object(fake.FakeDeploy,
@@ -133,7 +135,9 @@ class DoNodeDeployTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
             self.node = obj_utils.create_test_node(
                 self.context, driver='fake-hardware', name=None,
                 provision_state=states.DEPLOYING,
-                target_provision_state=states.ACTIVE)
+                target_provision_state=states.ACTIVE,
+                driver_internal_info={'agent_url': 'url',
+                                      'agent_secret_token': 'token'})
             task = task_manager.TaskManager(self.context, self.node.uuid)
 
             deployments.do_node_deploy(task, self.service.conductor.id,
@@ -148,6 +152,12 @@ class DoNodeDeployTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
                                                    expected_configdrive)
             else:
                 self.assertFalse(mock_store.called)
+            self.assertEqual(
+                fast_track,
+                bool(task.node.driver_internal_info.get('agent_url')))
+            self.assertEqual(
+                fast_track,
+                bool(task.node.driver_internal_info.get('agent_secret_token')))
 
     def test__do_node_deploy_ok(self):
         self._test__do_node_deploy_ok()
@@ -156,6 +166,9 @@ class DoNodeDeployTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
         configdrive = 'foo'
         self._test__do_node_deploy_ok(configdrive=configdrive)
 
+    def test__do_node_deploy_fast_track(self):
+        self._test__do_node_deploy_ok(fast_track=True)
+
     @mock.patch('openstack.baremetal.configdrive.build')
     def test__do_node_deploy_configdrive_as_dict(self, mock_cd):
         mock_cd.return_value = 'foo'
@@ -495,7 +508,8 @@ class DoNextDeployStepTestCase(mgr_utils.ServiceSetUpMixin,
         # Run all steps from start to finish (all synchronous)
         driver_internal_info = {'deploy_step_index': None,
                                 'deploy_steps': self.deploy_steps,
-                                'agent_url': 'url'}
+                                'agent_url': 'url',
+                                'agent_secret_token': 'token'}
         self._start_service()
         node = obj_utils.create_test_node(
             self.context, driver='fake-hardware',
@@ -518,6 +532,41 @@ class DoNextDeployStepTestCase(mgr_utils.ServiceSetUpMixin,
         mock_execute.assert_has_calls = [mock.call(self.deploy_steps[0]),
                                          mock.call(self.deploy_steps[1])]
         self.assertNotIn('agent_url', node.driver_internal_info)
+        self.assertNotIn('agent_secret_token', node.driver_internal_info)
+
+    @mock.patch('ironic.drivers.modules.fake.FakeDeploy.execute_deploy_step',
+                autospec=True)
+    def test__do_next_deploy_step_fast_track(self, mock_execute):
+        self.config(fast_track=True, group='deploy')
+        # Run all steps from start to finish (all synchronous)
+        driver_internal_info = {'deploy_step_index': None,
+                                'deploy_steps': self.deploy_steps,
+                                'agent_url': 'url',
+                                'agent_secret_token': 'token'}
+        self._start_service()
+        node = obj_utils.create_test_node(
+            self.context, driver='fake-hardware',
+            driver_internal_info=driver_internal_info,
+            deploy_step={})
+        mock_execute.return_value = None
+
+        task = task_manager.TaskManager(self.context, node.uuid)
+        task.process_event('deploy')
+
+        deployments.do_next_deploy_step(task, 1, self.service.conductor.id)
+
+        # Deploying should be complete
+        node.refresh()
+        self.assertEqual(states.ACTIVE, node.provision_state)
+        self.assertEqual(states.NOSTATE, node.target_provision_state)
+        self.assertEqual({}, node.deploy_step)
+        self.assertNotIn('deploy_step_index', node.driver_internal_info)
+        self.assertIsNone(node.driver_internal_info['deploy_steps'])
+        mock_execute.assert_has_calls = [mock.call(self.deploy_steps[0]),
+                                         mock.call(self.deploy_steps[1])]
+        self.assertEqual('url', node.driver_internal_info['agent_url'])
+        self.assertEqual('token',
+                         node.driver_internal_info['agent_secret_token'])
 
     @mock.patch.object(conductor_utils, 'LOG', autospec=True)
     @mock.patch('ironic.drivers.modules.fake.FakeDeploy.execute_deploy_step',

ironic/tests/unit/conductor/test_manager.py

@@ -1431,7 +1431,6 @@ class ServiceDoNodeDeployTestCase(mgr_utils.ServiceSetUpMixin,
                                                mock.ANY, None)
             mock_iwdi.assert_called_once_with(self.context, node.instance_info)
             self.assertFalse(node.driver_internal_info['is_whole_disk_image'])
-            self.assertNotIn('agent_url', node.driver_internal_info)
 
     def test_do_node_deploy_rebuild_active_state_error(self, mock_iwdi):
         # Tests manager.do_node_deploy() & deployments.do_next_deploy_step(),
@@ -2042,7 +2041,7 @@ class DoNodeTearDownTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
             instance_info={'foo': 'bar'},
             console_enabled=enabled_console,
             driver_internal_info={'is_whole_disk_image': False,
-                                  'clean_steps': {},
+                                  'deploy_steps': {},
                                   'root_uuid_or_disk_id': 'foo',
                                   'instance': {'ephemeral_gb': 10}})
         port = obj_utils.create_test_port(
@@ -2068,7 +2067,7 @@ class DoNodeTearDownTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
         self.assertIsNone(node.allocation_id)
         self.assertEqual({}, node.instance_info)
         self.assertNotIn('instance', node.driver_internal_info)
-        self.assertNotIn('clean_steps', node.driver_internal_info)
+        self.assertIsNone(node.driver_internal_info['deploy_steps'])
         self.assertNotIn('root_uuid_or_disk_id', node.driver_internal_info)
         self.assertNotIn('is_whole_disk_image', node.driver_internal_info)
         mock_tear_down.assert_called_once_with(task)

ironic/tests/unit/conductor/test_utils.py

@@ -2032,10 +2032,10 @@ class AgentTokenUtilsTestCase(tests_base.TestCase):
         conductor_utils.add_secret_token(self.node)
         self.assertIn('agent_secret_token', self.node.driver_internal_info)
 
-    def test_del_secret_token(self):
+    def test_wipe_deploy_internal_info(self):
         conductor_utils.add_secret_token(self.node)
         self.assertIn('agent_secret_token', self.node.driver_internal_info)
-        conductor_utils.del_secret_token(self.node)
+        conductor_utils.wipe_deploy_internal_info(mock.Mock(node=self.node))
         self.assertNotIn('agent_secret_token', self.node.driver_internal_info)
 
     def test_is_agent_token_present(self):

releasenotes/notes/agent-token-817a03776bd46d5b.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Fixes deployment in fast-track mode by keeping the required internal fields
+    (``agent_url`` and ``agent_secret_token``) intact when starting and
+    finishing deployment and cleaning.