Skip to content
This repository has been archived by the owner on Feb 7, 2024. It is now read-only.

Run retrace in a podman container instead of mock #277

Merged
merged 26 commits into from
Jan 29, 2020

Conversation

michalfabik
Copy link
Contributor

@michalfabik michalfabik commented Nov 6, 2019

This PR uses the existing mechanisms currently used for mock config to create a Dockerfile in the crash directory. This is then used to create a podman container into which the coredump and metadata and a gdb.sh script are copied and where the retrace itself is run. The use of podman is optional and enabled by setting the newly introduced RetraceEnvironment config variable to "podman". Changes to the retrace user are required during deployment in order to run retrace in a podman container, as described in DEPLOYING.md.

src/retrace/retrace.py Outdated Show resolved Hide resolved
retrace-server.spec.in Outdated Show resolved Hide resolved
retrace-server.spec.in Outdated Show resolved Hide resolved
@ernestask
Copy link
Contributor

Why remove the exploitable check?

src/retrace/retrace.py Outdated Show resolved Hide resolved
retrace-server.spec.in Outdated Show resolved Hide resolved
@michalfabik michalfabik force-pushed the use-podman branch 3 times, most recently from 98179b5 to 74ae6c7 Compare November 7, 2019 14:35
@michalfabik
Copy link
Contributor Author

Why remove the exploitable check?

I don't remember quite precisely to be honest but the gist is that the exploitable plugin (/usr/libexec/abrt-gdb-exploitable) is now part of the abrt-addon-ccpp package which gets installed in the Dockerfile (or the equivalent mock configuration) just before the check happens, so checking for its presence makes no sense. @mkutlak suggested doing away with the checks completely.

Now I see I shoud remove the EXPLOITABLE_PLUGIN_PATH variable as well, since it isn't used anywhere else.

@michalfabik michalfabik changed the title Run retrace in a podman container instead of mock [WIP] Run retrace in a podman container instead of mock Nov 7, 2019
@ernestask
Copy link
Contributor

I don't remember quite precisely to be honest but the gist is that the exploitable plugin (/usr/libexec/abrt-gdb-exploitable) is now part of the abrt-addon-ccpp package which gets installed in the Dockerfile (or the equivalent mock configuration) just before the check happens, so checking for its presence makes no sense. @mkutlak suggested doing away with the checks completely.

Now I see I shoud remove the EXPLOITABLE_PLUGIN_PATH variable as well, since it isn't used anywhere else.

Right, that makes sense. Which checks did Martin want to get rid of, exactly?

retrace-server.spec.in Outdated Show resolved Hide resolved
@mkutlak
Copy link
Contributor

mkutlak commented Nov 14, 2019

I would add a weak dependecy on podman to the spec file.

@@ -99,7 +100,42 @@ rm -f %{buildroot}%{_infodir}/dir
#retrace uid/gid reserved in setup, rhbz #706012
%define retrace_gid_uid 174
getent group retrace > /dev/null || groupadd -f -g %{retrace_gid_uid} --system retrace
getent passwd retrace > /dev/null || useradd --system -g retrace -u %{retrace_gid_uid} -d %{_datadir}/%{name} -s /sbin/nologin retrace
getent passwd retrace > /dev/null || useradd --system -g retrace -u %{retrace_gid_uid} -b %{_sharedstatedir} -s /sbin/nologin retrace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I encourted a problem with this. I already have a retrace user on my system (from older installation) with a home dir /usr/share/retrace.

Upgrading retrace-server does not change the home dir to /var/lib/retrace.

https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be working now. It's quite ugly because simple usermod -m -d will fail if there is a process run by the user retrace. So I just userdel -f the user and then add it again with a different home dir.

Side note for @ernestask: userdel removes the appropriate entries from /etc/sub[ug]id but it leaves the empty lines in place, meaning that the files did end up slightly malformed. After many testing installs and uninstalls they contained one entry for my user followed by many \n's. Fortunately, sort -n puts the blank lines at the top but I added a sed call to get rid of those anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In cut(1):

       -s, --only-delimited
              do not print lines not containing delimiters

That should do the trick as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By “malformed” I meant anything that isn’t empty or in the right format. Probably should have clarified.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dropped this whole part from the .spec file altogether and just documented it instead.

@michalfabik michalfabik changed the title [WIP] Run retrace in a podman container instead of mock Run retrace in a podman container instead of mock Dec 10, 2019
@abrt-bot
Copy link
Contributor

Can one of the admins verify this patch?

@mkutlak
Copy link
Contributor

mkutlak commented Dec 11, 2019

@trapas add to whitelist

src/retrace/retrace.py Outdated Show resolved Hide resolved
src/retrace/retrace.py Outdated Show resolved Hide resolved
retrace-server.spec.in Outdated Show resolved Hide resolved
retrace-server.spec.in Outdated Show resolved Hide resolved
retrace-server.spec.in Outdated Show resolved Hide resolved
src/retrace/retrace.py Outdated Show resolved Hide resolved
@mkutlak
Copy link
Contributor

mkutlak commented Dec 12, 2019

  1. It would be good to have podman honor RequireGPGCheck from config.
  2. Fix indentation commits squashed into 1.
  3. Maybe use run() instead of Popen/call.

Signed-off-by: Michal Fabik <[email protected]>
Signed-off-by: Michal Fabik <[email protected]>
Signed-off-by: Michal Fabik <[email protected]>
Signed-off-by: Michal Fabik <[email protected]>
Signed-off-by: Michal Fabik <[email protected]>

foo

Signed-off-by: Michal Fabik <[email protected]>
Signed-off-by: Michal Fabik <[email protected]>
Signed-off-by: Michal Fabik <[email protected]>
Signed-off-by: Michal Fabik <[email protected]>
@mkutlak
Copy link
Contributor

mkutlak commented Jan 29, 2020

Thank you @michalfabik

@DaveWysochanskiRH
Copy link
Collaborator

DaveWysochanskiRH commented Jan 29, 2020

This is very good work!

Unfortunately there must be something missed here because I'm seeing regressions in my testing which does not use either mock or podman but just vmcores and crash directly.

If I build on 45ed3a6 it gives "not enough arguments for format string"
[2020-01-29 15:14:06] [I] Calling prepare_debuginfo
[2020-01-29 15:14:06] [I] Version: '2.6.32'; Release: '504.el6'; Arch: 'x86_64'; _arch: 'x86_64'; Flavour: 'None'; Realtime: False
[2020-01-29 15:14:06] [I] Found cached vmlinux at path: /cores/retrace/repos/kernel/x86_64/usr/lib/debug/lib/modules/2.6.32-504.el6.x86_64/vmlinux
[2020-01-29 15:14:06] [I] Searching for kernel-debuginfo package for 2.6.32-504.el6.x86_64
[2020-01-29 15:14:06] [E] prepare_debuginfo failed: not enough arguments for format string

Earlier commits will fail with "local variable 'container_id' referenced before assignment:
[2020-01-29 14:56:52] [I] Found cached vmlinux at path: /cores/retrace/repos/kernel/x86_64/usr/lib/debug/lib/modules/2.6.32-504.el6.x86_64/vmlinux
[2020-01-29 14:56:52] [I] Searching for kernel-debuginfo package for 2.6.32-504.el6.x86_64
[2020-01-29 14:56:57] [I] Generating backtrace
[2020-01-29 14:56:57] [I] Stripping to 1 would have no effect
[2020-01-29 14:57:02] [E] local variable 'container_id' referenced before assignment

Here is the bisect log for the first failing commit about 'container_id':

$ git bisect log
git bisect start
# bad: [c7a97005054a9de84de32dcc33b2384d232b8dcd] Run hooks with podman as well
git bisect bad c7a97005054a9de84de32dcc33b2384d232b8dcd
# good: [437e002ac01c04d67f5a91fdb67ab7cfea2cab5e] Add RetraceEnvironment config item
git bisect good 437e002ac01c04d67f5a91fdb67ab7cfea2cab5e
# bad: [964ebe03a1f3733b3b1f7c710fbdee39175e8439] Fix indentation
git bisect bad 964ebe03a1f3733b3b1f7c710fbdee39175e8439
# good: [efd0a6f0162c339de3eeffb5506a8c074d86e000] Make mock-specific parts conditional
git bisect good efd0a6f0162c339de3eeffb5506a8c074d86e000
# bad: [a8f0cdd16432f2d19968de8a79e9198774dceec5] Run retrace in podman container
git bisect bad a8f0cdd16432f2d19968de8a79e9198774dceec5
# good: [5689ef8afdea417c87e165c38d26e700eb086711] Create Dockerfiles
git bisect good 5689ef8afdea417c87e165c38d26e700eb086711
# first bad commit: [a8f0cdd16432f2d19968de8a79e9198774dceec5] Run retrace in podman container

@DaveWysochanskiRH
Copy link
Collaborator

FWIW, I think we can remove the 'set_mock' and 'get_mock' and related logic as that is legacy code that was a wart anyway and it's not needed anymore. That might be related to this failure but I'm not sure yet.

DaveWysochanskiRH added a commit to DaveWysochanskiRH/retrace-server that referenced this pull request Jan 29, 2020
Remove the ability to run the 32-bit version of crash if it is installed.
This was a one-off stopgap because mock could not be used.  It is no
longer important so remove the logic surrounding this which simplifies
the new RetraceEnvironment selection logic.

This fixes a regression introduced with
abrt#277 where tasks
would fail with either
[2020-01-29 14:56:52] [I] Searching for kernel-debuginfo package for 2.6.32-504.el6.x86_64
[2020-01-29 14:56:57] [I] Generating backtrace
[2020-01-29 14:56:57] [I] Stripping to 1 would have no effect
[2020-01-29 14:57:02] [E] local variable 'container_id' referenced before assignment

or
[2020-01-29 15:14:06] [I] Calling prepare_debuginfo
[2020-01-29 15:14:06] [I] Version: '2.6.32'; Release: '504.el6'; Arch: 'x86_64'; _arch: 'x86_64'; Flavour: 'None'; Realtime: False
[2020-01-29 15:14:06] [I] Found cached vmlinux at path: /cores/retrace/repos/kernel/x86_64/usr/lib/debug/lib/modules/2.6.32-504.el6.x86_64/vmlinux
[2020-01-29 15:14:06] [I] Searching for kernel-debuginfo package for 2.6.32-504.el6.x86_64
[2020-01-29 15:14:06] [E] prepare_debuginfo failed: not enough arguments for format string

Signed-off-by: Dave Wysochanski <[email protected]>
@DaveWysochanskiRH
Copy link
Collaborator

Well a bug in run_crash_cmd exception handling code does not help (facepalm)
#293

@DaveWysochanskiRH
Copy link
Collaborator

Something in fd40267 needs fixed up I think.

@DaveWysochanskiRH
Copy link
Collaborator

Simple fixup b319f0a

mkutlak added a commit to abrt/ansible-role-retrace-server that referenced this pull request Mar 25, 2020
- Spool dir was changed in abrt/retrace-server#277
- Workdirs were removed in abrt/retrace-server#271
- Hooks were replaced in abrt/retrace-server#278
- Kmem was removed in abrt/retrace-server#252

Signed-off-by: Martin Kutlak <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants