openstack/watcher
Code Issues Proposed changes

Remove dead NovaHelper methods

Browse Source 
Remove 7 unused methods from NovaHelper that have no production
usage. These methods have been thoroughly verified against all 290
production files and are only used in unit tests or not used at all.

High-confidence removals:
- delete_instance(): Remnant from old snapshot-based migration
  workflow replaced in commit 4179c352 with Nova native migration API
- get_availability_zone_list(): Added in 2016 for audit scope, never
  used
- get_instance_by_name(): Added in 2019 refactor, never integrated
- get_instances_by_node(): Added in 2019 refactor, never integrated
- get_service(): Added in 2017 for graph model, never integrated

Security-related removals:
- swap_volume(): Usage removed in commit 4fca7d24 due to data loss
  risks and security concerns with credential forging
- wait_for_instance_status(): Unused since 2015, redundant with
  wait_for_instance_state()

All methods reverified against complete production file list.
Removing approximately 139 lines of code reduces maintenance burden
and eliminates code with known security issues.

Generated-By: cursor
Assisted-By: claude-code
Closes-Bug: #2126969
Change-Id: Ic4a40a78fbb7c1bb9f659d8e801f25713c66c461
Signed-off-by: Sean Mooney <work@seanmooney.info>
This commit is contained in:
Sean Mooney
2025-10-07 09:32:26 +01:00
parent 13721b3804
commit 6bfbee1265
3 changed files with 14 additions and 152 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
releasenotes/notes/remove-dead-nova-helper-methods-62125e08.yaml Normal file
View File

@@ -0,0 +1,14 @@
---
other:
- |
Seven unused methods have been removed from the NovaHelper class.
These methods had zero production usage and were either remnants from
old workflows or never integrated into production code. The removed
methods include delete_instance() (dead since the 2018 workflow
refactor in commit 4179c352), swap_volume() (removed in 2025 due to
security concerns), wait_for_instance_status() (unused since 2015),
get_availability_zone_list(), get_instance_by_name(),
get_instances_by_node(), and get_service(). This removal eliminates
approximately 139 lines of dead code and reduces maintenance burden
with no user-facing impact.

82
watcher/common/nova_helper.py
View File

