MDBF-814 for install/upgrade save logs on failure #607
Conversation
scripts/bash_lib.sh
Outdated
| @@ -437,22 +439,40 @@ upgrade_test_type() { | |||
| esac | |||
| } | |||
|
|
|||
| get_columnstore_logs() { | |||
| save_failure_logs() { | |||
| local logdir=/home/buildbot/logs/ | |||
There was a problem hiding this comment.
can't we use relative path? this has proven to be fragile on mac freebsd and other OS. I know we don't have install/upgrade test there but relative is better if possible since it makes it possible to have both dev and prod builders on the same machine...
There was a problem hiding this comment.
which relative path ../logs?
There was a problem hiding this comment.
to be tested but yes, probably
grooverdan
force-pushed
the
MDBF-814-save-cs-logs
branch
2 times, most recently
from
2996864 to
291c49c
Compare
scripts/bash_lib.sh
Outdated
| @@ -4,6 +4,8 @@ | |||
| # Include with: | |||
| # . ./bash_lib.sh | |||
|
|
|||
| trap save_failure_logs ERR | |||
There was a problem hiding this comment.
What will happen to the other failure points that don't need log collection? I suppose they will trigger log collection, reacting on the ERR signal of a command.
There are some code blocks which are not related to:
-
installation / upgrade process of the server
-
running queries to create or verify structures in the db
For example, blocks where we set-up the repository, make cache, query the repo and so on.
What I want to highlight is this most basic example:
set -e
trap save_failure_logs ERR
save_failure_logs() {
trap "" ERR
echo "I just saved the logs for Columnstore and MariaDB-Server"
}
echo "I am doing some pre-setup before Installing the Server"
echo "Presetup will fail, I have nothing installed. Do I really need to collect the logs?"
non_existent_command
There was a problem hiding this comment.
I like more the explicit approach in which we test the command and save the logs in case of a failure.
I am not in favor of trap because is fragile in case another command fails, before we need to actually look at the logs.
There was a problem hiding this comment.
traps are set after initial setup when any of the following commands can be meaningfully triggered by a server fault. The set +e to disable is at the end of the block if there becomes a big of adminstration.
Even a missing command will be during the process and evaluating some aspect of the package is correct. If this is the case its an error also. Maybe the logs unneeded in this case, but the failure is still required.
grooverdan
force-pushed
the
MDBF-814-save-cs-logs
branch
2 times, most recently
from
d0147de to
e248708
Compare
scripts/rpm-install.sh
Outdated
| fi | ||
| set -u | ||
|
|
||
| sudo "$pkg_cmd" -n install "${package_array[@]}" |
There was a problem hiding this comment.
Are you sure that on non-SUSE VM's , -n works?
There was a problem hiding this comment.
Right, missed the -n /-y difference - reverted back.
scripts/rpm-install.sh
Outdated
| echo "$pkg_list" | xargs sudo "$pkg_cmd" -n install | ||
| else | ||
| echo "$pkg_list" | xargs sudo "$pkg_cmd" -y install | ||
| if [ ${#package_array[@]} -eq 0 ]; then |
There was a problem hiding this comment.
I suppose this will work but refactoring other parts makes things really hard to follow in the scope of this MDBF.
scripts/rpm-upgrade.sh
Outdated
| bb_log_err "installation of a previous release failed, see the output above" | ||
| #fi | ||
| save_failure_logs |
There was a problem hiding this comment.
Here we are mixing approaches i.e. :
- command failures handled by IF or || need an explicit
save_failure_logscall - other command failures are handled by
trap save_failure_logs ERR
Why don't we stick to one across all scripts? Makes it easier to read. And it's easy to document how we handle errors.
There was a problem hiding this comment.
done. Unsure if documentation is complete enough.
| @@ -468,10 +479,7 @@ check_mariadb_server_and_create_structures() { | |||
| sudo mariadb -e "CREATE PROCEDURE db.p() SELECT * FROM db.v_merge" | |||
| sudo mariadb -e "CREATE FUNCTION db.f() RETURNS INT DETERMINISTIC RETURN 1" | |||
| if [[ $test_mode == "columnstore" ]]; then | |||
There was a problem hiding this comment.
A failure here is handled in the parent script i.e. deb/rpm - upgrade?
There was a problem hiding this comment.
right, set -e/+e in this function incorrect.
RazvanLiviuVarzaru
force-pushed
the
dev
branch
from
d7eb898 to
4be5d7c
Compare
This makes it more generic and expands the columnstore saving to the required information. * service logs * columnstore data dir (tar.bz2) * /tmp/columnstore_tmp_files (appended to log file) More information like the mariadb.service journal and coredumps are saved.
grooverdan
force-pushed
the
MDBF-814-save-cs-logs
branch
from
e248708 to
9214b9f
Compare
…te,verify}_structures These are handled by rpm/deb-install/upgrade.
the new packages in CI.
As we going be in set -e mode when this is called, the trap will handle the saving of the error information.
grooverdan
force-pushed
the
MDBF-814-save-cs-logs
branch
from
3c42b8b to
59f9a2f
Compare
This applies to Columnstore logs, data
core files also saved.
Also saves the systemd journal of applicable services.
Being an trap ERR it will execute only if an err occurs hence if conditions on columnstore SQL removed.
Unsure on if clean of the logdir is needed.