Modify share groups w/ manila-manage's update-host

We can also condense this code a bit to allow
adding more resources who have a "host" attribute.

Closes-Bug: #1895323
Change-Id: I5b750217e019b7bc5281006f63feba8fa06db534
Signed-off-by: Goutham Pacha Ravi <gouthampravi@gmail.com>

manila/cmd/manage.py

@@ -375,24 +375,28 @@ class ShareCommands(object):
     @args('--force', required=False, type=bool, default=False,
           help="Ignore validations.")
     def update_host(self, current_host, new_host, force=False):
-        """Modify the host name associated with a share and share server.
+        """Modify the host name associated with resources.
 
            Particularly to recover from cases where one has moved
            their Manila Share node, or modified their 'host' opt
            or their backend section name in the manila configuration file.
+           Affects shares, share servers and share groups
         """
         if not force:
             self._validate_hosts(current_host, new_host)
         ctxt = context.get_admin_context()
-        updated = db.share_instances_host_update(ctxt, current_host, new_host)
+        updated = db.share_resources_host_update(ctxt, current_host, new_host)
-        print("Updated host of %(count)s share instances on %(chost)s "
+        msg = ("Updated host of %(si_count)d share instances, "
-              "to %(nhost)s." % {'count': updated, 'chost': current_host,
+               "%(sg_count)d share groups and %(ss_count)d share servers on "
-                                 'nhost': new_host})
+               "%(chost)s to %(nhost)s.")
-
+        msg_args = {
-        servers = db.share_servers_host_update(ctxt, current_host, new_host)
+            'si_count': updated['instances'],
-        print("Updated host of %(count)s share servers on %(chost)s "
+            'sg_count': updated['groups'],
-              "to %(nhost)s." % {'count': servers, 'chost': current_host,
+            'ss_count': updated['servers'],
-                                 'nhost': new_host})
+            'chost': current_host,
+            'nhost': new_host,
+        }
+        print(msg % msg_args)
 
 
 CATEGORIES = {

manila/db/api.py

@@ -339,11 +339,6 @@ def share_instances_status_update(context, share_instance_ids, values):
         context, share_instance_ids, values)
 
 
-def share_instances_host_update(context, current_host, new_host):
-    """Update the host attr of all share instances that are on current_host."""
-    return IMPL.share_instances_host_update(context, current_host, new_host)
-
-
 def share_instances_get_all(context, filters=None):
     """Returns all share instances."""
     return IMPL.share_instances_get_all(context, filters=filters)
@@ -1026,11 +1021,6 @@ def share_server_backend_details_set(context, share_server_id, server_details):
                                                  server_details)
 
 
-def share_servers_host_update(context, current_host, new_host):
-    """Update the host attr of all share servers that are on current_host."""
-    return IMPL.share_servers_host_update(context, current_host, new_host)
-
-
 ##################
 
 
@@ -1304,6 +1294,11 @@ def share_group_snapshot_member_update(context, member_id, values):
     return IMPL.share_group_snapshot_member_update(context, member_id, values)
 
 
+def share_resources_host_update(context, current_host, new_host):
+    """Update the host attr of all share resources that are on current_host."""
+    return IMPL.share_resources_host_update(context, current_host, new_host)
+
+
 ####################
 
 def share_replicas_get_all(context, with_share_server=False,

manila/db/sqlalchemy/api.py

@@ -389,6 +389,33 @@ QUOTA_SYNC_FUNCTIONS = {
 }
 
 
+###################
+
+@require_admin_context
+def share_resources_host_update(context, current_host, new_host):
+    """Updates the 'host' attribute of resources"""
+
+    resources = {
+        'instances': models.ShareInstance,
+        'servers': models.ShareServer,
+        'groups': models.ShareGroup,
+    }
+    result = {}
+
+    session = get_session()
+    with session.begin():
+        for res_name, res_model in resources.items():
+            host_field = res_model.host
+            query = model_query(
+                context, res_model, session=session, read_deleted="no",
+            ).filter(host_field.like('{}%'.format(current_host)))
+            count = query.update(
+                {host_field: func.replace(host_field, current_host, new_host)},
+                synchronize_session=False)
+            result.update({res_name: count})
+    return result
+
+
 ###################
 
 
@@ -1395,20 +1422,6 @@ def _share_instance_create(context, share_id, values, session):
                               session=session)
 
 
-@require_admin_context
-def share_instances_host_update(context, current_host, new_host):
-    session = get_session()
-    host_field = models.ShareInstance.host
-    with session.begin():
-        query = model_query(
-            context, models.ShareInstance, session=session, read_deleted="no",
-        ).filter(host_field.like('{}%'.format(current_host)))
-        result = query.update(
-            {host_field: func.replace(host_field, current_host, new_host)},
-            synchronize_session=False)
-    return result
-
-
 @require_context
 def share_instance_update(context, share_instance_id, values,
                           with_share_data=False):
@@ -4051,20 +4064,6 @@ def share_server_backend_details_delete(context, share_server_id,
         item.soft_delete(session)
 
 
-@require_admin_context
-def share_servers_host_update(context, current_host, new_host):
-    session = get_session()
-    host_field = models.ShareServer.host
-    with session.begin():
-        query = model_query(
-            context, models.ShareServer, session=session, read_deleted="no",
-        ).filter(host_field.like('{}%'.format(current_host)))
-        result = query.update(
-            {host_field: func.replace(host_field, current_host, new_host)},
-            synchronize_session=False)
-    return result
-
-
 ###################
 
 def _driver_private_data_query(session, context, entity_id, key=None,

manila/tests/cmd/test_manage.py

@@ -404,13 +404,13 @@ class ManilaCmdManageTestCase(test.TestCase):
     def test_share_update_host_fail_validation(self, current_host, new_host):
         self.mock_object(context, 'get_admin_context',
                          mock.Mock(return_value='admin_ctxt'))
-        self.mock_object(db, 'share_instances_host_update')
+        self.mock_object(db, 'share_resources_host_update')
 
         self.assertRaises(SystemExit,
                           self.share_cmds.update_host,
                           current_host, new_host)
 
-        self.assertFalse(db.share_instances_host_update.called)
+        self.assertFalse(db.share_resources_host_update.called)
 
     @ddt.data({'current_host': 'controller-0@fancystore01#pool100',
                'new_host': 'controller-0@fancystore02#pool0'},
@@ -422,23 +422,18 @@ class ManilaCmdManageTestCase(test.TestCase):
                'new_host': 'controller-1@fancystore02', 'force': True})
     @ddt.unpack
     def test_share_update_host(self, current_host, new_host, force=False):
+        db_op = {'instances': 3, 'groups': 4, 'servers': 2}
         self.mock_object(context, 'get_admin_context',
                          mock.Mock(return_value='admin_ctxt'))
-        self.mock_object(db, 'share_instances_host_update',
+        self.mock_object(db, 'share_resources_host_update',
-                         mock.Mock(return_value=20))
+                         mock.Mock(return_value=db_op))
-        self.mock_object(db, 'share_servers_host_update',
-                         mock.Mock(return_value=5))
 
         with mock.patch('sys.stdout', new=six.StringIO()) as intercepted_op:
             self.share_cmds.update_host(current_host, new_host, force)
 
-        expected_op_si = ("Updated host of 20 share instances on "
+        expected_op = ("Updated host of 3 share instances, 4 share groups and "
-                          "%(chost)s to %(nhost)s." %
+                       "2 share servers on %(chost)s to %(nhost)s." %
-                          {'chost': current_host, 'nhost': new_host})
+                       {'chost': current_host, 'nhost': new_host})
-        expected_op_sv = ("Updated host of 5 share servers on "
+        self.assertEqual(expected_op, intercepted_op.getvalue().strip())
-                          "%(chost)s to %(nhost)s." %
+        db.share_resources_host_update.assert_called_once_with(
-                          {'chost': current_host, 'nhost': new_host})
-        self.assertEqual(expected_op_si + "\n" + expected_op_sv,
-                         intercepted_op.getvalue().strip())
-        db.share_instances_host_update.assert_called_once_with(
             'admin_ctxt', current_host, new_host)

manila/tests/db/sqlalchemy/test_api.py

@@ -4003,16 +4003,18 @@ class BackendInfoDatabaseAPITestCase(test.TestCase):
 
 
 @ddt.ddt
-class ShareInstancesTestCase(test.TestCase):
+class ShareResourcesAPITestCase(test.TestCase):
 
     def setUp(self):
-        super(ShareInstancesTestCase, self).setUp()
+        super(ShareResourcesAPITestCase, self).setUp()
         self.context = context.get_admin_context()
 
     @ddt.data('controller-100', 'controller-0@otherstore03',
               'controller-0@otherstore01#pool200')
-    def test_share_instances_host_update_no_matches(self, current_host):
+    def test_share_resources_host_update_no_matches(self, current_host):
         share_id = uuidutils.generate_uuid()
+        share_network_id = uuidutils.generate_uuid()
+        share_network_subnet_id = uuidutils.generate_uuid()
         if '@' in current_host:
             if '#' in current_host:
                 new_host = 'new-controller-X@backendX#poolX'
@@ -4020,7 +4022,8 @@ class ShareInstancesTestCase(test.TestCase):
                 new_host = 'new-controller-X@backendX'
         else:
             new_host = 'new-controller-X'
-        instances = [
+        resources = [  # noqa
+            # share instances
             db_utils.create_share_instance(
                 share_id=share_id,
                 host='controller-0@fancystore01#pool100',
@@ -4033,28 +4036,71 @@ class ShareInstancesTestCase(test.TestCase):
                 share_id=share_id,
                 host='controller-2@beststore07#pool200',
                 status=constants.STATUS_DELETING),
-        ]
+            # share groups
-        db_utils.create_share(id=share_id, instances=instances)
+            db_utils.create_share_group(
+                share_network_id=share_network_id,
+                host='controller-0@fancystore01#pool200',
+                status=constants.STATUS_AVAILABLE),
+            db_utils.create_share_group(
+                share_network_id=share_network_id,
+                host='controller-0@otherstore02#pool100',
+                status=constants.STATUS_ERROR),
+            db_utils.create_share_group(
+                share_network_id=share_network_id,
+                host='controller-2@beststore07#pool100',
+                status=constants.STATUS_DELETING),
+            # share servers
+            db_utils.create_share_server(
+                share_network_subnet_id=share_network_subnet_id,
+                host='controller-0@fancystore01',
+                status=constants.STATUS_ACTIVE),
+            db_utils.create_share_server(
+                share_network_subnet_id=share_network_subnet_id,
+                host='controller-0@otherstore02#pool100',
+                status=constants.STATUS_ERROR),
+            db_utils.create_share_server(
+                share_network_subnet_id=share_network_subnet_id,
+                host='controller-2@beststore07',
+                status=constants.STATUS_DELETING),
 
-        updates = db_api.share_instances_host_update(self.context,
+        ]
+
+        updates = db_api.share_resources_host_update(self.context,
                                                      current_host,
                                                      new_host)
 
+        expected_updates = {'instances': 0, 'servers': 0, 'groups': 0}
+        self.assertDictMatch(expected_updates, updates)
+        # validate that resources are unmodified:
         share_instances = db_api.share_instances_get_all(
             self.context, filters={'share_id': share_id})
-        self.assertEqual(0, updates)
+        share_groups = db_api.share_group_get_all(
+            self.context, filters={'share_network_id': share_network_id})
+        share_servers = db_api._server_get_query(self.context).filter_by(
+            share_network_subnet_id=share_network_subnet_id).all()
+        self.assertEqual(3, len(share_instances))
+        self.assertEqual(3, len(share_groups))
+        self.assertEqual(3, len(share_servers))
         for share_instance in share_instances:
             self.assertTrue(not share_instance['host'].startswith(new_host))
+        for share_group in share_groups:
+            self.assertTrue(not share_group['host'].startswith(new_host))
+        for share_server in share_servers:
+            self.assertTrue(not share_server['host'].startswith(new_host))
 
-    @ddt.data({'current_host': 'controller-2', 'expected_updates': 1},
+    @ddt.data(
-              {'current_host': 'controller-0@fancystore01',
+        {'current_host': 'controller-2',
-               'expected_updates': 2},
+         'expected_updates': {'instances': 1, 'servers': 2, 'groups': 1}},
-              {'current_host': 'controller-0@fancystore01#pool100',
+        {'current_host': 'controller-0@fancystore01',
-               'expected_updates': 1})
+         'expected_updates': {'instances': 2, 'servers': 1, 'groups': 2}},
+        {'current_host': 'controller-0@fancystore01#pool100',
+         'expected_updates': {'instances': 1, 'servers': 1, 'groups': 0}})
     @ddt.unpack
-    def test_share_instance_host_update_partial_matches(self, current_host,
+    def test_share_resources_host_update_partial_matches(self, current_host,
-                                                        expected_updates):
+                                                         expected_updates):
         share_id = uuidutils.generate_uuid()
+        share_network_id = uuidutils.generate_uuid()
+        share_network_subnet_id = uuidutils.generate_uuid()
         if '@' in current_host:
             if '#' in current_host:
                 new_host = 'new-controller-X@backendX#poolX'
@@ -4062,7 +4108,11 @@ class ShareInstancesTestCase(test.TestCase):
                 new_host = 'new-controller-X@backendX'
         else:
             new_host = 'new-controller-X'
-        instances = [
+        total_updates_expected = (expected_updates['instances']
+                                  + expected_updates['groups']
+                                  + expected_updates['servers'])
+        resources = [  # noqa
+            # share instances
             db_utils.create_share_instance(
                 share_id=share_id,
                 host='controller-0@fancystore01#pool100',
@@ -4075,19 +4125,50 @@ class ShareInstancesTestCase(test.TestCase):
                 share_id=share_id,
                 host='controller-2@beststore07#pool200',
                 status=constants.STATUS_DELETING),
+            # share groups
+            db_utils.create_share_group(
+                share_network_id=share_network_id,
+                host='controller-0@fancystore01#pool101',
+                status=constants.STATUS_ACTIVE),
+            db_utils.create_share_group(
+                share_network_id=share_network_id,
+                host='controller-0@fancystore01#pool101',
+                status=constants.STATUS_ERROR),
+            db_utils.create_share_group(
+                share_network_id=share_network_id,
+                host='controller-2@beststore07#pool200',
+                status=constants.STATUS_DELETING),
+            # share servers
+            db_utils.create_share_server(
+                share_network_subnet_id=share_network_subnet_id,
+                host='controller-0@fancystore01#pool100',
+                status=constants.STATUS_ACTIVE),
+            db_utils.create_share_server(
+                share_network_subnet_id=share_network_subnet_id,
+                host='controller-2@fancystore01',
+                status=constants.STATUS_ERROR),
+            db_utils.create_share_server(
+                share_network_subnet_id=share_network_subnet_id,
+                host='controller-2@beststore07#pool200',
+                status=constants.STATUS_DELETING),
         ]
-        db_utils.create_share(id=share_id, instances=instances)
 
-        actual_updates = db_api.share_instances_host_update(
+        actual_updates = db_api.share_resources_host_update(
             self.context, current_host, new_host)
 
         share_instances = db_api.share_instances_get_all(
             self.context, filters={'share_id': share_id})
+        share_groups = db_api.share_group_get_all(
+            self.context, filters={'share_network_id': share_network_id})
+        share_servers = db_api._server_get_query(self.context).filter_by(
+            share_network_subnet_id=share_network_subnet_id).all()
 
-        host_updates = [si for si in share_instances if
+        updated_resources = [
-                        si['host'].startswith(new_host)]
+            res for res in share_instances + share_groups + share_servers
-        self.assertEqual(actual_updates, expected_updates)
+            if res['host'].startswith(new_host)
-        self.assertEqual(expected_updates, len(host_updates))
+        ]
+        self.assertEqual(expected_updates, actual_updates)
+        self.assertEqual(total_updates_expected, len(updated_resources))
 
     def test_share_instances_status_update(self):
         for i in range(1, 3):

releasenotes/notes/bug-1881098-1895323-manila-manage-update-host-fixes-bbbc4fe2da48cae9.yaml

@@ -2,4 +2,4 @@
 fixes:
   - |
     The ``manila-manage share update_host`` command now updates the host
-    attribute of share servers in addition to shares.
+    attribute of share servers and share groups in addition to shares.