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.

Sign up for GitHub

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

Jump to bottom

handle volume snapshot status changes #439

Merged
merged 3 commits into from
Jun 13, 2018
Merged

handle volume snapshot status changes #439

merged 3 commits into from
Jun 13, 2018

Conversation

AlexanderZagaynov
Copy link
Contributor

@AlexanderZagaynov AlexanderZagaynov commented May 3, 2018 •
edited
Loading

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1561937
Related: ManageIQ/manageiq-content#285

Note:
AWS CloudWatch should send specific events.
To setup, one should create CloudWatch rule and target it to the AWSConfig_topic SNS topic.
This rule's event pattern should be:

{
  "source": [
    "aws.ec2"
  ],
  "detail-type": [
    "EBS Snapshot Notification"
  ]
}

More info: https://aws.amazon.com/ru/blogs/aws/new-cloudwatch-events-for-ebs-snapshots

@miq-bot miq-bot added the wip label May 3, 2018
@AlexanderZagaynov AlexanderZagaynov mentioned this pull request May 3, 2018
Merged
@Ladas
Copy link
Contributor

Ladas commented May 4, 2018 •
edited
Loading

@AlexanderZagaynov right, you have an event parser, that will store the event into our DB and Automate handler that will invoke target parsing and refresh.

So now we need the target parsing code:
https://github.com/Ladas/manageiq-providers-amazon/blob/4c66893fd1a91dc5fd3aa259a7c0ddd044c65576/app/models/manageiq/providers/amazon/cloud_manager/event_target_parser.rb#L26

With specs, verifying we can parse targets out of the event like:
https://github.com/Ladas/manageiq-providers-amazon/blob/a9a281f710075e348354e853614b30d8faed0460/spec/models/manageiq/providers/amazon/cloud_manager/event_target_parser_spec.rb#L176

Looks like we already parse snapshot as part of volumes?:
https://github.com/Ladas/manageiq-providers-amazon/blob/4c66893fd1a91dc5fd3aa259a7c0ddd044c65576/app/models/manageiq/providers/amazon/cloud_manager/event_target_parser.rb#L105

Then it looks like we already have targeted collector and perister for snapshot:
https://github.com/Ladas/manageiq-providers-amazon/blob/52636410835d6319627afd5e489cefe199e81a01/app/models/manageiq/providers/amazon/inventory/collector/target_collection.rb#L139

https://github.com/Ladas/manageiq-providers-amazon/blob/2b85fe1aa4bd65c4ff8933157cadce7740d4de50/app/models/manageiq/providers/amazon/inventory/persister/target_collection.rb#L62

And there is already a spec, we could just add a test for isolated snapshot:
https://github.com/Ladas/manageiq-providers-amazon/blob/240c429c52f3cda61067aa560d3efe157451229b/spec/models/manageiq/providers/amazon/cloud_manager/vcr_specs/targeted_refresh_spec.rb#L332

@AlexanderZagaynov AlexanderZagaynov force-pushed the BZ-1561937 branch 2 times, most recently from 7ded5d6 to 3526b0e Compare June 11, 2018 12:13
@AlexanderZagaynov AlexanderZagaynov changed the title [WIP] handle volume snapshot status changes handle volume snapshot status changes Jun 12, 2018
@AlexanderZagaynov
Copy link
Contributor Author

@Ladas please review

@miq-bot miq-bot removed the wip label Jun 12, 2018
Ladas
Ladas reviewed Jun 12, 2018
View reviewed changes
app/models/manageiq/providers/amazon/cloud_manager.rb Outdated
@@ -17,7 +17,7 @@ class ManageIQ::Providers::Amazon::CloudManager < ManageIQ::Providers::CloudMana
require_nested :Template
require_nested :Vm

OrchestrationTemplate.register_eligible_manager(self)
self::OrchestrationTemplate.register_eligible_manager(self)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was an issue, probably related to the rails dev env lazy code loading.
Rails was looking at the top level constant first, and there is no such method.
However, I forgot how to reproduce it now, so, reverting.

Ladas
Ladas reviewed Jun 12, 2018
View reviewed changes
app/models/manageiq/providers/amazon/storage_manager/ebs/cloud_volume_snapshot.rb Outdated
@@ -61,6 +61,14 @@ def self.create_snapshot(cloud_volume, options = {})

def raw_delete_snapshot
with_provider_object(&:delete)
update!(:status => 'deleting')
EmsRefresh.queue_refresh_task(
Copy link
Contributor

Choose a reason for hiding this comment

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

so, there is no event for deletion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep

Copy link
Contributor

Choose a reason for hiding this comment

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

@agrare is this ok? If the event is missing, I think we do not have much choice. (except full refresh)

Copy link
Member

Choose a reason for hiding this comment

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

Can we do a targeted refresh of the vm the cloud_volume is connected to? Not sure what amazon supports here for targets.
Also are you using the task that is returned? Can you just use EmsRefresh.queue_refresh?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's a volume snapshot, so Vm is only optional (volume doesn't have to be attached to anything)

Copy link
Member

Choose a reason for hiding this comment

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

Okay sounds like a full is the only way to go then.

Copy link
Contributor

Choose a reason for hiding this comment

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

And why not to just refresh the snapshot like it is now?

Copy link
Member

Choose a reason for hiding this comment

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

Oh we can target the snapshot? If we can do a targeted definitely lets do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agrare fixing queue_refresh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Ladas
Ladas approved these changes Jun 12, 2018
View reviewed changes
Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

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

Looks great 👍

@miq-bot
Copy link
Member

miq-bot commented Jun 12, 2018

Checked commits AlexanderZagaynov/manageiq-providers-amazon@3183efa~...b2a1a4b with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 0 offenses detected
Everything looks fine. 👍

@Ladas Ladas merged commit c9d39eb into ManageIQ:master Jun 13, 2018
@Ladas Ladas added this to the Sprint 88 Ending Jun 18, 2018 milestone Jun 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ladas Ladas Ladas approved these changes

@agrare agrare agrare approved these changes

Assignees

@Ladas Ladas

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AlexanderZagaynov @Ladas @miq-bot @agrare