upload_coredumps: Upload backtrace if COREDUMP_WITH_BACKTRACE is set#25137
upload_coredumps: Upload backtrace if COREDUMP_WITH_BACKTRACE is set#25137ricardobranco777 wants to merge 2 commits intoos-autoinst:masterfrom
Conversation
ricardobranco777
force-pushed
the
backtrace
branch
4 times, most recently
from
0802259 to
f37ac6b
Compare
foursixnine
left a comment
foursixnine
left a comment
There was a problem hiding this comment.
I only have a minor niptick
ricardobranco777
force-pushed
the
backtrace
branch
2 times, most recently
from
2a38bc9 to
391484a
Compare
There was a problem hiding this comment.
I think the intention is good, but adding host modifications to a subroutine which should only collect data adds to overall developer confusion and unintended side-effects when using a global library function.
I think we should extract the parts of the routine which modifies the system to ensure that the data collection remains side-effect free.
| record_info("COREDUMPS found", "we found coredumps on SUT, attempt to upload"); | ||
| # Record soft-failure only on selected cases to collect data | ||
| record_soft_failure("poo#197969 - Coredumps are being silently ignored in openQA tests") if (is_tumbleweed || get_var("CONTAINER_RUNTIMES")); | ||
| my $get_backtrace = get_var("COREDUMP_WITH_BACKTRACE") && !is_transactional; |
There was a problem hiding this comment.
Why is the !is_trancational switch needed when a dedicated setting is applied here?
Why not simply not apply the setting on transactional systems?
There was a problem hiding this comment.
Why is the
!is_trancationalswitch needed when a dedicated setting is applied here?Why not simply not apply the setting on transactional systems?
Because we're not scheduling the console/coredump_collect module in SLEM and only SLEM > 6.0 have the systemd-coredump package.
| record_soft_failure("poo#197969 - Coredumps are being silently ignored in openQA tests") if (is_tumbleweed || get_var("CONTAINER_RUNTIMES")); | ||
| my $get_backtrace = get_var("COREDUMP_WITH_BACKTRACE") && !is_transactional; | ||
| if ($get_backtrace) { | ||
| script_run('sed -i s/enabled=0/enabled=1/ /etc/zypp/repos.d/*-[Dd]ebug.repo'); |
There was a problem hiding this comment.
This should not be part of this routine.
This is an action with severe consequences for the host system. As such this should not be handled in the upload_coredumps routine, which does not suggest that new packages are installed and zypper repos are modified. This adds undesired side-effect to a routine, which should only collect data.
I'd suggest this gets it's own subroutine as preparation step either inside the library or in another test module.
There was a problem hiding this comment.
This should not be part of this routine.
This is an action with severe consequences for the host system. As such this should not be handled in the
upload_coredumpsroutine, which does not suggest that new packages are installed and zypper repos are modified. This adds undesired side-effect to a routine, which should only collect data.I'd suggest this gets it's own subroutine as preparation step either inside the library or in another test module.
This is not a problem because this is only done if you clone the job with COREDUMP_WITH_BACKTRACE=1.
There was a problem hiding this comment.
I'm sorry but I have to disagree with this for a library function. If this would be a subroutine in a test module, it would be ok like this.
But this is adding an invisible side-effect into a core library function, which is widely used. Here we should demand more careful handling.
Also counter-argument: This is action is only needed when COREDUMP_WITH_BACKTRACE=1 is set, then it shouldn't be difficult to find a matching place where this can be done outside the library function.
There was a problem hiding this comment.
I'm sorry but I have to disagree with this for a library function. If this would be a subroutine in a test module, it would be ok like this.
But this is adding an invisible side-effect into a core library function, which is widely used. Here we should demand more careful handling.
This is expected when you clone with COREDUMP_WITH_BACKTRACE=1 which transforms the VM into one used only for debugging purposes.
Also counter-argument: This is action is only needed when
COREDUMP_WITH_BACKTRACE=1is set, then it shouldn't be difficult to find a matching place where this can be done outside the library function.
What's your suggestion? It must be called from this function where it makes sense.
There was a problem hiding this comment.
I'd suggest to move the installation parts to coredump_collect.pm and add a "backtrace" parameter to the subroutine.
I think that's better because it makes the subroutine side-effect free and the specific actions that require additional packages is then disabled by default.
There was a problem hiding this comment.
I'd suggest to move the installation parts to coredump_collect.pm and add a "backtrace" parameter to the subroutine.
I think that's better because it makes the subroutine side-effect free and the specific actions that require additional packages is then disabled by default.
This subroutine is not used only by the coredump_collect module. We want to enable all callers of upload_coredumps to get backtraces on coredumps when they set this variable.
ricardobranco777
changed the title
ricardobranco777
force-pushed
the
backtrace
branch
from
391484a to
740c8c5
Compare
By cloning a job with COREDUMP_WITH_BACKTRACE=1 it's possible to get a backtrace on non-transactional systems. Related ticket: https://progress.opensuse.org/issues/197969
ricardobranco777
force-pushed
the
backtrace
branch
from
740c8c5 to
8891245
Compare
4 participants
Upload backtrace if DEBUG is set. By cloning a job with COREDUMP_WITH_BACKTRACE=1 it's possible to get a backtrace on non-transactional systems.
Related ticket: https://progress.opensuse.org/issues/197969
Verification run: https://openqa.opensuse.org/tests/5803106#downloads
Backtraces: