Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Edited backup script file paths to work in development environment & fixed engine restore bug #932

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BrooklynDewolf
Copy link
Collaborator

Changes introduced with this PR

  • Added the DEV_PREFIX value to backup paths to ensure that the correct files get found when backing up the engine

  • Fixed FATAL error when restoring the engine when no DWH, Cinderlib installation or Keycloak DB is used

Are you the owner of the code you are sending in, or do you have permission of the owner?

Yes

@didib
Copy link
Member

didib commented Mar 27, 2024

Did you check this on both devenv and RPM (as root)? With and without dwh/grafana/cinderlib/keycloak/etc.?

Generally speaking, we did not expose PREFIX to subsystems, but only more-specific vars, such as ENGINE_USE, ENGINE_ETC, etc. I think with an rpm build, PREFIX will be /usr, not "/".

Also, engine-backup was never designed to work in devenv. I can't believe something like the current patch will be enough for making it work. engine-setup code has tops of "if devenv: Do something else: Do something else", and I think you'll need to replicate many of these in engine-backup for it to work properly. Can you please clarify the need/use-case? Any doc with a plan? If it's for running it in a container, why do you even need to change engine-backup?

For simple stuff like changing the backup paths, you can rely on the "source_d" hooks. I don't remember anymore why we added them, but you can find on the net (very few) examples by searching for "/etc/ovirt-engine-backup/engine-backup".

@BrooklynDewolf
Copy link
Collaborator Author

The need for engine-backup to work in a development environment is because of #930. When rebuilding the container, data is lost. At this moment, the changes that I made seem to work fine in my case.

I will look into your feedback and get back to you.

@didib
Copy link
Member

didib commented Mar 27, 2024

The need for engine-backup to work in a development environment is because of #930. When rebuilding the container, data is lost.

Right, but is the data important? If it's just empty DB is created by engine-setup, perhaps just let it recreate. If you only need the DB - most likely, in a dev-env - perhaps it's easier to just dump/restore the DB itself with pg_dump/pg_restore. You can write very simple wrappers for these if you want - see e.g. packaging/dbscripts/engine-psql.sh.

At this moment, the changes that I made seem to work fine in my case.

Good, but what about the other cases? That's the question :-)

I will look into your feedback and get back to you.

Good luck!

When we were restoring the engine from a previous backup where there was
no active DWH, Cinderlib installation or Keycloak database, the restore
always resulted in a FATAL error:

-----
Rewriting /home/build/ovirt//etc/ovirt-engine/engine.conf.d/10-setup-database.conf
/home/build/ovirt/share/ovirt-engine/bin/engine-backup.sh: line 1419: [: syntax error: `-' unexpected
FATAL: Can not find /home/build/ovirt//etc/ovirt-engine/engine.conf.d/10-setup-cinderlib-database.conf
-----

This is because of a check that is done when restoring the engine. When
only restoring the DB, the value ${CHANGE_DB_CREDENTIALS} is true and
because of the OR condition, the false flag of for example
${CHANGE_DWH_CREDENTIALS} does not have any effect anymore. By removing
the CHANGE_DB_CREDENTIALS variable, the condition is dependent on only
the flag of the type of DB itself and the DB_USER.

There were also issues with running a backup in development mode.
When installing a development environment, a path is specified with the
PREFIX value. However, the backup script does not change the paths that
it is backing up relative to this PREFIX value. This has also been
changed to that it does backup the correct paths.

Signed-off-by: Brooklyn Dewolf <contact@brooklyn.gent>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants