DB Backups to Openstack Swift #371

jerryk55 · 2018-09-10T14:06:58Z

Add an Openstack Swift session object to allow backups to Swift.
This is part of the RFE contained in https://bugzilla.redhat.com/show_bug.cgi?id=1615488.
There is a Schema change already submitted for review in
ManageIQ/manageiq-schema#259.

Further PRs will be added including in manageiq-ui-classic, manageiq (core), and manageiq-appliance-console for the backup and restore side of the picture.

These changes are similar to those implemented for the Amazon S3 DB backups.

@carbonin @NickLaMuro @roliveri please review and merge when appropriate. Thank you.

jerryk55 · 2018-09-10T15:05:42Z

@nick LaMuro <nlamuro@redhat.com> I understand your concern and probably agree but that will not make it into this release which is supposed to be completed in the next two weeks. I submit that we revisit this once we have the functionality working. Is that agreeable to you?

…

***@***.**** commented on this pull request. ------------------------------ > + disconnect + logger.error("Access to Swift container ***@***.***_name} failed due to a bad username or password. #{err}") + msg = "Access to Swift container ***@***.***_name} failed due to a bad username or password. #{err}" + raise err, msg, err.backtrace + rescue => err + disconnect + logger.error("Error uploading #{local_file} to Swift container ***@***.***_name}. #{err}") + msg = "Error uploading #{local_file} to Swift container ***@***.***_name}. #{err}" + raise err, msg, err.backtrace + end + end + + private + + def swift + require 'manageiq/providers/openstack/legacy/openstack_handle' I don't like that we are requiring something here that isn't a dependency of this gem. Perhaps we should consider what @Fryguy <https://github.com/Fryguy> had suggested and move both this and the S3 provider code to their respective provider repos.

NickLaMuro

@jerryk55 First off, that one off comment was meant for this review, and I tried to delete it before you saw it, but I guess that didn't happen. Opps and sorry.

@carbonin and @jerryk55 #361 has been open for a month now, and this PR basically has proven that nobody has looked at it, even though I have invited you both to do so since pretty much the moment it has been opened. The subsequent code I have that has been open for about just as long (some of it longer) and heavily depends on the patterns built in #361, and this code directly conflicts with said patterns.

I have even gone to the trouble of already converting the S3 code to completely support the new framework, but this is basically going to have the same treatment done to it if it just gets merged as is.

I would prefer to get out of this rebasing loop I have been in for the past month and either determine if what I have is being scrapped (again), or if we need to get my changes merged so this PR can work with that new functionality.

-Nick

NickLaMuro · 2018-09-10T14:57:51Z

lib/gems/pending/util/mount/miq_swift_session.rb

+  private
+
+  def swift
+    require 'manageiq/providers/openstack/legacy/openstack_handle'


I don't like that we are requiring something here that isn't a dependency of this gem.

Perhaps we should consider what @Fryguy had suggested and move both this and the S3 provider code to their respective provider repos.

#371 (comment) was meant in response to this, but I deleted it and moved it into my full review (sorry for the confusion).

Answer to Jerry's response can be found in #371 (review)

I don't like that we are requiring something here that isn't a dependency of this gem.

I agree, in fact I wouldn't think this would work at all if not for the amount of autoload magic we do within the actual rails app. Is there no better way to get what we need to do this?

For what it is worth, OpenstackHandle::Handle is basically a thin wrapper around fog-openstack, so we probably could just include that and use that directly.

I will try and see what is necessary in my branch to make the changes later if I have some time.

Note: I still think the suggestion from above (move in to providers repos) is better long term, but agree that we are probably to close to the deadline to pull that off safely.

So - FWIW there is similar code in ManageIQ/manageiq#17967 for the same reason. Yes its a wrapper around fog-openstack but I believe it to be a necessary one. Short of pulling that code out into a repo/gem separate from the openstack provider, or moving this MountSession code into the Provider.

lib/gems/pending/util/mount/miq_swift_session.rb

+require 'util/mount/miq_generic_mount_session'
+require 'excon'
+
+class MiqSwiftSession < MiqGenericMountSession


lib/gems/pending/util/mount/miq_swift_session.rb

carbonin · 2018-09-10T18:50:50Z

lib/gems/pending/util/mount/miq_swift_session.rb

+      :ssl_cert_store => OpenSSL::X509::Store.new
+    }
+    extra_options[:domain_id] = @domain_id
+    extra_options[:omit_default_port] = ::Settings.ems.ems_openstack.excon.omit_default_port


I don't think we can use Settings here.

We can't use as in its an error? Or as in its wrong for some reason? Because the code works.....

I'm very surprised the code works. Settings is set up in the core repo and should not be relied upon in this gem.

@carbonin I hope you like surprises then. :-)

carbonin · 2018-09-10T18:51:45Z

lib/gems/pending/util/mount/miq_swift_session.rb

+    extra_options[:service] = "Compute"
+
+    @osh ||= OpenstackHandle::Handle.new(@username, @password, @host, @port, @api_version, @security_protocol, extra_options)
+    @osh.connection_options = {:instrumentor => $fog_log}


Is $fog_log defined in this repo?

Based on the fact that I don't know where any of the log variables are defined offhand, I'd have to answer "i don't know"

Defined in ManageIQ/manageiq here:

https://github.com/ManageIQ/manageiq/blob/c1156912/lib/vmdb/loggers.rb#L57

Yeah, then this also shouldn't be used here.

I'm just going to take that out then.

carbonin · 2018-09-10T18:54:00Z

lib/gems/pending/util/mount/miq_swift_session.rb

+
+  def query_params(query_string)
+    query_string.split('&').each do |query|
+      query_parts = query.split('=')


I think this would be easier to read as key, value = query.split('=')

Sure no problem.

We met out of band on how to go forward with this and #361, so going to dismiss this review based on that discussion.

miq-bot · 2018-09-12T18:12:33Z

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

NickLaMuro · 2018-09-18T02:07:54Z

@jerryk55 Been spending a good portion of today looking into how we can update these changes to work with the changes I introduced in #361

I have put together a branch that does just that:

master...NickLaMuro:swift-storage-as-object-storage

Not very well tested, specifically the swift parts (getting an environment for that is a [WIP] for me currently, and part of my TODO list), but should help integrate both the existing changes, as well as how file splitting could possibly be implemented as well.

I might break out some of the pieces into their own PRs as it isn't meant to be somewhat generic, but they didn't exist previously so I included them all in one spot for now.

Hope this helps.

cc @carbonin

carbonin

Core manageiq lists this gem as a dependency, as such we shouldn't be depending on resources from core (Settings, $fog_log) or from other provider gems like manageiq-providers-openstack.

carbonin · 2018-09-19T20:23:36Z

lib/gems/pending/util/mount/miq_swift_session.rb

+  private
+
+  def swift
+    require 'manageiq/providers/openstack/legacy/openstack_handle'


I don't like that we are requiring something here that isn't a dependency of this gem.

I agree, in fact I wouldn't think this would work at all if not for the amount of autoload magic we do within the actual rails app. Is there no better way to get what we need to do this?

carbonin · 2018-09-19T20:24:28Z

lib/gems/pending/util/mount/miq_swift_session.rb

+      :ssl_cert_store => OpenSSL::X509::Store.new
+    }
+    extra_options[:domain_id] = @domain_id
+    extra_options[:omit_default_port] = ::Settings.ems.ems_openstack.excon.omit_default_port


I'm very surprised the code works. Settings is set up in the core repo and should not be relied upon in this gem.

carbonin · 2018-09-19T20:24:56Z

lib/gems/pending/util/mount/miq_swift_session.rb

+    extra_options[:service] = "Compute"
+
+    @osh ||= OpenstackHandle::Handle.new(@username, @password, @host, @port, @api_version, @security_protocol, extra_options)
+    @osh.connection_options = {:instrumentor => $fog_log}


Yeah, then this also shouldn't be used here.

lib/gems/pending/util/mount/miq_swift_session.rb

jerryk55 · 2018-10-09T21:03:42Z

@carbonin removed reference to $fog_log that slipped through the cracks. This is ready for review again.

NickLaMuro

So I will leave a link to my current experimental branch here for anyone else reviewing:

master...NickLaMuro:swift-storage-as-object-storage

But most of this review is just commentary on what was pulled in, and things that can potentially be improved upon from that.

NickLaMuro · 2018-10-09T21:32:56Z

lib/gems/pending/util/miq_object_storage.rb


  attr_accessor :settings
  attr_writer   :logger

+  DEFAULT_CHUNKSIZE = Net::BufferedIO::BUFSIZE


For some reference, this comes from net/protocol, and it is the same chunk size that is used by Net::FTP when uploading. Figured it was a reasonable value to pick out of a hat and use here.

lib/gems/pending/util/miq_object_storage.rb

lib/gems/pending/util/mount/miq_generic_mount_session.rb

NickLaMuro · 2018-10-09T21:37:21Z

lib/gems/pending/util/object_storage/miq_swift_storage.rb

@@ -0,0 +1,148 @@
+require 'pp'
+# require 'util/miq_object_storage'


I might have left this in as a comment at one point, but I think you can delete this.

Actually... looking at this more, possibly this should change to use this line... let me get back to you on that one.

@NickLaMuro let me know...

I think I might have been trying to test this file in irb on it's own, and doing the below require made it easier to do that. I will give this a shot in a few with this uncommented and the other included and see if it should work.

I am pretty sure this line should be the one we want, but I want to make sure tests pass (unit and integration) before I say one way or the other.

This can be uncommented and the line below deleted. I think my explanation from above is the reason... I guess.

Anyway, sorry for the extra crap left lying around...

NickLaMuro · 2018-10-09T21:39:45Z

lib/gems/pending/util/object_storage/miq_swift_storage.rb

+    super(settings)
+
+    # NOTE: This line to be removed once manageiq-ui-class region change implemented.
+    @bucket_name         = URI(@settings[:uri]).host


Is this being used in the FileDepot code then? If so, then that is probably why I omitted it by mistake in my branch.

Sorry about that.

NickLaMuro · 2018-10-09T21:42:09Z

lib/gems/pending/util/object_storage/miq_swift_storage.rb

+    raise err, msg, err.backtrace
+  end
+
+  def download_single(source, destination)


Maybe raise an error in here for now instead of implementing it with s3 code 😉

NickLaMuro · 2018-10-09T21:45:14Z

lib/gems/pending/util/object_storage/miq_swift_storage.rb

+
+  def swift
+    return @swift if @swift
+    require 'manageiq/providers/openstack/legacy/openstack_handle'


So of note, I have written a version of this that makes use of fog/openstack directly:

manageiq-gems-pending/lib/gems/pending/util/object_storage/miq_swift_storage.rb

Lines 92 to 114 in cd2beb7

def swift

return @swift if @swift

require 'fog/openstack'

auth_url = URI::Generic.build(

:scheme => @security_protocol == 'non-ssl' ? "http" : "https",

:host => @host,

:port => @port.to_i,

:path => "/#{@api_version}#{@api_version == "v3" ? "/auth" : ".0"}/tokens"

).to_s

connection_params = {

:openstack_auth_url => auth_url,

:openstack_username => @username,

:openstack_api_key => @password,

:openstack_project_domain_id => @domain_id,

:openstack_user_domain_id => @domain_id,

:openstack_region => @region,

:connection_options => { :debug_request => true }

}

swift = Fog::Storage::OpenStack.new(connection_params)

end

And seems to work with the tests I have setup for this (rake tasks only). If we want to limit the amount of dependencies from that repo, that might be something to consider. I spent a decent amount of time pulling out the relevant pieces from the OpenstackHandle code base to make that work, so hopefully that does the trick for most use cases.

👍 For @NickLaMuro's approach. Based on the snippet linked here, I don't see much simplification in the way the code is currently vs using fog/openstack.

carbonin

In addition to my comments please add specs.

carbonin · 2018-10-10T13:16:51Z

lib/gems/pending/util/object_storage/miq_swift_storage.rb

+
+  def uri_to_object_path(remote_file)
+    # Strip off the leading "swift://" and the container name from the URI"
+    # Also remove teh leading delimiter.


Typo, s/teh/the/

carbonin · 2018-10-10T13:19:36Z

lib/gems/pending/util/object_storage/miq_swift_storage.rb

+
+  def swift
+    return @swift if @swift
+    require 'manageiq/providers/openstack/legacy/openstack_handle'


👍 For @NickLaMuro's approach. Based on the snippet linked here, I don't see much simplification in the way the code is currently vs using fog/openstack.

carbonin · 2018-10-10T13:19:52Z

lib/gems/pending/util/object_storage/miq_swift_storage.rb

+    extra_options[:domain_id] = @domain_id
+    extra_options[:service] = "Compute"
+     @osh ||= OpenstackHandle::Handle.new(@username, @password, @host, @port, @api_version, @security_protocol, extra_options)
+    @osh.connection_options = {:instrumentor => $fog_log}


$fog_log should be removed I think

Removed at some point in the last day or so...

carbonin · 2018-10-10T13:21:30Z

lib/gems/pending/util/object_storage/miq_swift_storage.rb

+
+    with_standard_s3_error_handling("downloading", source) do
+      if destination.kind_of?(IO)
+        s3.client.get_object(:bucket => bucket_name, :key => object_key) do |chunk|


I don't think s3 should be a thing in the swift code, right? Agree with @NickLaMuro, if this isn't implemented then let's raise a NotImplementedError rather than just copying and pasting unrelated code.

carbonin · 2018-10-10T13:22:30Z

lib/gems/pending/util/object_storage/miq_swift_storage.rb

+    extra_options = {}
+    extra_options[:domain_id] = @domain_id
+    extra_options[:service] = "Compute"
+     @osh ||= OpenstackHandle::Handle.new(@username, @password, @host, @port, @api_version, @security_protocol, extra_options)


There looks like there's some extra indentation on this line that isn't needed.

Fixed, and in a couple of other places.

carbonin · 2018-10-10T13:23:04Z

lib/gems/pending/util/object_storage/miq_swift_storage.rb

+     @osh ||= OpenstackHandle::Handle.new(@username, @password, @host, @port, @api_version, @security_protocol, extra_options)
+    @osh.connection_options = {:instrumentor => $fog_log}
+    begin
+      @swift ||= @osh.swift_service


No need for ||= here right? The first line in the method is return @swift if @swift

carbonin · 2018-10-10T13:24:19Z

lib/gems/pending/util/object_storage/miq_swift_storage.rb

+
+  def create_container
+    container = swift.directories.create(:key => container_name)
+    logger.debug("Swift container [#{container}] created")


What does this print? Is this just the container name or does this print the object inspect? Just want to make sure this isn't printing connection passwords by mistake.

changed to logger.debug("Swift container [#{container_name}] created")

lib/gems/pending/util/object_storage/miq_swift_storage.rb

NickLaMuro

Okay, think I caught some things from my last review, as well as some comment wording suggestions. They should be quick fixes.

NickLaMuro · 2018-10-10T21:54:34Z

lib/gems/pending/util/object_storage/miq_swift_storage.rb

+
+    raise "username and password are required values!" if @settings[:username].nil? || @settings[:password].nil?
+    _scheme, _userinfo, @host, @port, _registry, @mount_path, _opaque, query, _fragment = URI.split(URI.encode(@settings[:uri]))
+    query_params(query)


So I actually have a test for this:

https://github.com/ManageIQ/manageiq-gems-pending/compare/master...NickLaMuro:swift-storage-as-object-storage#diff-29a17f03343e08092b7e3f3fbbeaf113R97

That will actually fail with how things are currently (just noticed when running them when looking into something else). The issue is that passing a nil value to URI.decode_www_form doesn't play well, and that is what that test is checking against.

This can be solve by either adding a if query to the end of this line, or doing a nil check in #query_params. Up to you, but that should be done.

NickLaMuro · 2018-10-10T21:59:33Z

lib/gems/pending/util/object_storage/miq_swift_storage.rb

@@ -0,0 +1,148 @@
+require 'pp'
+# require 'util/miq_object_storage'


This can be uncommented and the line below deleted. I think my explanation from above is the reason... I guess.

Anyway, sorry for the extra crap left lying around...

NickLaMuro · 2018-10-10T22:04:33Z

lib/gems/pending/util/object_storage/miq_swift_storage.rb

+  #
+  # Instead of investigating further, we created a new method that is in charge of
+  # OpenStack container creation, and that is called from '#container' if 'nil' is
+  # returned, or the error for 'NotFound' is called to cover both cases.


or the error for 'NotFound' is called to cover both cases.

Maybe change it to this:

or call '#create_container' in the rescue case for 'NotFound' to cover that scenario as well.

(sorry, this is verbatim from my commit message, and I admit I wrote that poorly)

lib/gems/pending/util/object_storage/miq_swift_storage.rb

NickLaMuro · 2018-10-10T22:07:51Z

lib/gems/pending/util/object_storage/miq_swift_storage.rb

+  #
+  # Some calls to Fog::Storage::OpenStack::Directories#get will
+  # return 'nil', and not return an error.  This would cause errors down the
+  # line.


Maybe to be more specific:

... down the line in '#upload' or '#download'.

carbonin

Most of this should be covered by the bot, but I think it has been stuck for a while and is still catching up to PR changes.

Otherwise can we add specs for some of the more testable methods in MiqSwiftStorage?

I think some good candidates would be:

ensure that @container_name is set properly on initialize
#uri_to_local_path (looks like this was removed)
#uri_to_object_path
validate the auth_url in the #swift private method

lib/gems/pending/util/object_storage/miq_swift_storage.rb

spec/util/miq_file_storage_spec.rb

spec/util/miq_object_storage_spec.rb

lib/gems/pending/util/object_storage/miq_swift_storage.rb

NickLaMuro

Just some formatting things I noticed. Minor.

lib/gems/pending/util/object_storage/miq_swift_storage.rb

NickLaMuro

Syntax error that needs fixing.

NickLaMuro · 2018-10-15T15:35:28Z

lib/gems/pending/util/object_storage/miq_swift_storage.rb

+    @container ||= begin
+                     container = swift.directories.get(container_name)
+                     logger.debug("Swift container [#{container}] found") if container
+                     unless container raise Fog::Storage::OpenStack::NotFound


This is C style syntax for conditional, and is causing your syntax errors. Should be:

raise Fog::Storage::OpenStack::NotFound unless container

Yeah I already caught this with a test I was trying. Thanks.

lib/gems/pending/util/miq_object_storage.rb

NickLaMuro · 2018-10-15T18:30:41Z

Just an FYI, I "Unresolved" my most recent two comments, but I expect that a new push to this branch will automatically resolve them when that is done. Would prefer that they were resolved that way (and should be based on where I made the comments), even though I fully assume you will be doing the changes as you said.

I will keep an eye on them and resolve them again if github doesn't do it's job, however. Thanks!

(Update: Yay! it worked!)

carbonin

Would still like to see a spec for the auth_url in the #swift private method.

carbonin · 2018-10-15T19:01:20Z

lib/gems/pending/util/object_storage/miq_swift_storage.rb

+  def initialize(settings)
+    super(settings)
+
+    # NOTE: This line to be removed once manageiq-ui-class region change implemented.


Is this the case here also?

Also typo, s/manageiq-ui-class/manageiq-ui-classic/

carbonin · 2018-10-15T19:08:59Z

spec/util/object_storage/miq_swift_storage_spec.rb

+describe MiqSwiftStorage do
+  before(:each) do
+    @uri = "swift://foo.com/abc/def"
+    @object_storage = described_class.new(:uri => @uri, :username => 'user', :password => 'pass', :region => 'region')


This should really use let or subject rather than an instance variable.

carbonin · 2018-10-15T19:09:28Z

spec/util/object_storage/miq_swift_storage_spec.rb

+
+describe MiqSwiftStorage do
+  before(:each) do
+    @uri = "swift://foo.com/abc/def"


This should really use let rather than an instance variable.

carbonin · 2018-10-15T19:14:54Z

Also, can you either squash the commits in this PR or make them something more meaningful than "Review comments", "Rubocop fixes", "Tavis failure", etc.

NickLaMuro

Besides what @carbonin had mentioned, I don't see much else to change here besides the gemspec addition. The "Request changes" status is only here to make sure we have the dependency defined here as well (which I assume is what we want to do since we did the same for s3, and not have it a implied dependency).

Otherwise, I have tested this branch with my test suite and everything seems to be working, so looks good otherwise!

lib/gems/pending/util/object_storage/miq_swift_storage.rb

NickLaMuro · 2018-10-15T19:14:16Z

lib/gems/pending/util/object_storage/miq_swift_storage.rb

+
+  def swift
+    return @swift if @swift
+    require 'fog/openstack'


Oh, can we add this to the manageiq-gems-pending.gemspec now?

setting it to "=0.1.22" as in the manageiq-providers-openstack gemspec.

setting it to "=0.1.22" as in the manageiq-providers-openstack gemspec.

Please, don't. If we can use a less strict constraint for what we're doing here, then let's let the other repos dictate what exact version they need. Making this a hard constraint will only make it more difficult to change.

Ok, then setting it to ~>0.1.22 SHouldn't be less than the Provider...

Add an Openstack Swift session object to allow backups to Swift.

Rework query string processing as per review comments. Remove $fog_log reference.

S3 was moved over to the object file session rather than this mount session subclass by a separate PR, so adding it back in via this PR is incorrect and causes Travis build failures.

Removing the swift class for the Mount Session, and adding a new swift class for Object Storage instead. This works with the streaming db backups/dumps now implemented.

As per review comment don't use $fog_log here.

Review comments by various reviews. Some nitpicks but the major change is to not use Openstack Provider handle code but instead use Fog::Openstack.

Major and minor changes - some indentation, some new tests (thanks Nick L) Stubbed out download_single method since it isn't fully working yet.

Fixed spacing, parentheses, and reworded and added some comments.

Review comments, adding some tests, and removing unused uri_to_local_path method.

jerryk55 · 2018-10-17T14:07:52Z

@carbonin @NickLaMuro I believe I have addressed all of your comments. Please give this your blessing and merge if appropriate. Thanks.

carbonin · 2018-10-17T14:38:50Z

manageiq-gems-pending.gemspec

@@ -27,7 +27,9 @@ Gem::Specification.new do |s|
  s.add_runtime_dependency "aws-sdk",                 "~> 2.9.7"
  s.add_runtime_dependency "binary_struct",           "~> 2.1"
  s.add_runtime_dependency "bundler",                 ">= 1.8.4" # rails-assets requires bundler >= 1.8.4, see: https://rails-assets.org/
+  s.add_runtime_dependency "fog-openstack",           "~>0.1.22"


Can you format this like every other line in the gemspec? With a space between the > and the version?

spec/util/object_storage/miq_swift_storage_spec.rb

carbonin · 2018-10-17T14:51:55Z

spec/util/object_storage/miq_swift_storage_spec.rb

+    context "with non-ssl security protocol" do
+      let(:uri) { "swift://foo.com:5678/abc/def?region=region&api_version=v3&security_protocol=non-ssl" }
+      let(:object_storage) { described_class.new(:uri => uri, :username => 'user', :password => 'pass') }
+      let(:auth_url) { object_storage.send(:auth_url) }


This isn't really a good use of let. Since this is the actual method under test I think it would be better to call it directly in the test.

Got it. Amended and Pushed. All comments addressed (again) @NickLaMuro @carbonin

In order to test the private mechanisms within more easily, pull the auth_url construction out of the private method #swift. Add tests for it as well as for #uri_to_object_path, and to make sure #initialize sets the container name correctly. Add fog-openstack and mime-types to the gemspec.

NickLaMuro

These comments are minor or just commentary. I think this looks good. So feel free to merge as far as I am concerned.

NickLaMuro · 2018-10-17T15:08:30Z

spec/util/object_storage/miq_swift_storage_spec.rb

+require "util/object_storage/miq_swift_storage"
+
+describe MiqSwiftStorage do
+  let(:object_storage) { described_class.new(:uri => uri, :username => 'user', :password => 'pass') }


I was going to suggest moving this here. 😄

👍

spec/util/object_storage/miq_swift_storage_spec.rb

NickLaMuro · 2018-10-17T15:13:15Z

manageiq-gems-pending.gemspec

  s.add_runtime_dependency "linux_admin",             "~> 1.0"
+  s.add_runtime_dependency "mime-types",              "~> 3.0"


Ugh.... another gem that uses mime-types... kill me now...

For those completely confused with the above, see: ManageIQ/manageiq#14525

carbonin

This looks great! Thanks for the work on this @jerryk55 and @NickLaMuro

miq-bot · 2018-10-17T15:31:32Z

Checked commits jerryk55/manageiq-gems-pending@ecf715f~...518f48f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 5 offenses detected

lib/gems/pending/util/object_storage/miq_swift_storage.rb

⚠️ - Line 19, Col 101 - Lint/UriEscapeUnescape - URI.encode method is obsolete and should not be used. Instead, use CGI.escape, URI.encode_www_form or URI.encode_www_form_component depending on your specific use case.
⚠️ - Line 30, Col 41 - Lint/UriEscapeUnescape - URI.encode method is obsolete and should not be used. Instead, use CGI.escape, URI.encode_www_form or URI.encode_www_form_component depending on your specific use case.
❗ - Line 106, Col 20 - Style/RescueStandardError - Avoid rescuing without specifying an error class.
❗ - Line 145, Col 3 - Style/RescueStandardError - Avoid rescuing without specifying an error class.
❗ - Line 69, Col 5 - Style/RescueStandardError - Avoid rescuing without specifying an error class.

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

simaishi · 2018-10-17T20:38:10Z

Hammer backport details:

$ git log -1
commit 66584e4fdeac5863860591bb4eb34c1fe34ecd50
Author: Nick Carboni <ncarboni@redhat.com>
Date:   Wed Oct 17 11:32:28 2018 -0400

    Merge pull request #371 from jerryk55/swift_db_backup
    
    DB Backups to Openstack Swift
    
    (cherry picked from commit 33ec84a811d119f6c605385c2164b53af8ab678e)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1615488

NickLaMuro previously requested changes Sep 10, 2018

View reviewed changes

carbonin reviewed Sep 10, 2018

View reviewed changes

miq-bot added the unmergeable label Sep 12, 2018

jerryk55 force-pushed the swift_db_backup branch from 370a172 to 278608a Compare September 12, 2018 18:36

miq-bot removed the unmergeable label Sep 13, 2018

jerryk55 force-pushed the swift_db_backup branch from 278608a to 120c914 Compare September 17, 2018 13:53

NickLaMuro mentioned this pull request Sep 17, 2018

Openstack Swift DB Backups ManageIQ/manageiq#17967

Merged

jerryk55 mentioned this pull request Sep 19, 2018

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

Merged

carbonin suggested changes Sep 19, 2018

View reviewed changes

Fryguy reviewed Sep 19, 2018

View reviewed changes

lib/gems/pending/util/mount/miq_swift_session.rb Outdated Show resolved Hide resolved

jerryk55 force-pushed the swift_db_backup branch from 0f578f8 to be5eb9a Compare September 20, 2018 13:39

jerryk55 mentioned this pull request Sep 25, 2018

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

Merged

jerryk55 force-pushed the swift_db_backup branch from cd98e19 to 58267a7 Compare October 9, 2018 20:49

NickLaMuro reviewed Oct 9, 2018

View reviewed changes

carbonin reviewed Oct 10, 2018

View reviewed changes

NickLaMuro suggested changes Oct 10, 2018

View reviewed changes

carbonin reviewed Oct 11, 2018

View reviewed changes

NickLaMuro reviewed Oct 11, 2018

View reviewed changes

lib/gems/pending/util/object_storage/miq_swift_storage.rb Outdated Show resolved Hide resolved

lib/gems/pending/util/object_storage/miq_swift_storage.rb Outdated Show resolved Hide resolved

NickLaMuro suggested changes Oct 15, 2018

View reviewed changes

lib/gems/pending/util/miq_object_storage.rb Outdated Show resolved Hide resolved

carbonin reviewed Oct 15, 2018

View reviewed changes

NickLaMuro suggested changes Oct 15, 2018

View reviewed changes

DB Backups to Openstack Swift

ecf715f

Add an Openstack Swift session object to allow backups to Swift.

jerryk55 added 8 commits October 15, 2018 16:44

Review Comments

57a2d95

Rework query string processing as per review comments. Remove $fog_log reference.

Fix Travis Failure

5c3ebdc

S3 was moved over to the object file session rather than this mount session subclass by a separate PR, so adding it back in via this PR is incorrect and causes Travis build failures.

Swift DB Backup Support with MiqObjectStorage Methodology

f3a3f05

Removing the swift class for the Mount Session, and adding a new swift class for Object Storage instead. This works with the streaming db backups/dumps now implemented.

Remove ref to $fog_log

aa8f839

As per review comment don't use $fog_log here.

Review Comments

49ccf0f

Review comments by various reviews. Some nitpicks but the major change is to not use Openstack Provider handle code but instead use Fog::Openstack.

Initial Set of MiqSwiftStorage Tests and Stub #download_single.

11cb304

Major and minor changes - some indentation, some new tests (thanks Nick L) Stubbed out download_single method since it isn't fully working yet.

Lots of spacing and comment changes brought up in review.

c8730ec

Fixed spacing, parentheses, and reworded and added some comments.

Remove #uri_to_local_path and add initial tests for MiqSwiftStorage

2e8c9e8

Review comments, adding some tests, and removing unused uri_to_local_path method.

jerryk55 force-pushed the swift_db_backup branch 2 times, most recently from c62e61c to e33d295 Compare October 17, 2018 14:00

carbonin reviewed Oct 17, 2018

View reviewed changes

jerryk55 force-pushed the swift_db_backup branch from e33d295 to 90f3555 Compare October 17, 2018 14:52

carbonin reviewed Oct 17, 2018

View reviewed changes

jerryk55 force-pushed the swift_db_backup branch from 90f3555 to bea075e Compare October 17, 2018 15:04

jerryk55 force-pushed the swift_db_backup branch from bea075e to 47dd52c Compare October 17, 2018 15:07

NickLaMuro approved these changes Oct 17, 2018

View reviewed changes

Add tests to make sure Query String is empty in auth_url

518f48f

carbonin approved these changes Oct 17, 2018

View reviewed changes

carbonin self-assigned this Oct 17, 2018

carbonin added enhancement hammer/yes labels Oct 17, 2018

carbonin merged commit 33ec84a into ManageIQ:master Oct 17, 2018

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

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

Merge pull request #371 from jerryk55/swift_db_backup

66584e4

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

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

		@@ -0,0 +1,148 @@
		require 'pp'
		# require 'util/miq_object_storage'

	def swift
	return @swift if @swift
	require 'fog/openstack'

	auth_url = URI::Generic.build(
	:scheme => @security_protocol == 'non-ssl' ? "http" : "https",
	:host => @host,
	:port => @port.to_i,
	:path => "/#{@api_version}#{@api_version == "v3" ? "/auth" : ".0"}/tokens"
	).to_s

	connection_params = {
	:openstack_auth_url => auth_url,
	:openstack_username => @username,
	:openstack_api_key => @password,
	:openstack_project_domain_id => @domain_id,
	:openstack_user_domain_id => @domain_id,
	:openstack_region => @region,
	:connection_options => { :debug_request => true }
	}

	swift = Fog::Storage::OpenStack.new(connection_params)
	end

		s.add_runtime_dependency "linux_admin", "~> 1.0"
		s.add_runtime_dependency "mime-types", "~> 3.0"

DB Backups to Openstack Swift #371

DB Backups to Openstack Swift #371

Conversation

jerryk55 commented Sep 10, 2018

jerryk55 commented Sep 10, 2018 via email • edited Loading

NickLaMuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro Sep 10, 2018 • 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

This comment was marked as resolved.

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

miq-bot commented Sep 12, 2018

NickLaMuro commented Sep 18, 2018 • edited Loading

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

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

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

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

carbonin left a comment • edited Loading

Choose a reason for hiding this comment

NickLaMuro left a comment

Choose a reason for hiding this comment

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 commented Oct 15, 2018 • edited Loading

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

carbonin commented Oct 15, 2018

NickLaMuro left a comment

jerryk55 commented Sep 10, 2018 via email •

edited

Loading

NickLaMuro Sep 10, 2018 •

edited

Loading

NickLaMuro commented Sep 18, 2018 •

edited

Loading

carbonin left a comment •

edited

Loading

NickLaMuro commented Oct 15, 2018 •

edited

Loading