From 6cb4e2fa83a34346ebafffa2980f997f478653d1 Mon Sep 17 00:00:00 2001 From: jgilaber Date: Thu, 29 May 2025 18:35:49 +0200 Subject: [PATCH] Enable storage model collector by default By default Watcher enables only the compute model collector [1]. This change enables the storage one as well, since otherwise when doing volume migration the model quickly becomes obsolete if there are new volumes created while an audit is running. The storage model is only enabled if a cinder service is registered in keystone. [1] https://docs.openstack.org/watcher/latest/configuration/watcher.html#collector.collector_plugins Assisted-By: Cursor Closes-Bug: 2111785 Change-Id: I864d3fc12d6364f1932cf5d2348a6b68169641e9 Signed-off-by: jgilaber --- .zuul.yaml | 10 ++++ ...odel_enabled_default-48c197f5a540956c.yaml | 7 +++ watcher/common/keystone_helper.py | 13 ++++ watcher/conf/collector.py | 2 +- .../model/collector/manager.py | 25 +++++++- .../strategy/strategies/base.py | 2 +- watcher/tests/cmd/test_decision_engine.py | 3 + watcher/tests/common/test_keystone_helper.py | 60 +++++++++++++++++++ .../tests/decision_engine/test_scheduling.py | 5 ++ watcher/tests/fixtures/watcher.py | 24 ++++++++ 10 files changed, 148 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/storage_model_enabled_default-48c197f5a540956c.yaml create mode 100644 watcher/tests/common/test_keystone_helper.py diff --git a/.zuul.yaml b/.zuul.yaml index 7704a48b6..0f8999841 100644 --- a/.zuul.yaml +++ b/.zuul.yaml @@ -32,6 +32,11 @@ period: 120 watcher_cluster_data_model_collectors.storage: period: 120 + $CINDER_CONF: + # enable notifications in compute node, by default they are only + # configured in the controller + oslo_messaging_notifications: + driver: messagingv2 devstack_services: watcher-api: false watcher-decision-engine: true @@ -63,6 +68,11 @@ period: 120 watcher_cluster_data_model_collectors.storage: period: 120 + $CINDER_CONF: + # enable notifications in compute node, by default they are only + # configured in the controller + oslo_messaging_notifications: + driver: messagingv2 test-config: $TEMPEST_CONFIG: compute: diff --git a/releasenotes/notes/storage_model_enabled_default-48c197f5a540956c.yaml b/releasenotes/notes/storage_model_enabled_default-48c197f5a540956c.yaml new file mode 100644 index 000000000..ac25b6a60 --- /dev/null +++ b/releasenotes/notes/storage_model_enabled_default-48c197f5a540956c.yaml @@ -0,0 +1,7 @@ +--- +upgrade: + - | + The storage model collector is now enabled by default if a cinder service exists + in the deployment. This is done to prevent the storage model becoming obsolete + when running volume migrations if new volumes are created and the model was not + enabled in the configuration. diff --git a/watcher/common/keystone_helper.py b/watcher/common/keystone_helper.py index 3733fd1d6..4cfcaec14 100644 --- a/watcher/common/keystone_helper.py +++ b/watcher/common/keystone_helper.py @@ -88,3 +88,16 @@ class KeystoneHelper(object): message=(_("Domain name seems ambiguous: %s") % name_or_id)) return domains[0] + + def is_service_enabled_by_type(self, svc_type): + services = self.keystone.services.list(type=svc_type) + svcs_enabled = [svc for svc in services if svc.enabled] + if len(svcs_enabled) == 0: + LOG.warning(f"Service enabled not found for type: {svc_type}") + return False + elif len(svcs_enabled) > 1: + LOG.warning(f"Multiple services found for type: {svc_type}") + return False + # if there is only one enabled service, consider it a valid + # case + return True diff --git a/watcher/conf/collector.py b/watcher/conf/collector.py index 2577f3a97..0d4673352 100644 --- a/watcher/conf/collector.py +++ b/watcher/conf/collector.py @@ -22,7 +22,7 @@ collector = cfg.OptGroup(name='collector', COLLECTOR_OPTS = [ cfg.ListOpt('collector_plugins', - default=['compute'], + default=['compute', 'storage'], help=""" The cluster data model plugin names. diff --git a/watcher/decision_engine/model/collector/manager.py b/watcher/decision_engine/model/collector/manager.py index a606951c0..c508370ea 100644 --- a/watcher/decision_engine/model/collector/manager.py +++ b/watcher/decision_engine/model/collector/manager.py @@ -18,23 +18,46 @@ # limitations under the License. from oslo_config import cfg +from oslo_log import log +from watcher.common import clients +from watcher.common import keystone_helper from watcher.common import utils from watcher.decision_engine.loading import default +LOG = log.getLogger(__name__) + class CollectorManager(object): - def __init__(self): + def __init__(self, osc=None): self.collector_loader = default.ClusterDataModelCollectorLoader() self._collectors = None self._notification_endpoints = None + self.osc = osc if osc else clients.OpenStackClients() + + def is_cinder_enabled(self): + keystone = keystone_helper.KeystoneHelper(self.osc) + if keystone.is_service_enabled_by_type(svc_type='block-storage'): + return True + elif keystone.is_service_enabled_by_type(svc_type='volumev3'): + # volumev3 is a commonly used alias for the cinder keystone service + # type + return True + return False def get_collectors(self): if self._collectors is None: collectors = utils.Struct() collector_plugins = cfg.CONF.collector.collector_plugins for collector_name in collector_plugins: + if collector_name == 'storage': + if not self.is_cinder_enabled(): + LOG.warning( + "Block storage service is not enabled," + " skipping storage collector" + ) + continue collector = self.collector_loader.load(collector_name) collectors[collector_name] = collector self._collectors = collectors diff --git a/watcher/decision_engine/strategy/strategies/base.py b/watcher/decision_engine/strategy/strategies/base.py index 6ba103c0c..6f1200eb3 100644 --- a/watcher/decision_engine/strategy/strategies/base.py +++ b/watcher/decision_engine/strategy/strategies/base.py @@ -273,7 +273,7 @@ class BaseStrategy(loadable.Loadable, metaclass=abc.ABCMeta): @property def collector_manager(self): if self._collector_manager is None: - self._collector_manager = manager.CollectorManager() + self._collector_manager = manager.CollectorManager(osc=self.osc) return self._collector_manager @property diff --git a/watcher/tests/cmd/test_decision_engine.py b/watcher/tests/cmd/test_decision_engine.py index 66ab0b7cb..5e5c946d8 100644 --- a/watcher/tests/cmd/test_decision_engine.py +++ b/watcher/tests/cmd/test_decision_engine.py @@ -25,6 +25,7 @@ from watcher.common import service as watcher_service from watcher.decision_engine.audit import continuous from watcher.decision_engine import sync from watcher.tests import base +from watcher.tests.fixtures import watcher as watcher_fixtures class TestDecisionEngine(base.BaseTestCase): @@ -49,6 +50,7 @@ class TestDecisionEngine(base.BaseTestCase): continuous.ContinuousAuditHandler, "start") self.m_continuoushandler = p_continuoushandler.start() self.addCleanup(p_continuoushandler.stop) + self.fake_keystone = self.useFixture(watcher_fixtures.KeystoneClient()) def tearDown(self): super(TestDecisionEngine, self).tearDown() @@ -57,5 +59,6 @@ class TestDecisionEngine(base.BaseTestCase): @mock.patch.object(sync.Syncer, "sync", mock.Mock()) @mock.patch.object(service, "launch") def test_run_de_app(self, m_launch): + decisionengine.main() self.assertEqual(1, m_launch.call_count) diff --git a/watcher/tests/common/test_keystone_helper.py b/watcher/tests/common/test_keystone_helper.py new file mode 100644 index 000000000..0db62df43 --- /dev/null +++ b/watcher/tests/common/test_keystone_helper.py @@ -0,0 +1,60 @@ +# Copyright 2025 Red Hat, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. +# + +from unittest import mock + +import keystoneclient.v3.services as ks_service + +from watcher.common import keystone_helper +from watcher.tests import base as test +from watcher.tests.fixtures import watcher as watcher_fixtures + + +class TestKeystoneHelper(test.TestCase): + def setUp(self): + super(TestKeystoneHelper, self).setUp() + self.fake_keystone = self.useFixture(watcher_fixtures.KeystoneClient()) + self.keystone_svs = self.fake_keystone.m_keystone.return_value.services + + self.keystone_helper = keystone_helper.KeystoneHelper() + + def test_is_service_enabled(self): + self.keystone_svs.list.return_value = [mock.MagicMock( + spec=ks_service.Service, enabled=True)] + self.assertTrue( + self.keystone_helper.is_service_enabled_by_type('block-storage')) + self.keystone_svs.list.assert_called_once_with(type='block-storage') + + def test_is_service_enabled_not_found(self): + self.keystone_svs.list.return_value = [] + self.assertFalse( + self.keystone_helper.is_service_enabled_by_type('block-storage')) + self.keystone_svs.list.assert_called_once_with(type='block-storage') + + def test_is_service_enabled_with_multiple_services_one_enabled(self): + self.keystone_svs.list.return_value = [ + mock.MagicMock(spec=ks_service.Service, enabled=True), + mock.MagicMock(spec=ks_service.Service, enabled=False)] + self.assertTrue( + self.keystone_helper.is_service_enabled_by_type('block-storage')) + self.keystone_svs.list.assert_called_once_with(type='block-storage') + + def test_is_service_enabled_multiple_services_two_enabled(self): + self.keystone_svs.list.return_value = [ + mock.MagicMock(spec=ks_service.Service, enabled=True), + mock.MagicMock(spec=ks_service.Service, enabled=True)] + self.assertFalse( + self.keystone_helper.is_service_enabled_by_type('block-storage')) + self.keystone_svs.list.assert_called_once_with(type='block-storage') diff --git a/watcher/tests/decision_engine/test_scheduling.py b/watcher/tests/decision_engine/test_scheduling.py index d9b80b700..ea6abb842 100644 --- a/watcher/tests/decision_engine/test_scheduling.py +++ b/watcher/tests/decision_engine/test_scheduling.py @@ -32,6 +32,7 @@ from watcher import objects from watcher.tests import base from watcher.tests.db import base as db_base from watcher.tests.decision_engine.model import faker_cluster_state +from watcher.tests.fixtures import watcher as watcher_fixtures from watcher.tests.objects import utils as obj_utils @@ -80,6 +81,10 @@ class TestCancelOngoingAudits(db_base.DbTestCase): @mock.patch.object(objects.audit.Audit, 'list') class TestDecisionEngineSchedulingService(base.TestCase): + def setUp(self): + super(TestDecisionEngineSchedulingService, self).setUp() + self.fake_keystone = self.useFixture(watcher_fixtures.KeystoneClient()) + @mock.patch.object( default_loading.ClusterDataModelCollectorLoader, 'load') @mock.patch.object( diff --git a/watcher/tests/fixtures/watcher.py b/watcher/tests/fixtures/watcher.py index 74ec2d134..8176792cc 100644 --- a/watcher/tests/fixtures/watcher.py +++ b/watcher/tests/fixtures/watcher.py @@ -17,8 +17,13 @@ import logging as std_logging import os +from unittest import mock import fixtures +import keystoneclient.v3.client as ks_client +import keystoneclient.v3.services as ks_service + +from watcher.common.clients import OpenStackClients class NullHandler(std_logging.Handler): @@ -113,3 +118,22 @@ class StandardLogging(fixtures.Fixture): self.useFixture( fixtures.MonkeyPatch('oslo_log.log.setup', fake_logging_setup)) + + +class KeystoneClient(fixtures.Fixture): + """Fixture to mock the keystone client. + + Prevents the OpenStackClients from actually connecting to the + keystone server. + Mocks the services.get and services.list methods. + """ + + def setUp(self): + super(KeystoneClient, self).setUp() + mock_osc = mock.patch.object( + OpenStackClients, "keystone", + return_value=mock.MagicMock(spec=ks_client.Client)) + self.m_keystone = mock_osc.start() + self.m_keystone.return_value.services = mock.MagicMock( + spec=ks_service.ServiceManager) + self.addCleanup(mock_osc.stop)