DB Backups to AWS S3 #17689

jerryk55 · 2018-07-10T21:34:49Z

Changes required for DB backups. Works with manageiq-gems-pending PR #355.
UI changes still required.

@roliveri @carbonin please review as much as possible although it is still WIP based on some probably changes required for the validation and UI.

This PR is one of several required for https://bugzilla.redhat.com/show_bug.cgi?id=1513520

jerryk55 · 2018-07-10T21:35:10Z

@miq-bot add_label WIP

carbonin · 2018-07-11T15:18:08Z

lib/evm_database_ops.rb

    end

    block_result = yield db_opts if block_given?
    uri || block_result
  ensure
-    session.disconnect if session
+    session&.add(db_opts[:local_file], connect_opts[:uri], db_opts[:object_file]) if action == :backup && session.class.name == "MiqS3Session"


I don't think this belongs in an ensure block as if an error occurred there's no guarantee that a backup was produced.

carbonin · 2018-07-11T15:19:27Z

lib/evm_database_ops.rb

@@ -115,14 +116,18 @@ def self.restore(db_opts, connect_opts = {})
        uri = File.join(connect_opts[:uri], backup_folder, connect_opts[:remote_file_name])
      end

+      connect_opts[:username] = db_opts[:username] if connect_opts[:username].nil?
+      connect_opts[:password] = db_opts[:password] if connect_opts[:password].nil?


Why would we want to assume the connection options to the s3 store would ever be the same as the database authentication values?

carbonin · 2018-07-11T15:19:47Z

lib/evm_database_ops.rb

-  BACKUP_TMP_FILE = "/tmp/miq_backup".freeze
-  DUMP_TMP_FILE   = "/tmp/miq_pg_dump".freeze
+  BACKUP_TMP_FILE     = "/tmp/miq_backup".freeze
+  DUMP_TMP_FILE       = "/tmp/miq_pg_dump".freeze


Why was this changed?

I had another line in there that required aligning these lines. When I removed it I failed to undo the indentation. I'll fix.

carbonin · 2018-07-11T15:21:04Z

app/models/file_depot_s3.rb

+  private
+
+  def create_bucket(directory_path)
+    if (ssa_bucket = s3.bucket(directory_path)).exists?


What does ssa_bucket mean here? I don't think this has anything to do with Smart State Analysis right?

Absolutely correct.

carbonin · 2018-07-11T15:23:03Z

app/models/file_depot_s3.rb

+    base_path.join(file.destination_directory)
+  end
+
+  def base_path


I don't think base path is a good name for this. In fact I would assume the "base" of a URL would mean the directory or URL host, not the file name.

See file_depot_ftp.rb. Same code.

carbonin · 2018-07-16T19:57:44Z

app/models/file_depot_s3.rb

@@ -0,0 +1,54 @@
+class FileDepotS3 < FileDepot
+  include AmazonAuthenticationMixin


What is this and why doesn't the regular AuthenticaionMixin do what you need?

Because it doesn't understand Amazon connectivity. This is pulled in from the Amazon Provider mixin.

carbonin · 2018-07-16T20:08:41Z

lib/evm_database_ops.rb

-      db_opts[:local_file] = session.uri_to_local_path(uri)
+      db_opts[:local_file]  = session.uri_to_local_path(uri)
+      db_opts[:object_file] = session.uri_to_object_path(uri) if connect_opts[:object_store]
+      db_opts[:session]     = session if db_opts[:session].class.name == "MiqS3Session"


I don't think it's a good idea to alter this hash to include non-database related fields. This is passed into the code to actually connect to the database which expects these to all be valid options to pg_basebackup

Well, our choice is this or setting a global @session variable. I'm cool with either.

carbonin · 2018-07-16T20:09:24Z

lib/evm_database_ops.rb

    end

    block_result = yield db_opts if block_given?
    uri || block_result
  ensure
-    session.disconnect if session
+    session&.disconnect


Why not just leave this as is?

Can do. Previously there was another line that we moved that called the add method as well.

carbonin · 2018-07-16T20:10:46Z

lib/evm_database_ops.rb

@@ -116,13 +118,15 @@ def self.restore(db_opts, connect_opts = {})
      end

      session = MiqGenericMountSession.new_session(connect_opts)
-      db_opts[:local_file] = session.uri_to_local_path(uri)
+      db_opts[:local_file]  = session.uri_to_local_path(uri)
+      db_opts[:object_file] = session.uri_to_object_path(uri) if connect_opts[:object_store]


If you're using the session object to get this value, why pass it in separately to #add above? Couldn't the session object internally make the same call?

