Fixes #32030 - Don't crash upon localhost Ansible tasks
#391
Conversation
|
Can one of the admins verify this patch? |
domq
force-pushed
the
fix/ansible-runner-publish_data_for-unknown-host
branch
from
83ac28d
to
cdf7716
Compare
domq
force-pushed
the
fix/ansible-runner-publish_data_for-unknown-host
branch
from
cdf7716
to
7bac55f
Compare
When ansible-runner executes a local task (through `delegate_to: localhost` or any other available mechanism in Ansible to do so), it breaks the implicit assumption that all logs are related to a host that we know about (with “we” being Foreman in general, and the `inputs["targets"]` Dynflow structure in this particular case). - Guard against this possibility (and drop the log entry in this case). - Thwart silly Rubocop check - IMO "bite if angry" style with a composite "angry" clause (as in here) makes the code easy to misunderstand.
domq
force-pushed
the
fix/ansible-runner-publish_data_for-unknown-host
branch
from
7bac55f
to
b2232d8
Compare
|
Fixes #32030 |
|
As you mentioned Foreman assumes it knows about every single host in the job and each event generated by ansible-runner is related either to a specific known host or to all hosts. If the assumption doesn't hold, it crashes. As far as I'm concerned, having an unknown host in the run is an error state since it deviates from the intended use and should be treated accordingly. If I'm reading this right, with this patch all events which belong to any unexpected host get silently discarded. This isn't really ideal. If the job does something, then the user should be able to find out somewhere. It seems what you want to accomplish is more in line with what https://github.com/ATIX-AG/foreman_acd does |
|
I understand your line of reasoning, however sometimes Ansible needs to prep things on the local host e.g. to generate secrets etc. Would you be OK if I changed the PR to special-case |
As per code review: - Broadcast messages for `localhost` - Raise an error for unknown, “real” hosts
|
@ezr-ondrej any opinions? |
domq
changed the title
localhost Ansible tasks
|
I'd agree that localhost preparations is quite a valid use case for ansible and thus we should support it. While playing with this I've come up with a fix for another bug we are facing with this reporting: https://projects.theforeman.org/issues/32188. I've opened #398 containing both the fixes, just moving the unknown host guard to |
|
We should instead deny running tasks against |
Please don't do this. A number of Ansible use cases don't want to be forced to use ssh (e.g. when managing Kubernetes objects). Edit: some Ansible features (such as |
| elsif hostname == 'localhost' | ||
| broadcast_data(event['stdout'] + "\n", 'stdout') | ||
| else | ||
| raise "handle_host_event: unknown host #{hostname}" |
There was a problem hiding this comment.
Since this PR was opened, we moved to processing the artifact files on demand. That means this raise does not really make sense anymore because the events may be processed after the job has already finished.
From where I stand we have essentially three options:
- Broadcast events from unknown hosts the same way you do for localhost
- Broadcast events from unknown hosts, but mark them as unexpected by broadcasting them as stderr
- Just log those as errors
|
The issue this tried to resolve should be fixed by theforeman/smart_proxy_ansible#55 . Closing, thank you for proposing these changes |
When ansible-runner executes a local task (through
delegate_to: localhostor any other available mechanism in Ansible to do so), it breaks the implicit assumption that all logs are related to a host that we know about (with “we” being Foreman in general, and theinputs["targets"]Dynflow structure in this particular case).localhost, under the assumption that they consist of some kind of prep step that concerns all hosts