Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Re-insert MiqFileStorage and MiqFTPLib v2 #19742

Conversation

NickLaMuro
Copy link
Member

@NickLaMuro NickLaMuro commented Jan 21, 2020

IMPORTANT PR STATUS UPDATE: See comment below.

With 100% more commit grafting!

This is a re-working of #19547 with the base commit being "grafted" to the commit that removed these files originally:

NickLaMuro@250a0b4

I will leave out most of the description to the previous PR, since this is mostly the same changes, with just a slightly different path of getting there.

Links

TODO

  • Create a ManageIQ/manageiq-smartstate PR
  • Re-run the extraction with updated master, since it seems like some updates will be needed

NickLaMuro and others added 30 commits January 21, 2020 14:40
*** Original Commit message shown below ***

Author:    Oleg Barenboim [email protected] Tue May 3 23:53:27 2016 -0500
Committer: Oleg Barenboim [email protected] Tue May 3 23:53:27 2016 -0500

Merge pull request ManageIQ#8404 from Ladas/fix_event_monitor_example_script

Fix event_monitor_example.rb script

(transferred from ManageIQ/manageiq-gems-pending@c0e7a76)
Based on PR review comments, use the existing MountSession "add" operation
to upload the database file to S3.

Correctly handle the situation where the bucket name requested exists but
access is denied, indicating (probably) that is owned by another user.
(The S3 namespace is Global - each user does not get their own a namespace).


(transferred from ManageIQ/manageiq-gems-pending@67fd6ee)
`MiqFileStorage` is meant to be an top level class for fetching classes
that interface with `MiqFileStorage::Interface`.

`MiqFileStorage::Interface` is meant to be the super class for all of
the class to inherit from, to make sure that they conform to a spec that
`MiqFileStorage` can work with.


(transferred from ManageIQ/manageiq-gems-pending@1c5c8c5)
Changes to support restore of database from S3 objects.


(transferred from ManageIQ/manageiq-gems-pending@7485a64)
Based on review comments change MiqS3Session.disconnect to self.raw_disconnect.
Additionally remove the previously added 3rd argument to MiqGenericMountSession.add
which is no longer needed.


(transferred from ManageIQ/manageiq-gems-pending@60b37d0)
This mount session is intended as being a pass through for when local
files are used.

The same API's for `#add` from MiqGenericMountSession are still usable
for this class, and it will just write to the local file system instead.

This also allows us to test the file splitting code without needing a
mount session in place to do so.


(transferred from ManageIQ/manageiq-gems-pending@1bd3433)
Context that make sure to spin up an FTP server when included, and will
make some helper methods available.

Uses the `ftpd` gem to handle the ftp server.


(transferred from ManageIQ/manageiq-gems-pending@04552b0)
Since this is going to be overwritten in each class (most likely), and
already is for S3, it makes sense that we don't include this since it
doesn't do anything by being here.


(transferred from ManageIQ/manageiq-gems-pending@9f91d86)
This is the top level class for Object Storage based file storages,
which will include types like s3 and FTP.

Doesn't do much, but shared methods between all of the Object Storage
classes will be put here.


(transferred from ManageIQ/manageiq-gems-pending@d79deab)
This is lifted from the FileDepotFtp code in the manageiq repo, and is
just some shared methods around connecting to an FTP endpoint.

Adds some tests around it as well.


(transferred from ManageIQ/manageiq-gems-pending@25baa0b)
This was originally part of the MiqLocalMountSession spec, and could be
useful for other specs as well, specifically FTP based object stores in
the future.


(transferred from ManageIQ/manageiq-gems-pending@f272e2e)
Converts MiqS3Session to an MiqObjectStorage subclass, MiqS3Storage.

A decent amount here didn't change, but it updates the `#add` and
`#download` methods to use the MiqFileStorage::Interface and instead
override `#upload_single` and `#download_single`.

Many of the tests didn't make it over since a lot of it was in regards
to "mounting", which ObjectStorage classes don't do and is now obsolete,
as well as cleaned up MiqGenericMountSession from any references that
were s3 specific.

Also dried up the code a bit from what was implemented in MiqS3Session.

Regarding the elephant in the room...
-------------------------------------

Yes, I backported Aws::S3::MultipartStreamUploader from the v3 portion
of the aws-sdk as part of this commit.  Some form of "upload streaming"
was necessary for file splitting to work.

I could have implemented something myself with probably a little less
code, but what was already part of the v3 API, is probably tested
extensively, and works with the v2 Seahorse client and lib made more
sense to implement via this way instead of trying to role my own.

It is a lot of extra code, but I have a flag in place for when we do
upgrade to aws-sdk v3 in the future, an error should be raised in the
test suite to have this patch removed.


(transferred from ManageIQ/manageiq-gems-pending@f3e3e9c)
This adds FTP as a ObjectStorage target, which allows the FTP protocol
to be used in conjunction with the MiqFileStorage::Interface.

Most of this is pretty straight forward and uses the vanilla Net::FTP
mechanics, but upload_single got a bit involved.

Because we want to stream as much of this content as possible, and file
splitting is also a requirement for uploads, breaking into the internals
of Net::FTP was necessary to achieve uploads where we were only
partially reading the input.  `Net::FTP#storbinary` only allows sending
the entire file, and being able to specify the size of the upload chunks
and offset, but not how much of the file intotal to send.

IO.copy_stream thankfully does most of the heavy lifting and logic for
us, and since we are streaming the content through that, should send the
`.write` calls in a small enough size that doesn't go over the
`Net::FTP::DEFAULT_BLOCKSIZE`... I hope...


(transferred from ManageIQ/manageiq-gems-pending@11346b9)
…ad_target_abstractions

MiqFileStorage interface and subclassing (with file splitting)

(transferred from ManageIQ/manageiq-gems-pending@7ba5665)
Add an Openstack Swift session object to allow backups to Swift.


(transferred from ManageIQ/manageiq-gems-pending@ecf715f)
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.


(transferred from ManageIQ/manageiq-gems-pending@5c3ebdc)
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.


(transferred from ManageIQ/manageiq-gems-pending@f3a3f05)
As per review comment don't use $fog_log here.


(transferred from ManageIQ/manageiq-gems-pending@aa8f839)
Review comments by various reviews.
Some nitpicks but the major change is to not use Openstack Provider handle code
but instead use Fog::Openstack.


(transferred from ManageIQ/manageiq-gems-pending@49ccf0f)
NickLaMuro and others added 15 commits January 21, 2020 14:40
Removes some unneeded lines, and unnecessary words in `it` lines.


(transferred from ManageIQ/manageiq-gems-pending@354d08f)
Since we need a connection for any download/upload to work (sets @ftp on
the instance), we need a slight override of `#magic_number_for` to wrap
it with a connection.


(transferred from ManageIQ/manageiq-gems-pending@f70c48d)
…o-swift-object-store

Add MiqSwiftStorage#download_single

(transferred from ManageIQ/manageiq-gems-pending@1525c2c)
If there is no trailing slash in the parsed out `path` from `URI.parse`,
then setting of `@container_name` didn't match the regexp, and so it
caused the path generated for the URL in the `#container` method to
generate as:

  "/v1/AUTH_asdfglkjwearimsdf/%2Fcontainer_name"

Instead of:

  "/v1/AUTH_asdfglkjwearimsdf/container_name"

Where `"%2f"` is due to `fog-core`'s URI escaping code of the string
passed into the `swift.directories.get(container_name)` when it includes
a slash (so `"/container_name" => "%2Fcontainer_name"`).

The fix makes sure the regexp is correct, and the removal of the extra
`\/` check at the end was removed.  Some extra tests were put in place
to make sure everything still works as expected.


(transferred from ManageIQ/manageiq-gems-pending@242bf6f)
…to-miq-file-storage

[MiqFileStorage] Add #magic_number_for

(transferred from ManageIQ/manageiq-gems-pending@b3f99e1)
Does better fallback handling and nil checking for this method.

This can fail as part of `MiqGenericMountSession#add`'s `rescue`
statement when the `@source_input` is not defined, and that will mask
the actual error that was raised.

This fixes that bug so the proper error is shown in the logs.


(transferred from ManageIQ/manageiq-gems-pending@7f8b69a)
…rors

[MiqGenericMountSession] Fix .source_for_log

(transferred from ManageIQ/manageiq-gems-pending@bb0b1d4)
Since this has been moved from `manageiq-gems-pending`, this is no
longer needed, and keeping it in causes the build to fail.
- Adds ignore cases for two items that have been there for a bit
- Removes an outdated warning.
This is used by the `MiqFtpLib` specs
@miq-bot
Copy link
Member

miq-bot commented Jan 21, 2020

Some comments on commits NickLaMuro/manageiq@75ef1b6~...8820a90

spec/support/examples_group/with_ftp_server.rb

  • ⚠️ - 15 - Detected puts. Remove all debugging statements.
  • ⚠️ - 53 - Detected puts. Remove all debugging statements.

1 similar comment
@miq-bot
Copy link
Member

miq-bot commented Jan 21, 2020

Some comments on commits NickLaMuro/manageiq@75ef1b6~...8820a90

spec/support/examples_group/with_ftp_server.rb

  • ⚠️ - 15 - Detected puts. Remove all debugging statements.
  • ⚠️ - 53 - Detected puts. Remove all debugging statements.

@miq-bot
Copy link
Member

miq-bot commented Jan 21, 2020

Checked commits NickLaMuro/manageiq@75ef1b6~...8820a90 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
25 files checked, 115 offenses detected

lib/miq_file_storage.rb

lib/miq_ftp_lib.rb

lib/miq_object_storage.rb

lib/mount/miq_generic_mount_session.rb

lib/mount/miq_glusterfs_session.rb

  • ⚠️ - Line 16, Col 17 - 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 25, Col 116 - Layout/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

lib/mount/miq_nfs_session.rb

  • ⚠️ - Line 15, Col 13 - Lint/UselessAssignment - Useless assignment to variable - userinfo. Use _ or _userinfo as a variable name to indicate that it won't be used.
  • ⚠️ - Line 15, Col 30 - Lint/UselessAssignment - Useless assignment to variable - port. Use _ or _port as a variable name to indicate that it won't be used.
  • ⚠️ - Line 15, Col 36 - Lint/UselessAssignment - Useless assignment to variable - registry. Use _ or _registry as a variable name to indicate that it won't be used.
  • ⚠️ - Line 15, Col 5 - Lint/UselessAssignment - Useless assignment to variable - scheme. Use _ or _scheme as a variable name to indicate that it won't be used.
  • ⚠️ - Line 15, Col 59 - Lint/UselessAssignment - Useless assignment to variable - opaque. Use _ or _opaque as a variable name to indicate that it won't be used.
  • ⚠️ - Line 15, Col 67 - Lint/UselessAssignment - Useless assignment to variable - query. Use _ or _query as a variable name to indicate that it won't be used.
  • ⚠️ - Line 15, Col 74 - Lint/UselessAssignment - Useless assignment to variable - fragment. Use _ or _fragment as a variable name to indicate that it won't be used.
  • ⚠️ - Line 15, Col 95 - 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 4, Col 11 - Style/MutableConstant - Freeze mutable objects assigned to constants.

lib/mount/miq_smb_session.rb

  • ⚠️ - Line 17, Col 13 - Lint/UselessAssignment - Useless assignment to variable - userinfo. Use _ or _userinfo as a variable name to indicate that it won't be used.
  • ⚠️ - Line 17, Col 30 - Lint/UselessAssignment - Useless assignment to variable - port. Use _ or _port as a variable name to indicate that it won't be used.
  • ⚠️ - Line 17, Col 36 - Lint/UselessAssignment - Useless assignment to variable - registry. Use _ or _registry as a variable name to indicate that it won't be used.
  • ⚠️ - Line 17, Col 5 - Lint/UselessAssignment - Useless assignment to variable - scheme. Use _ or _scheme as a variable name to indicate that it won't be used.
  • ⚠️ - Line 17, Col 59 - Lint/UselessAssignment - Useless assignment to variable - opaque. Use _ or _opaque as a variable name to indicate that it won't be used.
  • ⚠️ - Line 17, Col 67 - Lint/UselessAssignment - Useless assignment to variable - query. Use _ or _query as a variable name to indicate that it won't be used.
  • ⚠️ - Line 17, Col 74 - Lint/UselessAssignment - Useless assignment to variable - fragment. Use _ or _fragment as a variable name to indicate that it won't be used.
  • ⚠️ - Line 17, Col 95 - 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 4, Col 11 - Style/MutableConstant - Freeze mutable objects assigned to constants.

lib/object_storage/miq_ftp_storage.rb

lib/object_storage/miq_swift_storage.rb

spec/lib/miq_ftp_lib_spec.rb

spec/lib/mount/miq_local_mount_session_spec.rb

spec/lib/object_storage/miq_ftp_storage_spec.rb

spec/support/custom_matchers/exist_on_ftp_server.rb

spec/support/custom_matchers/have_size_on_ftp_server_of.rb

spec/support/examples_group/with_ftp_server.rb

@NickLaMuro
Copy link
Member Author

FYI to anyone stumbling across this:

We are probably going to table this for the time being since the manageiq-smartstate dependency is kind of gross, and having that depend on core just seems wrong, even if it is meant to be a short term solution.

Going to experiment with an alternative location for this code, but for right now, expect this to stay [WIP] while other options are explored.

@miq-bot
Copy link
Member

miq-bot commented May 7, 2020

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

@NickLaMuro
Copy link
Member Author

Since this just became unmergeable and #19742 (comment) why this most likely won't be used, going to close. Can always re-open if we change our minds.

@NickLaMuro NickLaMuro closed this May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.