Related: ManageIQ/manageiq-gems-pending#355 (comment)

jerryk55 · 2018-07-26T17:23:33Z

@miq-bot remove_label wip

jerryk55 · 2018-07-26T17:24:52Z

@carbonin @roliveri PLease review. WIP label removed.

jerryk55 · 2018-07-26T18:55:22Z

Note that the "supports_objects?" failures are dependent upon ManageIQ/manageiq-gems-pending#357 being merged. Working on the other failures...

carbonin · 2018-07-27T14:15:11Z

app/models/mixins/amazon_s3_authentication_mixin.rb

+    self.class.raw_connect(username, password, service, region)
+  end
+
+  def with_depot_connection(options = {})


Is this mixin specific to depots? If not, where else will it be used?

Right now, yes. The code was modified from the code used for the provider but obviously we could not reference provider-specific code in the main repo. Additionally I needed some changes (and reductions) in functionality.

Okay, that makes more sense.

If this is really only meant for the FileDepotS3 class then I think we should probably hide and simplify this even more rather than making it a first-class mixin.

carbonin · 2018-07-27T14:21:16Z

app/models/mixins/amazon_s3_authentication_mixin.rb

+  # Connections
+  #
+  def connect(options = {})
+    username = options[:username] || authentication_userid(options[:auth_type])


These are defined in the AuthenticationMixin which happens to be included in FileDepot, but if any other class were to include AmazonS3AuthenticationMixin this would fail.

It feels like this file should be a part of the FileDepotS3 class or maybe a utility in /lib for wrapping S3 connections in easier to use methods.

So you'd prefer it if I pulled this out and included it in the FileDepotS3 class. I can do that.

carbonin · 2018-07-27T14:24:07Z

app/models/mixins/amazon_s3_authentication_mixin.rb

+    true
+  end
+
+  module ClassMethods


Why did you choose to implement these as class methods? I'm not sure I see the advantage.

Again - refer to the previous comment - pulled out of the AWS provider... I didn't give it that much original thought - the authentication was already working for the provider....

carbonin · 2018-07-27T14:28:22Z

lib/evm_database_ops.rb

      end

      session = MiqGenericMountSession.new_session(connect_opts)
      db_opts[:local_file] = session.uri_to_local_path(uri)
+      object_session       = session if session.class.name == "MiqS3Session"


Why not just yield session unconditionally? It can't hurt to provide the mount session for the other types too, right? Then we can remove the extra object_session variable entirely.

Because the "add" method on the session for backup (and the "download" method on the session for the restore in a separate PR I need to push out from my stash) are only invoked in EvmDatabaseOps if that session variable was yielded.

I think it would be more clear to only call add if the session responds to it or check the class when calling the method rather than conditionally yielding. That way the behavior is clear from the .backup method rather than having to figure out the cases when the session may not have been yielded.

carbonin · 2018-07-27T14:32:10Z

lib/evm_database_ops.rb

@@ -112,17 +114,18 @@ def self.restore(db_opts, connect_opts = {})
      else
        connect_opts[:remote_file_name] ||= File.basename(backup_file_name(action))
        backup_folder = action == :dump ? "db_dump" : "db_backup"
-        uri = File.join(connect_opts[:uri], backup_folder, connect_opts[:remote_file_name])
+        connect_opts[:uri] = uri = File.join(connect_opts[:uri], backup_folder, connect_opts[:remote_file_name])


Will this matter for the other mount types as we can't mount a file path? We were not previously changing the value of connection_opts[:uri] in the caller here.

Can you ensure that you re-test this with an NFS backup to be sure we don't regress?

carbonin · 2018-07-31T15:30:46Z

app/models/file_depot_s3.rb

+    new(:uri => settings[:uri]).verify_credentials(nil, settings.slice(:username, :password))
+  end
+
+  extend ActiveSupport::Concern


Why is this needed?

carbonin · 2018-07-31T15:31:16Z

app/models/file_depot_s3.rb

+  end
+
+  def self.validate_settings(settings)
+    _log.debug("Validating settings [#{settings}]")


Will this print the password into the logs? I don't think we want to do that.

carbonin · 2018-07-31T15:33:14Z

app/models/file_depot_s3.rb

+  #
+  # Connections
+  #
+  def raw_connect(access_key_id, secret_access_key, service, aws_region)


It looks like this is only being used once, can we just move this content into the #connect method?

carbonin · 2018-07-31T15:34:25Z

app/models/file_depot_s3.rb

+      :log_formatter     => Aws::Log::Formatter.new(Aws::Log::Formatter.default.pattern.chomp)
+    )
+
+    connection


This and the assignment above are not necessary.

carbonin · 2018-07-31T15:35:42Z

app/models/file_depot_s3.rb

+  def connect(options = {})
+    username = options[:username] || authentication_userid(options[:auth_type])
+    password = options[:password] || authentication_password(options[:auth_type])
+    service  = :S3


If we are hard-coding the service, can we use Aws::S3::Resource.new rather than passing this around and doing a const_get?

carbonin · 2018-07-31T15:39:20Z

lib/evm_database_ops.rb

      end

      session = MiqGenericMountSession.new_session(connect_opts)
      db_opts[:local_file] = session.uri_to_local_path(uri)
+      object_session       = session if session.class.name == "MiqS3Session"


I think it would be more clear to only call add if the session responds to it or check the class when calling the method rather than conditionally yielding. That way the behavior is clear from the .backup method rather than having to figure out the cases when the session may not have been yielded.

Changes to EvmDatabaseOps for PR ManageIQ/manageiq#17689 will require that the session add method be called on all sessions. This allows us to fail gracefully.

carbonin

Looks good to me.

Can we also squash the commits? I'm not sure how many of those are really useful to have in the history.

Changes required for DB backups. Works with manageiq-gems-pending PR ManageIQ#355. UI changes still required.

miq-bot · 2018-08-01T18:57:16Z

Checked commit jerryk55@81bb921 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 2 offenses detected

app/models/file_depot_s3.rb

❗ - Line 56, Col 3 - Style/RescueStandardError - Avoid rescuing without specifying an error class.

spec/lib/evm_database_ops_spec.rb

❗ - Line 10, Col 64 - Rails/FilePath - Please use Rails.root.join('path', 'to') instead.

miq-bot added the wip label Jul 10, 2018

carbonin reviewed Jul 11, 2018

View reviewed changes

This was referenced Jul 16, 2018

AWS S3 Database Backups ManageIQ/manageiq-ui-classic#4300

Merged

Support S3 for DB Backups ManageIQ/manageiq-gems-pending#355

Merged

carbonin suggested changes Jul 16, 2018

View reviewed changes

jerryk55 force-pushed the object_store_db_backups branch from c16c657 to 5250916 Compare July 20, 2018 17:18

jerryk55 mentioned this pull request Jul 26, 2018

S3 db restore ManageIQ/manageiq-gems-pending#357

Merged

miq-bot changed the title ~~[WIP] DB Backups to AWS S3~~ DB Backups to AWS S3 Jul 26, 2018

miq-bot removed the wip label Jul 26, 2018

carbonin reviewed Jul 27, 2018

View reviewed changes

jerryk55 force-pushed the object_store_db_backups branch from 3e830fd to d8ea1dd Compare July 30, 2018 16:59

carbonin reviewed Jul 31, 2018

View reviewed changes

jerryk55 mentioned this pull request Aug 1, 2018

Fix issue caused by missing source file in add call ManageIQ/manageiq-gems-pending#358

Closed

carbonin approved these changes Aug 1, 2018

View reviewed changes

DB Backups to AWS S3

81bb921

Changes required for DB backups. Works with manageiq-gems-pending PR ManageIQ#355. UI changes still required.

jerryk55 force-pushed the object_store_db_backups branch from 4a5a06b to 81bb921 Compare August 1, 2018 18:53

carbonin self-assigned this Aug 1, 2018

carbonin added enhancement gaprindashvili/no core labels Aug 1, 2018

carbonin merged commit be4c07e into ManageIQ:master Aug 1, 2018

carbonin added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 1, 2018

jerryk55 mentioned this pull request Aug 1, 2018

Support for DB Restore from Object Stores #17791

Merged

NickLaMuro mentioned this pull request Aug 3, 2018

[WIP] Update upload/download workflow for MiqS3Session #17798

Closed

		@@ -0,0 +1,54 @@
		class FileDepotS3 < FileDepot
		include AmazonAuthenticationMixin

DB Backups to AWS S3 #17689

DB Backups to AWS S3 #17689

Conversation

jerryk55 commented Jul 10, 2018 • edited Loading

jerryk55 commented Jul 10, 2018

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

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

jerryk55 commented Jul 26, 2018

jerryk55 commented Jul 26, 2018

jerryk55 commented Jul 26, 2018

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

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

Choose a reason for hiding this comment

carbonin left a comment

Choose a reason for hiding this comment

miq-bot commented Aug 1, 2018

jerryk55 commented Jul 10, 2018 •

edited

Loading