Remove usage of novaclient from Watcher

To finish the migration from novaclient to openstacksdk, this change removes all usage of novaclient and drops it as a dependency. It also changes the ironic and maas helpers to use the nova_helper instead of a nova client directly. Change-Id: I24bc6ccc32963d06300908cc4e82c440caa3264d Signed-off-by: jgilaber <jgilaber@redhat.com>
2026-01-27 19:59:27 +01:00
parent 8546305266
commit 67b0be383e
17 changed files with 90 additions and 119 deletions
--- a/releasenotes/notes/prepare-openstacksdk-migration-73cc43ab26ed47e6.yaml
+++ b/releasenotes/notes/prepare-openstacksdk-migration-73cc43ab26ed47e6.yaml
@@ -10,4 +10,5 @@ deprecations:
    configuration options in the [nova] configuration group.
 other:
  - |
-    Added support for openstacksdk as an alternative to novaclient.
+    Added support for openstacksdk as an alternative to novaclient. Usage of
+    novaclient has been removed and python-novaclient has been removed as a dependency.
--- a/requirements.txt
+++ b/requirements.txt
@@ -35,7 +35,6 @@ gnocchiclient>=7.0.1 # Apache-2.0
 openstacksdk>=4.4.0 # Apache-2.0
 python-cinderclient>=3.5.0 # Apache-2.0
 python-keystoneclient>=3.15.0 # Apache-2.0
-python-novaclient>=14.1.0 # Apache-2.0
 python-observabilityclient>=1.1.0 # Apache-2.0
 python-openstackclient>=3.14.0 # Apache-2.0
 python-ironicclient>=2.5.0 # Apache-2.0
--- a/watcher/common/clients.py
+++ b/watcher/common/clients.py
@@ -11,6 +11,7 @@
 # under the License.

 import debtcollector
+import microversion_parse
 from oslo_config import cfg
 import warnings

@@ -21,8 +22,6 @@ from keystoneauth1 import adapter as ka_adapter
 from keystoneauth1 import loading as ka_loading
 from keystoneauth1 import session as ka_session
 from keystoneclient import client as keyclient
-from novaclient import api_versions as nova_api_versions
-from novaclient import client as nvclient
 from openstack import connection

 from watcher.common import context
@@ -110,8 +109,11 @@ def check_min_nova_api_version(config_version):
    :raises: ValueError if the configured version is less than the required
        minimum
    """
-    min_required = nova_api_versions.APIVersion(MIN_NOVA_API_VERSION)
-    if nova_api_versions.APIVersion(config_version) < min_required:
+    min_required = microversion_parse.parse_version_string(
+        MIN_NOVA_API_VERSION
+    )
+
+    if microversion_parse.parse_version_string(config_version) < min_required:
        raise ValueError(f'Invalid nova.api_version {config_version}. '
                         f'{MIN_NOVA_API_VERSION} or greater is required.')

@@ -125,7 +127,6 @@ class OpenStackClients:
    def reset_clients(self):
        self._session = None
        self._keystone = None
-        self._nova = None
        self._gnocchi = None
        self._cinder = None
        self._ironic = None
@@ -168,23 +169,6 @@ class OpenStackClients:

        return self._keystone

-    @exception.wrap_keystone_exception
-    def nova(self):
-        if self._nova:
-            return self._nova
-
-        novaclient_version = CONF.nova.api_version
-
-        check_min_nova_api_version(novaclient_version)
-
-        nova_endpoint_type = self._get_client_option('nova', 'endpoint_type')
-        nova_region_name = self._get_client_option('nova', 'region_name')
-        self._nova = nvclient.Client(novaclient_version,
-                                     endpoint_type=nova_endpoint_type,
-                                     region_name=nova_region_name,
-                                     session=self.session)
-        return self._nova
-
    @exception.wrap_keystone_exception
    def gnocchi(self):
        if self._gnocchi:
--- a/watcher/common/metal_helper/base.py
+++ b/watcher/common/metal_helper/base.py
@@ -17,6 +17,7 @@ import abc

 from watcher.common import exception
 from watcher.common.metal_helper import constants as metal_constants
+from watcher.common import nova_helper


 class BaseMetalNode(abc.ABC):
@@ -69,7 +70,7 @@ class BaseMetalHelper(abc.ABC):
    @property
    def nova_client(self):
        if not getattr(self, "_nova_client", None):
-            self._nova_client = self._osc.nova()
+            self._nova_client = nova_helper.NovaHelper(self._osc)
        return self._nova_client

    @abc.abstractmethod
--- a/watcher/common/metal_helper/ironic.py
+++ b/watcher/common/metal_helper/ironic.py
@@ -74,7 +74,9 @@ class IronicHelper(base.BaseMetalHelper):
                            'of ironic node %s', node.uuid)
                continue

-            hypervisor_node = self.nova_client.hypervisors.get(hypervisor_id)
+            hypervisor_node = self.nova_client.get_compute_node_by_uuid(
+                hypervisor_id
+            )
            if hypervisor_node is None:
                LOG.warning('Cannot find hypervisor %s', hypervisor_id)
                continue
@@ -88,7 +90,9 @@ class IronicHelper(base.BaseMetalHelper):
        ironic_node = self._client.node.get(node_id)
        compute_node_id = ironic_node.extra.get('compute_node_id')
        if compute_node_id:
-            compute_node = self.nova_client.hypervisors.get(compute_node_id)
+            compute_node = self.nova_client.get_compute_node_by_uuid(
+                compute_node_id
+            )
        else:
            compute_node = None
        return IronicNode(ironic_node, compute_node, self._client)
--- a/watcher/common/metal_helper/maas.py
+++ b/watcher/common/metal_helper/maas.py
@@ -94,7 +94,10 @@ class MaasHelper(base.BaseMetalHelper):
            self._client.machines.list,
            timeout=CONF.maas_client.timeout)

-        compute_nodes = self.nova_client.hypervisors.list()
+        compute_nodes = self.nova_client.get_compute_node_list(
+            filter_ironic_nodes=False
+        )
+
        compute_node_map = dict()
        for compute_node in compute_nodes:
            compute_node_map[compute_node.hypervisor_hostname] = compute_node
@@ -111,7 +114,7 @@ class MaasHelper(base.BaseMetalHelper):
        return out_list

    def _get_compute_node_by_hostname(self, hostname):
-        compute_nodes = self.nova_client.hypervisors.search(
+        compute_nodes = self.nova_client.get_compute_node_by_hostname(
            hostname, detailed=True)
        for compute_node in compute_nodes:
            if compute_node.hypervisor_hostname == hostname:
--- a/watcher/common/nova_helper.py
+++ b/watcher/common/nova_helper.py
@@ -374,6 +374,7 @@ class NovaHelper:
        """
        self._config_overrides = False
        self._override_deprecated_configs()
