Remove marker from nova-manage cells_v2 map_instances UI

For a better user experience let's do the right thing. If no max-count
is passed in that means looping through all instances in batches, and if
max-count is used we store a marker to be used for later calls.

Because the marker is the last instance mapped there's already an entry
for that UUID in the instance_mappings table. In order to satisfy the
unique constraint on that column the marker UUID is modified by
replacing '-' with ' ' so that it can be transformed back when needed.

Change-Id: I921258d59e3054cbe874cab96cc8cc47c9a24845

nova/cmd/manage.py

@@ -1157,34 +1157,13 @@ class CellV2Commands(object):
                 database_connection=dbc)
         cell_mapping.create()
 
-    @args('--cell_uuid', metavar='<cell_uuid>', required=True,
+    def _get_and_map_instances(self, ctxt, cell_mapping, limit, marker):
-            help='Unmigrated instances will be mapped to the cell with the '
-                 'uuid provided.')
-    @args('--limit', metavar='<limit>',
-          help='Maximum number of instances to map')
-    @args('--marker', metavar='<marker',
-          help='The last updated instance UUID')
-    @args('--verbose', metavar='<verbose>',
-          help='Provide output for the registration')
-    def map_instances(self, cell_uuid, limit=None,
-                      marker=None, verbose=0):
-        if limit is not None:
-            limit = int(limit)
-            if limit < 0:
-                print('Must supply a positive value for limit')
-                return(1)
-        ctxt = context.get_admin_context(read_deleted='yes')
-        # Validate the cell exists
-        cell_mapping = objects.CellMapping.get_by_uuid(ctxt, cell_uuid)
         filters = {}
         instances = objects.InstanceList.get_by_filters(
-                ctxt, filters, sort_key='created_at', sort_dir='asc',
+                ctxt.elevated(read_deleted='yes'), filters,
-                limit=limit, marker=marker)
+                sort_key='created_at', sort_dir='asc', limit=limit,
-        if verbose:
+                marker=marker)
-            fmt = "%s instances retrieved to be mapped to cell %s"
-            print(fmt % (len(instances), cell_uuid))
 
-        mapped = 0
         for instance in instances:
             try:
                 mapping = objects.InstanceMapping(ctxt)
@@ -1193,16 +1172,77 @@ class CellV2Commands(object):
                 mapping.project_id = instance.project_id
                 mapping.create()
             except db_exc.DBDuplicateEntry:
-                if verbose:
-                    print("%s already mapped to cell" % instance.uuid)
                 continue
-            mapped += 1
 
-        fmt = "%s instances registered to cell %s"
+        if len(instances) == 0 or len(instances) < limit:
-        print(fmt % (mapped, cell_mapping.uuid))
+            # We've hit the end of the instances table
-        if instances:
+            marker = None
-            instance = instances[-1]
+        else:
-            print('Next marker: - %s' % instance.uuid)
+            marker = instances[-1].uuid
+        return marker
+
+    @args('--cell_uuid', metavar='<cell_uuid>', required=True,
+            help='Unmigrated instances will be mapped to the cell with the '
+                 'uuid provided.')
+    @args('--max-count', metavar='<max_count>',
+          help='Maximum number of instances to map')
+    def map_instances(self, cell_uuid, max_count=None):
+        """Map instances into the provided cell.
+
+        This assumes that Nova on this host is still configured to use the nova
+        database not just the nova-api database. Instances in the nova database
+        will be queried from oldest to newest and mapped to the provided cell.
+        A max-count can be set on the number of instance to map in a single
+        run. Repeated runs of the command will start from where the last run
+        finished so it is not necessary to increase max-count to finish. An
+        exit code of 0 indicates that all instances have been mapped.
+        """
+
+        if max_count is not None:
+            try:
+                max_count = int(max_count)
+            except ValueError:
+                max_count = -1
+            map_all = False
+            if max_count < 1:
+                print(_('Must supply a positive value for max-count'))
+                return 127
+        else:
+            map_all = True
+            max_count = 50
+
+        ctxt = context.RequestContext()
+        marker_project_id = 'INSTANCE_MIGRATION_MARKER'
+
+        # Validate the cell exists, this will raise if not
+        cell_mapping = objects.CellMapping.get_by_uuid(ctxt, cell_uuid)
+
+        # Check for a marker from a previous run
+        marker_mapping = objects.InstanceMappingList.get_by_project_id(ctxt,
+                marker_project_id)
+        if len(marker_mapping) == 0:
+            marker = None
+        else:
+            # There should be only one here
+            marker = marker_mapping[0].instance_uuid.replace(' ', '-')
+            marker_mapping[0].destroy()
+
+        next_marker = True
+        while next_marker is not None:
+            next_marker = self._get_and_map_instances(ctxt, cell_mapping,
+                    max_count, marker)
+            marker = next_marker
+            if not map_all:
+                break
+
+        if next_marker:
+            # Don't judge me. There's already an InstanceMapping with this UUID
+            # so the marker needs to be non destructively modified.
+            next_marker = next_marker.replace('-', ' ')
+            objects.InstanceMapping(ctxt, instance_uuid=next_marker,
+                    project_id=marker_project_id).create()
+            return 1
+        return 0
 
     # TODO(melwitt): Remove this when the oslo.messaging function
     # for assembling a transport url from ConfigOpts is available

nova/tests/unit/test_nova_manage.py

@@ -876,10 +876,94 @@ class CellV2CommandsTestCase(test.TestCase):
                 instance_uuid=instance_uuids[0],
                 cell_mapping=cell_mapping).create()
 
-        self.commands.map_instances(cell_uuid, verbose=True)
+        self.commands.map_instances(cell_uuid)
-        output = sys.stdout.getvalue().strip()
 
-        self.assertIn('%s already mapped to cell' % instance_uuids[0], output)
+        for uuid in instance_uuids:
+            inst_mapping = objects.InstanceMapping.get_by_instance_uuid(ctxt,
+                    uuid)
+            self.assertEqual(ctxt.project_id, inst_mapping.project_id)
+
+        mappings = objects.InstanceMappingList.get_by_project_id(ctxt,
+                ctxt.project_id)
+        self.assertEqual(3, len(mappings))
+
+    def test_map_instances_two_batches(self):
+        ctxt = context.RequestContext('fake-user', 'fake_project')
+        cell_uuid = uuidutils.generate_uuid()
+        cell_mapping = objects.CellMapping(
+                ctxt, uuid=cell_uuid, name='fake',
+                transport_url='fake://', database_connection='fake://')
+        cell_mapping.create()
+        instance_uuids = []
+        # Batch size is 50 in map_instances
+        for i in range(60):
+            uuid = uuidutils.generate_uuid()
+            instance_uuids.append(uuid)
+            objects.Instance(ctxt, project_id=ctxt.project_id,
+                             uuid=uuid).create()
+
+        ret = self.commands.map_instances(cell_uuid)
+        self.assertEqual(0, ret)
+
+        for uuid in instance_uuids:
+            inst_mapping = objects.InstanceMapping.get_by_instance_uuid(ctxt,
+                    uuid)
+            self.assertEqual(ctxt.project_id, inst_mapping.project_id)
+
+    def test_map_instances_max_count(self):
+        ctxt = context.RequestContext('fake-user', 'fake_project')
+        cell_uuid = uuidutils.generate_uuid()
+        cell_mapping = objects.CellMapping(
+                ctxt, uuid=cell_uuid, name='fake',
+                transport_url='fake://', database_connection='fake://')
+        cell_mapping.create()
+        instance_uuids = []
+        for i in range(6):
+            uuid = uuidutils.generate_uuid()
+            instance_uuids.append(uuid)
+            objects.Instance(ctxt, project_id=ctxt.project_id,
+                             uuid=uuid).create()
+
+        ret = self.commands.map_instances(cell_uuid, max_count=3)
+        self.assertEqual(1, ret)
+
+        for uuid in instance_uuids[:3]:
+            # First three are mapped
+            inst_mapping = objects.InstanceMapping.get_by_instance_uuid(ctxt,
+                    uuid)
+            self.assertEqual(ctxt.project_id, inst_mapping.project_id)
+        for uuid in instance_uuids[3:]:
+            # Last three are not
+            self.assertRaises(exception.InstanceMappingNotFound,
+                    objects.InstanceMapping.get_by_instance_uuid, ctxt,
+                    uuid)
+
+    def test_map_instances_marker_deleted(self):
+        ctxt = context.RequestContext('fake-user', 'fake_project')
+        cell_uuid = uuidutils.generate_uuid()
+        cell_mapping = objects.CellMapping(
+                ctxt, uuid=cell_uuid, name='fake',
+                transport_url='fake://', database_connection='fake://')
+        cell_mapping.create()
+        instance_uuids = []
+        for i in range(6):
+            uuid = uuidutils.generate_uuid()
+            instance_uuids.append(uuid)
+            objects.Instance(ctxt, project_id=ctxt.project_id,
+                             uuid=uuid).create()
+
+        ret = self.commands.map_instances(cell_uuid, max_count=3)
+        self.assertEqual(1, ret)
+
+        # Instances are mapped in the order created so we know the marker is
+        # based off the third instance.
+        marker = instance_uuids[2].replace('-', ' ')
+        marker_mapping = objects.InstanceMapping.get_by_instance_uuid(ctxt,
+                marker)
+        marker_mapping.destroy()
+
+        ret = self.commands.map_instances(cell_uuid)
+        self.assertEqual(0, ret)
 
         for uuid in instance_uuids:
             inst_mapping = objects.InstanceMapping.get_by_instance_uuid(ctxt,