Use PostgresAdmin directly for DB backups #160

NickLaMuro · 2021-04-05T23:49:28Z

Tested using https://github.com/NickLaMuro/manageiq-db_backup-tests to validate the changes.

There are a bunch of "deletes" that will follow this change, but this should not only be merged, but also released as a new version before those changes are merged. Otherwise the nightly builds of the appliance console will break.

TODO

Need to test against SMB/NFS
- ~~should work just like local, but currently have to rework connecting to the SMB/NFS shares prior to the tests running (as part of the test suite), and that currently doesn't exist~~ simpler than expected, and there are just a few kinks to work out. Will post a update to the manageiq-db_backup-tests when it is available.
- Update tests for rakeless DB admin NickLaMuro/manageiq-db_backup-tests#1
Determine if we want to keep file splitting
- ~~Probably not, but worth mentioning that this is a feature that will no longer be supported~~ Answer: Nope, burn it.
Fix specs
- See Use PostgresAdmin directly for DB backups #160 (comment) for details

Links

Part of Remove Database Backups from core (move to appliance console) manageiq#21100

NickLaMuro · 2021-04-08T22:03:20Z

lib/manageiq/appliance_console/database_admin.rb

+          raise message
+        end
+
+        MiqRegion.replication_type = :none


@Fryguy and/or @jrafanie So most of the methods in "green" here are pulled from lib/evm_database_ops.rb in ManageIQ/manageiq:

https://github.com/ManageIQ/manageiq/blob/4abf8c6ddd9f1985c1264ca407837423695b1dfd/lib/evm_database_ops.rb#L183-L216

and I was able to convert most of what was there previously (model or raw ActiveRecord) to straight Pg calls, but this one seems specific. After taking a look myself, I wasn't able to determine why this line specifically was added, as it seems that it will always be effectively a "no-op" when the method involved is called:

https://github.com/ManageIQ/manageiq/blob/61c77793f44a2453c0908997d4a8c5f79607faa3/app/models/miq_region.rb#L156-L165

But something tells me I am missing something. Can you enlighten me on what I am missing?

replication_type describes whether or not this database is the global side of replication or the remote side of replication. From a DB backup/restore point of view though I can't see why that would be important.

Oh I think what's happening here is that it's temporarily disabling replication. Setting replication_type to none would essentially unregister the replication provider.

EDIT: But I agree - it's almost like the code is missing the "uninstall" bit? Maybe @carbonin remembers?

There was a bug where restore couldn't drop the database because the replication backend was holding a connection open I think. There could have very well been more to it than that, but it should all bit in git history/bugzilla somewhere.

Also 👋

https://bugzilla.redhat.com/show_bug.cgi?id=1444993

PR: ManageIQ/manageiq#16942

@carbonin Okay, I think I see where my confusion was.

We pass into .replication_type= the desired_type, but the if current_type statements are getting the values from .replication_type, which will do some destroy/configure actions if needed, and I assume the methods called in .replication_type query against Postgres/PgLogical/stuffIVaguelyKnowAbout

Looks like I might need to look into what those do further, and see if we need to somehow replicate that logic over in the console.

Thanks for the response (and 👋 )

(oh, and yes, I did find that commit previously, but was misinterpretting the contents of the method. I now see the error in my ways)

oh hai @carbonin !! 👋

NickLaMuro · 2021-04-13T17:03:14Z

lib/manageiq/appliance_console/postgres_admin.rb

+      result[0]["count"] > 0
+    end
+
+    private_class_method def self.disable_replication(dbname)


I think this will handle all of the necessary concerns that were brought up previously:

https://github.com/ManageIQ/manageiq-appliance_console/pull/160/files/650c62b4dfc42ba519df50c8a8553b5aee848060#diff-4f5a9d143c19dfcadcdb0467ca432e5c0eba904c896862e8af14277978ec1479

I didn't delete the MiqRegion, as is currently done in PglogicalSubscription, but since this is a restore, that data will be blown away anyway, so we only need the parts that will be used prevent an error due to the HA code preventing a restore (which I think this is all of it... but I will double check).

I condensed a lot of what was in PglogicalSubscription to be a single method, so much of what is used here is spread out in the class between .find, .find_all, .delete, .safe_delete, and others. I also removed some of the column id conversion as I didn't think it was necessary, and just referenced it as sub_id for the purposes of the .drop_subscription portion of the code below.

So I did test this on a local appliance using:

NickLaMuro/manageiq-db_backup-tests#1

And it worked just fine, so at least from a non-HA appliance setup, these new changes don't seem to break anything. Don't really know how to set up a HA appliance to test the scenarios that were present in https://bugzilla.redhat.com/show_bug.cgi?id=1444993 so that is still a unknown, but the base case at least still works.

Allows this to be shared with other classes.

lib/manageiq/appliance_console/postgres_admin.rb

Fryguy · 2021-04-14T19:39:24Z

locales/appliance/en.yml

-    - smb
-    - s3
-    - ftp
-    - swift


Fryguy · 2021-04-14T19:40:53Z

lib/manageiq/appliance_console/postgres_admin.rb

+      with_pg_connection do |conn|
+        pglogical = PG::LogicalReplication::Client.new(conn)
+
+        if pglogical.subscriber?


If it makes sense, I'm ok with pushing down any of this logic into the pg/logical_replication gem. This could also be done in a follow-up

Just because of the delay I have had trying to get this PR, I am going to punt on doing this now and handle in a follow up.

So we don't loose track of it, I will open up an issue that can be triaged later, and we can determine if it makes sense then to push this into the gem.

NickLaMuro · 2021-04-14T21:10:20Z

So I have determined that there is a failure with the specs here that is based on the order that which they are run in... but have failed to track down what it is...

The [FIXUP] commits are currently some of those fixes, but I think one or two more will be necessary for me to track down this bug in the tests. Probably test specific related at this point, and just based on some of the changes I introduced into PostgresAdmin in this PR (which was setting up changes for #161 ).

Instead of calling a rake task in core, call `PostgresAdmin` which is now part of this repository. This takes many of the checks that exist in `lib/evm_database_ops.rb` (core) and puts them in this class, tweaking them slightly so they don't require things like `ActiveRecord` to function, or classes that only exist in core `(VmdbDatabaseConnection` for example) In addition, this also doesn't include any of the `.with_file_storage` calls as that will be phased out. `PostgresAdmin` just needs a local file to write to, and since `MiqFileStorage` used a FIFO to write data to external sources (s3, swift, FTP), there is no change required there, and this can just accept a local file and pass that to `PostgresAdmin`.

NickLaMuro · 2021-05-15T00:18:40Z

Finally... it passed...

Now I just have to get the PR rebased... but that is a problem for "Monday Nick" to deal with...

The prepare logic should live in the PostgresAdmin class as it is something that could be used for not only the console, but also CLI. Also, fixed a few things that wouldn't have worked (integration testing of `restore` hasn't been done with this code yet): - Removed the MiqRegion.replication_type= call (not used, I don't think) - Remove connection_pool disconnect (not relevant, since we aren't using ActiveRecord with this code base) - Fixed .connection_count to handle things differently per backup type and dbname (this existed previously with VmdbDatabaseConnection, but was not properly ported over) Everything else should be the same logically speaking, just updated for working within the PostgresAdmin class itself.

Use PG::LogicalReplication::Client to remove subscriptions or publications on a db prior to doing a database reload. Doesn't drop the `MiqRegion`, as was previously done as part of PglogicalSubscription in core: https://github.com/ManageIQ/manageiq/blob/8ca93da126aada556685db5d05aaf5e9a0666900/app/models/pglogical_subscription.rb#L70 But since this is a database reload, and the Region will be removed from the DB anyway, this should not matter, and the necessary code required to perform that delete will be pointless to include in this gem.

If an error occurs while connecting (for example, if you have the wrong user), then this will fail with just that error, instead of an error that exists in the ensure as well.

During a restore, when calling `PostgresAdmin.with_pg_connection`, use the one defined in `PostgresRunner` since it includes connection information for the secondary DB, instead of the one running default one running on the user's machine.

Adds a helper method to PostgresRunner (.hard_reset), which is used to reset the database to a fresh state. Required for local tests only since the DB backup was taken off of an appliance, so when that is restored first, it changes the default DB roles to that `.initial_setup` create and is assumed by the class.

For the `.restore` specs, ensure the env is setup properly so the specs can run properly. With the new methods that check the existing database connections in `PostgresAdmin` directly, this allows said connection to work properly. For CI, we don't want to do this, since it doesn't actually setup the ENV (bad method name, my bad...), but runs the restore in a task all by itself.

Doesn't seem to be working currently (at least locally), and supporting this isn't terribly necessary now that we won't be streaming files in Ruby. Might delete later...

Previously, the logic to pre-determine the backup type required knowledge of the file storage type that PostgresAdmin did not have. In the case where the file storage data was being streamed (S3, FTP, etc.) and was only read from the source once, you had to call out to the `PostgresAdmin.pg_dump_file?` and `PostgresAdmin.base_backup_file?` calls manually to determine what type file was going to be restored so the proper code could be executed, and `PostgresAdmin` didn't have to worry if the file data was being streamed or was just read off of a disk/mount. Since .validate_backup_file_type (not show in diff) is now part of .restore, this extra case logic is no longer necessary, as `backup_type` will always be set if not passed in. As such, the code and spec for it has been updated to reflect that.

When not in a RSpec context (example: when we are doing database operations in a separate process in CI), ensure that we have configured a `ManageIQ::ApplianceConsole.logger` so things don't break in CI.

Using `>>` requires being at the proper elevated permission at the time the command is executed, and since that isn't possible with these scripts, it is simpler to use sed for this, and append a `sudo` to the front.

This seems to cause issues, and postgres on Travis is setup to use `trust` instead of `peer`. Since the tests generally work with the default, continue using that instead of using what is brought over from a `.restore`.

miq-bot · 2021-05-17T15:51:00Z

Checked commits NickLaMuro/manageiq-appliance_console@b57d6f4~...bcb52d7 with ruby 2.6.3, rubocop 1.13.0, haml-lint 0.35.0, and yamllint
8 files checked, 4 offenses detected

lib/manageiq/appliance_console/postgres_admin.rb

❗ - Line 390, Col 14 - Layout/ExtraSpacing - Unnecessary spacing detected.
❗ - Line 54, Col 7 - Style/SafeNavigation - Use safe navigation (&.) instead of checking if an object exists before calling the method.

spec/postgres_runner_helper.rb

⚠️ - Line 65, Col 29 - Lint/EmptyBlock - Empty block detected.
❗ - Line 118, Col 5 - Naming/MemoizedInstanceVariableName - Memoized variable @port does not match method name set_port. Use @set_port instead.

NickLaMuro · 2021-05-17T16:20:26Z

@Fryguy I think this is finally ready for your review. Tests should be passing, and I have rebased/cleaned up the commits.

Fryguy · 2021-05-18T19:48:03Z

lib/manageiq/appliance_console/postgres_admin.rb

+    end
+
+    private_class_method def self.disable_replication(dbname)
+      require 'pg/logical_replication'


~~Do we have to include this gem in the gemspec now?~~ nvm - I searched the page and didn't find it and then suddenly I saw it 🤷‍♂️

Well, if you searched for it using the pg/..., then yeah, it wouldn't show up. But yes, it is required, but I think the specs wouldn't have passed if I didn't have it. 😉

Fryguy · 2021-05-18T20:36:37Z

spec/postgres_admin_spec.rb

-    context "with a pg_dump file from a pipe" do
-      let(:dbname)    { "pg_dump_restore_of_simple_db_from_pipe" }
-      let(:fifo_path) { Pathname.new(Dir::Tmpname.create("") {}) }
+    # context "with a pg_dump file from a pipe" do


Do we need this anymore?

Probably not... it was failing, but there is no real reason that it should though.

But it isn't really needed, so it can be tossed. Will do that in a follow up.

Fryguy · 2021-05-18T20:39:49Z

spec/postgres_admin_spec.rb

+          CiPostgresRunner.stop
+          CiPostgresRunner.start


Super-minor, but you could add hard_reset method to CiPostgresRunner for consistency with PostgresRunner.

Sure, I can do that in a followup.

Fryguy · 2021-05-18T20:41:08Z

Merged...everything else can be follow-up, if needed.

Enhancements: - Add database dump/backup/restore to CLI - #161 - Use PostgresAdmin directly for DB backups - #160 Bugs: - Chown database.yml as manageiq:manageiq - #165 - Create /opt/kafka/config/keystore directory - #170 - Add missing parse_errors() method - #169

NickLaMuro mentioned this pull request Apr 5, 2021

Remove Database Backups from core (move to appliance console) ManageIQ/manageiq#21100

Closed

15 tasks

miq-bot added the wip label Apr 5, 2021

NickLaMuro force-pushed the rakeless-db-admin branch from bc96d54 to 650c62b Compare April 6, 2021 21:15

NickLaMuro mentioned this pull request Apr 8, 2021

Update tests for rakeless DB admin NickLaMuro/manageiq-db_backup-tests#1

Merged

NickLaMuro commented Apr 8, 2021

View reviewed changes

NickLaMuro force-pushed the rakeless-db-admin branch from 7964013 to 0cbfa5b Compare April 13, 2021 16:58

NickLaMuro commented Apr 13, 2021

View reviewed changes

NickLaMuro force-pushed the rakeless-db-admin branch 3 times, most recently from 7d586d7 to 03dc939 Compare April 13, 2021 19:07

NickLaMuro mentioned this pull request Apr 13, 2021

Add database dump/backup/restore to CLI #161

Merged

NickLaMuro added 2 commits April 13, 2021 16:41

[DatabaseAdmin] Delete all but local backups

b57d6f4

[PostgresAdmin] Move .with_pg_connection method

aac2c37

Allows this to be shared with other classes.

NickLaMuro force-pushed the rakeless-db-admin branch 3 times, most recently from 38f7280 to c02d242 Compare April 14, 2021 14:58

NickLaMuro closed this Apr 14, 2021

NickLaMuro reopened this Apr 14, 2021

NickLaMuro force-pushed the rakeless-db-admin branch from c02d242 to 88f647d Compare April 14, 2021 15:02

Fryguy reviewed Apr 14, 2021

View reviewed changes

lib/manageiq/appliance_console/postgres_admin.rb Outdated Show resolved Hide resolved

Fryguy reviewed Apr 14, 2021

View reviewed changes

locales/appliance/en.yml

- smb

- s3

- ftp

- swift

Copy link

Member

Fryguy Apr 14, 2021

Choose a reason for hiding this comment

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

😍

Fryguy reviewed Apr 14, 2021

View reviewed changes

NickLaMuro force-pushed the rakeless-db-admin branch from 88f647d to b4e26aa Compare April 14, 2021 20:21

NickLaMuro force-pushed the rakeless-db-admin branch 5 times, most recently from 49a6508 to c6a991c Compare April 27, 2021 19:51

NickLaMuro added 2 commits May 17, 2021 10:07

[PostgresAdmin] Ensure dir exists for .backup_pg_dump

62d1c28

NickLaMuro force-pushed the rakeless-db-admin branch from a9fc272 to a6f9740 Compare May 17, 2021 15:42

NickLaMuro added 10 commits May 17, 2021 10:48

[PostgresAdmin] Close connection only if exists

fa461c6

If an error occurs while connecting (for example, if you have the wrong user), then this will fail with just that error, instead of an error that exists in the ensure as well.

[postgres_admin_spec.rb] Disable pipe spec

98bf4fb

Doesn't seem to be working currently (at least locally), and supporting this isn't terribly necessary now that we won't be streaming files in Ruby. Might delete later...

[CiPostgresRunner] Stub ManageIQ:ApplianceConsole.logger

662dcee

When not in a RSpec context (example: when we are doing database operations in a separate process in CI), ensure that we have configured a `ManageIQ::ApplianceConsole.logger` so things don't break in CI.

[CiPostgresRunner] sudo cp

607d6c5

[CiPostgresRunner] sudo sed instead of >>

7612d81

Using `>>` requires being at the proper elevated permission at the time the command is executed, and since that isn't possible with these scripts, it is simpler to use sed for this, and append a `sudo` to the front.

NickLaMuro force-pushed the rakeless-db-admin branch from a6f9740 to 9fd0741 Compare May 17, 2021 15:48

[CiPostgresRunner] Don't copy pg_hba.conf

bcb52d7

This seems to cause issues, and postgres on Travis is setup to use `trust` instead of `peer`. Since the tests generally work with the default, continue using that instead of using what is brought over from a `.restore`.

NickLaMuro force-pushed the rakeless-db-admin branch from 9fd0741 to bcb52d7 Compare May 17, 2021 15:49

NickLaMuro changed the title ~~[WIP] Use PostgresAdmin directly for DB backups~~ Use PostgresAdmin directly for DB backups May 17, 2021

miq-bot removed the wip label May 17, 2021

Fryguy reviewed May 18, 2021

View reviewed changes

Fryguy merged commit c62dfe8 into ManageIQ:master May 18, 2021

Fryguy added the enhancement label May 18, 2021

Fryguy self-assigned this May 18, 2021

agrare mentioned this pull request May 4, 2023

Update fog-openstack to v1.0 ManageIQ/manageiq-gems-pending#558

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use PostgresAdmin directly for DB backups #160

Use PostgresAdmin directly for DB backups #160

NickLaMuro commented Apr 5, 2021 •

edited

Loading

NickLaMuro Apr 8, 2021 •

edited

Loading

Fryguy Apr 9, 2021

Fryguy Apr 9, 2021 •

edited

Loading

carbonin Apr 9, 2021

carbonin Apr 9, 2021

NickLaMuro Apr 9, 2021 •

edited

Loading

Fryguy Apr 9, 2021

NickLaMuro Apr 13, 2021

NickLaMuro Apr 13, 2021

Fryguy Apr 14, 2021

Fryguy Apr 14, 2021

NickLaMuro May 17, 2021

NickLaMuro May 17, 2021

NickLaMuro commented Apr 14, 2021

NickLaMuro commented May 15, 2021

miq-bot commented May 17, 2021

NickLaMuro commented May 17, 2021

Fryguy May 18, 2021 •

edited

Loading

NickLaMuro May 18, 2021

Fryguy May 18, 2021

NickLaMuro May 18, 2021

Fryguy May 18, 2021

NickLaMuro May 18, 2021

Fryguy commented May 18, 2021

Use PostgresAdmin directly for DB backups #160

Use PostgresAdmin directly for DB backups #160

Conversation

NickLaMuro commented Apr 5, 2021 • edited Loading

TODO

Links

NickLaMuro Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro commented Apr 14, 2021

NickLaMuro commented May 15, 2021

miq-bot commented May 17, 2021

NickLaMuro commented May 17, 2021

Fryguy May 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented May 18, 2021

NickLaMuro commented Apr 5, 2021 •

edited

Loading

NickLaMuro Apr 8, 2021 •

edited

Loading

Fryguy Apr 9, 2021 •

edited

Loading

NickLaMuro Apr 9, 2021 •

edited

Loading

Fryguy May 18, 2021 •

edited

Loading