AAP-39365 facts are unintentionally deleted when the inventory is modified during a job execution #15910
Conversation
…e undesired behaviour with fact cache Signed-off-by: onetti7 <[email protected]>
…e undesired behaviour with fact cache Signed-off-by: onetti7 <[email protected]>
Signed-off-by: onetti7 <[email protected]>
Signed-off-by: onetti7 <[email protected]>
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. ❌ Your project check has failed because the head coverage (52.88%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage.
🚀 New features to boost your workflow:
|
… contributor" This reverts commit f1a056a.
…ecific to the particular slice when applicable
awx/main/tasks/facts.py
Outdated
| @@ -130,6 +131,9 @@ def finish_fact_cache(hosts, destination, facts_write_time, log_data, job_id=Non | |||
| log_data['unmodified_ct'] += 1 | |||
| else: | |||
| # if the file goes missing, ansible removed it (likely via clear_facts) | |||
| # if the file goes missing, but the host hasnot started facts, then we should not clear the facts | |||
| if hosts_cached and host.name not in hosts_cached: | |||
| continue | |||
There was a problem hiding this comment.
The outer iteration loop no longer makes any sense, that being for host in hosts:. On the surface-level you might as well change that to for host in hosts_cached: (and any associated changes), and get rid of the continue added here.
This requires changing the method signature (this is a structural change and that should not be surprising). As passing in job.get_hosts_for_fact_cache() for hosts carries a non-trivial cost and it is no longer needed considering what you're doing.
Given that you don't need to pass it into finish_fact_cache, I'm also unclear if it should be passed into start_fact_cache. A semi-isolated way to do that might be to pass the full Inventory object from the outset.
I talked a good deal with @chrismeyersfsu about further consequences and I'm not sure if the refactor should even stop there. The contract between the start/finish methods is mainly the artifacts directory, and passing around the list of cached hosts in memory (given this particular kind of toxic code path) may be less maintainable than passing it around on disk. I would seriously consider saving the data structure in a file next to the fact cache folder (both being inside of artifacts). If you did that, I also expect it would lead into removing the facts_write_time argument.
…f it resolves git diff issue
…e unneeded continue
|
| last_filepath_written = None | ||
| for host in hosts: | ||
| for host in hosts_cached: |
There was a problem hiding this comment.
The start_fact_cache still needs to get the host list from the database, which is job.get_hosts_for_fact_cache(), which is still passed in as hosts to this method still. It's only the finishing method that should defer to the cached hosts from the starting method... somehow.
SUMMARY
This change fixes facts are unintentionally deleted when the inventory is modified during a job execution
ISSUE TYPE
COMPONENT NAME
AWX VERSION
ADDITIONAL INFORMATION