Introduce Amazon Block Storage Manager (EBS) #101

gberginc · 2017-01-11T21:01:26Z

This commit sets up the core structure of the new AWS EC2 block storage
manager. It provides just the basic stubs and tests that are to be
extended to allow for complete management of Amazon block storage

The refresher collects data about existing volumes and private
snapshots. Volumes are not yet linked to disks of virtual machines
collected by the cloud manager.

Depends on Add storage manager mixins PR from core repo.

@miq-bot add_label enhancement

gberginc · 2017-01-11T21:02:34Z

@aiperon please review

gberginc · 2017-01-12T19:45:43Z

@agrare @Ladas could ManageIQ/manageiq@d557f19 be affecting travis tests?

     Failure/Error:
       def initialize(inventory)
         @inventory             = inventory
         @inventory_collections = inventory.inventory_collections
       end
     
     ArgumentError:
       wrong number of arguments (given 2, expected 1)

Is there any doc on how to approach the refresh parsers now?

agrare · 2017-01-12T20:36:59Z

@gberginc yeah it is failing for one of my branches using InventoryObjectRefresh refresh also.
@Ladas can you change RefreshParserInventoryObject so it doesn't have to use the experimental inventory abstraction?

gberginc · 2017-01-12T20:41:47Z

Now that you mentioned another branch, I checked also other PRs in Amazon repo and noticed the one from @Ladas #102.

I guess it's best to wait until #102 is passing and then ensure storage manager is using the same approach.

miq-bot · 2017-01-13T09:47:35Z

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

This commit sets up the core structure of the new AWS EC2 block storage manager. It provides just the basic stubs and tests that are to be extended to allow for complete management of Amazon block storage The refresher collects data about existing volumes and private snapshots. Volumes are not yet linked to disks of virtual machines collected by the cloud manager. Signed-off-by: Gregor Berginc <gregor.berginc@xlab.si>

Ladas · 2017-01-13T10:04:58Z

@gberginc the CI should be fixed, but I've caused a conflict in the specs, you'll need to rebase

gberginc · 2017-01-13T10:12:24Z

@Ladas thanks. The conflict is simple to resolve as just the number of ext_management_systems is different. However I guess it would be better to use the same approach for storage manager as it is for cloud/network. I'll try to update the PR.

Another thing I wanted to ask: I currently only use stubbed specs, but there are also those that are using actual AWS account - how should I approach this as using our account will change the VCR as well as all the existing spec data.

Ladas · 2017-01-13T12:33:30Z

@gberginc cool

So the VCR kinda sucks there, because we have a VCR env, that we use for that. So option would be that I would record it for you, once you are ready, or we would share some readonly creds for the VCR env. So for now, I think easiest will be that I will record and send the VCRs, once you have all the API calls in place.

We were thinking about getting away from the VCRs and just use stubbed specs, but then it's nice to have a spec that is using real env. data.

gberginc · 2017-01-13T13:33:30Z

@Ladas I've just added additional commit for the inventory object support. I am not sure if you are the one to review this PR (if not, please let me know so that I can ask someone else).

Regarding VCR recording: do I understand correctly, I should now add EmsRefresh.refresh(@ems.block_storage_manager) everywhere VCR cassettes are used, for example:

https://github.com/ManageIQ/manageiq-providers-amazon/blob/master/spec/models/manageiq/providers/amazon/cloud_manager/refresher_inventory_object_spec.rb#L38

Then you run the spec and share the VCR with me so that I can update the actual tests?

Ladas · 2017-01-13T13:43:53Z

@gberginc I'll do the review

Regarding the VCR, yes, lets do the VCR specs as a next PR

chargio · 2017-01-16T17:16:53Z

Hey @Ladas, did you review it?

gberginc · 2017-01-16T18:38:46Z

I think this is partly dependent on the current changes to the refresh parsers, #105.

I would nevertheless appreciate feedback to continue working on storage managers.

aiperon · 2017-01-17T01:49:48Z

app/models/manageiq/providers/amazon/block_storage_manager/refresh_parser_inventory_object.rb

+ end
+
+ private
+ def get_volumes


We're using the same parsing code for RefreshParser and RefreshParserInventoryObject.
Wouldn't it be better if we move it into ManageIQ::Providers::Amazon::RefreshHelperMethods ?

there is a slight difference in the parsing code. But yeah, we can abstract the parsing part. But that can be done in another PR.

But my goal is to deprecate/delete the old parsing code and use the new one, so maybe it won't be needed. :-)

Ladas · 2017-01-17T08:51:12Z

app/models/manageiq/providers/amazon/block_storage_manager/refresh_parser_inventory_object.rb

+ end
+ end
+
+


extra blank lines

Ladas · 2017-01-17T08:51:43Z

app/models/manageiq/providers/amazon/block_storage_manager/refresh_parser_inventory_object.rb

+ inventory_collections
+ end
+
+ private


blank line before and after private

Ladas · 2017-01-17T08:54:04Z

app/models/manageiq/providers/amazon/cloud_manager.rb

+ block_storage_manager.name = "#{name} Block Storage Manager"
+ block_storage_manager.zone_id = zone_id
+ block_storage_manager.provider_region = provider_region
+ true


why we return true?

Ladas · 2017-01-17T08:56:08Z

app/models/manageiq/providers/amazon/cloud_manager.rb

@@ -54,6 +72,14 @@ def ensure_managers
 network_manager.provider_region = provider_region
 end

+ def ensure_block_storage_managers
+ build_block_storage_manager unless block_storage_manager


I think we can just put this body to ensure_managers. Or calling this from the body of ensure_managers, then we should extract the current into :ensure_network_manager, then add :ensure_block_storage_manager

@Ladas I've merged the two into ensure_managers as there is currently no need to have them separated. I also fixed the other rubocop issues.

Ladas · 2017-01-17T08:59:53Z

@gberginc just few nits, mostly rubocop that should be fixed. After that I am ready to merge, so we can move forward. :-)

Similarly to ManageIQ#102 this patch adds support for inventory collection using inventory abstraction. Corresponding targets as well as the refresh parser inventory object. It mimics existing cloud and network managers. Signed-off-by: Gregor Berginc <gregor.berginc@xlab.si>

miq-bot · 2017-01-17T09:27:54Z

Checked commits gberginc/manageiq-providers-amazon@db2a32f~...66465a6 with ruby 2.2.6, rubocop 0.46.0, and haml-lint 0.19.0
18 files checked, 17 offenses detected

app/models/manageiq/providers/amazon/block_storage_manager/refresh_parser.rb

❗ - Line 24, Col 7 - Style/AccessorMethodName - Do not prefix reader method names with get_.
❗ - Line 29, Col 7 - Style/AccessorMethodName - Do not prefix reader method names with get_.

app/models/manageiq/providers/amazon/block_storage_manager/refresh_parser_inventory_object.rb

❗ - Line 17, Col 7 - Style/AccessorMethodName - Do not prefix reader method names with get_.
❗ - Line 21, Col 7 - Style/AccessorMethodName - Do not prefix reader method names with get_.
❕ - Line 1, Col 121 - Metrics/LineLength - Line is too long. [133/120]
❕ - Line 22, Col 121 - Metrics/LineLength - Line is too long. [123/120]
❕ - Line 5, Col 121 - Metrics/LineLength - Line is too long. [136/120]

app/models/manageiq/providers/amazon/block_storage_manager/refresher.rb

❗ - Line 13, Col 21 - Style/EmptyElse - Redundant else-clause.
❕ - Line 29, Col 121 - Metrics/LineLength - Line is too long. [134/120]

app/models/manageiq/providers/amazon/inventory/targets/block_storage_manager.rb

❕ - Line 1, Col 121 - Metrics/LineLength - Line is too long. [124/120]

spec/models/manageiq/providers/amazon/aws_stubs.rb

❗ - Line 367, Col 36 - Rails/TimeZone - Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.to_i, Time.now.to_f instead.
❗ - Line 384, Col 30 - Rails/TimeZone - Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.to_i, Time.now.to_f instead.

spec/models/manageiq/providers/amazon/block_storage_manager/stubbed_refresher_spec.rb

❗ - Line 19, Col 6 - Style/MultilineArrayBraceLayout - Closing array brace must be on the same line as the last array element when opening brace is on the same line as the first array element.
❕ - Line 130, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for assert_table_counts is too high. [40.01/20]
❕ - Line 130, Col 3 - Metrics/MethodLength - Method has too many lines. [39/25]
❕ - Line 176, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for assert_ems is too high. [25.02/20]
❕ - Line 86, Col 3 - Metrics/MethodLength - Method has too many lines. [38/25]

gberginc · 2017-01-17T12:17:20Z

@Ladas I amended the commit, so the following comment is not visible. Just reposting to make sure it doesn't get lost.

I've merged the two into ensure_managers as there is currently no need to have them separated. I also fixed the other rubocop issues - I think the remaining issues can be left as they are following existing code.

Ladas · 2017-01-17T12:40:58Z

@gberginc awesome :-) Thank you for a great contribution.

Yeah, the rest of the robocop warnings are on the edge of a correctness. :-)

chargio · 2017-01-17T15:48:58Z

👍

roliveri · 2017-01-17T16:14:05Z

@gberginc @Ladas @Fryguy Actually, this shouldn't have been merged yet. We've been naming managers based on concrete names instead of type. This enables providers to support different/multiple implementations of the same type of manager. So, I would suggest a name change.

From:

ManageIQ::Providers::Amazon::BlockStorageManager < ManageIQ::Providers::StorageManager

To:

ManageIQ::Providers::Amazon:: StorageManager::EBS < ManageIQ::Providers::StorageManager

gberginc · 2017-01-17T16:27:18Z

@roliveri do you think we could proceed with the pending PRs on the manageiq repo and then introduce this change now that it's merged?

roliveri · 2017-01-17T16:47:58Z

@gberginc Is there a benefit in doing that? It seems that 2 of the pending PRs rely on the same naming scheme:

ManageIQ/manageiq#13458
ManageIQ/manageiq#13457

So I would make the same comment on those. If I merge those, you'll only have to submit new PRs updating those as well.

gberginc · 2017-01-17T16:52:43Z

@roliveri the only consideration was that ManageIQ/manageiq was failing tests because of this merge. I though it would be faster to make the existing PRs merged and then do a followup patch that would fix the names.

roliveri · 2017-01-17T18:52:30Z

@gberginc Ah, i see. It looks like ManageIQ/manageiq#13458 has been merged, and doing so has fixed the test failures.

So, ManageIQ/manageiq#13457 remains. I can hold off merging that until the names are fixed, or I can merge it, but then I'd like for you to open a Github issue for the name change - just to make sure it doesn't get overlooked.

let me know which way you'd like to go.

gberginc · 2017-01-17T19:32:07Z

@roliveri I already started on renaming the class. I therefore suggest you wait with the worker as it is not required for functional test.

miq-bot added the enhancement label Jan 11, 2017

This was referenced Jan 11, 2017

Add Amazon block storage refresh worker class ManageIQ/manageiq#13457

Closed

Add Amazon block storage automation models ManageIQ/manageiq#13458

Merged

gberginc force-pushed the introduce_ebs_manager branch 2 times, most recently from e206395 to 45b0bc7 Compare January 11, 2017 21:39

gberginc mentioned this pull request Jan 11, 2017

Add assets for EC2 block and storage managers ManageIQ/manageiq-ui-classic#133

Merged

gberginc force-pushed the introduce_ebs_manager branch from 45b0bc7 to 13d5e74 Compare January 12, 2017 19:21

Ladas self-assigned this Jan 13, 2017

miq-bot added the unmergeable label Jan 13, 2017

gberginc force-pushed the introduce_ebs_manager branch from 13d5e74 to 3684e11 Compare January 13, 2017 13:23

miq-bot removed the unmergeable label Jan 13, 2017

aiperon reviewed Jan 17, 2017

View reviewed changes

aiperon mentioned this pull request Jan 17, 2017

Introduce Amazon S3 StorageManager #106

Merged

Ladas reviewed Jan 17, 2017

View reviewed changes

app/models/manageiq/providers/amazon/block_storage_manager/refresh_parser_inventory_object.rb

end

end

Copy link

Contributor

Ladas Jan 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra blank lines

Ladas reviewed Jan 17, 2017

View reviewed changes

gberginc force-pushed the introduce_ebs_manager branch from 3684e11 to 6e048be Compare January 17, 2017 09:15

gberginc force-pushed the introduce_ebs_manager branch from 6e048be to 66465a6 Compare January 17, 2017 09:23

Ladas merged commit 599a744 into ManageIQ:master Jan 17, 2017

Ladas added the euwe/no label Jan 17, 2017

gberginc mentioned this pull request Jan 17, 2017

Rename Amazon block storage manager #107

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Amazon Block Storage Manager (EBS) #101

Introduce Amazon Block Storage Manager (EBS) #101

gberginc commented Jan 11, 2017

gberginc commented Jan 11, 2017

gberginc commented Jan 12, 2017 •

edited

Loading

agrare commented Jan 12, 2017

gberginc commented Jan 12, 2017

miq-bot commented Jan 13, 2017

Ladas commented Jan 13, 2017

gberginc commented Jan 13, 2017

Ladas commented Jan 13, 2017

gberginc commented Jan 13, 2017

Ladas commented Jan 13, 2017

chargio commented Jan 16, 2017

gberginc commented Jan 16, 2017

aiperon Jan 17, 2017

Ladas Jan 17, 2017

Ladas Jan 17, 2017

Ladas Jan 17, 2017

Ladas Jan 17, 2017

Ladas Jan 17, 2017

gberginc Jan 17, 2017

Ladas commented Jan 17, 2017

miq-bot commented Jan 17, 2017

gberginc commented Jan 17, 2017

Ladas commented Jan 17, 2017

chargio commented Jan 17, 2017

roliveri commented Jan 17, 2017

gberginc commented Jan 17, 2017

roliveri commented Jan 17, 2017

gberginc commented Jan 17, 2017

roliveri commented Jan 17, 2017

gberginc commented Jan 17, 2017

Introduce Amazon Block Storage Manager (EBS) #101

Introduce Amazon Block Storage Manager (EBS) #101

Conversation

gberginc commented Jan 11, 2017

gberginc commented Jan 11, 2017

gberginc commented Jan 12, 2017 • edited Loading

agrare commented Jan 12, 2017

gberginc commented Jan 12, 2017

miq-bot commented Jan 13, 2017

Ladas commented Jan 13, 2017

gberginc commented Jan 13, 2017

Ladas commented Jan 13, 2017

gberginc commented Jan 13, 2017

Ladas commented Jan 13, 2017

chargio commented Jan 16, 2017

gberginc commented Jan 16, 2017

aiperon Jan 17, 2017

Choose a reason for hiding this comment

Ladas Jan 17, 2017

Choose a reason for hiding this comment

Ladas Jan 17, 2017

Choose a reason for hiding this comment

Ladas Jan 17, 2017

Choose a reason for hiding this comment

Ladas Jan 17, 2017

Choose a reason for hiding this comment

Ladas Jan 17, 2017

Choose a reason for hiding this comment

gberginc Jan 17, 2017

Choose a reason for hiding this comment

Ladas commented Jan 17, 2017

miq-bot commented Jan 17, 2017

gberginc commented Jan 17, 2017

Ladas commented Jan 17, 2017

chargio commented Jan 17, 2017

roliveri commented Jan 17, 2017

gberginc commented Jan 17, 2017

roliveri commented Jan 17, 2017

gberginc commented Jan 17, 2017

roliveri commented Jan 17, 2017

gberginc commented Jan 17, 2017

gberginc commented Jan 12, 2017 •

edited

Loading