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

Fix parallel states with long ID dec or name #49777

Merged
merged 7 commits into from Sep 30, 2018

Conversation

Projects
None yet
6 participants
@terminalmage
Copy link
Contributor

commented Sep 25, 2018

The cache files used for these are based on the state's tag, so longer ID decs or names will cause errors when the filename's length exceeds the max allowed by the kernel.

This fixes these errors by taking (up to) the first 32 chars of the result of base64-encoding the tag, and using that as the parallel cache filename.

Fixes #49738.

@salt-jenkins salt-jenkins requested a review from saltstack/team-state Sep 25, 2018

@terminalmage

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

I found some other weirdness with the tests as I was writing the test for this PR. The setUp should have been a setUpClass, and there is a tearDown that only impacts a single test and should not be run after every test, but as the finally portion of a try/finally block.

@terminalmage

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

OK, I've pushed another commit that replaces the setUp/tearDown.

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2018

@terminalmage It appears the windows tests didn't like this change: https://jenkinsci.saltstack.com/job/pr-windows2016-py2/job/PR-49777/2/

Can you take a look?

terminalmage added some commits Sep 25, 2018

Fix parallel states with long ID dec or name
The cache files used for these are based on the state's tag, so longer
ID decs or names will cause errors when the filename's length exceeds
the max allowed by the kernel.

This fixes these errors by taking (up to) the first 32 chars of the
result of base64-encoding the tag, and using that as the parallel cache
filename.
Remove/replace unnecessary setUp/tearDown
The setUp only needs to run at the beginning so it can be a setUpClass,
and the tearDown logic only applied to a single test and should not be
run after each test.

@terminalmage terminalmage force-pushed the terminalmage:issue49738 branch from 3313b65 to 5a11067 Sep 26, 2018

@terminalmage

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

@rallytime the failing test on Windows was due to parallel states apparently not working on Windows. I have skipped the failing test on Windows only for the meantime, and I've opened #49808 to track getting parallel states working.

@terminalmage terminalmage requested a review from dwoz Sep 27, 2018

@dwoz

dwoz approved these changes Sep 27, 2018

Explicitly import salt.utils.hashutils
The code works without this because at least one of the other modules
being imported also imports this. But explicitly importing it here will
keep us from getting bitten if the imports change elsewhere later on.
@terminalmage

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

I added one last commit to make the salt.utils.hashutils import explicit.

@damon-atkins

This comment has been minimized.

Copy link
Member

commented Sep 28, 2018

Won't two commands with the same first 22 characters (32 encoded) use the same file, within the same job id?

test_cmd_tag_check_dog:
  cmd.run:
    - name: 'echo quick brown fox jumped over the lazy dog'
    - parallel: True
 test_cmd_tag_check_sheep:
  cmd.run:
    - name: 'echo quick brown fox jumped over the lazy sheep'
    - parallel: True

How about "SLS ID" and documenting they have a length limit determined by the file system.

@terminalmage

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

Absolutely not, that is just an arbitrary limit.

I've switched from base64 to SHA1. A SHA1 hash of the tag should provide a short enough (while still remaining reasonably unique) filename.

@terminalmage terminalmage requested review from dwoz, gtmanfred and rallytime Sep 28, 2018

@damon-atkins

This comment has been minimized.

Copy link
Member

commented Sep 28, 2018

Is it worth added the SLS ID into the string as well before generating SHA1 to cover someone running the same command string more then once in parallel or something like this?

parallel state cache were previously based on the tag for each chunk,
and longer ID decs or name params can cause the cache file to be longer
than the operating system's max file name length. To counter this we
instead base64-encode the chunk's tag (as this is a bit faster than

This comment has been minimized.

Copy link
@damon-atkins

damon-atkins Sep 28, 2018

Member

Suggest this needs some rewording at some stage.

@terminalmage

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

Is it worth added the SLS ID into the string as well before generating SHA1 to cover someone running the same command string parallel or something like this?

No. The tag which is being hashed is already unique because it includes the ID declaration and name param, and the ID dec is guaranteed to be unique (or else the SLS file fails to render due to conflicting ID).

@terminalmage

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

And actually, that means that the base64-encoded result should be unique as well for that matter. But SHA1 should still be less likely to collide (maybe? ¯\_(ツ)_/¯)

@@ -82,6 +82,16 @@ def md5_digest(instr):
return hashlib.md5(instr).hexdigest()


def sha1_digest(instr):
'''
Generate an sha1 hash of a given string.

This comment has been minimized.

Copy link
@damon-atkins

damon-atkins Sep 28, 2018

Member

Might want to add a comment "sha1 is not consider secure, however an acceptable use is for generating fix sized filenames". Maybe md5 should have the same comment as well. Just a thought.

@rallytime rallytime merged commit 297031b into saltstack:2017.7 Sep 30, 2018

5 of 10 checks passed

jenkins/pr/py2-centos-7 The py2-centos-7 job has failed
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has failed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has failed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has failed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has failed
Details
WIP ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details

@terminalmage terminalmage deleted the terminalmage:issue49738 branch Oct 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.