+        clients.check_min_nova_api_version(CONF.nova.api_version)
        self.osc = osc if osc else clients.OpenStackClients()
        self.cinder = self.osc.cinder()
        self._create_sdk_connection(
@@ -434,17 +435,23 @@ class NovaHelper:

    @nova_retries
    @handle_nova_error("Compute node")
-    def get_compute_node_list(self):
-        """Get the list of all compute nodes (hypervisors).
+    def get_compute_node_list(self, filter_ironic_nodes=True):
+        """Get the list of compute nodes (hypervisors).

-        Baremetal (ironic) nodes are filtered out from the result.
-
-        :returns: list of Hypervisor wrapper objects
+        :param filter_ironic_nodes: If True, exclude baremetal (ironic) nodes
+            from the returned list. Defaults to True.
+        :returns: List of Hypervisor objects.
        """
        hypervisors = self.connection.compute.hypervisors(details=True)
        # filter out baremetal nodes from hypervisors
-        compute_nodes = [Hypervisor.from_openstacksdk(node) for node in
-                         hypervisors if node.hypervisor_type != 'ironic']
+        compute_nodes = [
+            Hypervisor.from_openstacksdk(node) for node in hypervisors
+        ]
+        if filter_ironic_nodes:
+            compute_nodes = [
+                node for node in compute_nodes
+                if node.hypervisor_type != 'ironic'
+            ]
        return compute_nodes

    @nova_retries
--- a/watcher/decision_engine/datasources/grafana.py
+++ b/watcher/decision_engine/datasources/grafana.py
@@ -48,7 +48,6 @@ class GrafanaHelper(base.DataSourceBase):
    def __init__(self, osc=None):
        """:param osc: an OpenStackClients instance"""
        self.osc = osc if osc else clients.OpenStackClients()
-        self.nova = self.osc.nova()
        self.configured = False
        self._base_url = None
        self._headers = None
--- a/watcher/decision_engine/model/collector/nova.py
+++ b/watcher/decision_engine/model/collector/nova.py
@@ -223,7 +223,6 @@ class NovaModelBuilder(base.BaseModelBuilder):
        self.model = None
        self.model_scope = dict()
        self.no_model_scope_flag = False
-        self.nova = osc.nova()
        self.nova_helper = nova_helper.NovaHelper(osc=self.osc)
        self.placement_helper = placement_helper.PlacementHelper(osc=self.osc)
        self.executor = threading.DecisionEngineThreadPool()
@@ -464,7 +463,7 @@ class NovaModelBuilder(base.BaseModelBuilder):
        limit = len(instances) if len(instances) <= 1000 else -1
        # Get all servers on this compute host.
        # Note that the advantage of passing the limit parameter is
-        # that it can speed up the call time of novaclient. 1000 is
+        # that it can speed up the call time of Nova API. 1000 is
        # the default maximum number of return servers provided by
        # compute API. If we need to request more than 1000 servers,
        # we can set limit=-1. For details, please see:
--- a/watcher/decision_engine/strategy/strategies/saving_energy.py
+++ b/watcher/decision_engine/strategy/strategies/saving_energy.py
@@ -83,7 +83,6 @@ class SavingEnergy(base.SavingEnergyBaseStrategy):

        super().__init__(config, osc)
        self._metal_helper = None
-        self._nova_client = None

        self.with_vms_node_pool = []
        self.free_poweron_node_pool = []
@@ -97,12 +96,6 @@ class SavingEnergy(base.SavingEnergyBaseStrategy):
            self._metal_helper = metal_helper_factory.get_helper(self.osc)
        return self._metal_helper

-    @property
-    def nova_client(self):
-        if not self._nova_client:
-            self._nova_client = self.osc.nova()
-        return self._nova_client
-
    @classmethod
    def get_name(cls):
        return "saving_energy"
--- a/watcher/tests/unit/common/metal_helper/test_base.py
+++ b/watcher/tests/unit/common/metal_helper/test_base.py
@@ -18,6 +18,7 @@ from unittest import mock
 from watcher.common import exception
 from watcher.common.metal_helper import base as m_helper_base
 from watcher.common.metal_helper import constants as m_constants
+from watcher.common import nova_helper
 from watcher.tests.unit import base


@@ -92,5 +93,4 @@ class TestBaseMetalHelper(base.TestCase):
        self._helper = MockMetalHelper(self._osc)

    def test_nova_client_attr(self):
-        self.assertEqual(self._osc.nova.return_value,
-                         self._helper.nova_client)
+        self.assertIsInstance(self._helper.nova_client, nova_helper.NovaHelper)
--- a/watcher/tests/unit/common/metal_helper/test_ironic.py
+++ b/watcher/tests/unit/common/metal_helper/test_ironic.py
@@ -13,6 +13,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.

+import fixtures
 from unittest import mock

 from watcher.common.metal_helper import constants as m_constants
@@ -75,7 +76,9 @@ class TestIronicHelper(base.TestCase):
        super().setUp()

        self._mock_osc = mock.Mock()
-        self._mock_nova_client = self._mock_osc.nova.return_value
+        self._mock_nova_client = self.useFixture(
+            fixtures.MockPatch("watcher.common.nova_helper.NovaHelper")
+        ).mock.return_value
        self._mock_ironic_client = self._mock_osc.ironic.return_value
        self._helper = ironic.IronicHelper(osc=self._mock_osc)

@@ -92,7 +95,7 @@ class TestIronicHelper(base.TestCase):

        self._mock_ironic_client.node.list.return_value = mock_machines
        self._mock_ironic_client.node.get.side_effect = mock_machines
-        self._mock_nova_client.hypervisors.get.side_effect = (
+        self._mock_nova_client.get_compute_node_by_uuid.side_effect = (
            mock_hypervisor, None)

        out_nodes = self._helper.list_compute_nodes()
@@ -111,8 +114,8 @@ class TestIronicHelper(base.TestCase):

        out_node = self._helper.get_node(mock.sentinel.id)

-        self.assertEqual(self._mock_nova_client.hypervisors.get.return_value,
-                         out_node._nova_node)
+        self._mock_nova_client.get_compute_node_by_uuid.return_value = \
+            out_node._nova_node
        self.assertEqual(self._mock_ironic_client, out_node._ironic_client)
        self.assertEqual(mock_machine, out_node._ironic_node)

@@ -122,7 +125,7 @@ class TestIronicHelper(base.TestCase):

        out_node = self._helper.get_node(mock.sentinel.id)

-        self._mock_nova_client.hypervisors.get.assert_not_called()
+        self._mock_nova_client.get_compute_node_by_uuid.assert_not_called()
        self.assertIsNone(out_node._nova_node)
        self.assertEqual(self._mock_ironic_client, out_node._ironic_client)
        self.assertEqual(mock_machine, out_node._ironic_node)
--- a/watcher/tests/unit/common/metal_helper/test_maas.py
+++ b/watcher/tests/unit/common/metal_helper/test_maas.py
@@ -13,6 +13,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.

+import fixtures
 from unittest import mock

 try:
@@ -77,7 +78,9 @@ class TestMaasHelper(base.TestCase):
        super().setUp()

        self._mock_osc = mock.Mock()
-        self._mock_nova_client = self._mock_osc.nova.return_value
+        self._mock_nova_client = self.useFixture(
+            fixtures.MockPatch("watcher.common.nova_helper.NovaHelper")
+        ).mock.return_value
        self._mock_maas_client = self._mock_osc.maas.return_value
        self._helper = maas.MaasHelper(osc=self._mock_osc)

@@ -97,7 +100,8 @@ class TestMaasHelper(base.TestCase):
        ]

        self._mock_maas_client.machines.list.return_value = mock_machines
-        self._mock_nova_client.hypervisors.list.return_value = mock_hypervisors
+        self._mock_nova_client.get_compute_node_list.return_value = \
+            mock_hypervisors

        out_nodes = self._helper.list_compute_nodes()
        self.assertEqual(1, len(out_nodes))
@@ -116,7 +120,7 @@ class TestMaasHelper(base.TestCase):
            mock.Mock(hypervisor_hostname="compute-0"),
            mock.Mock(hypervisor_hostname="compute-01"),
        ]
-        self._mock_nova_client.hypervisors.search.return_value = (
+        self._mock_nova_client.get_compute_node_by_hostname.return_value = (
            mock_compute_nodes)

        out_node = self._helper.get_node(mock.sentinel.id)
--- a/watcher/tests/unit/common/test_clients.py
+++ b/watcher/tests/unit/common/test_clients.py
@@ -21,7 +21,6 @@ from ironicclient import client as irclient
 from ironicclient.v1 import client as irclient_v1
 from keystoneauth1 import adapter as ka_adapter
 from keystoneauth1 import loading as ka_loading
-from novaclient import client as nvclient

 from watcher.common import clients
 from watcher.common import context
@@ -110,50 +109,6 @@ class TestClients(TestBaseClients):
        self.assertEqual(expected['project_domain_id'],
                         sess.auth._project_domain_id)

-    @mock.patch.object(nvclient, 'Client')
-    @mock.patch.object(clients.OpenStackClients, 'session')
-    def test_clients_nova(self, mock_session, mock_call):
-        osc = clients.OpenStackClients()
-        osc._nova = None
-        osc.nova()
-        mock_call.assert_called_once_with(
-            CONF.nova.api_version,
-            endpoint_type=CONF.nova_client.endpoint_type,
-            region_name=CONF.nova_client.region_name,
-            session=mock_session)
-
-    @mock.patch.object(clients.OpenStackClients, 'session')
-    def test_clients_nova_diff_vers(self, mock_session):
-        CONF.set_override('api_version', '2.60', group='nova')
-        osc = clients.OpenStackClients()
-        osc._nova = None
-        osc.nova()
-        self.assertEqual('2.60', osc.nova().api_version.get_string())
-
-    @mock.patch.object(clients.OpenStackClients, 'session')
-    def test_clients_nova_bad_min_version(self, mock_session):
-        CONF.set_override('api_version', '2.47', group='nova')
-        osc = clients.OpenStackClients()
-        osc._nova = None
-        ex = self.assertRaises(ValueError, osc.nova)
-        self.assertIn('Invalid nova.api_version 2.47', str(ex))
-
-    @mock.patch.object(clients.OpenStackClients, 'session')
-    def test_clients_nova_diff_endpoint(self, mock_session):
-        CONF.set_override('endpoint_type', 'publicURL', group='nova_client')
-        osc = clients.OpenStackClients()
-        osc._nova = None
-        osc.nova()
-        self.assertEqual('publicURL', osc.nova().client.interface)
-
-    @mock.patch.object(clients.OpenStackClients, 'session')
-    def test_clients_nova_cached(self, mock_session):
-        osc = clients.OpenStackClients()
-        osc._nova = None
-        nova = osc.nova()
-        nova_cached = osc.nova()
-        self.assertEqual(nova, nova_cached)
-
    @mock.patch.object(gnclient, 'Client')
    @mock.patch.object(clients.OpenStackClients, 'session')
    def test_clients_gnocchi(self, mock_session, mock_call):
--- a/watcher/tests/unit/common/test_nova_helper.py
+++ b/watcher/tests/unit/common/test_nova_helper.py
@@ -1096,6 +1096,37 @@ class TestNovaHelper(test_utils.NovaResourcesMixin, base.TestCase):
        self.assertEqual(hypervisor1_name,
                         compute_nodes[0].hypervisor_hostname)

+    def test_get_compute_node_list_with_ironic(self, mock_cinder):
+        nova_util = nova_helper.NovaHelper()
+        hypervisor1_id = utils.generate_uuid()
+        hypervisor1_name = "fake_hypervisor_1"
+        hypervisor1 = self.create_openstacksdk_hypervisor(
+            id=hypervisor1_id, name=hypervisor1_name,
+            hypervisor_type="QEMU"
+        )
+
+        hypervisor2_id = utils.generate_uuid()
+        hypervisor2_name = "fake_ironic"
+        hypervisor2 = self.create_openstacksdk_hypervisor(
+            id=hypervisor2_id, name=hypervisor2_name,
+            hypervisor_type="ironic"
+        )
+
+        self.mock_connection.compute.hypervisors.return_value = [
+            hypervisor1, hypervisor2
+        ]
+
+        compute_nodes = nova_util.get_compute_node_list(
+            filter_ironic_nodes=False
+        )
+
+        # baremetal node should be included
+        self.assertEqual(2, len(compute_nodes))
+        self.assertEqual(hypervisor1_name,
+                         compute_nodes[0].hypervisor_hostname)
+        self.assertEqual(hypervisor2_name,
+                         compute_nodes[1].hypervisor_hostname)
+
    def test_find_instance(self, mock_cinder):
        nova_util = nova_helper.NovaHelper()
        kwargs = {
@@ -2077,7 +2108,6 @@ class TestServerMigrationWrapper(test_utils.NovaResourcesMixin, base.TestCase):
        self.assertNotEqual(mig1a, "not-a-migration")


-@mock.patch.object(clients.OpenStackClients, 'nova', autospec=True)
@mock.patch.object(clients.OpenStackClients, 'cinder', autospec=True)
 class TestNovaHelperConfigOverrides(base.TestCase):
    """Test suite for the NovaHelper config override functionality.
@@ -2091,7 +2121,7 @@ class TestNovaHelperConfigOverrides(base.TestCase):
            fixtures.MockPatch("watcher.common.clients.get_sdk_connection")
        )

-    def test_endpoint_type_override_public_url(self, mock_cinder, mock_nova):
+    def test_endpoint_type_override_public_url(self, mock_cinder):
        """Test endpoint_type publicURL is converted to public."""
        self.flags(endpoint_type='publicURL', group='nova_client')

@@ -2099,7 +2129,7 @@ class TestNovaHelperConfigOverrides(base.TestCase):

        self.assertEqual(['public'], CONF.nova.valid_interfaces)

-    def test_endpoint_type_override_internal_url(self, mock_cinder, mock_nova):
+    def test_endpoint_type_override_internal_url(self, mock_cinder):
        """Test endpoint_type internalURL is converted to internal."""
        self.flags(endpoint_type='internalURL', group='nova_client')

@@ -2107,7 +2137,7 @@ class TestNovaHelperConfigOverrides(base.TestCase):

        self.assertEqual(['internal'], CONF.nova.valid_interfaces)

-    def test_endpoint_type_override_admin_url(self, mock_cinder, mock_nova):
+    def test_endpoint_type_override_admin_url(self, mock_cinder):
        """Test endpoint_type adminURL is converted to admin."""
        self.flags(endpoint_type='adminURL', group='nova_client')

@@ -2115,8 +2145,7 @@ class TestNovaHelperConfigOverrides(base.TestCase):

        self.assertEqual(['admin'], CONF.nova.valid_interfaces)

