From b5a8aaf0be168bb6c6b07d5f0e05291e2940fffd Mon Sep 17 00:00:00 2001 From: Steve Baker Date: Thu, 7 May 2020 08:02:43 +1200 Subject: [PATCH] oVirt driver: Close connection after each API call Currently a new connection is created for every API call which is never closed. This results in open connections accumulating over time due to periodic calls like power state polling, which is further compounded by the number of ovirt nodes deployed. This change converts _getvm into a generator based context manager so that the connection can be closed after every call. Change-Id: I30f9695c591f72dae21467b0b31f6f67fad7cc8a (cherry picked from commit 343a57570e28ed85a8d41fd1931f4ce252cdd71b) --- ironic_staging_drivers/ovirt/ovirt.py | 88 +++++++++++-------- .../tests/unit/ovirt/test_ovirt.py | 53 ++++++----- .../ovirt-close-conn-2f87f0312be044a5.yaml | 9 ++ 3 files changed, 91 insertions(+), 59 deletions(-) create mode 100644 releasenotes/notes/ovirt-close-conn-2f87f0312be044a5.yaml diff --git a/ironic_staging_drivers/ovirt/ovirt.py b/ironic_staging_drivers/ovirt/ovirt.py index b500cd3..a03ae48 100644 --- a/ironic_staging_drivers/ovirt/ovirt.py +++ b/ironic_staging_drivers/ovirt/ovirt.py @@ -19,6 +19,9 @@ via oVirt sdk API. For use in dev and test environments. """ + +import contextlib + from ironic.common import boot_devices from ironic.common import exception from ironic.common.i18n import _ @@ -119,11 +122,12 @@ def _parse_driver_info(node): insecure = driver_info['ovirt_insecure'] ovirt_ca_file = driver_info['ovirt_ca_file'] if not insecure and ovirt_ca_file is None: - msg = _("Missing ovirt_ca_file in the node's driver_info") - raise exception.MissingParameterValue(msg) + msg = _("Missing ovirt_ca_file in the node's driver_info") + raise exception.MissingParameterValue(msg) return driver_info +@contextlib.contextmanager def _getvm(driver_info): address = driver_info['ovirt_address'] username = driver_info['ovirt_username'] @@ -149,15 +153,20 @@ def _getvm(driver_info): ca_file=ca_file) vms_service = connection.system_service().vms_service() vmsearch = vms_service.list(search='name=%s' % name) + if vmsearch: + yield vms_service.vm_service(vmsearch[0].id) + else: + raise staging_exception.OVirtError(_("VM with name " + "%s was not found") % name) except sdk.Error as e: LOG.error("Could not fetch information about VM vm %(name)s, " "got error: %(error)s", {'name': name, 'error': e}) raise staging_exception.OVirtError(err=e) - if vmsearch: - return vms_service.vm_service(vmsearch[0].id) - else: - raise staging_exception.OVirtError(_("VM with name " - "%s was not found") % name) + finally: + try: + connection.close() + except sdk.Error as e: + LOG.warning("Error closing connection: %(error)s", {'error': e}) class OVirtPower(base.PowerInterface): @@ -188,8 +197,8 @@ class OVirtPower(base.PowerInterface): """ driver_info = _parse_driver_info(task.node) vm_name = driver_info['ovirt_vm_name'] - vm = _getvm(driver_info) - status = vm.get().status.value + with _getvm(driver_info) as vm: + status = vm.get().status.value if status not in OVIRT_TO_IRONIC_POWER_MAPPING: msg = ("oVirt returned unknown state for node %(node)s " "and vm %(vm)s") @@ -213,26 +222,27 @@ class OVirtPower(base.PowerInterface): """ driver_info = _parse_driver_info(task.node) vm_name = driver_info['ovirt_vm_name'] - vm = _getvm(driver_info) - try: - if target_state == states.POWER_OFF: - vm.stop() - elif target_state == states.POWER_ON: - vm.start() - elif target_state == states.REBOOT: - status = vm.get().status.value - if status == 'down': + with _getvm(driver_info) as vm: + try: + if target_state == states.POWER_OFF: + vm.stop() + elif target_state == states.POWER_ON: vm.start() + elif target_state == states.REBOOT: + status = vm.get().status.value + if status == 'down': + vm.start() + else: + vm.reboot() else: - vm.reboot() - else: - msg = _("'set_power_state' called with invalid power " - "state '%s'") % target_state - raise exception.InvalidParameterValue(msg) - except sdk.Error as e: - LOG.error("Could not change status of VM vm %(name)s " - "got error: %(error)s", {'name': vm_name, 'error': e}) - raise staging_exception.OVirtError(err=e) + msg = _("'set_power_state' called with invalid power " + "state '%s'") % target_state + raise exception.InvalidParameterValue(msg) + except sdk.Error as e: + LOG.error("Could not change status of VM vm %(name)s " + "got error: %(error)s", + {'name': vm_name, 'error': e}) + raise staging_exception.OVirtError(err=e) @task_manager.require_exclusive_lock def reboot(self, task, timeout=None): @@ -291,8 +301,8 @@ class OVirtManagement(base.ManagementInterface): oVirt operation. """ driver_info = _parse_driver_info(task.node) - vm = _getvm(driver_info) - boot_dev = vm.os.boot[0].get_dev() + with _getvm(driver_info) as vm: + boot_dev = vm.os.boot[0].get_dev() persistent = True ironic_boot_dev = OVIRT_TO_IRONIC_DEVICE_MAPPING.get(boot_dev) if not ironic_boot_dev: @@ -324,16 +334,16 @@ class OVirtManagement(base.ManagementInterface): "Invalid boot device %s specified.") % device) driver_info = _parse_driver_info(task.node) - vm = _getvm(driver_info) - try: - boot = otypes.Boot(devices=[otypes.BootDevice(boot_dev)]) - bootos = otypes.OperatingSystem(boot=boot) - vm.update(otypes.Vm(os=bootos)) - except sdk.Error as e: - LOG.error("Setting boot device failed for node %(node_id)s " - "with error: %(error)s", - {'node_id': task.node.uuid, 'error': e}) - raise staging_exception.OVirtError(err=e) + with _getvm(driver_info) as vm: + try: + boot = otypes.Boot(devices=[otypes.BootDevice(boot_dev)]) + bootos = otypes.OperatingSystem(boot=boot) + vm.update(otypes.Vm(os=bootos)) + except sdk.Error as e: + LOG.error("Setting boot device failed for node %(node_id)s " + "with error: %(error)s", + {'node_id': task.node.uuid, 'error': e}) + raise staging_exception.OVirtError(err=e) def get_sensors_data(self, task): """Get sensors data. diff --git a/ironic_staging_drivers/tests/unit/ovirt/test_ovirt.py b/ironic_staging_drivers/tests/unit/ovirt/test_ovirt.py index c0fa2f7..7d3f73e 100644 --- a/ironic_staging_drivers/tests/unit/ovirt/test_ovirt.py +++ b/ironic_staging_drivers/tests/unit/ovirt/test_ovirt.py @@ -62,28 +62,31 @@ class OVirtDriverTestCase(db_base.DbTestCase): self.node['driver_info']['ovirt_address'] = u'127.0.0.1' driver_info = ovirt_power._parse_driver_info(self.node) - ovirt_power._getvm(driver_info) - ovirt_power.sdk.Connection.assert_called_with( - ca_file=None, insecure='False', password='changeme', - url='https://127.0.0.1/ovirt-engine/api', - username='jhendrix@internal' - ) + with ovirt_power._getvm(driver_info): + ovirt_power.sdk.Connection.assert_called_with( + ca_file=None, insecure='False', password='changeme', + url='https://127.0.0.1/ovirt-engine/api', + username='jhendrix@internal' + ) url = ovirt_power.sdk.Connection.mock_calls[0][-1]['url'] self.assertIsInstance(url, six.string_types) + ovirt_power.sdk.Connection.return_value.close.assert_called() + @mock.patch.object(ovirt_power, "sdk", create=True) def test_getvm_unicode(self, sdk): self.node['driver_info']['ovirt_address'] = u'host\u20141' driver_info = ovirt_power._parse_driver_info(self.node) - ovirt_power._getvm(driver_info) - ovirt_power.sdk.Connection.assert_called_with( - ca_file=None, insecure='False', password='changeme', - url=u'https://host\u20141/ovirt-engine/api', - username='jhendrix@internal' - ) + with ovirt_power._getvm(driver_info): + ovirt_power.sdk.Connection.assert_called_with( + ca_file=None, insecure='False', password='changeme', + url=u'https://host\u20141/ovirt-engine/api', + username='jhendrix@internal' + ) url = ovirt_power.sdk.Connection.mock_calls[0][-1]['url'] self.assertIsInstance(url, six.text_type) + ovirt_power.sdk.Connection.return_value.close.assert_called() def test_get_properties(self): expected = list(ovirt_power.PROPERTIES.keys()) @@ -138,18 +141,28 @@ class OVirtDriverTestCase(db_base.DbTestCase): mock_power.assert_called_once_with(task.driver.management, task, boot_devices.DISK) - @mock.patch.object(ovirt_power, '_getvm') - def test_set_reboot_when_down(self, mock_vm): - mock_vm.return_value.get.return_value.status.value = 'down' + @mock.patch.object(ovirt_power, "sdk", create=True) + def test_set_reboot_when_down(self, sdk): + vm = mock.Mock() + vm.get.return_value.status.value = 'down' + conn = sdk.Connection.return_value + vms_service = conn.system_service.return_value.vms_service.return_value + vms_service.vm_service.return_value = vm with task_manager.acquire(self.context, self.node.uuid) as task: task.driver.power.reboot(task) - mock_vm.return_value.start.assert_called_once() + vm.start.assert_called_once() + conn.close.assert_called() - @mock.patch.object(ovirt_power, '_getvm') - def test_set_reboot_when_up(self, mock_vm): - mock_vm.return_value.get.return_value.status.value = 'up' + @mock.patch.object(ovirt_power, "sdk", create=True) + def test_set_reboot_when_up(self, sdk): + vm = mock.Mock() + vm.get.return_value.status.value = 'up' + conn = sdk.Connection.return_value + vms_service = conn.system_service.return_value.vms_service.return_value + vms_service.vm_service.return_value = vm with task_manager.acquire(self.context, self.node.uuid) as task: task.driver.power.reboot(task) - mock_vm.return_value.reboot.assert_called_once() + vm.reboot.assert_called_once() + conn.close.assert_called() diff --git a/releasenotes/notes/ovirt-close-conn-2f87f0312be044a5.yaml b/releasenotes/notes/ovirt-close-conn-2f87f0312be044a5.yaml new file mode 100644 index 0000000..190100f --- /dev/null +++ b/releasenotes/notes/ovirt-close-conn-2f87f0312be044a5.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + oVirt driver: Close connection after each API call + + A new connection was being created for every API call which was + never closed. This resulted in open connections accumulating over time + due to periodic calls like power state polling, which was further + compounded by the number of ovirt nodes deployed.