Support for Swift Backup/Restore #66

jerryk55 · 2018-09-25T14:54:27Z

Ability to restore backups previous saved to Swift Object STorage.

In support of the RFE defined in BZ https://bugzilla.redhat.com/show_bug.cgi?id=1615488

Related PRs are ManageIQ/manageiq-ui-classic#4684
ManageIQ/manageiq#17967
ManageIQ/manageiq-gems-pending#371
and
ManageIQ/manageiq-schema#259.

@NickLaMuro @carbonin @roliveri please review. Thanks.

miq-bot · 2018-09-28T19:43:41Z

This pull request is not mergeable. Please rebase and repush.

carbonin

I think the change @kbrock made that you're having an issue with there is the way the i18n file is read to determine the backup file options. I think you'll need to add swift to the database_admin section of locales/appliance/en.yml

carbonin · 2018-10-03T13:31:25Z

lib/manageiq/appliance_console/database_admin.rb

+      def security_protocol_menu_args
+        [
+          "OpenStack Security Protocol",
+          SECURITY_PROTOCOL_OPTIONS,


If this is the only place this is used, I think it would be better to just list the strings here, no need to declare a constant at the class level.

I can do that.

carbonin · 2018-10-03T13:31:44Z

lib/manageiq/appliance_console/database_admin.rb

+      def api_version_menu_args
+        [
+          "OpenStack API Version",
+          API_VERSION_OPTIONS,


Same thing here, I don't think this needs to be a constant.

carbonin · 2018-10-03T13:33:18Z

lib/manageiq/appliance_console/database_admin.rb

+        domain_ident      = just_ask("OpenStack V3 Domain Identifier") if api_version == "v3"
+
+        @task          = "evm:db:#{action}:remote"
+        @uri           = URI.parse("#{URI(@uri).scheme}://#{URI(@uri).host}:#{port}#{URI(@uri).path}")


What is happening here? I'm not sure what this line is trying to accomplish, but my gut feeling is that it can be done in a better way.

To add to this, this is basically parsing the @uri 4 times, and it seems pretty unecessary. I think doing the following would suffice:

@uri = URI(ask_for_uri(*remote_file_prompt_args_for("swift"))) # ... @uri.port = just_ask("OpenStack Swift Port", "5000")

And then you can remove this line we are commenting on entirely.

Adding the port into the URI. I'm sure there are other ways to do it. I am open to suggestions other than this one.

NickLaMuro · 2018-10-03T14:37:17Z

@miq-bot remove_label unmergable

miq-bot · 2018-10-03T14:38:16Z

@NickLaMuro Cannot remove the following label because they are not recognized: unmergable

NickLaMuro · 2018-10-03T14:44:05Z

*grumbles*

@miq-bot remove_label unmergeable

happy!?

NickLaMuro

I will continue my review in "another review", but this is responding to @carbonin 's comment and @jerryk55 's question for a better way to do this.

NickLaMuro · 2018-10-03T14:48:15Z

lib/manageiq/appliance_console/database_admin.rb

+        domain_ident      = just_ask("OpenStack V3 Domain Identifier") if api_version == "v3"
+
+        @task          = "evm:db:#{action}:remote"
+        @uri           = URI.parse("#{URI(@uri).scheme}://#{URI(@uri).host}:#{port}#{URI(@uri).path}")


To add to this, this is basically parsing the @uri 4 times, and it seems pretty unecessary. I think doing the following would suffice:

@uri = URI(ask_for_uri(*remote_file_prompt_args_for("swift"))) # ... @uri.port = just_ask("OpenStack Swift Port", "5000")

And then you can remove this line we are commenting on entirely.

jerryk55 · 2018-10-03T15:06:30Z

@NickLaMuro doh! That works and is much easier. Thanks!

NickLaMuro

I think most of these changes should be pretty quick.

The only other thing I would suggest is possibly adding some specs for ask_swift_file_options, similar to what I have for FTP:

https://github.com/ManageIQ/manageiq-appliance_console/blob/6d4379db/spec/database_admin_spec.rb#L396-L484

Keep in mind, that is nested under "DB Restore", and I do also have specs for Backup and Dump as well.

NickLaMuro · 2018-10-03T14:57:32Z

lib/manageiq/appliance_console/database_admin.rb

+      def ask_swift_file_options
+        require 'uri'
+        @uri              = ask_for_uri(*remote_file_prompt_args_for("swift"))
+        user              = just_ask(USER_PROMPT)


So, I named this constant poorly, and this should have been SMB_USER_PROMPT in retrospect. Because of that, I don't think you should use that for this prompt.

The USER_PROMPT's example is:

Example: 'mydomain.com/user'

Which I think only make sense in the case of SMB, and not swift (I could be wrong, but pretty sure it is a non-domain based username for swift)

Yup. good catch...

NickLaMuro · 2018-10-03T15:00:44Z

lib/manageiq/appliance_console/database_admin.rb

+      def api_version_menu_args
+        [
+          "OpenStack API Version",
+          [["Keystone v2".freeze, "v2".freeze], ["Keystone v3".freeze, "v3".freeze], [CANCEL, nil]].freeze,


Is cancel providing the right "verb-age" here? Seems like it would suggest to the user that we are going to "Cancel" the transaction, but it would just provide a nil value to the question and move on to the next. Correct?

For what it is worth, @kbrock has provided a way in ask_file_location for CANCEL to work as expected, if you actually are intending for the user to eject out of the questions for this part.

def ask_file_location @backup_type = ask_with_menu(*file_menu_args) do |menu| menu.choice(CANCEL) { |_| raise MiqSignalError } end # ... end

It is a bit ugly, and I might make a PR to build this into prompts.rb more seemlessly, but it will work for now if that is your intent.

If you want a nil value, then I would say this should just be a prompt of "None", or something to that extent instead of "Cancel".

None it is.

NickLaMuro · 2018-10-03T15:01:18Z

lib/manageiq/appliance_console/database_admin.rb

+      def security_protocol_menu_args
+        [
+          "OpenStack Security Protocol",
+          [["SSL without validation".freeze, "ssl".freeze], ["SSL".freeze, "ssl-with-validation".freeze], ["Non-SSL".freeze, "non-ssl".freeze], [CANCEL, nil]].freeze,


Same here with the CANCEL option.

Yup. "None" again.

NickLaMuro · 2018-10-03T15:07:08Z

lib/manageiq/appliance_console/database_admin.rb

@@ -11,6 +11,7 @@ class DatabaseAdmin < HighLine
      SMB_FILE   = "smb".freeze
      S3_FILE    = "s3".freeze
      FTP_FILE   = "ftp".freeze
+      SWIFT_FILE = "swift".freeze


Besides the specs, I think this constant isn't used anywhere now. I think @kbrock should have removed these as part of his PR, but it is probably fine for now.

Mostly commenting as this should probably be a quick TODO for some cleanup.

yea, these have been removed / are not necessary anymore. we are now just hardcoding them a few times in the specs

NickLaMuro · 2018-10-03T15:12:22Z

lib/manageiq/appliance_console/database_admin.rb

+        @task_params = [
+          "--",
+          {
+            :uri          => @uri,


I would say for safety, maybe do @uri.to_s. I am pretty sure AwesomeSpawn would handle this just fine when passing the options, but I would rather be safe than sorry on this one, since we don't have tests around actually executing the rake tasks.

NickLaMuro · 2018-10-03T15:21:46Z

lib/manageiq/appliance_console/database_admin.rb

@@ -153,6 +154,35 @@ def ask_custom_file_options(server_uri)
        @task_params = ["--", params]
      end

+      def ask_swift_file_options
+        require 'uri'
+        @uri              = URI(ask_for_uri(*remote_file_prompt_args_for("swift")))


In addition to asking for @uri, you are probably going to want to follow the pattern of asking for @filename for non-restore tasks as well:

manageiq-appliance_console/lib/manageiq/appliance_console/database_admin.rb

Lines 86 to 101 in 6d4379d

def ask_smb_file_options

@filename = just_ask(*filename_prompt_args) unless action == :restore

@uri = ask_for_uri(*remote_file_prompt_args_for("smb"))

user = just_ask(USER_PROMPT)

pass = ask_for_password("password for #{user}")

params = {

:uri => uri,

:uri_username => user,

:uri_password => pass

}

params[:remote_file_name] = filename if filename

@task = "evm:db:#{action}:remote"

@task_params = ["--", params]

end

jerryk55 · 2018-10-18T15:41:47Z

Note that I am still working the issues with the spec tests. I am aware of the failures.

kbrock

Sorry we're conflicting Jerry.

This stuff looks nice

kbrock · 2018-10-18T18:32:32Z

lib/manageiq/appliance_console/database_admin.rb

@@ -11,6 +11,7 @@ class DatabaseAdmin < HighLine
      SMB_FILE   = "smb".freeze
      S3_FILE    = "s3".freeze
      FTP_FILE   = "ftp".freeze
+      SWIFT_FILE = "swift".freeze


yea, these have been removed / are not necessary anymore. we are now just hardcoding them a few times in the specs

jerryk55 · 2018-10-22T20:14:00Z

@NickLaMuro @carbonin specs pushed out. Not aware of any other outstanding comments but I'm open to any others - If we can get this pushed out for the Sprint that would be awesome.

NickLaMuro

I only really have one final request, and it should be easy:

The specs you added are specifically for "Restore", but there are also sections for "Database Dump" and "Database Backup", as mentioned here:

#66 (review)

For what it is worth, it is mostly a copy paste exercise, and should be relatively easy to convert what you have here to dump/restore specs. The only thing that changes is there is also the :remote_file_name param that also gets asked for, so you have to update that for those two.

Otherwise, looks good!

NickLaMuro · 2018-10-22T20:58:01Z

lib/manageiq/appliance_console/database_admin.rb

+        @uri       = URI(ask_for_uri(*remote_file_prompt_args_for("swift")) { |q| q.readline = false })
+        @task      = "evm:db:#{action}:remote"
+        user       = just_ask(swift_user_prompt) { |q| q.readline = false }
+        pass       = ask_for_password("password for #{user}") { |q| q.readline = false }


Just putting this out there for anyone else reviewing:

The q.readline = false stuff that is being splattered across these changes is simply to allow the tests to pass seamlessly.

There is no functional difference (from what I can tell), but it allows a single IO stream for the input to be used, and prevents the one from either readline or Ruby's IO from slurping up all of the say values in the tests, and leaving nothing for further input.

I might look into making this a config option at some point in prompts.rb, or change how we are passing IO in spec/support/ui.rb, but I don't think that is worth the effort in this PR for now.

NickLaMuro · 2018-10-22T20:58:56Z

lib/manageiq/appliance_console/database_admin.rb

+        @task_params = ["--", params]
+      end
+
+      def swift_query_elements


I like this change to moving it into it's own method. Keeps ask_swift_file_options from getting too bloated.

👍

spec/database_admin_spec.rb

Ability to restore backups previous saved to Swift Object STorage.

As another PR moved prompting to this YML file, need to update as appropriate.

Instead of adding constants, just use the required strings when necessary.

Instead of parsing the URI and putting the whole thing back together, just set the port simply. Based on review comments.

Add many Swift spec tests. Split out #swift_query_elements from #ask_swift_file_options to allow simpler testing and reduce complexity in the latter method. Change some prompting to enable spec testing to work properly as per changes to appliance_console found in previous PRs.

Add spec tests for running DB Backups to Swift via appliance_console. Fix the misleading "local" file prompts when doing backups to Object Stores (currently S3 or Swift).

NickLaMuro

Going to start with these, since they are probably part of the reason specs are failing.

lib/manageiq/appliance_console/database_admin.rb

Simplify processing for the filename prompts. The previous incarnation was called out as overly complex by CodeClimate. In addition, due to a previous PR some of the backup_type constants had been removed in favor of simple methods so use the #local_backup? when appropriate, and add #object_store_backup? instead of constants for Swift and S3.

miq-bot · 2018-10-30T15:43:10Z

Checked commits jerryk55/manageiq-appliance_console@7c4a28e~...04c2e26 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍪

NickLaMuro

Ran this against the appliance console specs I already had and everything seems to be working as expected. I don't have swift tests in place for walking though the appliance console, so it was mostly testing everything else was still working as expected.

Thanks for the extra work on this!

carbonin · 2018-10-31T15:55:07Z

lib/manageiq/appliance_console/database_admin.rb

+      SSL_WITH_VALIDATION        = "ssl-with-validation".freeze
+      NON_SSL_PROMPT             = "Non-SSL".freeze
+      NON_SSL                    = "non-ssl".freeze
+      SECURITY_PROTOCOL_OPTIONS  = [[SSL_WO_VALIDATION_PROMPT, SSL_WO_VALIDATION], [SSL_WITH_VALIDATION_PROMPT, SSL_WITH_VALIDATION], [NON_SSL_PROMPT, NON_SSL], [CANCEL, nil]].freeze


I don't see any of these constants used anywhere in this patch. Can we remove them?

Oh... good catch. Were they meant for api_version_menu_args and security_protocol_menu_args?

If we go back to using these instead, just make sure you change CANCEL to "None".

A parallel PR has removed the need for a set of constants so they were left inline but unused. Removing them.

miq-bot added the unmergeable label Sep 28, 2018

jerryk55 force-pushed the swift_backups branch 4 times, most recently from 24c3097 to ecc0a19 Compare October 3, 2018 13:00

carbonin reviewed Oct 3, 2018

View reviewed changes

miq-bot removed the unmergeable label Oct 3, 2018

NickLaMuro reviewed Oct 3, 2018

View reviewed changes

NickLaMuro suggested changes Oct 3, 2018

View reviewed changes

NickLaMuro mentioned this pull request Oct 9, 2018

Openstack Swift DB Backups ManageIQ/manageiq#17967

Merged

jerryk55 force-pushed the swift_backups branch from 3ac0753 to 7f8addb Compare October 18, 2018 15:31

kbrock reviewed Oct 18, 2018

View reviewed changes

jerryk55 force-pushed the swift_backups branch 2 times, most recently from 54a7908 to 7183d4b Compare October 22, 2018 19:43

NickLaMuro suggested changes Oct 22, 2018

View reviewed changes

jerryk55 added 6 commits October 29, 2018 16:32

Support for Swift Backup/Restore

7c4a28e

Ability to restore backups previous saved to Swift Object STorage.

Add Swift prompting to YML file

8794f87

As another PR moved prompting to this YML file, need to update as appropriate.

Review Comments

63415f1

Instead of adding constants, just use the required strings when necessary.

Simplify URI Parsing

3a70d41

Instead of parsing the URI and putting the whole thing back together, just set the port simply. Based on review comments.

Add Swift DB Restore Spec tests

b5dfe15

jerryk55 force-pushed the swift_backups branch from 7183d4b to 8bf8743 Compare October 29, 2018 20:35

Swift DB Backup Tests and Fix Filename Prompting

fbbbbea

Add spec tests for running DB Backups to Swift via appliance_console. Fix the misleading "local" file prompts when doing backups to Object Stores (currently S3 or Swift).

jerryk55 force-pushed the swift_backups branch from 8bf8743 to fbbbbea Compare October 29, 2018 20:41

NickLaMuro suggested changes Oct 29, 2018

View reviewed changes

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

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

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

jerryk55 closed this Oct 30, 2018

jerryk55 reopened this Oct 30, 2018

NickLaMuro approved these changes Oct 31, 2018

View reviewed changes

carbonin reviewed Oct 31, 2018

View reviewed changes

Remove Constants No Longer Used

35ee659

A parallel PR has removed the need for a set of constants so they were left inline but unused. Removing them.

carbonin self-assigned this Nov 5, 2018

carbonin added the enhancement label Nov 5, 2018

carbonin approved these changes Nov 5, 2018

View reviewed changes

carbonin merged commit 25de364 into ManageIQ:master Nov 5, 2018

carbonin added this to the Sprint 98 Ending Nov 5, 2018 milestone Nov 5, 2018

	def ask_smb_file_options
	@filename = just_ask(*filename_prompt_args) unless action == :restore
	@uri = ask_for_uri(*remote_file_prompt_args_for("smb"))
	user = just_ask(USER_PROMPT)
	pass = ask_for_password("password for #{user}")

	params = {
	:uri => uri,
	:uri_username => user,
	:uri_password => pass
	}
	params[:remote_file_name] = filename if filename

	@task = "evm:db:#{action}:remote"
	@task_params = ["--", params]
	end

Support for Swift Backup/Restore #66

Support for Swift Backup/Restore #66

Conversation

jerryk55 commented Sep 25, 2018

miq-bot commented Sep 28, 2018

carbonin left a 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

NickLaMuro commented Oct 3, 2018

miq-bot commented Oct 3, 2018

NickLaMuro commented Oct 3, 2018

NickLaMuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryk55 commented Oct 3, 2018

NickLaMuro left a 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 Oct 18, 2018

kbrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryk55 commented Oct 22, 2018

NickLaMuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro left a comment

Choose a reason for hiding this comment

miq-bot commented Oct 30, 2018

NickLaMuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment