From df5d18710ab6efadfee9f5932ba1a65a927a5896 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adolfo=20G=C3=B3mez=20Garc=C3=ADa?= Date: Thu, 14 Mar 2024 23:20:44 +0100 Subject: [PATCH] Fix service assignments and serialization issues on tests after refactoring storage --- .../services/OpenStack/deployment_fixed.py | 2 +- .../uds/services/OpenStack/service_fixed.py | 3 +- .../src/uds/services/Proxmox/service_fixed.py | 3 +- server/src/uds/services/Xen/service_fixed.py | 2 +- server/tests/core/test_environment.py | 14 ++- .../test_serialization_deployment.py | 2 +- .../test_serialization_deployment.py | 2 +- .../test_serialization_deployment.py | 2 +- .../services/openstack/test_userservice.py | 5 +- .../openstack/test_userservice_fixed.py | 90 +++++++++---------- .../ovirt/test_serialization_deployment.py | 2 +- server/tests/services/proxmox/fixtures.py | 2 +- .../proxmox/test_serialization_deployment.py | 2 +- .../services/proxmox/test_service_fixed.py | 11 ++- .../proxmox/test_userservice_fixed.py | 11 ++- .../xen/test_serialization_deployment.py | 2 +- 16 files changed, 80 insertions(+), 75 deletions(-) diff --git a/server/src/uds/services/OpenStack/deployment_fixed.py b/server/src/uds/services/OpenStack/deployment_fixed.py index 59a88b848..29317b46c 100644 --- a/server/src/uds/services/OpenStack/deployment_fixed.py +++ b/server/src/uds/services/OpenStack/deployment_fixed.py @@ -77,7 +77,7 @@ class OpenStackUserServiceFixed(FixedUserService, autoserializable.AutoSerializa except Exception as e: return self._error(f'Machine not found: {e}') - if server_info.status == 'stopped': + if server_info.power_state == openstack_types.PowerState.SHUTDOWN: self._queue = [Operation.START, Operation.FINISH] return self._execute_queue() diff --git a/server/src/uds/services/OpenStack/service_fixed.py b/server/src/uds/services/OpenStack/service_fixed.py index bb196dba5..7f04d8470 100644 --- a/server/src/uds/services/OpenStack/service_fixed.py +++ b/server/src/uds/services/OpenStack/service_fixed.py @@ -153,7 +153,8 @@ class OpenStackServiceFixed(FixedService): # pylint: disable=too-many-public-me return [ gui.choice_item(k, servers[k]) for k in self.machines.as_list() - if k not in assigned_servers and k in servers + if k not in assigned_servers + and k in servers # Only machines not assigned, and that exists on provider will be available ] def assign_from_assignables( diff --git a/server/src/uds/services/Proxmox/service_fixed.py b/server/src/uds/services/Proxmox/service_fixed.py index d63f0b6db..d300ba53e 100644 --- a/server/src/uds/services/Proxmox/service_fixed.py +++ b/server/src/uds/services/Proxmox/service_fixed.py @@ -53,7 +53,6 @@ if typing.TYPE_CHECKING: logger = logging.getLogger(__name__) - class ProxmoxServiceFixed(FixedService): # pylint: disable=too-many-public-methods """ Proxmox fixed machines service. @@ -134,7 +133,7 @@ class ProxmoxServiceFixed(FixedService): # pylint: disable=too-many-public-meth gui.choice_item(k, vms[int(k)]) for k in self.machines.as_list() if k not in assigned_vms - and int(k) not in vms + and int(k) in vms # Only machines not assigned, and that exists on provider will be available ] def assign_from_assignables( diff --git a/server/src/uds/services/Xen/service_fixed.py b/server/src/uds/services/Xen/service_fixed.py index 84f6f1bdb..29b685cf2 100644 --- a/server/src/uds/services/Xen/service_fixed.py +++ b/server/src/uds/services/Xen/service_fixed.py @@ -179,7 +179,7 @@ class XenFixedService(FixedService): # pylint: disable=too-many-public-methods gui.choice_item(k, vms[k]) for k in self.machines.as_list() if k not in assigned_vms - and k in vms # Only machines not assigned, and that exists on xen will be available + and k in vms # Only machines not assigned, and that exists on provider will be available ] def assign_from_assignables( diff --git a/server/tests/core/test_environment.py b/server/tests/core/test_environment.py index 0a4620a18..b261281ff 100644 --- a/server/tests/core/test_environment.py +++ b/server/tests/core/test_environment.py @@ -31,15 +31,11 @@ """ Author: Adolfo Gómez, dkmaster at dkmon dot com """ -import collections.abc -import dataclasses -from enum import unique import typing from uds.core import environment from uds.core.util.cache import Cache from uds.core.util.storage import Storage -from uds.core.util.unique_id_generator import UniqueIDGenerator from ..utils.test import UDSTransactionTestCase @@ -60,7 +56,7 @@ class TestEnvironment(UDSTransactionTestCase): self.assertEqual(env.key, expected_key) env.storage.put('test', 'test') - self.assertEqual(env.storage.get('test'), 'test') + self.assertEqual(env.storage.read('test'), 'test') env.cache.put('test', 'test') self.assertEqual(env.cache.get('test'), 'test') @@ -76,10 +72,10 @@ class TestEnvironment(UDSTransactionTestCase): self.assertEqual(env.key, expected_key) if is_persistent: - self.assertEqual(env.storage.get('test'), 'test') + self.assertEqual(env.storage.read('test'), 'test') self.assertEqual(env.cache.get('test'), 'test') else: - self.assertEqual(env.storage.get('test'), None) + self.assertEqual(env.storage.read('test'), None) self.assertEqual(env.cache.get('test'), None) def test_global_environment(self) -> None: @@ -112,7 +108,7 @@ class TestEnvironment(UDSTransactionTestCase): unique_key = env.key # store for later test env.storage.put('test', 'test') - self.assertEqual(env.storage.get('test'), 'test') + self.assertEqual(env.storage.read('test'), 'test') env.cache.put('test', 'test') self.assertEqual(env.cache.get('test'), 'test') @@ -125,5 +121,5 @@ class TestEnvironment(UDSTransactionTestCase): self.assertIsInstance(env.storage, Storage) self.assertIsInstance(env._id_generators, dict) self.assertEqual(env.key, unique_key) - self.assertEqual(env.storage.get('test'), None) + self.assertEqual(env.storage.read('test'), None) self.assertEqual(env.cache.get('test'), None) diff --git a/server/tests/services/opengnsys/test_serialization_deployment.py b/server/tests/services/opengnsys/test_serialization_deployment.py index 16dd4de2b..73d7ab163 100644 --- a/server/tests/services/opengnsys/test_serialization_deployment.py +++ b/server/tests/services/opengnsys/test_serialization_deployment.py @@ -116,7 +116,7 @@ class OpenGnsysDeploymentSerializationTest(UDSTransactionTestCase): # queue is kept on "storage", so we need always same environment environment = Environment.testing_environment() # Store queue - environment.storage.put_pickle('queue', TEST_QUEUE) + environment.storage.save_pickled('queue', TEST_QUEUE) def _create_instance(unmarshal_data: 'bytes|None' = None) -> deployment.OpenGnsysUserService: instance = deployment.OpenGnsysUserService(environment=environment, service=None) # type: ignore diff --git a/server/tests/services/opennebula/test_serialization_deployment.py b/server/tests/services/opennebula/test_serialization_deployment.py index bc47cd0dc..b9e01735e 100644 --- a/server/tests/services/opennebula/test_serialization_deployment.py +++ b/server/tests/services/opennebula/test_serialization_deployment.py @@ -117,7 +117,7 @@ class OpenNebulaDeploymentSerializationTest(UDSTransactionTestCase): # queue is kept on "storage", so we need always same environment environment = Environment.testing_environment() # Store queue - environment.storage.put_pickle('queue', TEST_QUEUE) + environment.storage.save_pickled('queue', TEST_QUEUE) def _create_instance(unmarshal_data: 'bytes|None' = None) -> deployment.OpenNebulaLiveDeployment: instance = deployment.OpenNebulaLiveDeployment(environment=environment, service=None) # type: ignore diff --git a/server/tests/services/openstack/test_serialization_deployment.py b/server/tests/services/openstack/test_serialization_deployment.py index 648a2a73d..aaf7651e4 100644 --- a/server/tests/services/openstack/test_serialization_deployment.py +++ b/server/tests/services/openstack/test_serialization_deployment.py @@ -116,7 +116,7 @@ class OpenStackDeploymentSerializationTest(UDSTransactionTestCase): # queue is kept on "storage", so we need always same environment environment = Environment.testing_environment() # Store queue - environment.storage.put_pickle('queue', TEST_QUEUE) + environment.storage.save_pickled('queue', TEST_QUEUE) def _create_instance(unmarshal_data: 'bytes|None' = None) -> deployment.OpenStackLiveUserService: instance = deployment.OpenStackLiveUserService(environment=environment, service=None) # type: ignore diff --git a/server/tests/services/openstack/test_userservice.py b/server/tests/services/openstack/test_userservice.py index b3c37d639..be8c34c97 100644 --- a/server/tests/services/openstack/test_userservice.py +++ b/server/tests/services/openstack/test_userservice.py @@ -91,14 +91,15 @@ class TestOpenstackLiveDeployment(UDSTransactionTestCase): self.assertEqual(state, types.states.TaskState.FINISHED, f'Error on {to_test} deployment') + # userservice name is UDS-U- self.assertEqual( - userservice._name[: len(service.get_basename())], + userservice._name[6: 6+len(service.get_basename())], service.get_basename(), f'Error on {to_test} deployment', ) self.assertEqual( len(userservice._name), - len(service.get_basename()) + service.get_lenname(), + len(service.get_basename()) + service.get_lenname() + 6, # for UDS-U- prefix f'Error on {to_test} deployment', ) diff --git a/server/tests/services/openstack/test_userservice_fixed.py b/server/tests/services/openstack/test_userservice_fixed.py index 01d57c0f9..309090359 100644 --- a/server/tests/services/openstack/test_userservice_fixed.py +++ b/server/tests/services/openstack/test_userservice_fixed.py @@ -42,61 +42,61 @@ from ...utils.generators import limited_iterator # We use transactions on some related methods (storage access, etc...) -class TestProxmovLinkedService(UDSTransactionTestCase): +class TestOpenstackLiveService(UDSTransactionTestCase): def test_userservice_fixed_user(self) -> None: """ Test the user service """ - with fixtures.patch_provider_api() as _api: - userservice = fixtures.create_userservice_fixed() - service = userservice.service() - self.assertEqual(service._get_assigned_machines(), set()) + for prov in (fixtures.create_provider_legacy(), fixtures.create_provider()): + with fixtures.patch_provider_api(legacy=prov.legacy) as _api: + service = fixtures.create_fixed_service(prov) # Will use provider patched api + userservice = fixtures.create_fixed_userservice(service) - # patch userservice db_obj() method to return a mock - userservice_db = mock.MagicMock() - userservice.db_obj = mock.MagicMock(return_value=userservice_db) - # Test Deploy for cache, should raise Exception due - # to the fact fixed services cannot have cached items - with self.assertRaises(Exception): - userservice.deploy_for_cache(level=1) + # patch userservice db_obj() method to return a mock + userservice_db = mock.MagicMock() + userservice.db_obj = mock.MagicMock(return_value=userservice_db) + # Test Deploy for cache, should raise Exception due + # to the fact fixed services cannot have cached items + with self.assertRaises(Exception): + userservice.deploy_for_cache(level=1) - state = userservice.deploy_for_user(models.User()) + state = userservice.deploy_for_user(models.User()) - self.assertEqual(state, types.states.TaskState.RUNNING) + self.assertEqual(state, types.states.TaskState.RUNNING) - while state == types.states.TaskState.RUNNING: - state = userservice.check_state() + for _ in limited_iterator(lambda: state == types.states.TaskState.RUNNING, limit=32): + state = userservice.check_state() - self.assertEqual(state, types.states.TaskState.FINISHED) + self.assertEqual(state, types.states.TaskState.FINISHED) - # userservice_db Should have halle set_in_use(True) - userservice_db.set_in_use.assert_called_once_with(True) + # userservice_db Should have halle set_in_use(True) + userservice_db.set_in_use.assert_called_once_with(True) - # vmid should have been assigned, so it must be in the assigned machines - self.assertEqual(set(userservice._vmid), service._get_assigned_machines()) + # vmid should have been assigned, so it must be in the assigned machines + with service._assigned_machines_access() as assigned_machines: + self.assertEqual({userservice._vmid}, assigned_machines) - # Now, let's release the service - state = userservice.destroy() - - self.assertEqual(state, types.states.TaskState.RUNNING) - - while state == types.states.TaskState.RUNNING: - state = userservice.check_state() - - self.assertEqual(state, types.states.TaskState.FINISHED) - - # must be empty now - self.assertEqual(service._get_assigned_machines(), set()) - - # set_ready, machine is "stopped" in this test, so must return RUNNING - state = userservice.set_ready() - self.assertEqual(state, types.states.TaskState.RUNNING) - - for _ in limited_iterator(lambda: state == types.states.TaskState.RUNNING, limit=32): - state = userservice.check_state() - - # Should be finished now - self.assertEqual(state, types.states.TaskState.FINISHED) - - \ No newline at end of file + # Now, let's release the service + state = userservice.destroy() + + self.assertEqual(state, types.states.TaskState.RUNNING) + + while state == types.states.TaskState.RUNNING: + state = userservice.check_state() + + self.assertEqual(state, types.states.TaskState.FINISHED) + + # must be empty now + with service._assigned_machines_access() as assigned_machines: + self.assertEqual(set(), assigned_machines) + + # set_ready, machine is "stopped" in this test, so must return RUNNING + state = userservice.set_ready() + self.assertEqual(state, types.states.TaskState.RUNNING) + + for _ in limited_iterator(lambda: state == types.states.TaskState.RUNNING, limit=32): + state = userservice.check_state() + + # Should be finished now + self.assertEqual(state, types.states.TaskState.FINISHED) diff --git a/server/tests/services/ovirt/test_serialization_deployment.py b/server/tests/services/ovirt/test_serialization_deployment.py index 674542c4a..97ef25330 100644 --- a/server/tests/services/ovirt/test_serialization_deployment.py +++ b/server/tests/services/ovirt/test_serialization_deployment.py @@ -120,7 +120,7 @@ class OvirtDeploymentSerializationTest(UDSTransactionTestCase): # queue is kept on "storage", so we need always same environment environment = Environment.testing_environment() # Store queue - environment.storage.put_pickle('queue', TEST_QUEUE) + environment.storage.save_pickled('queue', TEST_QUEUE) def _create_instance(unmarshal_data: 'bytes|None' = None) -> Deployment: instance = Deployment(environment=environment, service=None) # type: ignore diff --git a/server/tests/services/proxmox/fixtures.py b/server/tests/services/proxmox/fixtures.py index 28cd98e80..275d50502 100644 --- a/server/tests/services/proxmox/fixtures.py +++ b/server/tests/services/proxmox/fixtures.py @@ -400,7 +400,7 @@ SERVICE_LINKED_VALUES_DICT: typing.Final[gui.ValuesDictType] = { 'lenname': 4, } -SERVICE_FIXED_VALUES_DICT: typing.Final[gui.ValuesDictType] = { +SERVICE_FIXED_VALUES_DICT: gui.ValuesDictType = { 'token': '', 'pool': POOLS[0].poolid, 'machines': [str(VMS_INFO[2].vmid), str(VMS_INFO[3].vmid), str(VMS_INFO[4].vmid)], diff --git a/server/tests/services/proxmox/test_serialization_deployment.py b/server/tests/services/proxmox/test_serialization_deployment.py index c1b4dd5fe..4f73250c6 100644 --- a/server/tests/services/proxmox/test_serialization_deployment.py +++ b/server/tests/services/proxmox/test_serialization_deployment.py @@ -123,7 +123,7 @@ class ProxmoxDeploymentSerializationTest(UDSTransactionTestCase): # queue is kept on "storage", so we need always same environment environment = Environment.testing_environment() # Store queue - environment.storage.put_pickle('queue', TEST_QUEUE) + environment.storage.save_pickled('queue', TEST_QUEUE) def _create_instance(unmarshal_data: 'bytes|None' = None) -> Deployment: instance = Deployment(environment=environment, service=fake.fake_service()) diff --git a/server/tests/services/proxmox/test_service_fixed.py b/server/tests/services/proxmox/test_service_fixed.py index 154ddd49b..5a8066381 100644 --- a/server/tests/services/proxmox/test_service_fixed.py +++ b/server/tests/services/proxmox/test_service_fixed.py @@ -104,7 +104,8 @@ class TestProxmovFixedService(UDSTransactionTestCase): self.assertEqual(service.get_and_assign_machine(), vmid2) # Now two machies should be assigned - self.assertEqual(service._get_assigned_machines(), set([vmid, vmid2])) + with service._assigned_machines_access() as assigned_machines: + self.assertEqual(assigned_machines, set([vmid, vmid2])) def test_service_methods_2(self) -> None: with fixtures.patch_provider_api(): @@ -124,9 +125,13 @@ class TestProxmovFixedService(UDSTransactionTestCase): # Remove and free machine # Fist, assign a machine vmid = service.get_and_assign_machine() - self.assertEqual(service._get_assigned_machines(), set([vmid])) + with service._assigned_machines_access() as assigned_machines: + self.assertEqual(assigned_machines, set([vmid])) + + # And now free it self.assertEqual(service.remove_and_free_machine(vmid), types.states.State.FINISHED) - self.assertEqual(service._get_assigned_machines(), set()) + with service._assigned_machines_access() as assigned_machines: + self.assertEqual(assigned_machines, set()) def test_process_snapshot(self) -> None: with fixtures.patch_provider_api() as api: diff --git a/server/tests/services/proxmox/test_userservice_fixed.py b/server/tests/services/proxmox/test_userservice_fixed.py index 01d57c0f9..56a489096 100644 --- a/server/tests/services/proxmox/test_userservice_fixed.py +++ b/server/tests/services/proxmox/test_userservice_fixed.py @@ -51,7 +51,8 @@ class TestProxmovLinkedService(UDSTransactionTestCase): with fixtures.patch_provider_api() as _api: userservice = fixtures.create_userservice_fixed() service = userservice.service() - self.assertEqual(service._get_assigned_machines(), set()) + with service._assigned_machines_access() as assigned_machines: + self.assertEqual(assigned_machines, set()) # patch userservice db_obj() method to return a mock userservice_db = mock.MagicMock() @@ -74,8 +75,9 @@ class TestProxmovLinkedService(UDSTransactionTestCase): userservice_db.set_in_use.assert_called_once_with(True) # vmid should have been assigned, so it must be in the assigned machines - self.assertEqual(set(userservice._vmid), service._get_assigned_machines()) - + with service._assigned_machines_access() as assigned_machines: + self.assertEqual({userservice._vmid}, assigned_machines) + # Now, let's release the service state = userservice.destroy() @@ -87,7 +89,8 @@ class TestProxmovLinkedService(UDSTransactionTestCase): self.assertEqual(state, types.states.TaskState.FINISHED) # must be empty now - self.assertEqual(service._get_assigned_machines(), set()) + with service._assigned_machines_access() as assigned_machines: + self.assertEqual(assigned_machines, set()) # set_ready, machine is "stopped" in this test, so must return RUNNING state = userservice.set_ready() diff --git a/server/tests/services/xen/test_serialization_deployment.py b/server/tests/services/xen/test_serialization_deployment.py index cadb81879..8f4a766c8 100644 --- a/server/tests/services/xen/test_serialization_deployment.py +++ b/server/tests/services/xen/test_serialization_deployment.py @@ -122,7 +122,7 @@ class XenDeploymentSerializationTest(UDSTransactionTestCase): # queue is kept on "storage", so we need always same environment environment = Environment.testing_environment() # Store queue - environment.storage.put_pickle('queue', TEST_QUEUE) + environment.storage.save_pickled('queue', TEST_QUEUE) def _create_instance(unmarshal_data: 'bytes|None' = None) -> Deployment: instance = Deployment(environment=environment, service=None) # type: ignore # service is not used