-    def test_endpoint_type_override_without_url_suffix(
-            self, mock_cinder, mock_nova):
+    def test_endpoint_type_override_without_url_suffix(self, mock_cinder):
        """Test endpoint_type without URL suffix is preserved."""
        self.flags(endpoint_type='public', group='nova_client')

@@ -2124,8 +2153,7 @@ class TestNovaHelperConfigOverrides(base.TestCase):

        self.assertEqual(['public'], CONF.nova.valid_interfaces)

-    def test_endpoint_type_override_internal_without_suffix(
-            self, mock_cinder, mock_nova):
+    def test_endpoint_type_override_internal_without_suffix(self, mock_cinder):
        """Test endpoint_type internal without suffix is preserved."""
        self.flags(endpoint_type='internal', group='nova_client')

@@ -2133,8 +2161,7 @@ class TestNovaHelperConfigOverrides(base.TestCase):

        self.assertEqual(['internal'], CONF.nova.valid_interfaces)

-    def test_endpoint_type_override_admin_without_suffix(
-            self, mock_cinder, mock_nova):
+    def test_endpoint_type_override_admin_without_suffix(self, mock_cinder):
        """Test endpoint_type admin without suffix is preserved."""
        self.flags(endpoint_type='admin', group='nova_client')

--- a/watcher/tests/unit/decision_engine/datasources/test_grafana_helper.py
+++ b/watcher/tests/unit/decision_engine/datasources/test_grafana_helper.py
@@ -20,7 +20,6 @@ from unittest import mock
 from oslo_config import cfg
 from oslo_log import log

