MDBF-814 for install/upgrade save logs on failure by grooverdan · Pull Request #607 · MariaDB/buildbot

grooverdan · 2024-11-06T04:41:40Z

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.

fauust · 2024-11-06T07:55:20Z

scripts/bash_lib.sh


-get_columnstore_logs() {
+save_failure_logs() {
+  local logdir=/home/buildbot/logs/


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...

which relative path ../logs?

to be tested but yes, probably

scripts/bash_lib.sh

RazvanLiviuVarzaru · 2024-11-07T10:51:13Z

scripts/bash_lib.sh

 # Include with:
 # . ./bash_lib.sh

+trap save_failure_logs ERR


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

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.

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.

scripts/bash_lib.sh

RazvanLiviuVarzaru · 2024-11-14T09:58:50Z

scripts/rpm-install.sh

 fi
-set -u
+
+sudo "$pkg_cmd" -n install "${package_array[@]}"


Are you sure that on non-SUSE VM's , -n works?

Right, missed the -n /-y difference - reverted back.

RazvanLiviuVarzaru · 2024-11-14T09:59:47Z

scripts/rpm-install.sh

-  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


I suppose this will work but refactoring other parts makes things really hard to follow in the scope of this MDBF.

ack. merged off.

RazvanLiviuVarzaru · 2024-11-14T10:06:43Z

scripts/rpm-upgrade.sh

+if sudo "$pkg_cmd" "$pkg_cmd_options" install "${package_array[@]}"; then
  bb_log_err "installation of a previous release failed, see the output above"
-#fi
+  save_failure_logs


Here we are mixing approaches i.e. :

command failures handled by IF or || need an explicit save_failure_logs call

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.

done. Unsure if documentation is complete enough.

RazvanLiviuVarzaru · 2024-11-14T10:12:31Z

scripts/bash_lib.sh

@@ -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


A failure here is handled in the parent script i.e. deb/rpm - upgrade?

right, set -e/+e in this function incorrect.

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.

…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.

fauust reviewed Nov 6, 2024

View reviewed changes

grooverdan force-pushed the MDBF-814-save-cs-logs branch 2 times, most recently from 2996864 to 291c49c Compare November 6, 2024 22:53

RazvanLiviuVarzaru reviewed Nov 7, 2024

View reviewed changes

scripts/bash_lib.sh Show resolved Hide resolved

grooverdan force-pushed the MDBF-814-save-cs-logs branch 2 times, most recently from d0147de to e248708 Compare November 13, 2024 04:01

RazvanLiviuVarzaru reviewed Nov 14, 2024

View reviewed changes

RazvanLiviuVarzaru force-pushed the dev branch from d7eb898 to 4be5d7c Compare November 19, 2024 11:49

grooverdan added 2 commits November 27, 2024 14:26

master-libvirt: saveArtifactsStep for deb/rpm upgrade

516c3ad

grooverdan force-pushed the MDBF-814-save-cs-logs branch from e248708 to 9214b9f Compare November 27, 2024 04:02

grooverdan added 7 commits November 27, 2024 15:17

bash_lib: do not change set +-e during check_mariadb_server_and_{crea…

36ef633

…te,verify}_structures These are handled by rpm/deb-install/upgrade.

deb: add trap for save_failure logs when we start to process

638214d

the new packages in CI.

bash_lib: remove explict call to (remove) get_columnstore_logs

0ee9034

As we going be in set -e mode when this is called, the trap will handle the saving of the error information.

deb-install: remove duplicate columnstore error routine

5d44ada

deb-upgrade: replace explicit columnstore save logs

87ac989

rpm-upgrade: add trap for error handling

03821e3

rpm-install: use save_failure_logs trap

59f9a2f

grooverdan force-pushed the MDBF-814-save-cs-logs branch from 3c42b8b to 59f9a2f Compare November 27, 2024 04:17

grooverdan mentioned this pull request Nov 27, 2024

Cleanups to rpm/deb install upgrade scripts #644

Draft

grooverdan marked this pull request as draft December 2, 2024 07:34

Conversation

grooverdan commented Nov 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RazvanLiviuVarzaru Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

RazvanLiviuVarzaru Nov 7, 2024 •

edited

Loading