dnf: successful etckeeper commit causes successful package install to fail #54949
Comments
|
Here's all this, which isnt really necessary: ---
# playbook.yml
- hosts: all
name: Install etckeeper
tags: ['etckeeper']
become: true
roles:
- role: etckeeper
tags: ['etckeeper']
---
# etckeeper/tasks/main.yml
# tasks file for etckeeper
- name: Load etckeeper-pre-task and etckeeper-post-task action_plugins
import_role:
name: hlovdal.etckeeper_actions
tags: ['etckeeper']
- name: Install etckeeper
tags: [etckeeper]
become: true
block:
- name: Check for /etc/.git and etckeeper
shell: test -d "/etc/.git" && test -e /usr/bin/etckeeper
ignore_errors: true
failed_when: etckeeper.rc > 1
changed_when: etckeeper.rc > 1
register: etckeeper
- etckeeper-pre-task:
when:
- etckeeper.rc == 0
- name: Install etckeeper package
package:
name: etckeeper
# See: https://github.com/ansible/ansible/issues/54949
- name: Configure etckeeper.conf
lineinfile:
path: /etc/etckeeper/etckeeper.conf
regexp: '^GIT_COMMIT_OPTIONS=.*'
line: 'GIT_COMMIT_OPTIONS="-q"'
- name: Run etckeeper init
command: etckeeper init
args:
creates: "/etc/.git"
register: etckinit
- name: Check git user
command: git config --local --get user.name
args:
chdir: /etc
failed_when: gituser.rc > 1
changed_when: gituser.rc > 1
register: gituser
- name: Check git email
command: git config --local --get user.email
args:
chdir: /etc
failed_when: gitemail.rc > 1
changed_when: gitemail.rc > 1
register: gitemail
- name: Set git user if not set
command: git config --add user.name Root
args:
chdir: /etc
when: gituser.rc != 0
- name: Set git email if not set
command: git config --add user.email root@localhost
args:
chdir: /etc
when: gitemail.rc != 0
#- name: Check whether there are uncommited etckeeper entries
# command: etckeeper unclean
# failed_when: etck.rc > 1
# changed_when: etck.rc == 0
# register: etck
- name: Create initial etckeeper commit
command: etckeeper commit "Initial commit by ansible"
when: etckinit.changed == true
#- name: Commit uncommited changes in /etc with etckeeper
# command: etckeeper commit "Commit uncommitted by ansible"
# when:
# - etck.rc == 0
# - etckinit.changed == false
- etckeeper-post-task: |
ansibot
added
affects_2.7
bcoca
added
needs_verified
|
This appears to be related to how Ansible parses module's result as identified here: |
|
I think the reproducer should use We may need to temporary patch stdout to catch that... |
webknjaz
added
verified
|
Clean Docker-based reproducers (at the time of writing installs Ansible 2.8.0-2 in all cases), crashes in all cases:
|
|
So the root of the issue is that class Etckeeper(dnf.Plugin):
...
def resolved(self):
...
Oops -->command = '%s %s' % ('etckeeper', " pre-install")
ret = os.system(command) <-- Oops!
...
def transaction(self):
...
Oops -->command = '%s %s > /dev/null' % ('etckeeper', "post-install")
os.system(command) <-- Oops! |
The Ansible dnf module uses the python dnf bindings. In contexts like these, stdout/stderr is owned by the host app (Ansible). dnf should not mess with stdout/stderr, unless the host app asks it to log there. Specifically, it was breaking the JSON output of the Ansible module. This was only noticeable when the etckeeper message began with a "[" character. Ansible has a mechanism to try and skip header messages like login banners. However, "[" is a valid character to start a JSON document. https://unix.stackexchange.com/questions/511210/ansible-dnf-module-module-failure/ This requires decoding the etckeeper messages into unicode. For gory details, see the code comment. Also enable unicode string literals. This reduces differences between py2 and py3 semantics; it is universal inside the dnf code. Also when I tested the failure case, I noticed the exit code was not printed correctly. Fix it.
|
So I've found out that @sourcejedi posted a patch @ sourcejedi/etckeeper@afbdce2 but it's not clear when it gets accepted by the upstream. |
|
Call stack FTR: File "/root/.ansible/tmp/ansible-tmp-1559833967.9878116-163794092439180/AnsiballZ_dnf.py", line 114, in <module>
_ansiballz_main()
File "/root/.ansible/tmp/ansible-tmp-1559833967.9878116-163794092439180/AnsiballZ_dnf.py", line 106, in _ansiballz_main
invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)
File "/root/.ansible/tmp/ansible-tmp-1559833967.9878116-163794092439180/AnsiballZ_dnf.py", line 49, in invoke_module
imp.load_module('__main__', mod, module, MOD_DESC)
File "/usr/lib64/python3.7/imp.py", line 234, in load_module
return load_source(name, filename, file)
File "/usr/lib64/python3.7/imp.py", line 169, in load_source
module = _exec(spec, sys.modules[name])
File "<frozen importlib._bootstrap>", line 630, in _exec
File "<frozen importlib._bootstrap_external>", line 728, in exec_module
File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
File "/tmp/ansible_dnf_payload_r65mifyv/__main__.py", line 1303, in <module>
main()
File "/tmp/ansible_dnf_payload_r65mifyv/__main__.py", line 1287, in main
module_implementation.run()
File "/tmp/ansible_dnf_payload_r65mifyv/__main__.py", line 1265, in run
self.ensure()
File "/tmp/ansible_dnf_payload_r65mifyv/__main__.py", line 1144, in ensure
if not self.base.resolve(allow_erasing=allow_erasing):
File "/usr/lib/python3.7/site-packages/dnf/base.py", line 785, in resolve
self._plugins.run_resolved()
File "/usr/lib/python3.7/site-packages/dnf/plugin.py", line 156, in run_resolved
self._caller('resolved')
File "/usr/lib/python3.7/site-packages/dnf/plugin.py", line 104, in _caller
getattr(plugin, method)()
File "/usr/lib/python3.7/site-packages/dnf-plugins/etckeeper.py", line 40, in resolved
set_trace() |
|
Hrm, the module junk filter would normally take care of this, but because it has support for JSON arrays in addition to dicts (though not sure why, module results should always be a dict IIRC), that first line that starts with |
|
Have to be careful because update_json will need to deliver a stream of dicts. A list is once way to represent that. But @sivel discovered an RFC which used a specific single unicode character. to represent a stream of json. If we used that, we might be able to remove list checking from the junk filter (but might have to add handling for the separator instead). |
|
@abadger there's also JSON-L which is basically LF delimited lines with JSON objects... |
|
I'm curious whether we could put some magic byte (sequence) in front of the emitted JSON result to make it explicit where it starts... |
|
Here is what I had previously linked to: https://en.wikipedia.org/wiki/JSON_streaming#Record_separator-delimited_JSON Which is formally described by https://tools.ietf.org/html/rfc7464 |
|
@sivel It's similar to what I had in mind: RS could be a protocol version.. |
|
Could there be a force_not_json=True parameter somewhere?
…On Friday, June 7, 2019, Sviatoslav Sydorenko ***@***.***> wrote:
@sivel <https://github.com/sivel> It's similar to what I had in mind: RS
could be a protocol version..
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#54949?email_source=notifications&email_token=AAAMNS4BMTVQITCPQZDVUU3PZJV57A5CNFSM4HEB4HGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXF7JTI#issuecomment-499905741>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAMNS2VUZD36P4DQBNDSHDPZJV57ANCNFSM4HEB4HGA>
.
|
|
Is it written somewhere in the docs that output should not start with a
JSON control character if it's not JSON? { [
…On Friday, June 7, 2019, Wes Turner ***@***.***> wrote:
Could there be a force_not_json=True parameter somewhere?
On Friday, June 7, 2019, Sviatoslav Sydorenko ***@***.***>
wrote:
> @sivel <https://github.com/sivel> It's similar to what I had in mind: RS
> could be a protocol version..
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#54949?email_source=notifications&email_token=AAAMNS4BMTVQITCPQZDVUU3PZJV57A5CNFSM4HEB4HGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXF7JTI#issuecomment-499905741>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAAMNS2VUZD36P4DQBNDSHDPZJV57ANCNFSM4HEB4HGA>
> .
>
|
|
I think that'd be not good from a design perspective. |
Modules communicate their results back as JSON responses. If something else outputs data, regardless of it being JSON can cause parsing issues. Due to misconfigurations of bash profiles and other configurations, we try to strip out non JSON lines, but in the case of etckeeper it is printing out lines that look like JSON, so we don't strip those lines, which then breaks our ability to parse the module responses. So in general, nothing other than the module JSON response should be printed to stdout during a module execution. We just do a best effort, to make our JSON consumption more robust. |
IDGI. Isn't EDIT: if the "old" module doesn't use the python Ansible library code. |
|
Right. For backward compat, yes. |
SUMMARY
When etckeeper commits, dnf package installs (that are successfully installed) fail.
It may be due to the etckeeper output preceding dnf output?
Workarounds:
/etc/etckeeper/etckeeper.conf, setGIT_OPTIONS="-q"etckeeper commit(and in the ansible logs, TBH)ISSUE TYPE
COMPONENT NAME
dnf module
ANSIBLE VERSION
CONFIGURATION
OS / ENVIRONMENT
STEPS TO REPRODUCE
package: name=etckeeperecho "TestTest" | sudo tee /etc/README.mdpackage: name=ctags-etagsEXPECTED RESULTS
package: name=ctags-etagsinstalls successfullyACTUAL RESULTS
package: name=ctags-etagsdoes not installThe text was updated successfully, but these errors were encountered: