From b37f3892b6cfc34d028b60a8e856f6f2ab59778b Mon Sep 17 00:00:00 2001 From: Dirk Julich Date: Fri, 22 May 2026 14:45:08 +0200 Subject: [PATCH] [AAP-74343] Decouple installed_collections/ansible_version from indirect node counting flag (#16453) * [AAP-74343] Decouple installed_collections and ansible_version from indirect node counting flag The indirect_instance_count callback plugin and its artifact processing were entirely gated behind FEATURE_INDIRECT_NODE_COUNTING_ENABLED. This caused installed_collections and ansible_version to remain unpopulated when the flag was off, even though these are baseline analytics fields unrelated to indirect host counting. Always run the callback plugin and persist installed_collections and ansible_version to the database. Only the indirect-counting-specific parts (EventQuery creation, event_queries_processed flag, and vendor collections) remain gated behind the feature flag. * [AAP-74343] Read callbacks_enabled from ansible.cfg so user-configured callbacks are preserved The check for 'callbacks_enabled' in config_values was dead code because read_ansible_config was never asked to read that setting. Now that the callback registration runs unconditionally, fix this by including 'callbacks_enabled' in the variables of interest. * [AAP-74343] Use comma delimiter for ANSIBLE_CALLBACKS_ENABLED Ansible's CALLBACKS_ENABLED config is type list and splits on commas. The colon delimiter would cause combined callback names to be treated as a single invalid name. * [AAP-74343] Add tests for ANSIBLE_CALLBACKS_ENABLED configuration Verify that indirect_instance_count is always set, user-configured callbacks from ansible.cfg are preserved, and the comma delimiter is used as ansible-core expects. * [AAP-74343] Use public API for namespace package path access Replace library.__path__._path[0] with library.__path__[0] to avoid relying on a private CPython implementation detail of _NamespacePath. * [AAP-74343] Skip host query scanning when indirect counting flag is off The indirect_instance_count callback plugin now checks AWX_COLLECT_HOST_QUERIES to decide whether to scan for host query files. When the feature flag is off, the plugin only collects collection metadata (name + version) and ansible_version, skipping the expensive embedded/external query file discovery. * [AAP-74343] Set AWX_COLLECT_HOST_QUERIES in query discovery tests The TestExternalQueryDiscovery tests exercise the host query scanning path, which now requires AWX_COLLECT_HOST_QUERIES=1 in the environment. * [AAP-74343] Use Ansible plugin config system for collect_host_queries Declare collect_host_queries as a formal plugin option in DOCUMENTATION with env var AWX_COLLECT_HOST_QUERIES, replacing the raw os.getenv() call with self.get_option(). This follows the standard Ansible plugin configuration pattern. * [AAP-74343] Add test for disabled collect_host_queries path Verify that when collect_host_queries is false, the plugin still enumerates collections for metadata but skips host query file scanning. --------- Co-authored-by: Claude Opus 4.6 --- awx/main/tasks/callback.py | 32 +++--- awx/main/tasks/jobs.py | 34 +++--- .../tests/unit/tasks/test_runner_callback.py | 106 +++++++++++++++++- .../unit/test_indirect_query_discovery.py | 33 ++++++ awx/main/tests/unit/test_tasks.py | 75 +++++++++++++ .../library/indirect_instance_count.py | 40 ++++--- 6 files changed, 267 insertions(+), 53 deletions(-) diff --git a/awx/main/tasks/callback.py b/awx/main/tasks/callback.py index c6d89d0b79..266fd22015 100644 --- a/awx/main/tasks/callback.py +++ b/awx/main/tasks/callback.py @@ -54,9 +54,6 @@ def try_load_query_file(artifact_dir) -> Tuple[bool, Optional[dict]]: returns the contents of ansible_data.json if present """ - if not flag_enabled("FEATURE_INDIRECT_NODE_COUNTING_ENABLED"): - return False, None - queries_path = os.path.join(artifact_dir, COLLECTION_FILENAME) if not os.path.isfile(queries_path): logger.info(f"no query file found: {queries_path}") @@ -277,20 +274,6 @@ class RunnerCallback: def artifacts_handler(self, artifact_dir): success, query_file_contents = try_load_query_file(artifact_dir) if success: - self.delay_update(event_queries_processed=False) - collections_info = collect_queries(query_file_contents) - for collection, data in collections_info.items(): - version = data['version'] - event_query = data['host_query'] - instance = EventQuery(fqcn=collection, collection_version=version, event_query=event_query) - try: - instance.validate_unique() - instance.save() - - logger.info(f"eventy query for collection {collection}, version {version} created") - except ValidationError as e: - logger.info(e) - if 'installed_collections' in query_file_contents: self.delay_update(installed_collections=query_file_contents['installed_collections']) else: @@ -301,6 +284,21 @@ class RunnerCallback: else: logger.warning(f'The file {COLLECTION_FILENAME} unexpectedly did not contain ansible_version') + if flag_enabled("FEATURE_INDIRECT_NODE_COUNTING_ENABLED"): + self.delay_update(event_queries_processed=False) + collections_info = collect_queries(query_file_contents) + for collection, data in collections_info.items(): + version = data['version'] + event_query = data['host_query'] + instance = EventQuery(fqcn=collection, collection_version=version, event_query=event_query) + try: + instance.validate_unique() + instance.save() + + logger.info(f"event query for collection {collection}, version {version} created") + except ValidationError as e: + logger.info(e) + self.artifacts_processed = True diff --git a/awx/main/tasks/jobs.py b/awx/main/tasks/jobs.py index 69250e967d..c441e6ce1a 100644 --- a/awx/main/tasks/jobs.py +++ b/awx/main/tasks/jobs.py @@ -1138,12 +1138,11 @@ class RunJob(SourceControlMixin, BaseTask): ('ANSIBLE_COLLECTIONS_PATH', 'collections_path', 'requirements_collections', '~/.ansible/collections:/usr/share/ansible/collections'), ] - if flag_enabled("FEATURE_INDIRECT_NODE_COUNTING_ENABLED"): - path_vars.append( - ('ANSIBLE_CALLBACK_PLUGINS', 'callback_plugins', 'plugins_path', '~/.ansible/plugins:/plugins/callback:/usr/share/ansible/plugins/callback'), - ) + path_vars.append( + ('ANSIBLE_CALLBACK_PLUGINS', 'callback_plugins', 'plugins_path', '~/.ansible/plugins:/plugins/callback:/usr/share/ansible/plugins/callback'), + ) - config_values = read_ansible_config(os.path.join(private_data_dir, 'project'), list(map(lambda x: x[1], path_vars))) + config_values = read_ansible_config(os.path.join(private_data_dir, 'project'), list(map(lambda x: x[1], path_vars)) + ['callbacks_enabled']) for env_key, config_setting, folder, default in path_vars: paths = default.split(':') @@ -1158,11 +1157,12 @@ class RunJob(SourceControlMixin, BaseTask): paths = [os.path.join(CONTAINER_ROOT, folder)] + paths env[env_key] = os.pathsep.join(paths) - if flag_enabled("FEATURE_INDIRECT_NODE_COUNTING_ENABLED"): - env['ANSIBLE_CALLBACKS_ENABLED'] = 'indirect_instance_count' - if 'callbacks_enabled' in config_values: - env['ANSIBLE_CALLBACKS_ENABLED'] += ':' + config_values['callbacks_enabled'] + env['ANSIBLE_CALLBACKS_ENABLED'] = 'indirect_instance_count' + if 'callbacks_enabled' in config_values: + env['ANSIBLE_CALLBACKS_ENABLED'] += ',' + config_values['callbacks_enabled'] + if flag_enabled("FEATURE_INDIRECT_NODE_COUNTING_ENABLED"): + env['AWX_COLLECT_HOST_QUERIES'] = '1' # Add vendor collections path for external query file discovery vendor_collections_path = os.path.join(CONTAINER_ROOT, 'vendor_collections') env['ANSIBLE_COLLECTIONS_PATH'] = f"{vendor_collections_path}:{env['ANSIBLE_COLLECTIONS_PATH']}" @@ -1612,16 +1612,14 @@ class RunProjectUpdate(BaseTask): shutil.copytree(cache_subpath, dest_subpath, symlinks=True) logger.debug('{0} {1} prepared {2} from cache'.format(type(project).__name__, project.pk, dest_subpath)) - if flag_enabled("FEATURE_INDIRECT_NODE_COUNTING_ENABLED"): - # copy the special callback (not stdout type) plugin to get list of collections - pdd_plugins_path = os.path.join(job_private_data_dir, 'plugins_path') - if not os.path.exists(pdd_plugins_path): - os.mkdir(pdd_plugins_path) - from awx.playbooks import library + pdd_plugins_path = os.path.join(job_private_data_dir, 'plugins_path') + if not os.path.exists(pdd_plugins_path): + os.mkdir(pdd_plugins_path) + from awx.playbooks import library - plugin_file_source = os.path.join(library.__path__._path[0], 'indirect_instance_count.py') - plugin_file_dest = os.path.join(pdd_plugins_path, 'indirect_instance_count.py') - shutil.copyfile(plugin_file_source, plugin_file_dest) + plugin_file_source = os.path.join(library.__path__[0], 'indirect_instance_count.py') + plugin_file_dest = os.path.join(pdd_plugins_path, 'indirect_instance_count.py') + shutil.copyfile(plugin_file_source, plugin_file_dest) def post_run_hook(self, instance, status): super(RunProjectUpdate, self).post_run_hook(instance, status) diff --git a/awx/main/tests/unit/tasks/test_runner_callback.py b/awx/main/tests/unit/tasks/test_runner_callback.py index fb04842e10..54c964cc1c 100644 --- a/awx/main/tests/unit/tasks/test_runner_callback.py +++ b/awx/main/tests/unit/tasks/test_runner_callback.py @@ -1,4 +1,9 @@ -from awx.main.tasks.callback import RunnerCallback +import json +import os +import tempfile +from unittest import mock + +from awx.main.tasks.callback import RunnerCallback, try_load_query_file from awx.main.constants import ANSIBLE_RUNNER_NEEDS_UPDATE_MESSAGE from django.utils.translation import gettext_lazy as _ @@ -50,3 +55,102 @@ def test_special_ansible_runner_message(mock_me): 'Traceback:\ngot an unexpected keyword argument\nFile: bar.py\n' f'{ANSIBLE_RUNNER_NEEDS_UPDATE_MESSAGE}' ) + + +SAMPLE_ANSIBLE_DATA = { + 'installed_collections': { + 'ansible.builtin': {'version': '2.16.0'}, + 'community.general': {'version': '8.0.0', 'host_query': 'SELECT * FROM hosts'}, + }, + 'ansible_version': '2.16.0', +} + + +class TestTryLoadQueryFile: + def test_loads_file_without_feature_flag(self): + with tempfile.TemporaryDirectory() as tmpdir: + path = os.path.join(tmpdir, 'ansible_data.json') + with open(path, 'w') as f: + json.dump(SAMPLE_ANSIBLE_DATA, f) + + with mock.patch('awx.main.tasks.callback.flag_enabled', return_value=False): + success, data = try_load_query_file(tmpdir) + + assert success is True + assert data['ansible_version'] == '2.16.0' + assert 'ansible.builtin' in data['installed_collections'] + + def test_loads_file_with_feature_flag(self): + with tempfile.TemporaryDirectory() as tmpdir: + path = os.path.join(tmpdir, 'ansible_data.json') + with open(path, 'w') as f: + json.dump(SAMPLE_ANSIBLE_DATA, f) + + with mock.patch('awx.main.tasks.callback.flag_enabled', return_value=True): + success, data = try_load_query_file(tmpdir) + + assert success is True + assert data == SAMPLE_ANSIBLE_DATA + + def test_returns_false_when_file_missing(self): + with tempfile.TemporaryDirectory() as tmpdir: + success, data = try_load_query_file(tmpdir) + + assert success is False + assert data is None + + +class TestArtifactsHandler: + def test_always_persists_metadata_when_flag_off(self, mock_me): + rc = RunnerCallback() + with tempfile.TemporaryDirectory() as tmpdir: + path = os.path.join(tmpdir, 'ansible_data.json') + with open(path, 'w') as f: + json.dump(SAMPLE_ANSIBLE_DATA, f) + + with mock.patch('awx.main.tasks.callback.flag_enabled', return_value=False): + rc.artifacts_handler(tmpdir) + + assert rc.extra_update_fields['installed_collections'] == SAMPLE_ANSIBLE_DATA['installed_collections'] + assert rc.extra_update_fields['ansible_version'] == '2.16.0' + assert 'event_queries_processed' not in rc.extra_update_fields + assert rc.artifacts_processed is True + + @mock.patch('awx.main.tasks.callback.EventQuery') + def test_creates_event_queries_when_flag_on(self, mock_event_query, mock_me): + rc = RunnerCallback() + with tempfile.TemporaryDirectory() as tmpdir: + path = os.path.join(tmpdir, 'ansible_data.json') + with open(path, 'w') as f: + json.dump(SAMPLE_ANSIBLE_DATA, f) + + with mock.patch('awx.main.tasks.callback.flag_enabled', return_value=True): + rc.artifacts_handler(tmpdir) + + assert rc.extra_update_fields['installed_collections'] == SAMPLE_ANSIBLE_DATA['installed_collections'] + assert rc.extra_update_fields['ansible_version'] == '2.16.0' + assert rc.extra_update_fields['event_queries_processed'] is False + mock_event_query.assert_called_once() + + @mock.patch('awx.main.tasks.callback.EventQuery') + def test_no_event_queries_when_flag_off(self, mock_event_query, mock_me): + rc = RunnerCallback() + with tempfile.TemporaryDirectory() as tmpdir: + path = os.path.join(tmpdir, 'ansible_data.json') + with open(path, 'w') as f: + json.dump(SAMPLE_ANSIBLE_DATA, f) + + with mock.patch('awx.main.tasks.callback.flag_enabled', return_value=False): + rc.artifacts_handler(tmpdir) + + mock_event_query.assert_not_called() + + def test_handles_missing_artifact_file(self, mock_me): + rc = RunnerCallback() + with tempfile.TemporaryDirectory() as tmpdir: + with mock.patch('awx.main.tasks.callback.flag_enabled', return_value=False): + rc.artifacts_handler(tmpdir) + + assert 'installed_collections' not in rc.extra_update_fields + assert 'ansible_version' not in rc.extra_update_fields + assert rc.artifacts_processed is True diff --git a/awx/main/tests/unit/test_indirect_query_discovery.py b/awx/main/tests/unit/test_indirect_query_discovery.py index 159873614e..3694cf8466 100644 --- a/awx/main/tests/unit/test_indirect_query_discovery.py +++ b/awx/main/tests/unit/test_indirect_query_discovery.py @@ -39,6 +39,13 @@ def create_queries_dir_mock(file_lookup_func): class MockCallbackBase: def __init__(self): self._display = mock.MagicMock() + self._plugin_options = {} + + def get_option(self, key): + return self._plugin_options.get(key) + + def set_option(self, key, value): + self._plugin_options[key] = value def v2_playbook_on_stats(self, stats): pass @@ -289,6 +296,7 @@ class TestExternalQueryDiscovery: callback = CallbackModule() callback._display = mock.Mock() + callback.set_option('collect_host_queries', True) with mock.patch('builtins.open', mock.mock_open()): with mock.patch('json.dumps', return_value='{}'): @@ -318,6 +326,7 @@ class TestExternalQueryDiscovery: callback = CallbackModule() callback._display = mock.Mock() + callback.set_option('collect_host_queries', True) with mock.patch('builtins.open', mock.mock_open()): with mock.patch('json.dumps', return_value='{}'): @@ -342,6 +351,7 @@ class TestExternalQueryDiscovery: callback = CallbackModule() callback._display = mock.Mock() + callback.set_option('collect_host_queries', True) with mock.patch('builtins.open', mock.mock_open()): with mock.patch('json.dumps', return_value='{}'): @@ -372,6 +382,7 @@ class TestExternalQueryDiscovery: callback = CallbackModule() callback._display = mock.Mock() + callback.set_option('collect_host_queries', True) with mock.patch('builtins.open', mock.mock_open()): with mock.patch('json.dumps', return_value='{}'): @@ -382,6 +393,28 @@ class TestExternalQueryDiscovery: assert '4.1.0' in call_args assert 'community.vmware' in call_args + @mock.patch('awx.playbooks.library.indirect_instance_count.list_collections') + @mock.patch('awx.playbooks.library.indirect_instance_count.files') + @mock.patch('awx.playbooks.library.indirect_instance_count.find_external_query_with_fallback') + @mock.patch.dict('os.environ', {'AWX_ISOLATED_DATA_DIR': '/tmp/artifacts'}) + def test_queries_not_collected_when_option_disabled(self, mock_fallback, mock_files, mock_list_collections): + """Host query scanning is skipped when collect_host_queries is disabled.""" + from awx.playbooks.library.indirect_instance_count import CallbackModule + + mock_list_collections.return_value = [mock.Mock(namespace='demo', name='query', ver='1.0.0', fqcn='demo.query')] + + callback = CallbackModule() + callback._display = mock.Mock() + callback.set_option('collect_host_queries', False) + + with mock.patch('builtins.open', mock.mock_open()): + with mock.patch('json.dumps', return_value='{}'): + callback.v2_playbook_on_stats(mock.Mock()) + + mock_list_collections.assert_called_once() + mock_files.assert_not_called() + mock_fallback.assert_not_called() + class TestPrivateDataDirIntegration: """Tests for vendor collection copying (AC7.10-AC7.11).""" diff --git a/awx/main/tests/unit/test_tasks.py b/awx/main/tests/unit/test_tasks.py index 41b6ff058b..62c7f42b0a 100644 --- a/awx/main/tests/unit/test_tasks.py +++ b/awx/main/tests/unit/test_tasks.py @@ -918,6 +918,81 @@ class TestJobCredentials(TestJobExecution): assert env['FOO'] == 'BAR' +class TestCallbacksEnabled(TestJobExecution): + @pytest.fixture(autouse=True) + def mock_flag_enabled(self): + with mock.patch('awx.main.tasks.jobs.flag_enabled', return_value=False): + yield + + def test_callbacks_enabled_default(self, patch_Job, private_data_dir, execution_environment, mock_me): + job = Job(project=Project(), inventory=Inventory()) + job.execution_environment = execution_environment + + task = jobs.RunJob() + task.instance = job + task._write_extra_vars_file = mock.Mock() + + with mock.patch.object(task, 'build_credentials_list', return_value=[], autospec=True): + env = task.build_env(job, private_data_dir) + + assert env['ANSIBLE_CALLBACKS_ENABLED'] == 'indirect_instance_count' + + def test_callbacks_enabled_preserves_user_config(self, patch_Job, private_data_dir, execution_environment, mock_me): + job = Job(project=Project(), inventory=Inventory()) + job.execution_environment = execution_environment + + task = jobs.RunJob() + task.instance = job + task._write_extra_vars_file = mock.Mock() + + with mock.patch.object(task, 'build_credentials_list', return_value=[], autospec=True): + with mock.patch('awx.main.tasks.jobs.read_ansible_config', return_value={'callbacks_enabled': 'custom_callback,another_callback'}): + env = task.build_env(job, private_data_dir) + + assert env['ANSIBLE_CALLBACKS_ENABLED'] == 'indirect_instance_count,custom_callback,another_callback' + + def test_callbacks_enabled_uses_comma_delimiter(self, patch_Job, private_data_dir, execution_environment, mock_me): + job = Job(project=Project(), inventory=Inventory()) + job.execution_environment = execution_environment + + task = jobs.RunJob() + task.instance = job + task._write_extra_vars_file = mock.Mock() + + with mock.patch.object(task, 'build_credentials_list', return_value=[], autospec=True): + with mock.patch('awx.main.tasks.jobs.read_ansible_config', return_value={'callbacks_enabled': 'my_callback'}): + env = task.build_env(job, private_data_dir) + + assert env['ANSIBLE_CALLBACKS_ENABLED'] == 'indirect_instance_count,my_callback' + + def test_collect_host_queries_set_when_flag_on(self, patch_Job, private_data_dir, execution_environment, mock_me): + job = Job(project=Project(), inventory=Inventory()) + job.execution_environment = execution_environment + + task = jobs.RunJob() + task.instance = job + task._write_extra_vars_file = mock.Mock() + + with mock.patch.object(task, 'build_credentials_list', return_value=[], autospec=True): + with mock.patch('awx.main.tasks.jobs.flag_enabled', return_value=True): + env = task.build_env(job, private_data_dir) + + assert env['AWX_COLLECT_HOST_QUERIES'] == '1' + + def test_collect_host_queries_not_set_when_flag_off(self, patch_Job, private_data_dir, execution_environment, mock_me): + job = Job(project=Project(), inventory=Inventory()) + job.execution_environment = execution_environment + + task = jobs.RunJob() + task.instance = job + task._write_extra_vars_file = mock.Mock() + + with mock.patch.object(task, 'build_credentials_list', return_value=[], autospec=True): + env = task.build_env(job, private_data_dir) + + assert 'AWX_COLLECT_HOST_QUERIES' not in env + + @pytest.mark.usefixtures("patch_Organization") class TestProjectUpdateGalaxyCredentials(TestJobExecution): @pytest.fixture diff --git a/awx/playbooks/library/indirect_instance_count.py b/awx/playbooks/library/indirect_instance_count.py index 4973f76c05..4cbce89a43 100644 --- a/awx/playbooks/library/indirect_instance_count.py +++ b/awx/playbooks/library/indirect_instance_count.py @@ -17,6 +17,13 @@ DOCUMENTATION = ''' requirements: - Whitelist in configuration - Set AWX_ISOLATED_DATA_DIR, AWX will do this + options: + collect_host_queries: + description: When enabled, scan collections for host query files used in indirect node counting. + type: bool + default: false + env: + - name: AWX_COLLECT_HOST_QUERIES ''' import os @@ -168,29 +175,28 @@ class CallbackModule(CallbackBase): if not artifact_dir: raise RuntimeError('Only suitable in AWX, did not find private_data_dir') + collect_host_queries = self.get_option('collect_host_queries') + collections_print = {} - # Loop over collections, from ansible-core these are Candidate objects for candidate in list_collections(): collection_print = { 'version': candidate.ver, } - # 1. Check for embedded query file (takes precedence) - embedded_query_file = files(f'ansible_collections.{candidate.namespace}.{candidate.name}') / 'extensions' / 'audit' / 'event_query.yml' - if embedded_query_file.exists(): - with embedded_query_file.open('r') as f: - collection_print['host_query'] = f.read() - self._display.vv(f"Using embedded query for {candidate.fqcn} v{candidate.ver}") - else: - # 2. Check for external query file with version fallback - query_content, fallback_used, version_used = find_external_query_with_fallback(candidate.namespace, candidate.name, candidate.ver) - if query_content: - collection_print['host_query'] = query_content - if fallback_used: - # AC5.6: Log when fallback is used - self._display.v(f"Using external query {version_used} for {candidate.fqcn} v{candidate.ver}.") - else: - self._display.v(f"Using external query for {candidate.fqcn} v{candidate.ver}") + if collect_host_queries: + embedded_query_file = files(f'ansible_collections.{candidate.namespace}.{candidate.name}') / 'extensions' / 'audit' / 'event_query.yml' + if embedded_query_file.exists(): + with embedded_query_file.open('r') as f: + collection_print['host_query'] = f.read() + self._display.vv(f"Using embedded query for {candidate.fqcn} v{candidate.ver}") + else: + query_content, fallback_used, version_used = find_external_query_with_fallback(candidate.namespace, candidate.name, candidate.ver) + if query_content: + collection_print['host_query'] = query_content + if fallback_used: + self._display.v(f"Using external query {version_used} for {candidate.fqcn} v{candidate.ver}.") + else: + self._display.v(f"Using external query for {candidate.fqcn} v{candidate.ver}") collections_print[candidate.fqcn] = collection_print