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 roots modification time check #47700

Merged
merged 4 commits into from May 18, 2018

Conversation

Projects
None yet
3 participants
@yannj-fr
Contributor

yannj-fr commented May 17, 2018

What does this PR do?

Fix cache modification time comparison. When converting the modification from os.path.getmtime to string, the number is stripped and the comparison is always false. the hash is recomputed everytime

What issues does this PR fix or reference?

Previous Behavior

hash was compute everytime like if the cache was not present

New Behavior

cache is used

Tests written?

No

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

Yann Jouanin
@@ -255,7 +255,7 @@ def file_hash(load, fnd):
except OSError:
pass
return file_hash(load, fnd)
if os.path.getmtime(path) == mtime:
if "{}".format(os.path.getmtime(path)) == mtime:

This comment has been minimized.

@terminalmage

terminalmage May 17, 2018

Member

Why not just use str()?

This comment has been minimized.

@yannj-fr

yannj-fr May 18, 2018

Contributor

The mtime is created using format for the cache file , I prefer to use the exact same method in case implementation varies.

This comment has been minimized.

@terminalmage

This comment has been minimized.

@yannj-fr

yannj-fr May 18, 2018

Contributor

Sorry my explanation was not clear enough.
The mtime is read from the cache.
but:

  • when the mtime was written in the file it used this line:
   cache_object = '{0}:{1}'.format(ret['hsum'], os.path.getmtime(path))
    with salt.utils.flopen(cache_path, 'w') as fp_:
        fp_.write(cache_object)
    return ret

Because format will convert a float into string with only 2 decimals, like this:

>>> import os 
>>> os.path.getmtime('/dev/null')
>>> ts = os.path.getmtime('/dev/null')
>>> ts
1526395491.048
>>> "{}".format(ts)
'1526395491.05'

the timestamp cached in the cache file will be rounded to 2 decimals, and this will lead to a not matching comparison with the original line:

if os.path.getmtime(path) == mtime:

You can check this behaviour with a simple test:

>>> os.path.getmtime('/dev/null') == "{}".format(os.path.getmtime('/dev/null'))
False

The solution in to format the timestamp the same way it was formated when cached. my choice here is to you the format method as well as in the code I quoted in the beginning of this message.
str() may give exactly the same result, but I feel safer in using the exact same method than the one used to write the cache file

This comment has been minimized.

@terminalmage

terminalmage May 18, 2018

Member

Thanks for the explanation. However, as noted in the Python docs an empty format specifier is equivalent to calling str() on the value.

This comment has been minimized.

@yannj-fr

yannj-fr May 18, 2018

Contributor

do you want me to use str() instead?

This comment has been minimized.

@terminalmage

terminalmage May 18, 2018

Member

Yes, that would avoid the unnecessary overhead of allocating a string and invoking its .format() method.

This comment has been minimized.

@terminalmage

terminalmage May 18, 2018

Member

Yes, that would avoid the unnecessary overhead of allocating a string and invoking its .format() method.

This comment has been minimized.

@yannj-fr

yannj-fr May 18, 2018

Contributor

just changed the PR with str()

yannj-fr and others added some commits May 18, 2018

Yann Jouanin

@rallytime rallytime merged commit d610c19 into saltstack:2017.7.6 May 18, 2018

4 of 9 checks passed

jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5043 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #22977 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10013 — FAILURE
Details
default Build started sha1 is merged.
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #19100 — RUNNING
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25233 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17330 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #21960 — SUCCESS
Details

rallytime added a commit that referenced this pull request May 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment