Remove dead code and consolidate redundant patterns

Remove MigrationStatus enum (inline strings), dead _previous_hash
state, unreachable VALIDATION_PATTERNS entries, unused
_check_critical_services params, redundant validate_input in
_host_fence, single-use _cleanup_dict_by_condition helper, and
redundant requests.exceptions import. Consolidate 5 manual
error+traceback log pairs into _safe_log_exception calls.
Deduplicate _get_nova_connection by delegating to
_establish_nova_connection.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: lmiccini

templates/instanceha/bin/instanceha.py

@@ -12,7 +12,6 @@
 from enum import Enum
 import concurrent.futures
 import requests
-import requests.exceptions
 import yaml
 import subprocess
 from yaml.loader import SafeLoader
@@ -179,18 +178,9 @@ class MatchType(Enum):
     ZONE = "zone"
 
 
-class MigrationStatus(Enum):
-    """Migration status values."""
-    COMPLETED = "completed"
-    DONE = "done"
-    ERROR = "error"
-    FAILED = "failed"
-    CANCELLED = "cancelled"
-
-
 # Migration status constants for checking migration state
-MIGRATION_STATUS_COMPLETED = [MigrationStatus.COMPLETED.value, MigrationStatus.DONE.value]
-MIGRATION_STATUS_ERROR = [MigrationStatus.ERROR.value, MigrationStatus.FAILED.value, MigrationStatus.CANCELLED.value]
+MIGRATION_STATUS_COMPLETED = ["completed", "done"]
+MIGRATION_STATUS_ERROR = ["error", "failed", "cancelled"]
 
 
 # Dataclasses
@@ -312,8 +302,6 @@ def _retry_with_backoff(func, max_attempts, label, delay=FENCING_RETRY_DELAY_SEC
     'power_action': (['on', 'off', 'status', 'reset', 'cycle', 'soft', 'On', 'ForceOff',
                       'GracefulShutdown', 'GracefulRestart', 'ForceRestart', 'Nmi',
                       'ForceOn', 'PushPowerButton'], None),
-    'ip_address': ('ip', None),  # Special marker for IP address validation
-    'port': ('port', None),  # Special marker for port validation
     'username': (r'^[a-zA-Z0-9_-]{1,64}$', USERNAME_MAX_LENGTH),
     'hostname': (r'^[a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?$', USERNAME_MAX_LENGTH)
 }
