From 3ef8b6638a84a73e9ff2db857d04f0112e45202b Mon Sep 17 00:00:00 2001 From: Pierre Riteau Date: Tue, 12 Feb 2019 17:17:09 +0000 Subject: [PATCH] Move hosts back to freepool in case of failure When reserving multiple hosts, if a host fails to be moved to the host aggregate of the reservation, all hosts previously moved into the same aggregate were left in it instead of being moved back to the freepool. This would result in failures to start future leases allocating any of these hosts. Change-Id: I007a9c7c644b6488a7a08e9ebf3029b15cb33b45 Closes-Bug: #1791733 --- blazar/plugins/oshosts/host_plugin.py | 17 +++-- .../oshosts/test_physical_host_plugin.py | 6 +- blazar/tests/utils/openstack/test_nova.py | 23 +++++- blazar/utils/openstack/nova.py | 72 ++++++++++++------- ...sts-back-to-freepool-041fcda9fb3fc6c3.yaml | 7 ++ 5 files changed, 87 insertions(+), 38 deletions(-) create mode 100644 releasenotes/notes/move-hosts-back-to-freepool-041fcda9fb3fc6c3.yaml diff --git a/blazar/plugins/oshosts/host_plugin.py b/blazar/plugins/oshosts/host_plugin.py index 9f98d30d..a9d50af7 100644 --- a/blazar/plugins/oshosts/host_plugin.py +++ b/blazar/plugins/oshosts/host_plugin.py @@ -174,11 +174,12 @@ class PhysicalHostPlugin(base.BasePlugin, nova.NovaClientWrapper): """Add the hosts in the pool.""" host_reservation = db_api.host_reservation_get(resource_id) pool = nova.ReservationPool() + hosts = [] for allocation in db_api.host_allocation_get_all_by_values( reservation_id=host_reservation['reservation_id']): host = db_api.host_get(allocation['compute_host_id']) - pool.add_computehost(host_reservation['aggregate_id'], - host['service_name']) + hosts.append(host['service_name']) + pool.add_computehost(host_reservation['aggregate_id'], hosts) def before_end(self, resource_id): """Take an action before the end of a lease.""" @@ -665,16 +666,18 @@ class PhysicalHostPlugin(base.BasePlugin, nova.NovaClientWrapper): str(min_hosts) + '-' + str(max_hosts), dates_after['start_date'], dates_after['end_date']) if len(host_ids) >= min_hosts: + new_hosts = [] pool = nova.ReservationPool() for host_id in host_ids: db_api.host_allocation_create( {'compute_host_id': host_id, 'reservation_id': reservation_id}) - if reservation_status == status.reservation.ACTIVE: - # Add new host into the aggregate. - new_host = db_api.host_get(host_id) - pool.add_computehost(host_reservation['aggregate_id'], - new_host['service_name']) + new_host = db_api.host_get(host_id) + new_hosts.append(new_host['service_name']) + if reservation_status == status.reservation.ACTIVE: + # Add new hosts into the aggregate. + pool.add_computehost(host_reservation['aggregate_id'], + new_hosts) else: raise manager_ex.NotEnoughHostsAvailable() diff --git a/blazar/tests/plugins/oshosts/test_physical_host_plugin.py b/blazar/tests/plugins/oshosts/test_physical_host_plugin.py index 349d7b20..b2b344e5 100644 --- a/blazar/tests/plugins/oshosts/test_physical_host_plugin.py +++ b/blazar/tests/plugins/oshosts/test_physical_host_plugin.py @@ -1423,8 +1423,7 @@ class PhysicalHostPluginTestCase(tests.TestCase): 'reservation_id': '706eb3bc-07ed-4383-be93-b32845ece672' } ) - add_computehost.assert_called_with( - 1, 'host3_hostname') + add_computehost.assert_called_with(1, ['host3_hostname']) host_reservation_update.assert_called_with( '91253650-cc34-4c4f-bbe8-c943aa7d0c9b', {'count_range': '1-3'} @@ -1685,8 +1684,7 @@ class PhysicalHostPluginTestCase(tests.TestCase): self.fake_phys_plugin.on_start(u'04de74e8-193a-49d2-9ab8-cba7b49e45e8') - add_computehost.assert_called_with( - 1, 'host1_hostname') + add_computehost.assert_called_with(1, ['host1_hostname']) def test_before_end_with_no_action(self): host_reservation_get = self.patch(self.db_api, 'host_reservation_get') diff --git a/blazar/tests/utils/openstack/test_nova.py b/blazar/tests/utils/openstack/test_nova.py index 819b832a..79cfe888 100644 --- a/blazar/tests/utils/openstack/test_nova.py +++ b/blazar/tests/utils/openstack/test_nova.py @@ -16,6 +16,7 @@ import uuid as uuidgen from keystoneauth1 import session from keystoneauth1 import token_endpoint +import mock from novaclient import client as nova_client from novaclient import exceptions as nova_exceptions from novaclient.v2 import availability_zones @@ -158,7 +159,7 @@ class ReservationPoolTestCase(tests.TestCase): def _patch_get_aggregate_from_name_or_id(self): def get_fake_aggregate(*args): - if self.freepool_name in args: + if self.freepool_name in args or self.fake_freepool.id in args: return self.fake_freepool else: return self.fake_aggregate @@ -286,7 +287,7 @@ class ReservationPoolTestCase(tests.TestCase): check0 = self.nova.aggregates.add_host check0.assert_any_call(self.fake_aggregate.id, 'host3') check1 = self.nova.aggregates.remove_host - check1.assert_any_call(self.fake_aggregate.id, 'host3') + check1.assert_any_call(self.fake_freepool.id, 'host3') def test_add_computehost_with_host_id(self): # NOTE(sbauza): Freepool.hosts only contains names of hosts, not UUIDs @@ -335,6 +336,24 @@ class ReservationPoolTestCase(tests.TestCase): check = self.nova.aggregates.add_host check.assert_called_once_with(self.fake_freepool.id, 'host2') + def test_add_computehost_revert(self): + self._patch_get_aggregate_from_name_or_id() + self.fake_freepool.hosts = ['host1', 'host2'] + self.assertRaises(manager_exceptions.HostNotInFreePool, + self.pool.add_computehost, + 'pool', ['host1', 'host2', 'host3']) + + check0 = self.nova.aggregates.add_host + check0.assert_has_calls([mock.call(self.fake_aggregate.id, 'host1'), + mock.call(self.fake_aggregate.id, 'host2'), + mock.call('freepool', 'host1'), + mock.call('freepool', 'host2')]) + check1 = self.nova.aggregates.remove_host + check1.assert_has_calls([mock.call(self.fake_freepool.id, 'host1'), + mock.call(self.fake_freepool.id, 'host2'), + mock.call(self.fake_aggregate.id, 'host1'), + mock.call(self.fake_aggregate.id, 'host2')]) + def test_remove_computehost_from_freepool(self): self._patch_get_aggregate_from_name_or_id() self.pool.remove_computehost(self.freepool_name, 'host3') diff --git a/blazar/utils/openstack/nova.py b/blazar/utils/openstack/nova.py index 54251a99..87dc578a 100644 --- a/blazar/utils/openstack/nova.py +++ b/blazar/utils/openstack/nova.py @@ -327,20 +327,24 @@ class ReservationPool(NovaClientWrapper): except manager_exceptions.AggregateNotFound: return [] - def add_computehost(self, pool, host, stay_in=False): - """Add a compute host to an aggregate. + def add_computehost(self, pool, hosts, stay_in=False): + """Add compute host(s) to an aggregate. - The `host` must exist otherwise raise an error - and the `host` must be in the freepool. + Each host must exist and be in the freepool, otherwise raise an error. :param pool: Name or UUID of the pool to rattach the host - :param host: Name (not UUID) of the host to associate - :type host: str + :param hosts: Names (not UUID) of hosts to associate + :type host: str or list of str Return the related aggregate. Raise an aggregate exception if something wrong. """ + if not isinstance(hosts, list): + hosts = [hosts] + + added_hosts = [] + removed_hosts = [] agg = self.get_aggregate_from_name_or_id(pool) try: @@ -348,26 +352,44 @@ class ReservationPool(NovaClientWrapper): except manager_exceptions.AggregateNotFound: raise manager_exceptions.NoFreePool() - if freepool_agg.id != agg.id and not stay_in: - if host not in freepool_agg.hosts: - raise manager_exceptions.HostNotInFreePool( - host=host, freepool_name=freepool_agg.name) - LOG.info("removing host '%(host)s' from aggregate freepool " - "%(name)s", {'host': host, 'name': freepool_agg.name}) - try: - self.remove_computehost(freepool_agg.id, host) - except nova_exception.NotFound: - raise manager_exceptions.HostNotFound(host=host) - - LOG.info("adding host '%(host)s' to aggregate %(id)s", - {'host': host, 'id': agg.id}) try: - return self.nova.aggregates.add_host(agg.id, host) - except nova_exception.NotFound: - raise manager_exceptions.HostNotFound(host=host) - except nova_exception.Conflict as e: - raise manager_exceptions.AggregateAlreadyHasHost( - pool=pool, host=host, nova_exception=str(e)) + for host in hosts: + if freepool_agg.id != agg.id and not stay_in: + if host not in freepool_agg.hosts: + raise manager_exceptions.HostNotInFreePool( + host=host, freepool_name=freepool_agg.name) + LOG.info("removing host '%(host)s' from freepool " + "aggregate %(name)s", + {'host': host, 'name': freepool_agg.name}) + try: + self.remove_computehost(freepool_agg.id, host) + removed_hosts.append(host) + except nova_exception.NotFound: + raise manager_exceptions.HostNotFound(host=host) + + LOG.info("adding host '%(host)s' to aggregate %(id)s", + {'host': host, 'id': agg.id}) + try: + self.nova.aggregates.add_host(agg.id, host) + added_hosts.append(host) + except nova_exception.NotFound: + raise manager_exceptions.HostNotFound(host=host) + except nova_exception.Conflict as e: + raise manager_exceptions.AggregateAlreadyHasHost( + pool=pool, host=host, nova_exception=str(e)) + except Exception as e: + if added_hosts: + LOG.warn('Removing hosts added to aggregate %s: %s', + agg.id, added_hosts) + for host in added_hosts: + self.nova.aggregates.remove_host(agg.id, host) + if removed_hosts: + LOG.warn('Adding hosts back to freepool: %s', removed_hosts) + for host in removed_hosts: + self.nova.aggregates.add_host(freepool_agg.name, host) + raise e + + return self.get_aggregate_from_name_or_id(pool) def remove_all_computehosts(self, pool): """Remove all compute hosts attached to an aggregate.""" diff --git a/releasenotes/notes/move-hosts-back-to-freepool-041fcda9fb3fc6c3.yaml b/releasenotes/notes/move-hosts-back-to-freepool-041fcda9fb3fc6c3.yaml new file mode 100644 index 00000000..c5b3f663 --- /dev/null +++ b/releasenotes/notes/move-hosts-back-to-freepool-041fcda9fb3fc6c3.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + If a host fails to be moved to a host aggregate while starting a + reservation, any host previously moved into the same aggregate is now moved + back to the freepool. This helps to prevent failures to start future leases + allocating the same hosts.