-
Notifications
You must be signed in to change notification settings - Fork 74
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 variable value changes not invalidating cache issue and etc. #255
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank for this PR! Awesome changes and very detailed explanation, this is how PRs should be done! :)
if image_name: | ||
self._display.display("caching the task result in an image '%s'" % image_name) | ||
|
||
@staticmethod | ||
def get_task_content(task: Task): | ||
|
||
def get_templated_ds(task: Task): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please move this function out of the class (e.g. to the top of this file) and please specify in the docstring it's meant to be used in this method
# The hacky solution to this is to use ansible.executor.process.worker.WorkerProcess obj. | ||
# These params can be found in this obj. | ||
# The WorkerProcess obj references the Task obj while running, | ||
# so we can use gc.get_referrers method to find the WorkerProcess obj based on Task obj. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for all the details, this is super-helpful!
# so we can use gc.get_referrers method to find the WorkerProcess obj based on Task obj. | ||
try: | ||
required_fields = ["_loader", "_shared_loader_obj", "_task_vars"] | ||
worker_process_dict = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make this an empty dict please since you treat as a dict below
templar = Templar( | ||
loader=worker_process_dict["_loader"], | ||
shared_loader_obj=worker_process_dict["_shared_loader_obj"], | ||
variables=worker_process_dict["_task_vars"], | ||
) | ||
return templar.template(task.get_ds()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably assert first that those keys are present in the dict so we wouldn't get KeyErrors in future if ansible-core ever changes its internals
serialized_data = task.get_ds() | ||
serialized_data = get_templated_ds(task) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should fall back if get_templated_ds runs into problems
It seems the test cases are wrong actually after reading your explanation. Please update them to what you feel like it's the right thing to do :) |
Also sorry for replying so late, I was super-busy past few weeks, will try to provide replies in a timely manner so we can merge this next week. |
Fix caching: changing variables does not invalidate cache #104. The idea here is doing the templating (variable substitution) for the result of
task.get_ds()
to get the correct fingerprint of the task. To do the templating we need some runtime information, which we are not able to obtain inTask
object. The hacky way I use is usinggc
module to obtain theWorkerProcess
obj that referencesTask
obj.Fix Failed ansible tasks are written to cache #226. Let me explain why this issue happens: when a task failed, it is very likely (always when I tried it)
is_failed
here still returnsFalse
. It's a little bit complicated to explain why this happens behind the scene. Briefly speaking, thesend_callback
function does aclean_copy
for task result that is gonna be sent to the hooks. And theclean_copy
will wipe off thefailed
key in task result. The solution is to use status specific hooks instead ofv2_on_any
.I tested my changes locally and two tests failed:
integration/test_api.py::test_caching
andintegration/test_api.py::test_caching_mechanism
but I think they failed for some reasons.For
test_caching
, I am not sure I understand why it asserts that the task should not be cached (https://github.com/ansible-community/ansible-bender/blob/master/tests/integration/test_api.py#L43)The corresponding task is a file action, but the source file does not change in between two builds. It should be cached.
And this test passes before I made the changes due to a bug. Briefly speaking, in https://github.com/ansible-community/ansible-bender/blob/master/ansible_bender/callback_plugins/snapshoter.py, the task content is calculated twice (
get_task_content
called twice), before the task execution and after the task execution. For file-related task the result of two calculation will differ, causing wrong caching.For
test_caching_mechanism
, it asserts this task will be cachedIf we do go down the variable substitution path, it makes more sense that this task does not get cached, because the
ANSIBLE_CONFIG
variable changes every run.So we'll need to discuss about how we modify the tests, if I am not getting it wrong.
This is the first time I contribute to an open source project. Let me know if I am missing anything. Thanks!