-from watcher.common import clients
 from watcher.common import exception
 from watcher.decision_engine.datasources import grafana
 from watcher.tests.unit import base
@@ -32,7 +31,6 @@ CONF = cfg.CONF
 LOG = log.getLogger(__name__)


-@mock.patch.object(clients.OpenStackClients, 'nova', mock.Mock())
 class TestGrafana(base.BaseTestCase):
    """Test the GrafanaHelper datasource

--- a/watcher/tests/unit/decision_engine/strategy/strategies/test_saving_energy.py
+++ b/watcher/tests/unit/decision_engine/strategy/strategies/test_saving_energy.py
@@ -16,7 +16,6 @@

 from unittest import mock

-from watcher.common import clients
 from watcher.common.metal_helper import constants as m_constants
 from watcher.common import utils
 from watcher.decision_engine.strategy import strategies
@@ -35,10 +34,6 @@ class TestSavingEnergy(TestBaseStrategy):
        self._metal_helper = mock.Mock()
        self._metal_helper.list_compute_nodes.return_value = self.fake_nodes

-        p_nova = mock.patch.object(clients.OpenStackClients, 'nova')
-        self.m_nova = p_nova.start()
-        self.addCleanup(p_nova.stop)
-
        self.m_c_model.return_value = self.fake_c_cluster.generate_scenario_1()

        self.strategy = strategies.SavingEnergy(
@@ -50,7 +45,6 @@ class TestSavingEnergy(TestBaseStrategy):
        self.strategy.free_used_percent = 10.0
        self.strategy.min_free_hosts_num = 1
        self.strategy._metal_helper = self._metal_helper
-        self.strategy._nova_client = self.m_nova

    def test_get_hosts_pool_with_vms_node_pool(self):
        self._metal_helper.list_compute_nodes.return_value = [