Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

archive.extracted: don't try to cache local sources #38285

Merged
merged 3 commits into from
Dec 19, 2016
Merged

archive.extracted: don't try to cache local sources #38285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e774080
archive.extracted: don't try to cache local sources
terminalmage Dec 15, 2016
d018447
Update tests to reflect change in cache behavior
terminalmage Dec 16, 2016
f0aefc6
Add mock return for /bin/tar
terminalmage Dec 19, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 22 additions & 11 deletions salt/states/archive.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,14 @@ def extracted(name,
urlparsed_source = _urlparse(source_match)
source_hash_basename = urlparsed_source.path or urlparsed_source.netloc

source_is_local = urlparsed_source.scheme in ('', 'file')
if source_is_local:
# Get rid of "file://" from start of source_match
source_match = urlparsed_source.path
if not os.path.isfile(source_match):
ret['comment'] = 'Source file \'{0}\' does not exist'.format(source_match)
return ret

valid_archive_formats = ('tar', 'rar', 'zip')
if not archive_format:
archive_format = salt.utils.files.guess_archive_type(source_hash_basename)
Expand Down Expand Up @@ -756,16 +764,19 @@ def extracted(name,
)
return ret

cached_source = os.path.join(
__opts__['cachedir'],
'files',
__env__,
re.sub(r'[:/\\]', '_', source_hash_basename),
)
if source_is_local:
cached_source = source_match
else:
cached_source = os.path.join(
__opts__['cachedir'],
'files',
__env__,
re.sub(r'[:/\\]', '_', source_hash_basename),
)

if os.path.isdir(cached_source):
# Prevent a traceback from attempting to read from a directory path
salt.utils.rm_rf(cached_source)
if os.path.isdir(cached_source):
# Prevent a traceback from attempting to read from a directory path
salt.utils.rm_rf(cached_source)

if source_hash:
try:
Expand All @@ -787,7 +798,7 @@ def extracted(name,
else:
source_sum = {}

if not os.path.isfile(cached_source):
if not source_is_local and not os.path.isfile(cached_source):
if __opts__['test']:
ret['result'] = None
ret['comment'] = \
Expand Down Expand Up @@ -1304,7 +1315,7 @@ def extracted(name,
ret['changes']['directories_created'] = [name]
ret['changes']['extracted_files'] = files
ret['comment'] = '{0} extracted to {1}'.format(source_match, name)
if not keep:
if not source_is_local and not keep:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I reading this right that the default behavior now is to remove the local cached copy of a remove archive? It looks like the docs say the default for keep is supposed to be True but here it looks like the default is False.

I noticed a large increase in network activity and time when I updated some minions recently and tracked down the source to archive.extracted. The source files are no longer being stored in the minion cache and are being downloaded to the minions each time highstate is run.

Could this be the cause of the change in behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ntrepid8 The default behavior prior to 2017.7.3 would have been to remove the cached source archive. This was changed to keep the cached source in #43518 (backported for the 2017.7.3 release in #43681).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

log.debug('Cleaning cached source file %s', cached_source)
try:
os.remove(cached_source)
Expand Down
93 changes: 57 additions & 36 deletions tests/unit/states/archive_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
# Import python libs
from __future__ import absolute_import
import os
import tempfile

# Import Salt Testing libs
from salttesting import TestCase, skipIf
Expand All @@ -31,6 +30,24 @@
archive.__env__ = 'test'


def _isfile_side_effect(path):
'''
MagicMock side_effect for os.path.isfile(). We don't want to use dict.get
here because we want the test to fail if there's a path we haven't
accounted for, so that we can add it.

NOTE: This may fall over on some platforms if /usr/bin/tar does not exist.
If so, just add an entry in the dictionary for the path being used for tar.
'''
return {
'/tmp/foo.tar.gz': True,
'/tmp/out': False,
'/usr/bin/tar': True,
'/bin/tar': True,
'/tmp/test_extracted_tar': False,
}[path]


@skipIf(NO_MOCK, NO_MOCK_REASON)
class ArchiveTestCase(TestCase):

Expand All @@ -45,8 +62,8 @@ def test_extracted_tar(self):
archive.extracted tar options
'''

source = '/tmp/file.tar.gz'
tmp_dir = os.path.join(tempfile.gettempdir(), 'test_archive', '')
source = '/tmp/foo.tar.gz'
tmp_dir = '/tmp/test_extracted_tar'
test_tar_opts = [
'--no-anchored foo',
'v -p --opt',
Expand Down Expand Up @@ -74,6 +91,7 @@ def test_extracted_tar(self):
'top_level_dirs': [],
'top_level_files': ['saltines', 'cheese'],
})
isfile_mock = MagicMock(side_effect=_isfile_side_effect)

with patch.dict(archive.__opts__, {'test': False,
'cachedir': tmp_dir}):
Expand All @@ -84,26 +102,25 @@ def test_extracted_tar(self):
'cmd.run_all': mock_run,
'archive.list': list_mock,
'file.source_list': mock_source_list}):
filename = os.path.join(
tmp_dir,
'files/test/_tmp_file.tar.gz'
)
for test_opts, ret_opts in zip(test_tar_opts, ret_tar_opts):
ret = archive.extracted(tmp_dir,
source,
options=test_opts,
enforce_toplevel=False)
ret_opts.append(filename)
mock_run.assert_called_with(ret_opts, cwd=tmp_dir, python_shell=False)
with patch.object(os.path, 'isfile', isfile_mock):
for test_opts, ret_opts in zip(test_tar_opts, ret_tar_opts):
ret = archive.extracted(tmp_dir,
source,
options=test_opts,
enforce_toplevel=False)
ret_opts.append(source)
mock_run.assert_called_with(ret_opts,
cwd=tmp_dir + os.sep,
python_shell=False)

def test_tar_gnutar(self):
'''
Tests the call of extraction with gnutar
'''
gnutar = MagicMock(return_value='tar (GNU tar)')
source = '/tmp/foo.tar.gz'
missing = MagicMock(return_value=False)
nop = MagicMock(return_value=True)
mock_false = MagicMock(return_value=False)
mock_true = MagicMock(return_value=True)
state_single_mock = MagicMock(return_value={'local': {'result': True}})
run_all = MagicMock(return_value={'retcode': 0, 'stdout': 'stdout', 'stderr': 'stderr'})
mock_source_list = MagicMock(return_value=(source, None))
Expand All @@ -113,30 +130,32 @@ def test_tar_gnutar(self):
'top_level_dirs': [],
'top_level_files': ['stdout'],
})
isfile_mock = MagicMock(side_effect=_isfile_side_effect)

with patch.dict(archive.__salt__, {'cmd.run': gnutar,
'file.directory_exists': missing,
'file.file_exists': missing,
'file.directory_exists': mock_false,
'file.file_exists': mock_false,
'state.single': state_single_mock,
'file.makedirs': nop,
'file.makedirs': mock_true,
'cmd.run_all': run_all,
'archive.list': list_mock,
'file.source_list': mock_source_list}):
ret = archive.extracted('/tmp/out',
source,
options='xvzf',
enforce_toplevel=False,
keep=True)
self.assertEqual(ret['changes']['extracted_files'], 'stdout')
with patch.object(os.path, 'isfile', isfile_mock):
ret = archive.extracted('/tmp/out',
source,
options='xvzf',
enforce_toplevel=False,
keep=True)
self.assertEqual(ret['changes']['extracted_files'], 'stdout')

def test_tar_bsdtar(self):
'''
Tests the call of extraction with bsdtar
'''
bsdtar = MagicMock(return_value='tar (bsdtar)')
source = '/tmp/foo.tar.gz'
missing = MagicMock(return_value=False)
nop = MagicMock(return_value=True)
mock_false = MagicMock(return_value=False)
mock_true = MagicMock(return_value=True)
state_single_mock = MagicMock(return_value={'local': {'result': True}})
run_all = MagicMock(return_value={'retcode': 0, 'stdout': 'stdout', 'stderr': 'stderr'})
mock_source_list = MagicMock(return_value=(source, None))
Expand All @@ -146,21 +165,23 @@ def test_tar_bsdtar(self):
'top_level_dirs': [],
'top_level_files': ['stderr'],
})
isfile_mock = MagicMock(side_effect=_isfile_side_effect)

with patch.dict(archive.__salt__, {'cmd.run': bsdtar,
'file.directory_exists': missing,
'file.file_exists': missing,
'file.directory_exists': mock_false,
'file.file_exists': mock_false,
'state.single': state_single_mock,
'file.makedirs': nop,
'file.makedirs': mock_true,
'cmd.run_all': run_all,
'archive.list': list_mock,
'file.source_list': mock_source_list}):
ret = archive.extracted('/tmp/out',
source,
options='xvzf',
enforce_toplevel=False,
keep=True)
self.assertEqual(ret['changes']['extracted_files'], 'stderr')
with patch.object(os.path, 'isfile', isfile_mock):
ret = archive.extracted('/tmp/out',
source,
options='xvzf',
enforce_toplevel=False,
keep=True)
self.assertEqual(ret['changes']['extracted_files'], 'stderr')

if __name__ == '__main__':
from integration import run_tests
Expand Down