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.