When insights client is killed the directories in /var/tmp are not re…

…moved rhbz#2009773 (#3396)

* New prefix for temp archive directory and tests

Signed-off-by: ahitacat <[email protected]>

* keep-archive is stored in /var/cache

Signed-off-by: ahitacat <[email protected]>

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

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'
     },

insights/tests/client/test_archive.py

@@ -1,22 +1,25 @@
 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'
 test_archive_name = 'insights-testhostname-000000'
 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))
 @patch('insights.client.archive.determine_hostname', Mock(return_value=test_hostname))
 @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)

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')