diff --git a/insights/client/archive.py b/insights/client/archive.py index d7a689fea3..33976d2f67 100644 --- a/insights/client/archive.py +++ b/insights/client/archive.py @@ -2,6 +2,9 @@ Handle adding files and preparing the archive for upload """ from __future__ import absolute_import +import glob +from signal import SIGTERM, signal +import sys import time import os import shutil @@ -38,11 +41,12 @@ def __init__(self, config): Initialize the Insights Archive """ self.config = config + self.cleanup_previous_archive() # input this to core collector as `tmp_path` - self.tmp_dir = tempfile.mkdtemp(prefix='/var/tmp/') + self.tmp_dir = tempfile.mkdtemp(dir='/var/tmp/', prefix='insights-archive-') # we don't really need this anymore... - self.archive_tmp_dir = tempfile.mkdtemp(prefix='/var/tmp/') + self.archive_tmp_dir = tempfile.mkdtemp(dir='/var/tmp/', prefix='insights-archive-') # We should not hint the hostname in the archive if it has to be obfuscated if config.obfuscate_hostname: @@ -62,8 +66,11 @@ def __init__(self, config): self.cmd_dir = None self.compressor = config.compressor + self.archive_stored = None self.tar_file = None + self.keep_archive_dir = '/var/cache/insights-client' atexit.register(self.cleanup_tmp) + signal(SIGTERM, self.sigterm_handler) def create_archive_dir(self): """ @@ -155,7 +162,7 @@ def create_tar_file(self): """ Create tar file to be compressed """ - if not self.archive_tmp_dir: + if not self.tmp_dir: # we should never get here but bail out if we do raise RuntimeError('Archive temporary directory not defined.') tar_file_name = os.path.join(self.archive_tmp_dir, self.archive_name) @@ -229,12 +236,39 @@ def cleanup_tmp(self): and tar_file exists. ''' if self.config.keep_archive and self.tar_file: + self.storing_archive() if self.config.no_upload: - logger.info('Archive saved at %s', self.tar_file) + logger.info('Archive saved at %s', self.archive_stored) else: - logger.info('Insights archive retained in %s', self.tar_file) - if self.config.obfuscate: - return # return before deleting tmp_dir - else: - self.delete_archive_file() + logger.info('Insights archive retained in %s', self.archive_stored) self.delete_tmp_dir() + + def sigterm_handler(_signo, _stack_frame, _context): + sys.exit(1) + + def cleanup_previous_archive(self): + ''' + Used at the start, this will clean the temporary directory of previous killed runs + ''' + for file in glob.glob('/var/tmp/insights-archive-*'): + os.path.join('', file) + logger.debug("Deleting previous archive %s", file) + shutil.rmtree(file, True) + + def storing_archive(self): + if not os.path.exists(self.keep_archive_dir): + try: + os.makedirs(self.keep_archive_dir) + except OSError: + logger.error('ERROR: Could not create %s', self.keep_archive_dir) + raise + + archive_name = os.path.basename(self.tar_file) + self.archive_stored = os.path.join(self.keep_archive_dir, archive_name) + logger.info('Copying archive from %s to %s', self.tar_file, self.archive_stored) + try: + shutil.copyfile(self.tar_file, self.archive_stored) + except OSError: + # file exists already + logger.error('ERROR: Could not stored archive to %s', self.archive_stored) + raise diff --git a/insights/client/config.py b/insights/client/config.py index f0e15ab3e0..1bdde3a011 100644 --- a/insights/client/config.py +++ b/insights/client/config.py @@ -211,7 +211,7 @@ def _core_collect_default(): 'keep_archive': { 'default': False, 'opt': ['--keep-archive'], - 'help': 'Do not delete archive after upload', + 'help': 'Store archive in /var/cache/insights-client/ after upload', 'action': 'store_true', 'group': 'debug' }, diff --git a/insights/tests/client/test_archive.py b/insights/tests/client/test_archive.py index eb4f32b7cd..d5b42de9d7 100644 --- a/insights/tests/client/test_archive.py +++ b/insights/tests/client/test_archive.py @@ -1,6 +1,8 @@ from insights.client.archive import InsightsArchive from mock.mock import patch, Mock, call from unittest import TestCase +from pytest import raises + test_timestamp = '000000' test_hostname = 'testhostname' @@ -8,6 +10,7 @@ test_archive_dir = '/var/tmp/test/insights-testhostname-000000' test_obfuscated_archive_dir = '/var/tmp/test/insights-localhost-000000' test_cmd_dir = '/var/tmp/test/insights-testhostname-000000/insights_commands' +test_tmp_dir = '/var/tmp/insights-archive-000000' @patch('insights.client.archive.time.strftime', Mock(return_value=test_timestamp)) @@ -15,8 +18,8 @@ @patch('insights.client.archive.tempfile.mkdtemp') @patch('insights.client.archive.atexit.register') class TestInsightsArchive(TestCase): - - def test_init_archive(self, register, mkdtemp): + @patch('insights.client.archive.InsightsArchive.cleanup_previous_archive') + def test_init_archive(self, cleanup, register, mkdtemp): ''' Verify archive is created with default parameters ''' @@ -26,14 +29,13 @@ def test_init_archive(self, register, mkdtemp): assert archive.config == config assert archive.tmp_dir - assert archive.archive_tmp_dir assert archive.archive_dir is None assert archive.cmd_dir is None assert archive.compressor == config.compressor assert archive.archive_name == test_archive_name - mkdtemp.assert_has_calls([call(prefix='/var/tmp/'), - call(prefix='/var/tmp/')]) + cleanup.assert_called_once() + mkdtemp.assert_has_calls([call(dir='/var/tmp/', prefix='insights-archive-')]) register.assert_called_once() @patch('insights.client.archive.os.makedirs') @@ -56,6 +58,20 @@ def test_create_archive_dir_default(self, makedirs, _, __): # ensure the retval and attr are the same assert result == archive.archive_dir + @patch('insights.client.archive.glob.glob', return_value=[]) + @patch('insights.client.archive.shutil.rmtree') + def test_tmp_directory_no_cleanup(self, rmtree, glob, _, __): + InsightsArchive(Mock()) + glob.assert_called_with('/var/tmp/insights-archive-*') + rmtree.assert_not_called() + + @patch('insights.client.archive.glob.glob', return_value=[test_tmp_dir]) + @patch('insights.client.archive.shutil.rmtree') + def test_tmp_directory_cleanup(self, rmtree, glob, _, __): + InsightsArchive(Mock()) + glob.assert_called_with('/var/tmp/insights-archive-*') + rmtree.assert_called_with(test_tmp_dir, True) + @patch('insights.client.archive.os.makedirs') @patch('insights.client.archive.os.path.exists', Mock(return_value=False)) def test_create_archive_dir_obfuscated(self, makedirs, _, __): @@ -183,3 +199,39 @@ def test_copy_dir(self, create_archive_dir, _, __): archive = InsightsArchive(Mock()) archive.copy_dir('test') create_archive_dir.assert_called_once() + + @patch('insights.client.archive.shutil.copyfile') + @patch('insights.client.archive.os.path.join', side_effect=['/var/tmp/test-archive/insights-archive-test.tar.gz']) + @patch('insights.client.archive.os.path.isdir', Mock()) + def test_keep_archive(self, os_, copyfile, _, __): + archive = InsightsArchive(Mock()) + archive.tar_file = '/var/tmp/insights-archive-test.tar.gz' + archive.keep_archive_dir = '/var/tmp/test-archive' + archive.storing_archive() + copyfile.assert_called_once_with(archive.tar_file, '/var/tmp/test-archive/insights-archive-test.tar.gz') + + @patch('insights.client.archive.shutil.copyfile', side_effect=OSError) + @patch('insights.client.archive.os.path.join', Mock()) + @patch('insights.client.archive.os.path.isdir', Mock()) + @patch('insights.client.archive.os.path.basename', Mock()) + @patch('insights.client.archive.logger') + def test_keep_archive_err_during_copy(self, logger, copyfile, _, __): + archive = InsightsArchive(Mock()) + archive.archive_stored = '/var/tmp/test-archive/test-store-archive' + archive.keep_archive_dir = '/var/tmp/test-archive' + with raises(Exception): + archive.storing_archive() + logger.error.assert_called_once_with('ERROR: Could not stored archive to %s', archive.archive_stored) + + @patch('insights.client.archive.os.makedirs', side_effect=OSError) + @patch('insights.client.archive.os.path.exists', return_value=False) + @patch('insights.client.archive.os.path.join', Mock()) + @patch('insights.client.archive.os.path.isdir', Mock()) + @patch('insights.client.archive.os.path.basename', Mock()) + @patch('insights.client.archive.logger') + def test_keep_arhive_err_creating_directory(self, logger, path_exists, mkdir, _, __): + archive = InsightsArchive(Mock()) + archive.keep_archive_dir = '/var/tmp/test-archive' + with raises(Exception): + archive.storing_archive() + logger.error.assert_called_once_with('ERROR: Could not create %s', archive.keep_archive_dir) diff --git a/insights/tests/client/test_client.py b/insights/tests/client/test_client.py index e81f6a7324..a431a195ce 100644 --- a/insights/tests/client/test_client.py +++ b/insights/tests/client/test_client.py @@ -341,18 +341,34 @@ def test_upload_412_write_unregistered_file(_, upload_archive, write_unregistere sys.argv = tmp -def test_cleanup_tmp(): - config = InsightsConfig(keep_archive=True) +@patch('insights.client.archive.InsightsArchive.storing_archive') +def test_cleanup_tmp(storing_archive): + config = InsightsConfig(keep_archive=False) arch = InsightsArchive(config) - arch.tar_file = os.path.join(arch.archive_tmp_dir, 'test.tar.gz') + arch.tar_file = os.path.join(arch.tmp_dir, 'test.tar.gz') arch.cleanup_tmp() assert not os.path.exists(arch.tmp_dir) - assert os.path.exists(arch.archive_tmp_dir) + storing_archive.assert_not_called() - config.keep_archive = False + config.keep_archive = True arch.cleanup_tmp() assert not os.path.exists(arch.tmp_dir) - assert not os.path.exists(arch.archive_tmp_dir) + storing_archive.assert_called_once() + + +@patch('insights.client.archive.InsightsArchive.storing_archive') +def test_cleanup_tmp_obfuscation(storing_archive): + config = InsightsConfig(keep_archive=False, obfuscate=True) + arch = InsightsArchive(config) + arch.tar_file = os.path.join(arch.tmp_dir, 'test.tar.gz') + arch.cleanup_tmp() + assert not os.path.exists(arch.tmp_dir) + storing_archive.assert_not_called() + + config.keep_archive = True + arch.cleanup_tmp() + assert not os.path.exists(arch.tmp_dir) + storing_archive.assert_called_once() @patch('insights.client.client._legacy_handle_registration')