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

Backup and restore container gateway postgres DB #905

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ianballou
Copy link
Contributor

@ianballou ianballou commented Jul 23, 2024

Adds backup and restore support for the container gateway DB. Only adds support for the postgres version of the database for now.

One notable change was that the DB commands need to be run as foreman-proxy since no password is saved for the database. The container gateway uses unix socket auth for the connection, so only the owning user can access the DB (as far as I know).

As such, I've added support for running generic commands as a different user.

To test, try backup and restore on both a normal Foreman/Katello install and on a smart proxy with the container gateway installed.

ToDos:

  • Figure out why stderr is ending up in the container gateway database dump
  • Write tests

@ianballou
Copy link
Contributor Author

I looked into the issue where stderr makes it into the dump file. It seems that this was always the case since the final command looks like sudo -u foreman-proxy -- pg_dump -Fc database > dump_file 2>&1. That would redirect stderr into stdout, which is being redirected into dump_file.

I'm guessing this wasn't an issue before because pg_dump was always getting run as root. In my case, foreman-proxy had no permissions to change directories into the place where I started the command, which caused an error which then made it literally into the dump file.

@ianballou ianballou force-pushed the backup-container-gateway branch 4 times, most recently from 214ff13 to 4818b98 Compare July 24, 2024 21:15
@ianballou
Copy link
Contributor Author

I also ended up adding lines to change the backup ownership to foreman-proxy:foreman-proxy in order for the foreman-proxy user to have access. I noticed we deal with this issue slightly differently for restore -- there's a simple error telling the user to change the backup permissions. This is a bit different for backup though, because the user isn't the one creating the inner backup directory. I'm curious if anyone can see any issues coming out of this.

@ianballou ianballou force-pushed the backup-container-gateway branch 2 times, most recently from 3ae9e8c to 9f3a763 Compare July 24, 2024 21:47
@ianballou ianballou marked this pull request as ready for review July 24, 2024 21:48
@ianballou ianballou force-pushed the backup-container-gateway branch 2 times, most recently from 0762bac to 1a0161a Compare July 25, 2024 13:53
Copy link
Contributor

@wbclark wbclark left a comment

Choose a reason for hiding this comment

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

Hey @ianballou !

I've shared some thoughts from my initial reading of this.

Regarding the proposal for some more abstraction here now that we've added a fourth(!) DB, it looks like it could also make sense for the definitions for online DB backup procedures and DB restore procedures?

I am curious to know what you think about it, and perhaps @evgeni would have some insight as well about whether this has been discussed before, and whether it's worth doing now

Comment on lines 52 to 53
feature(:foreman_database) || feature(:candlepin_database) ||
feature(:pulpcore_database) || feature(:container_gateway_database)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
feature(:foreman_database) || feature(:candlepin_database) ||
feature(:pulpcore_database) || feature(:container_gateway_database)
%i[
foreman_database
candlepin_database
pulpcore_database
container_gateway_database
].any? { |db| feature(db) }

[nit] this one sparks joy

@@ -0,0 +1,29 @@
module Checks
module ContainerGateway
class DBUp < ForemanMaintain::Check
Copy link
Contributor

@wbclark wbclark Aug 6, 2024

Choose a reason for hiding this comment

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

[idea] It looks like we'll have nearly identical code at definitions/checks/{foreman,candlepin,pulpcore,container_gateway}/db_up.rb... what do you think about abstracting these into a common subclass of ForemanMaintain::Check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It sounds like a good idea to me, the files appear to be very similar.

@ianballou
Copy link
Contributor Author

I originally thought this would be required for Katello 4.14 since I assume the change to postgres would introduce a regression in backup & restore. It looks like it does not, in fact it appears that offline backup is backing up the container gateway DB for free somehow. Once the installer is run, the container_gateway DB is correctly created. So, users restoring at least can still just do a smart proxy sync to restore their content.

As such, I don't think we need to rush this for the upcoming Foreman release in case there are other concerns with the implementation that pop up. I'm personally a bit busy with getting other things in shape for the release, but please let me know if anyone thinks there's a new regression that I'm missing.

@ianballou
Copy link
Contributor Author

@wbclark I've implemented your ideas

@evgeni
Copy link
Member

evgeni commented Aug 13, 2024

It looks like it does not, in fact it appears that offline backup is backing up the container gateway DB for free somehow.

It will not anymore (since we merged #893), so caution! :)

if connection_string
uri = URI.parse(connection_string)
@configuration['connection_string'] = connection_string
@configuration['user'] = 'foreman-proxy'
Copy link
Member

Choose a reason for hiding this comment

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

can't the user configure a different db user, and this one is just the fallback?

Copy link
Member

Choose a reason for hiding this comment

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

oh wait, no, reading further below, that's the "shell user" not "database user"? I'm confused :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is indeed a shell user. The container gateway database (and maybe more in the future) does not configure any password by default since it uses unix-domain socket authentication. As such, root can't (as far as I can tell) log into the database in any way, so I've created a generic way to set the shell user for doing database commands.

@ianballou
Copy link
Contributor Author

Since this has slipped a little on our team priorities, I'm going to call this a PoC and get things updated once the strategy is determined to be sound.

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.

3 participants