@@ -768,7 +756,6 @@ def __init__(self, config_manager: ConfigManager, cloud_client: Optional[OpenSta
         self.current_hash = ""
         self.hash_update_successful = False
         self._last_hash_time = 0
-        self._previous_hash = ""
         self.ready = False
         self.k8s_api_reachable = False
 
@@ -838,7 +825,6 @@ def update_health_hash(self, hash_interval: Optional[int] = None) -> None:
         if current_timestamp - self._last_hash_time > hash_interval:
             self.current_hash = hashlib.sha256(str(current_timestamp).encode()).hexdigest()
             self.hash_update_successful = True
-            self._previous_hash = self.current_hash
             self._last_hash_time = current_timestamp
 
     def get_connection(self) -> Optional[OpenStackClient]:
@@ -953,8 +939,7 @@ def _get_cached_evacuable(self, cache_attr: str, config_key: str,
         try:
             cache_data = fetch_func(connection)
         except Exception as e:
-            logging.error("Failed to get evacuable resources (%s): %s", config_key, e)
-            logging.debug('Exception traceback:', exc_info=True)
+            _safe_log_exception(f"Failed to get evacuable resources ({config_key})", e, include_traceback=True)
             cache_data = []
         with self._cache_lock:
             setattr(self, cache_attr, cache_data)
@@ -1115,8 +1100,7 @@ def is_aggregate_evacuable(self, connection: OpenStackClient, host: str, aggrega
             return any(host in agg.hosts for agg in aggregates
                       if self._is_resource_evacuable(agg, self.evacuable_tag, ['metadata']))
         except Exception as e:
-            logging.error("Failed to check aggregate evacuability for %s: %s", host, e)
-            logging.debug('Exception traceback:', exc_info=True)
+            _safe_log_exception(f"Failed to check aggregate evacuability for {host}", e, include_traceback=True)
             return False
 
     def get_hosts_with_servers_cached(self, connection, services):
@@ -1290,15 +1274,7 @@ def do_GET(self):
         except Exception as e:
             logging.error('Health check server failed: %s', e)
 
-    def _cleanup_dict_by_condition(self, dictionary: dict, condition_func: Callable[[Any, Any], bool],
-                                   log_message: Optional[str] = None) -> int:
-        """Remove dictionary entries matching condition function."""
-        to_remove = [k for k, v in dictionary.items() if condition_func(k, v)]
-        for k in to_remove:
-            del dictionary[k]
-            if log_message:
-                logging.debug(log_message.format(k))
-        return len(to_remove)
+
 
     def refresh_evacuable_cache(self, connection=None, force=False, cache_timeout=CACHE_TIMEOUT_SECONDS):
         """Refresh evacuable flavors and images cache with intelligent timing."""
@@ -1551,8 +1527,7 @@ def _update_service_disable_reason(connection, host, service_id=None) -> bool:
         logging.debug('Updated disabled reason for host %s after evacuation failure', host)
         return True
     except Exception as e:
-        logging.error('Failed to update disable_reason for host %s. Error: %s', host, e)
-        logging.debug('Exception traceback:', exc_info=True)
+        _safe_log_exception(f"Failed to update disable_reason for host {host}", e, include_traceback=True)
         return False
 
 
@@ -1899,9 +1874,7 @@ def _monitor_evacuation(connection, server_id, response_uuid, start_time,
 
         except Exception as e:
             api_error_count += 1
-            logging.error("Error checking evacuation status for %s: %s",
-                         response_uuid, _sanitize_message(str(e)))
-            logging.debug('Exception traceback:', exc_info=True)
+            _safe_log_exception(f"Error checking evacuation status for {response_uuid}", e, include_traceback=True)
             if api_error_count >= MAX_API_RETRIES:
                 logging.error("Too many API errors checking evacuation status for %s. Giving up.",
                              response_uuid)
@@ -1993,9 +1966,7 @@ def _server_evacuate_future(connection, server, target_host=None,
         return result
 
     except Exception as e:
-        logging.error("Unexpected error during evacuation of server %s: %s",
-                     server.id, _sanitize_message(str(e)))
-        logging.debug('Exception traceback:', exc_info=True)
+        _safe_log_exception(f"Unexpected error during evacuation of server {server.id}", e, include_traceback=True)
         _emit_k8s_event(source_host, 'InstanceEvacuationFailed',
                         f'Instance {server.id} evacuation error: {_sanitize_message(str(e))}',
                         event_type='Warning')
@@ -2520,12 +2491,6 @@ def _host_fence(host, action, service):
         logging.error("Invalid fence parameters: missing host")
         return False
 
-    # Validate action parameter at entry point
-    if not validate_input(action, 'power_action', "host fencing"):
-        logging.error("Invalid fence action: %s", action)
-        return False
-
-    # Additional check for fence-specific actions
     if action not in ['on', 'off']:
         logging.error("Unsupported fence action: %s (only 'on' and 'off' supported)", action)
         return False
@@ -2558,14 +2523,7 @@ def _host_fence(host, action, service):
 
 def _get_nova_connection(service):
     """Establish a connection to Nova using service configuration."""
-    try:
-        return service.create_connection()
-    except NovaConnectionError:
-        logging.error("Nova connection failed")
-        return None
-    except Exception as e:
-        _safe_log_exception("Failed to establish Nova connection", e)
-        return None
+    return _establish_nova_connection(service, fatal=False)
 
 
 def _enable_matching_reserved_host(conn, failed_service, reserved_hosts, service,
@@ -3019,7 +2977,7 @@ def _categorize_services(services: List[Any], target_date: datetime) -> tuple:
 
     return compute_nodes, resume, reenable
 
-def _check_critical_services(conn, services, compute_nodes):
+def _check_critical_services(services):
     """Check if critical Nova services are operational for evacuation."""
     # Check if at least one scheduler is up
     schedulers = [s for s in services if s.binary == 'nova-scheduler']
@@ -3048,10 +3006,11 @@ def _filter_processing_hosts(service, compute_nodes, to_resume):
 
     with service.processing_lock:
         # Clean up expired processing entries
-        service._cleanup_dict_by_condition(
-            service.hosts_processing,
-            lambda h, t: current_time - t > max_processing_time + MAX_PROCESSING_TIME_PADDING_SECONDS,
-            'Cleaned up expired processing entry for {}')
+        expired = [h for h, t in service.hosts_processing.items()
+                   if current_time - t > max_processing_time + MAX_PROCESSING_TIME_PADDING_SECONDS]
+        for h in expired:
+            del service.hosts_processing[h]
+            logging.debug('Cleaned up expired processing entry for %s', h)
 
         # Filter out hosts currently being processed
         original_count = len(compute_nodes)
@@ -3278,7 +3237,7 @@ def _process_stale_services(conn, service, services, compute_nodes, to_resume):
     # Process evacuations
     if not service.config.get_config_value('DISABLED'):
         # Check if critical services are operational
-        can_evacuate, error_msg = _check_critical_services(conn, services, compute_nodes)
+        can_evacuate, error_msg = _check_critical_services(services)
         if not can_evacuate:
             logging.error('Cannot evacuate: %s. Skipping evacuation.', error_msg)
             _cleanup_filtered_hosts(service, marked_hostnames, set(), current_time)

test/instanceha/test_config_features.py

@@ -711,37 +711,31 @@ def test_all_schedulers_down(self):
         ]
         compute_nodes = [services[2]]
 
-        can_evacuate, error_msg = instanceha._check_critical_services(mock_conn, services, compute_nodes)
+        can_evacuate, error_msg = instanceha._check_critical_services(services)
 
         self.assertFalse(can_evacuate)
         self.assertIn('nova-scheduler', error_msg)
 
     def test_at_least_one_scheduler_up(self):
         """Test that check passes when at least one scheduler is up."""
-        mock_conn = Mock()
-
         services = [
             Mock(binary='nova-scheduler', state='down', host='controller-1'),
             Mock(binary='nova-scheduler', state='up', host='controller-2'),
             Mock(binary='nova-compute', state='down', host='compute-1')
         ]
-        compute_nodes = [services[2]]
 
-        can_evacuate, error_msg = instanceha._check_critical_services(mock_conn, services, compute_nodes)
+        can_evacuate, error_msg = instanceha._check_critical_services(services)
 
         self.assertTrue(can_evacuate)
         self.assertEqual(error_msg, "")
 
     def test_no_schedulers_defined(self):
         """Test when no schedulers are in services list."""
-        mock_conn = Mock()
-
         services = [
             Mock(binary='nova-compute', state='down', host='compute-1')
         ]
-        compute_nodes = [services[0]]
 
-        can_evacuate, error_msg = instanceha._check_critical_services(mock_conn, services, compute_nodes)
+        can_evacuate, error_msg = instanceha._check_critical_services(services)
 
         # Should allow evacuation if no schedulers are in the list
         self.assertTrue(can_evacuate)

test/instanceha/test_coverage_gaps.py

@@ -912,7 +912,6 @@ def _make_service(self):
         svc.refresh_evacuable_cache = Mock()
         svc.get_hosts_with_servers_cached = Mock(return_value={})
         svc.filter_hosts_with_servers = Mock(return_value=[])
-        svc._cleanup_dict_by_condition = Mock()
         return svc
 
     def test_empty_compute_nodes_and_resume(self):
@@ -939,7 +938,7 @@ def test_threshold_exceeded_skips_evacuation(self):
         service.get_hosts_with_servers_cached.return_value = {'host-1': ['server-1']}
         service.filter_hosts_with_servers.return_value = [svc1]
         with patch('instanceha._emit_k8s_event'):
-            with patch('instanceha._check_critical_services', return_value=(True, None)):
+            with patch('instanceha._check_critical_services', return_value=(True, "")):
                 instanceha._process_stale_services(conn, service, [svc1], [svc1], [])
 
     def test_disabled_mode_skips_evacuation(self):