From 85ec83c3fd97490a1f7d7384cda7c8903d0ec547 Mon Sep 17 00:00:00 2001 From: Alan Rominger Date: Mon, 28 Mar 2022 10:52:09 -0400 Subject: [PATCH 1/3] Minor tweaks to ansible-runner cleanup task arguments --- awx/main/models/ha.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/awx/main/models/ha.py b/awx/main/models/ha.py index 5200f99d3e..763a24c5b8 100644 --- a/awx/main/models/ha.py +++ b/awx/main/models/ha.py @@ -186,12 +186,16 @@ class Instance(HasPolicyEditsMixin, BaseModel): returns a dict that is passed to the python interface for the runner method corresponding to that command any kwargs will override that key=value combination in the returned dict """ - vargs = dict() + vargs = dict(grace_period=60) # grace period of 60 minutes, need to set because CLI default will not take effect if settings.AWX_CLEANUP_PATHS: vargs['file_pattern'] = os.path.join(settings.AWX_ISOLATION_BASE_PATH, JOB_FOLDER_PREFIX % '*') + '*' vargs.update(kwargs) if 'exclude_strings' not in vargs and vargs.get('file_pattern'): - active_pks = list(UnifiedJob.objects.filter(execution_node=self.hostname, status__in=('running', 'waiting')).values_list('pk', flat=True)) + active_pks = list( + UnifiedJob.objects.filter( + (models.Q(execution_node=self.hostname) | models.Q(controller_node=self.hostname)) & models.Q(status__in=('running', 'waiting')) + ).values_list('pk', flat=True) + ) if active_pks: vargs['exclude_strings'] = [JOB_FOLDER_PREFIX % job_id for job_id in active_pks] if 'remove_images' in vargs or 'image_prune' in vargs: From deac08ba8a9b913a78607740182b9eb70066cd46 Mon Sep 17 00:00:00 2001 From: Alan Rominger Date: Mon, 28 Mar 2022 22:23:33 -0400 Subject: [PATCH 2/3] Add regression test for overly agressive cleanup behavior --- awx/main/models/ha.py | 4 ++- awx/main/tests/functional/test_tasks.py | 43 +++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/awx/main/models/ha.py b/awx/main/models/ha.py index 763a24c5b8..1b77deb77f 100644 --- a/awx/main/models/ha.py +++ b/awx/main/models/ha.py @@ -186,10 +186,12 @@ class Instance(HasPolicyEditsMixin, BaseModel): returns a dict that is passed to the python interface for the runner method corresponding to that command any kwargs will override that key=value combination in the returned dict """ - vargs = dict(grace_period=60) # grace period of 60 minutes, need to set because CLI default will not take effect + vargs = dict() if settings.AWX_CLEANUP_PATHS: vargs['file_pattern'] = os.path.join(settings.AWX_ISOLATION_BASE_PATH, JOB_FOLDER_PREFIX % '*') + '*' vargs.update(kwargs) + if not isinstance(vargs.get('grace_period'), int): + vargs['grace_period'] = 60 # grace period of 60 minutes, need to set because CLI default will not take effect if 'exclude_strings' not in vargs and vargs.get('file_pattern'): active_pks = list( UnifiedJob.objects.filter( diff --git a/awx/main/tests/functional/test_tasks.py b/awx/main/tests/functional/test_tasks.py index 14c48fa5ff..cb66a07ba3 100644 --- a/awx/main/tests/functional/test_tasks.py +++ b/awx/main/tests/functional/test_tasks.py @@ -1,10 +1,12 @@ import pytest from unittest import mock import os +import tempfile +import shutil from awx.main.tasks.jobs import RunProjectUpdate, RunInventoryUpdate -from awx.main.tasks.system import execution_node_health_check -from awx.main.models import ProjectUpdate, InventoryUpdate, InventorySource, Instance +from awx.main.tasks.system import execution_node_health_check, _cleanup_images_and_files +from awx.main.models import ProjectUpdate, InventoryUpdate, InventorySource, Instance, Job @pytest.fixture @@ -80,3 +82,40 @@ class TestDependentInventoryUpdate: # Verify that it bails after 1st update, detecting a cancel assert is2.inventory_updates.count() == 0 iu_run_mock.assert_called_once() + + +@pytest.fixture +def mock_job_folder(request): + pdd_path = tempfile.mkdtemp(prefix='awx_123_') + + def test_folder_cleanup(): + if os.path.exists(pdd_path): + shutil.rmtree(pdd_path) + + request.addfinalizer(test_folder_cleanup) + + return pdd_path + + +@pytest.mark.django_db +def test_folder_cleanup_stale_file(mock_job_folder, mock_me): + _cleanup_images_and_files() + assert os.path.exists(mock_job_folder) # grace period should protect folder from deletion + + _cleanup_images_and_files(grace_period=0) + assert not os.path.exists(mock_job_folder) # should be deleted + + +@pytest.mark.django_db +def test_folder_cleanup_running_job(mock_job_folder, mock_me): + me_inst = Instance.objects.create(hostname='local_node', uuid='00000000-0000-0000-0000-000000000000') + with mock.patch.object(Instance.objects, 'me', return_value=me_inst): + + job = Job.objects.create(id=123, controller_node=me_inst.hostname, status='running') + _cleanup_images_and_files(grace_period=0) + assert os.path.exists(mock_job_folder) # running job should prevent folder from getting deleted + + job.status = 'failed' + job.save(update_fields=['status']) + _cleanup_images_and_files(grace_period=0) + assert not os.path.exists(mock_job_folder) # job is finished and no grace period, should delete From f17ceca7a06cc79d46c58fd9cbff9e08944b8724 Mon Sep 17 00:00:00 2001 From: Alan Rominger Date: Mon, 28 Mar 2022 22:39:59 -0400 Subject: [PATCH 3/3] Add in default value to unit tests --- awx/main/tests/unit/models/test_ha.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/awx/main/tests/unit/models/test_ha.py b/awx/main/tests/unit/models/test_ha.py index a0ce3476e2..86b8d65aaf 100644 --- a/awx/main/tests/unit/models/test_ha.py +++ b/awx/main/tests/unit/models/test_ha.py @@ -93,7 +93,7 @@ class TestInstanceGroup(object): def test_cleanup_params_defaults(): inst = Instance(hostname='foobar') - assert inst.get_cleanup_task_kwargs(exclude_strings=['awx_423_']) == {'exclude_strings': ['awx_423_'], 'file_pattern': '/tmp/awx_*_*'} + assert inst.get_cleanup_task_kwargs(exclude_strings=['awx_423_']) == {'exclude_strings': ['awx_423_'], 'file_pattern': '/tmp/awx_*_*', 'grace_period': 60} def test_cleanup_params_for_image_cleanup(): @@ -104,4 +104,5 @@ def test_cleanup_params_for_image_cleanup(): 'process_isolation_executable': 'podman', 'remove_images': ['quay.invalid/foo/bar'], 'image_prune': True, + 'grace_period': 60, }