@@ -130,32 +130,15 @@ class NovaHelper:
self.nova.servers.list(search_opts={"all_tenants": True,
"uuid": instance_uuid})]
def get_instance_by_name(self, instance_name):
return [instance for instance in
self.nova.servers.list(search_opts={"all_tenants": True,
"name": instance_name})]
def get_instances_by_node(self, host):
return [instance for instance in
self.nova.servers.list(search_opts={"all_tenants": True,
"host": host},
limit=-1)]
def get_flavor_list(self):
return self.nova.flavors.list(**{'is_public': None})
def get_service(self, service_id):
return self.nova.services.find(id=service_id)
def get_aggregate_list(self):
return self.nova.aggregates.list()
def get_aggregate_detail(self, aggregate_id):
return self.nova.aggregates.get(aggregate_id)
def get_availability_zone_list(self):
return self.nova.availability_zones.list(detailed=True)
def get_service_list(self):
return self.nova.services.list(binary='nova-compute')
@@ -461,23 +444,6 @@ class NovaHelper:
return status
def delete_instance(self, instance_id):
"""This method deletes a given instance.
:param instance_id: the unique id of the instance to delete.
"""
LOG.debug("Trying to remove instance %s ...", instance_id)
instance = self.find_instance(instance_id)
if not instance:
LOG.debug("Instance not found: %s", instance_id)
return False
else:
self.nova.servers.delete(instance_id)
LOG.debug("Instance %s removed.", instance_id)
return True
def stop_instance(self, instance_id):
"""This method stops a given instance.
@@ -546,60 +512,12 @@ class NovaHelper:
retry -= 1
return getattr(server, 'OS-EXT-STS:vm_state') == state
def wait_for_instance_status(self, instance, status_list, retry, sleep):
"""Waits for instance to be in a specific status
The status can be one of the following
: BUILD, ACTIVE, ERROR, VERIFY_RESIZE, SHUTOFF
:param instance: instance object.
:param status_list: tuple containing the list of
status we are waiting for
:param retry: how many times to retry
:param sleep: seconds to sleep between the retries
"""
if not instance:
return False
while instance.status not in status_list and retry:
LOG.debug("Current instance status: %s", instance.status)
time.sleep(sleep)
instance = self.nova.servers.get(instance.id)
retry -= 1
LOG.debug("Current instance status: %s", instance.status)
return instance.status in status_list
def get_hostname(self, instance):
return str(getattr(instance, 'OS-EXT-SRV-ATTR:host'))
def get_running_migration(self, instance_id):
return self.nova.server_migrations.list(server=instance_id)
def swap_volume(self, old_volume, new_volume,
retry=120, retry_interval=10):
"""Swap old_volume for new_volume"""
attachments = old_volume.attachments
instance_id = attachments[0]['server_id']
# do volume update
self.nova.volumes.update_server_volume(
instance_id, old_volume.id, new_volume.id)
while getattr(new_volume, 'status') != 'in-use' and retry:
new_volume = self.cinder.volumes.get(new_volume.id)
LOG.debug('Waiting volume update to %s', new_volume)
time.sleep(retry_interval)
retry -= 1
LOG.debug("retry count: %s", retry)
if getattr(new_volume, 'status') != "in-use":
LOG.error("Volume update retry timeout or error")
return False
host_name = getattr(new_volume, "os-vol-host-attr:host")
LOG.debug(
"Volume update succeeded : "
"Volume %(volume)s is now on host '%(host)s'.",
{'volume': new_volume.id, 'host': host_name})
return True
def _check_nova_api_version(self, client, version):
api_version = api_versions.APIVersion(version_str=version)
try:

70
watcher/tests/common/test_nova_helper.py
View File

@@ -278,23 +278,6 @@ class TestNovaHelper(base.TestCase):
result = nova_util.start_instance(instance_id)
self.assertFalse(result)
@mock.patch.object(time, 'sleep', mock.Mock())
def test_delete_instance(self, mock_cinder, mock_nova):
nova_util = nova_helper.NovaHelper()
instance_id = utils.generate_uuid()
# verify that the method will return False when the instance does
# not exist.
self.fake_nova_find_list(nova_util, fake_find=None, fake_list=None)
result = nova_util.delete_instance(instance_id)
self.assertFalse(result)
# verify that the method will return True when the instance exists.
server = self.fake_server(instance_id)
self.fake_nova_find_list(nova_util, fake_find=server, fake_list=None)
result = nova_util.delete_instance(instance_id)
self.assertTrue(result)
@mock.patch.object(time, 'sleep', mock.Mock())
def test_resize_instance(self, mock_cinder, mock_nova):
nova_util = nova_helper.NovaHelper()
@@ -522,27 +505,6 @@ class TestNovaHelper(base.TestCase):
volume.availability_zone = kwargs.get('availability_zone', 'nova')
return volume
@mock.patch.object(time, 'sleep', mock.Mock())
def test_swap_volume(self, mock_cinder, mock_nova):
nova_util = nova_helper.NovaHelper()
server = self.fake_server(self.instance_uuid)
self.fake_nova_find_list(nova_util, fake_find=server, fake_list=server)
old_volume = self.fake_volume(
status='in-use', attachments=[{'server_id': self.instance_uuid}])
new_volume = self.fake_volume(
id=utils.generate_uuid(), status='in-use')
result = nova_util.swap_volume(old_volume, new_volume)
self.assertTrue(result)
# verify that the method will return False when the status of
# new_volume is 'fake-use'.
new_volume = self.fake_volume(
id=utils.generate_uuid(), status='fake-use')
result = nova_util.swap_volume(old_volume, new_volume)
self.assertFalse(result)
@mock.patch.object(time, 'sleep', mock.Mock())
def test_wait_for_volume_status(self, mock_cinder, mock_nova):
nova_util = nova_helper.NovaHelper()
@@ -585,38 +547,6 @@ class TestNovaHelper(base.TestCase):
result = nova_util._check_nova_api_version(nova_util.nova, "2.56")
self.assertFalse(result)
@mock.patch.object(time, 'sleep', mock.Mock())
def test_wait_for_instance_status(self, mock_cinder, mock_nova):
nova_util = nova_helper.NovaHelper()
instance = self.fake_server(self.instance_uuid)
# verify that the method will return True when the status of instance
# is in the expected status.
result = nova_util.wait_for_instance_status(
instance,
('ACTIVE', 'ERROR'),
5,
10)
self.assertTrue(result)
# verify that the method will return False when the instance is None.
result = nova_util.wait_for_instance_status(
None,
('ACTIVE', 'ERROR'),
5,
10)
self.assertFalse(result)
# verify that the method will return False when the status of instance
# is not in the expected status.
self.fake_nova_find_list(nova_util, fake_find=instance, fake_list=None)
result = nova_util.wait_for_instance_status(
instance,
('ERROR'),
5,
10)
self.assertFalse(result)
@mock.patch.object(time, 'sleep', mock.Mock())
def test_confirm_resize(self, mock_cinder, mock_nova):
nova_util = nova_helper.NovaHelper()