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.

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

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

Merged

Conversation

terminalmage
Copy link
Contributor

This will keep us from trying to cache file when we already have it locally,
which will help significantly with larger archives.

This will keep us from trying to cache file when we already have it
locally, which will help significantly with larger archives.
@rallytime
Copy link
Contributor

@terminalmage Looks like there are some tests that need to be looked at here.

@rallytime rallytime added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Dec 16, 2016
@terminalmage
Copy link
Contributor Author

Yeah I only checked the integration tests, forgot to look at unit tests. This is likely just due to the logic changes since we're not caching the file any more. I'll get these taken care of.

@terminalmage
Copy link
Contributor Author

Tests are updated

@terminalmage
Copy link
Contributor Author

I've got a fix for the lint failures, waiting for the rest of the tests to complete before I push the lint fixes though.

@rallytime rallytime closed this Dec 16, 2016
@rallytime rallytime reopened this Dec 16, 2016
@rallytime
Copy link
Contributor

@terminalmage Hrm. It looks like some of these related tests are still failing on Ubuntu 14.

This fixes failing tests for Ubuntu 14
@rallytime rallytime merged commit 79231a5 into saltstack:2016.11 Dec 19, 2016
gitebra pushed a commit to gitebra/salt that referenced this pull request Dec 21, 2016
* upstream/develop: (35 commits)
  Pylint fix
  Add space around equality operator
  Fix wrong function names
  Remove unused import
  Pylint fix
  Allow the creation of an internal interface for an openvswitch port
  Add NM_CONTROLLED to redhat configured interfaces
  Remove support for client_acl and client_acl_blacklist
  Remove enable/disable funcs from apache_* states
  Remove PidfileMixin as it is deprecated for removal in Nitrogen
  Remove doc markup references from 2016.11 branch
  archive.extracted: don't try to cache local sources (saltstack#38285)
  ZMQ: add an option for zmq.BACKLOG to salt master (zmq_backlog)
  allowing states from other saltenvs to be includes in top.sls
  Add --ssh-option to salt-ssh
  Fix influxdb_database.present state
  Don't check the doc/conf.py file for doc markup refs
  Update tests to reflect change in cache behavior
  archive.extracted: don't try to cache local sources (2016.3 branch)
  Add a unit test to search for new doc markup refs
  ...
@terminalmage terminalmage deleted the archive-extracted-local-source branch April 25, 2017 19:16
@@ -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:

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

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants