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

Don't warn when trying to read missing cache file #57646

Open
wants to merge 4 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@steffann
Copy link
Contributor

commented Jun 10, 2019

SUMMARY

A missing cache entry should fail silently. Showing warning messages when failing to open a cache
file, and then working as expected, is confusing to the user. This patch changes the warning to
a debug message.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

cache

ADDITIONAL INFORMATION

Currently warning message such as this are shown:

[WARNING]: error in 'jsonfile' cache plugin while trying to read .cache/inventory/netbox_b42bfs_ef540 : b"[Errno 2] No such file or directory: '.cache/inventory/netbox_b42bfs_ef540'"

steffann added some commits Jun 10, 2019

@s-hertel
Copy link
Contributor

left a comment

Looks good to me.

@s-hertel

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

This could use a changelog

@bcoca

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

also,i would make this a toggle, since the key existing is an indicator that a file did exist at one point and something might be wrong with the cache (another process removed the cache file while ansible is running).

@steffann

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

also,i would make this a toggle, since the key existing is an indicator that a file did exist at one point and something might be wrong with the cache (another process removed the cache file while ansible is running).

I don't see the benefit of that. By their nature caches can disappear without warning at any point in time. That is not an abnormal situation.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

The test ansible-test sanity --test changelog [explain] failed with 1 error:

changelogs/fragments/cache-file-missing-warning.yaml:0:0: section "minor_changes" list items must be type str not dict

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Jun 10, 2019

@samdoran samdoran removed the needs_triage label Jun 11, 2019

@bcoca

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

caches should not disappear mid run, it is fine if they disappear across runs

@s-hertel

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@steffann Since this bug happens across runs, https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/cache/__init__.py#L316-L324
might be able to be modified as an alternative to a toggle, for example by checking self._plugin.contains(key).

@ansibot ansibot added core_review and removed needs_revision labels Jun 11, 2019

@bcoca

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

ah, is that new? i thought we always checked contains, that is probably why this is happening more often, it used to only happen when file was removed in middle of run. It should really return KeyError if no cache was ever present.

@s-hertel

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

Since inventory plugins have used caching (even before the linked code existed) it has been a bug across runs. I didn't think about FactCache though, which would only happen in the middle of runs, and I agree those errors shouldn't be suppressed.

@bcoca

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

k, then i think we need to fix that bug, not hide the warning, but prevent false positives.

@steffann

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

I'm not really sure what the correct fix is. I'd be happy to help if someone can tell me where to start

@bcoca

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

unsure, but i think we just need to remove this keyerror capture and let it go through https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/cache/__init__.py#L320

@s-hertel

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@s-hertel

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@bcoca That won't fix the warning. The KeyError happens below anyway. But +1 for removing the try/except/else in __getitem__ even though that doesn't solve this bug - I think I must have copied from the get method below it (the KeyError is passed there because I was trying to make the cache more like a dict).

@bcoca

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

i need to revise the whole stack, i suspect we are doing 'multilevel caching' at plugin, at wrappers and at consumer ... and at the 'cache source itself'.

We should only have 2, one for the 'run' and the cache source, the first to avoid 'mid run expirations', the 2nd to cache across runs. <= for fact caching, after discussing with @s-hertel for inventory cache we only need 'source cache' and keyerrors should be ignored, which should not be the case for 'facts'.

@ansibot ansibot added the stale_ci label Jun 20, 2019

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.