Openstack Swift DB Backups #17967

jerryk55 · 2018-09-10T21:14:32Z

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

Add a new FileDepot subclass for Swift.
Since we need to pass info in to the Mount Session (in gems-pending) regarding the Openstack
connection, utilize the URI with Query Parameters to do so.

@carbonin @NickLaMuro @roliveri please review.

Links

jerryk55 · 2018-09-11T15:34:24Z

I would agree 100% if that was what we are doing. But its not. The credentials are being passed/stored just as they are for the other FileDepot implementations. We are only adding the additional parameters not used by other FileDepots to the URI as Query parameters.

…

On Tue, Sep 11, 2018 at 9:56 AM Marek Aufart ***@***.***> wrote: Just $0.02 from OpenStack provider point of view. This seems to be as pretty much of duplication of credentials and config needed to connect to OpenStack. Storing credentials in encoded URL looks to me to have also security implications since it is different (older) way than Authentications are stored now. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#17967 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF1cZy_p1BwpK7GVzrZcUcWrZoIKP7JWks5uZ8EMgaJpZM4WiMPF> .

-- JERRY KESELMAN PRINCIPAL SOFTWARE ENGINEER, CLOUDFORMS VM SMARTSTATE ANALYSIS & STORAGE Red Hat Inc. <https://www.redhat.com/> jerryk@redhat.com Gitter: jerryk55 IRC: jerrykbiker

jerryk55 · 2018-09-17T19:29:12Z

@carbonin any chance of a final review and merge on this? Thanks!

NickLaMuro

Just something I noticed that is in regards to what I am working on. (Update: Derp... this is wrong... resolving)

lib/evm_database_ops.rb

I was clearly confused... ignore me

carbonin

@agrare can you weigh in on how we should be writing out Open[s|S]tack?

carbonin · 2018-09-17T23:48:24Z

app/models/database_backup.rb

+    'smb'   => 'Samba',
+    'nfs'   => 'Network File System',
+    's3'    => 'AWS S3',
+    'swift' => 'Openstack Swift'


Should this be OpenStack? Not sure how exact we need to be with branding here (I'm not sure if this is displayed to the user).

I believe this should be OpenStack per https://www.redhat.com/en/technologies/linux-platforms/openstack-platform and https://www.openstack.org/

Ok cool. I have to change this in here and in the UI code as well. Thanks for pointing it out.

carbonin · 2018-09-17T23:51:13Z

app/models/file_depot_swift.rb

+  end
+
+  def verify_credentials(auth_type = nil, options = {})
+    auth_type ||= 'default'


Couldn't we just make 'default' the .. well .. default value for the parameter in the method definition? Why have the parameter default be nil then set a value if it's nil.

Absolutely.

carbonin · 2018-09-18T00:00:42Z

app/models/mixins/file_depot_mixin.rb

+    'smb'   => 'Samba',
+    'nfs'   => 'Network File System',
+    's3'    => 'Amazon Web Services',
+    'swift' => 'Openstack Swift'


Same question here about Openstack vs OpenStack

carbonin · 2018-09-18T00:10:03Z

app/models/file_depot_swift.rb

+    uri.query = [uri.query, "region=#{openstack_region}"].compact.join('&') if openstack_region.present?
+    uri.query = [uri.query, "api_version=#{keystone_api_version}"].compact.join('&') if keystone_api_version.present?
+    uri.query = [uri.query, "domain_id=#{v3_domain_ident}"].compact.join('&') if v3_domain_ident.present?
+    uri.query = [uri.query, "security_protocol=#{security_protocol}"].compact.join('&') if security_protocol.present?


I think we could simplify this a bit by building a list of query elements outside of the uri.query string object. Like this:

query_elements = [] query_elements << "region=#{openstack_region}" if openstack_region.present? query_elements << "api_version=#{keystone_api_version}" if keystone_api_version.present? query_elements << "domain_id=#{v3_domain_ident}"] if v3_domain_ident.present? query_elements << "security_protocol=#{security_protocol}"] if security_protocol.present? uri.query = query_elements.join('&').presence

Also we can drop the .present? calls if these will only ever be nil or usable.

If its nil then we'll end up with query strings like "region=&api_version=&domain_id=", won't we?

Nope, I'm pretty sure my version works as I'm only adding the key=value if the value is present, then joining only when the array is fully assembled.

Also, I'm not sure what "it" you're referencing here, but we should probably have a test for this method either way. I would suggest writing some tests that cover nil values and then we can refactor with confidence.

@carbonin I'm referring to your question about dropping the ".present?" calls if these will be nil or usable. if they're nil, and there is no .present? call, then the wrong thing will happen. Your code works correctly as written.

if they're nil, and there is no .present? call, then the wrong thing will happen.

No, nil is falsey.

This won't print anything:

foo = nil puts "this won't print" if foo

What present? and presence do is move empty strings (and also other empty objects like arrays, hashes, etc) to also be false or nil respectively. So when I saw you using present? I assumed that these could possibly be empty strings, but that seemed strange, which is why I asked.

Doh! Obviously my physical exhaustion is affecting my mental capacity. Of course what you're saying is true. I think I need more coffee...

carbonin · 2018-09-18T00:10:24Z

locale/en.yml

@@ -730,6 +730,7 @@ en:
      FileDepotFtpAnonymous:    Anonymous FTP
      FileDepotNfs:             NFS
      FileDepotS3:              AWS S3
+      FileDepotSwift:           Openstack Swift


Same as the others.

carbonin

We should add specs here. At least for FileDepotSwift#merged_uri, but ideally for more.

carbonin · 2018-10-03T13:45:41Z

app/models/file_depot_swift.rb

+
+  def merged_uri(uri, api_port)
+    port           = api_port.presence || 5000
+    uri            = URI.parse("#{URI(uri).scheme}://#{URI(uri).host}:#{port}#{URI(uri).path}")


Ah this was supposed to be part of the review, but this is rather complicated and I don't understand what it is accomplishing. Specs would help.

Putting the port in the URI.

Since we had the same kind of things here:

ManageIQ/manageiq-appliance_console#66 (review)

Can we switch to just assigning port to the uri object as well? (instead of the quadruple URI.parse)

yes! I thought that was in this PR someplace - I forgot where we'd done it. Thanks!

jerryk55 · 2018-10-09T19:11:17Z

@NickLaMuro @carbonin I've fixed the issue with credential verification. I see that there was a request for a spec in one case but other than this should be ready to go. The UI PR is ready to go and waiting on this one. I have gems-pending code from Nick L to push out and we should be ready at least with the Backup side of the equation if I can get buy-in from everyone. Let me look at a spec here.

NickLaMuro

I think this looks good to me, but I am a bit fuzzy on the workflow of the FileDepot code, so I might have missed something.

The comments I left are minor, so they don't need to hold up the merge.

app/models/file_depot_swift.rb

NickLaMuro · 2018-10-09T20:03:24Z

app/models/file_depot_swift.rb

@@ -0,0 +1,69 @@
+class FileDepotSwift < FileDepot
+  attr_accessor :swift


Is this used?

Not that I'm aware of. Removing.

jerryk55 · 2018-10-09T20:37:58Z

@carbonin tests added. ready to rock and roll?

app/models/file_depot_swift.rb

carbonin

Nearly there, just some minor cleanup type stuff.

carbonin · 2018-10-10T13:58:33Z

app/models/file_depot_swift.rb

+    connect(options.merge(:auth_type => auth_type))
+  rescue Excon::Errors::Unauthorized => err
+    msg = "Access to Swift host #{host} failed due to a bad username or password."
+    logger.error("Access to Swift host #{host} failed due to a bad username or password. #{err}")


app/models/file_depot_swift.rb

app/models/miq_schedule.rb

spec/models/file_depot_swift_spec.rb

jerryk55 · 2018-10-10T17:36:19Z

@carbonin All comments addressed and pushed.

carbonin · 2018-10-11T13:47:28Z

spec/models/file_depot_swift_spec.rb

+    expect(FileDepotSwift.uri_prefix).to eq "swift"
+  end
+
+  context "valid merged uri" do


This should really be the name of the method under test in this block. Also I prefer describe for blocks which are testing a particular method and use context for blocks which define shared setup.

So this would be the convention we normally would use here:

describe "#merged_uri" do

In support of the RFE described in BZ https://bugzilla.redhat.com/show_bug.cgi?id=1615488. Add a new FileDepot subclass for Swift. Since we need to pass info in to the Mount Session (in gems-pending) regarding the Openstack connection, utilize the URI with Query Parameters to do so.

Several changes based on PR review comments - 1) Change "Openstack Swift" to "OpenStack Swift" 2) Fix default argument handling 3) Refactor Query String processing.

Fix credentials verification. Cleanup extra log messages.

Nuff said.

Add some spec tests for the class including the merged_uri method.

Fixed one test so that it doesn't use the default port for the URI. Cleaned up some msg logging to be more efficient.

The merged_uri method was working incorrectly - coded to let the spec tests succeed but the validation of the Swift connection to fail.

Some formatting changes based on review comments.

jerryk55 · 2018-10-11T15:23:42Z

@carbonin spec changes made as requested.

carbonin · 2018-10-11T15:51:18Z

@jerryk55 We're still targeting hammer for this right?

jerryk55 · 2018-10-12T13:08:06Z

Yup. Assuming that the restore side can be finished by then. Which we are relying on @NickLaMuro to do the bulk of that... UI, Core, and Schema are merged. I'm working through comments on the backup side of the gems-pending PR, and there is still an outstanding appliance_console PR.

Openstack Swift DB Backups (cherry picked from commit 520f007) https://bugzilla.redhat.com/show_bug.cgi?id=1615488

simaishi · 2018-10-12T15:47:42Z

Hammer backport details:

$ git log -1
commit e5e724a18e9887514fc0753b7d122991c9f81ec4
Author: Nick Carboni <ncarboni@redhat.com>
Date:   Thu Oct 11 11:48:16 2018 -0400

    Merge pull request #17967 from jerryk55/swift_db_backups
    
    Openstack Swift DB Backups
    
    (cherry picked from commit 520f0077af3b3373bab9b692f50224575cfc332b)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1615488

miq-bot · 2018-10-12T16:04:03Z

Checked commits jerryk55/manageiq@fc21e30~...c25ac99 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
8 files checked, 4 offenses detected

app/models/file_depot_swift.rb

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

spec/models/file_depot_swift_spec.rb

❗ - Line 23, Col 8 - Layout/TrailingWhitespace - Trailing whitespace detected.
❗ - Line 27, Col 8 - Layout/TrailingWhitespace - Trailing whitespace detected.

jerryk55 force-pushed the swift_db_backups branch from a5a526a to 4becfca Compare September 11, 2018 13:29

NickLaMuro previously requested changes Sep 17, 2018

View reviewed changes

lib/evm_database_ops.rb Show resolved Hide resolved

carbonin reviewed Sep 18, 2018

View reviewed changes

jerryk55 force-pushed the swift_db_backups branch from 4becfca to 1978b36 Compare September 18, 2018 16:35

This was referenced Sep 19, 2018

Swift DB Backups ManageIQ/manageiq-ui-classic#4684

Merged

DB Backups to Openstack Swift ManageIQ/manageiq-gems-pending#371

Merged

Support for Swift Backup/Restore ManageIQ/manageiq-appliance_console#66

Merged

carbonin suggested changes Oct 3, 2018

View reviewed changes

carbonin reviewed Oct 3, 2018

View reviewed changes

jerryk55 force-pushed the swift_db_backups branch from 1978b36 to 6c20977 Compare October 9, 2018 18:54

NickLaMuro approved these changes Oct 9, 2018

View reviewed changes

carbonin reviewed Oct 10, 2018

View reviewed changes

app/models/file_depot_swift.rb Show resolved Hide resolved

carbonin reviewed Oct 10, 2018

View reviewed changes

jerryk55 force-pushed the swift_db_backups branch from 886a660 to 7c17051 Compare October 10, 2018 17:35

jerryk55 force-pushed the swift_db_backups branch from 7c17051 to c25ac99 Compare October 10, 2018 21:16

carbonin reviewed Oct 11, 2018

View reviewed changes

jerryk55 added 8 commits October 11, 2018 11:22

Rubocop Warnings

6f36316

Review Comments

431ecb3

Several changes based on PR review comments - 1) Change "Openstack Swift" to "OpenStack Swift" 2) Fix default argument handling 3) Refactor Query String processing.

Credential Verification and Cleanup

12d0c20

Fix credentials verification. Cleanup extra log messages.

Assign URI Port More Simply

2b2c246

Nuff said.

Add FileDepotSwift specs

f2a9640

Add some spec tests for the class including the merged_uri method.

Another FileDepotSwift Test

24e1197

remove unused attr_accessor for swift variable

bc76d2c

jerryk55 added 3 commits October 11, 2018 11:22

Review comments for some cleanup

871b514

Fixed one test so that it doesn't use the default port for the URI. Cleaned up some msg logging to be more efficient.

Fix Failing Test and Merged URI Handling

d42bd4e

The merged_uri method was working incorrectly - coded to let the spec tests succeed but the validation of the Swift connection to fail.

Fix Spec Tests Based on Comments

3700e44

Some formatting changes based on review comments.

jerryk55 force-pushed the swift_db_backups branch from c25ac99 to 3700e44 Compare October 11, 2018 15:22

carbonin approved these changes Oct 11, 2018

View reviewed changes

carbonin merged commit 520f007 into ManageIQ:master Oct 11, 2018

carbonin self-assigned this Oct 11, 2018

carbonin added enhancement core labels Oct 11, 2018

carbonin added this to the Sprint 97 Ending Oct 22, 2018 milestone Oct 11, 2018

carbonin added the hammer/yes label Oct 12, 2018

simaishi pushed a commit that referenced this pull request Oct 12, 2018

Merge pull request #17967 from jerryk55/swift_db_backups

e5e724a

Openstack Swift DB Backups (cherry picked from commit 520f007) https://bugzilla.redhat.com/show_bug.cgi?id=1615488

simaishi added hammer/backported and removed hammer/yes labels Oct 12, 2018

NickLaMuro mentioned this pull request Feb 27, 2019

default merged_uri should return the parameter not the attribute #18497

Merged

		@@ -0,0 +1,69 @@
		class FileDepotSwift < FileDepot
		attr_accessor :swift

Openstack Swift DB Backups #17967

Openstack Swift DB Backups #17967

Conversation

jerryk55 commented Sep 10, 2018

Links

jerryk55 commented Sep 11, 2018 via email

jerryk55 commented Sep 17, 2018

NickLaMuro left a comment • edited Loading

Choose a reason for hiding this comment

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

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

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 9, 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

jerryk55 commented Oct 9, 2018

carbonin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryk55 commented Oct 10, 2018

Choose a reason for hiding this comment

jerryk55 commented Oct 11, 2018

carbonin commented Oct 11, 2018

jerryk55 commented Oct 12, 2018

simaishi commented Oct 12, 2018

miq-bot commented Oct 12, 2018

NickLaMuro left a comment •

edited

Loading