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

Added a skip_files_list_verify argument to archive.extracted state #55700

Merged
merged 3 commits into from Dec 23, 2019

Conversation

Oloremo
Copy link
Contributor

@Oloremo Oloremo commented Dec 19, 2019

What does this PR do?

This PR adding an ability to improve the archive.extracted state performance and traffic efficiency by adding an optional argument skip_files_list_verify which will allow to skip the archive file list validation replacing it by checksum validation.

What issues does this PR fix or reference?

#55443

Previous Behavior

Before that, it was a choice between re-downloading source every time OR keeping all archives sources in the cache without any ability to purge it efficiently.

New Behavior

Setting skip_files_list_verify to True will enforce the checksum check and if the checksums are match - we're done here.

Tests written?

Yes

Commits signed with GPG?

Yes

@Oloremo Oloremo requested a review from as a code owner Dec 19, 2019
@ghost ghost requested a review from waynew Dec 19, 2019
@Oloremo Oloremo force-pushed the 55443-archive-hash-trust branch 2 times, most recently from 4ef9413 to 0f0e4ef Compare Dec 19, 2019
@codecov
Copy link

@codecov codecov bot commented Dec 19, 2019

Codecov Report

Merging #55700 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #55700      +/-   ##
==========================================
- Coverage   18.82%   18.81%   -0.01%     
==========================================
  Files         819      819              
  Lines      175421   175243     -178     
  Branches    37622    37597      -25     
==========================================
- Hits        33008    32953      -55     
+ Misses     139758   139640     -118     
+ Partials     2655     2650       -5
Flag Coverage Δ
#archlts 18.09% <ø> (-0.01%) ⬇️
#centos7 23.72% <ø> (-0.01%) ⬇️
#proxy 23.75% <ø> (-0.02%) ⬇️
#py2 18.6% <ø> (-0.01%) ⬇️
#py3 18.44% <ø> (-0.01%) ⬇️
#runtests 18.81% <ø> (-0.01%) ⬇️
#ubuntu1604 23.7% <ø> (-0.02%) ⬇️
#zeromq 18.81% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
salt/modules/cp.py 22.15% <ø> (ø) ⬆️
salt/utils/ctx.py 87.5% <0%> (-8.33%) ⬇️
salt/modules/vsphere.py 14.67% <0%> (-0.54%) ⬇️
salt/utils/vmware.py 8.16% <0%> (-0.31%) ⬇️
salt/utils/event.py 38.36% <0%> (-0.3%) ⬇️
salt/config/__init__.py 27.46% <0%> (-0.17%) ⬇️
salt/modules/virt.py 10.06% <0%> (-0.01%) ⬇️
salt/modules/dockermod.py 10.82% <0%> (ø) ⬆️
salt/utils/win_update.py 10.66% <0%> (ø) ⬆️
salt/modules/rh_service.py 20.47% <0%> (ø) ⬆️
... and 3 more

@Oloremo
Copy link
Contributor Author

@Oloremo Oloremo commented Dec 19, 2019

salt/utils/ctx.py | 87.5% <0%> (-8.33)

So I'm reducing code coverage of files I didn't even touch. That's my secret superpower!

@Oloremo Oloremo force-pushed the 55443-archive-hash-trust branch from 0f0e4ef to b55087a Compare Dec 20, 2019
salt/states/archive.py Outdated Show resolved Hide resolved
@Oloremo Oloremo force-pushed the 55443-archive-hash-trust branch from b55087a to 514aa40 Compare Dec 21, 2019
@@ -533,7 +553,7 @@ def extracted(name,
then re-create that directory before extracting. Note that ``clean``
and ``clean_parent`` are mutually exclusive.
.. versionadded:: Sodium
.. versionadded:: Neon
Copy link
Contributor Author

@Oloremo Oloremo Dec 21, 2019

Choose a reason for hiding this comment

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

not related to this PR, just a mistake from my previous one. I hope it's ok to fix it here too

@Oloremo
Copy link
Contributor Author

@Oloremo Oloremo commented Dec 21, 2019

I tested this logic on some scenarios like a remote source, local source, remote source hash, source_hash as a string - all seems good. Not sure about all possible edge cases - salt file caching is complicated. But this is an optional feature disabled by default so I hope it's ok.

What I'm worried about in the docs. I'm not sure if I described it clearly.

@Oloremo
Copy link
Contributor Author

@Oloremo Oloremo commented Dec 21, 2019

re-run full macosxmojave-py2

dwoz
dwoz approved these changes Dec 23, 2019
@dwoz dwoz merged commit 0eb09fc into saltstack:master Dec 23, 2019
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants