Refactor compute service actions

Remove checks for services ialready in desired state so actions don't fail, service disable will update reason
Point action at functions in openstack_api to simplify code
Update unit tests

Author: gmatthews20

actions/hv.compute.service.disable.yaml

@@ -7,7 +7,7 @@ parameters:
   timeout:
     default: 5400
   lib_entry_point:
-    default: workflows.hv_service_actions.hv_service_disable
+    default: openstack_api.openstack_service.disable_service
     immutable: true
     type: string
   requires_openstack:

actions/hv.compute.service.enable.yaml

@@ -7,7 +7,7 @@ parameters:
   timeout:
     default: 5400
   lib_entry_point:
-    default: workflows.hv_service_actions.hv_service_enable
+    default: openstack_api.openstack_service.enable_service
     immutable: true
     type: string
   requires_openstack:

lib/openstack_api/openstack_service.py

@@ -5,7 +5,6 @@
 
 def disable_service(
     conn: Connection,
-    service: Service,
     hypervisor_name: str,
     service_binary: str,
     disabled_reason: str,
@@ -19,22 +18,18 @@ def disable_service(
     :param disabled_reason: The reason for disabling the service.
     :return: Returns the Service object.
     """
-
-    if service.status == "enabled":
-        return conn.compute.disable_service(
-            service,
-            host=hypervisor_name,
-            binary=service_binary,
-            disabled_reason=disabled_reason,
-        )
-    raise RuntimeError(
-        f"Failed to disable {service_binary} on {hypervisor_name}. Already disabled."
+    service = conn.compute.find_service(
+        service_binary, ignore_missing=False, host=hypervisor_name
+    )
+    # Needs to be service.id since just passing service won't update the service correctly
+    return conn.compute.disable_service(
+        service=service.id,
+        disabled_reason=disabled_reason,
     )
 
 
 def enable_service(
     conn: Connection,
-    service: Service,
     hypervisor_name: str,
     service_binary: str,
 ) -> Optional[Service]:
@@ -46,11 +41,7 @@ def enable_service(
     :param service_binary: The name of the service.
     :return: Returns the Service object.
     """
-
-    if service.status == "disabled":
-        return conn.compute.enable_service(
-            service, host=hypervisor_name, binary=service_binary
-        )
-    raise RuntimeError(
-        f"Failed to enable {service_binary} on {hypervisor_name}. Already enabled."
+    service = conn.compute.find_service(
+        service_binary, ignore_missing=False, host=hypervisor_name
     )
+    return conn.compute.enable_service(service=service.id)

lib/workflows/hv_service_actions.py

@@ -1,92 +1,51 @@
 from unittest.mock import MagicMock, NonCallableMagicMock
-import pytest
-from openstack_api.openstack_service import disable_service, enable_service
+from lib.openstack_api.openstack_service import enable_service, disable_service
 
 
-def test_disable_service_function_disables_service_when_enabled():
-    """tests that openstack disables the service when it is currently enabled"""
+def test_service_disabled_successfully():
+    """test that hv_service_disable disables an openstack service"""
 
     mock_conn = MagicMock()
-    mock_service = MagicMock()
     mock_hypervisor_name = NonCallableMagicMock()
     mock_service_binary = NonCallableMagicMock()
-    mock_disable_reason = NonCallableMagicMock()
+    mock_disabled_reason = NonCallableMagicMock()
 
-    mock_service.status = "enabled"
+    mock_service = MagicMock()
+    mock_conn.compute.find_service.return_value = mock_service
 
-    res = disable_service(
+    disable_service(
         mock_conn,
-        mock_service,
         mock_hypervisor_name,
         mock_service_binary,
-        mock_disable_reason,
+        mock_disabled_reason,
     )
 
-    mock_conn.compute.disable_service.assert_called_once_with(
-        mock_service,
-        host=mock_hypervisor_name,
-        binary=mock_service_binary,
-        disabled_reason=mock_disable_reason,
+    mock_conn.compute.find_service.assert_called_once_with(
+        mock_service_binary, ignore_missing=False, host=mock_hypervisor_name
     )
 
-    assert res == mock_conn.compute.disable_service.return_value
+    mock_conn.compute.disable_service.assert_called_once_with(
+        service=mock_service.id,
+        disabled_reason=mock_disabled_reason,
+    )
 
 
-def test_disable_service_function_returns_none_when_disabled():
-    """tests that openstack returns None when the service is currently disabled"""
+def test_hv_service_enabled_successfully():
+    """test that hv_service_enable enables an openstack service"""
 
     mock_conn = MagicMock()
-    mock_service = MagicMock()
     mock_hypervisor_name = NonCallableMagicMock()
     mock_service_binary = NonCallableMagicMock()
-    mock_disable_reason = NonCallableMagicMock()
 
-    mock_service.status = "disabled"
-
-    with pytest.raises(RuntimeError):
-        disable_service(
-            mock_conn,
-            mock_service,
-            mock_hypervisor_name,
-            mock_service_binary,
-            mock_disable_reason,
-        )
-
-
-def test_enable_service_function_enables_service_when_disabled():
-    """tests that openstack enables the service when it is currently disabled"""
-
-    mock_conn = MagicMock()
     mock_service = MagicMock()
-    mock_hypervisor_name = NonCallableMagicMock()
-    mock_service_binary = NonCallableMagicMock()
+    mock_conn.compute.find_service.return_value = mock_service
 
-    mock_service.status = "disabled"
+    enable_service(mock_conn, mock_hypervisor_name, mock_service_binary)
 
-    res = enable_service(
-        mock_conn, mock_service, mock_hypervisor_name, mock_service_binary
+    mock_conn.compute.find_service.assert_called_once_with(
+        mock_service_binary, ignore_missing=False, host=mock_hypervisor_name
     )
 
     mock_conn.compute.enable_service.assert_called_once_with(
-        mock_service,
-        host=mock_hypervisor_name,
-        binary=mock_service_binary,
+        service=mock_service.id,
     )
-
-    assert res == mock_conn.compute.enable_service.return_value
-
-
-def test_enable_service_function_returns_none_when_enabled():
-    """tests that openstack returns none when the service is currently enabled"""
-
-    mock_conn = MagicMock()
-    mock_service = MagicMock()
-    mock_hypervisor_name = NonCallableMagicMock()
-    mock_service_binary = NonCallableMagicMock()
-
-    mock_service.status = "enabled"
-
-    with pytest.raises(RuntimeError):
-        enable_service(
-            mock_conn, mock_service, mock_hypervisor_name, mock_service_binary
-        )