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

Move snapshot code to Vm in service model. #12726

Merged
merged 1 commit into from
Nov 21, 2016

Conversation

lfu
Copy link
Member

@lfu lfu commented Nov 17, 2016

The snapshot code in automate was moved from VmOrTemplate to VMware specific class by PR #3707 when VMware was the only class that supported snapshot.

But snapshot support has been added to RHEVM and Openstack as of version Euwe.
The snapshot code in automate should be made available at Vm level.

Links

https://bugzilla.redhat.com/show_bug.cgi?id=1395175

cc @gmcculloug @mkanoor

@gmcculloug
Copy link
Member

@lfu You are now exposing these methods on every VM/Template which is not valid in a lot of cases. Before it was only exposed on VMs from automate and I think we should keep it at that level, just move it into the base VM service model.

Also, we should raise an error for providers where we do not support snapshot features with a "Feature not supported" error message.

@durandom I see there is a supports_snapshots? method. Are there granular checks we can use for the different operations?

For example, VMware supports create_snapshot, remove_all_snapshots, remove_snapshot and revert_to_snapshot. RHEV supports all except remove_all_snapshots. It would be nice to raise the proper error message based on the supports logic.

@lfu lfu force-pushed the ae_snapshot_1395175 branch 2 times, most recently from d417398 to 6738731 Compare November 18, 2016 17:59
@lfu lfu changed the title Move snapshot code to VmOrTemplate in service model. Move snapshot code to Vm in service model. Nov 18, 2016
end

def snapshot_operation(task, options = {})
options.merge!(:ids => [id], :task => task.to_s)
Copy link
Member

Choose a reason for hiding this comment

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

This does not provide any feedback to the caller if snapshots are not supported on the provider. Since we are moving from a specific provider implementation to a generic VM one we should add a call at the top of this method to raise an error if not supported.

LIke this (also address rubocop issues):

def snapshot_operation(task, options = {})
  raise "#{task} operation not supported for #{self.class.name}" unless @object.supports_snapshots?

  options[:ids]  = [id]
  options[:task] = task.to_s
  Vm.process_tasks(options)
end

@durandom
Copy link
Member

For example, VMware supports create_snapshot, remove_all_snapshots, remove_snapshot and revert_to_snapshot. RHEV supports all except remove_all_snapshots. It would be nice to raise the proper error message based on the supports logic.

I guess we would have to add all operations as features and then ask the Vm if it supports that operation. So far I only see snapshot features for openstack cloud_volumes and rh vms.

❯ git --no-pager grep -E 'supports(_not)? :.*snapshot'
app/models/manageiq/providers/openstack/cloud_manager/cloud_volume.rb:  supports :snapshot_create
app/models/manageiq/providers/redhat/infra_manager/vm/operations/snapshot.rb:    supports :snapshots do

@gmcculloug
Copy link
Member

@lfu Please review failing tests

@lfu
Copy link
Member Author

lfu commented Nov 21, 2016

@miq-bot add_label automate, bug, euwe/yes

The snapshot code in automate was moved from VmOrTemplate to VMware specific class by PR ManageIQ#3707 when VMware was the only class that supports snapshot.
But snapshot support has been added to RHEVM and Openstack as of version Euwe.
The snapshot code in automate should be available at Vm level.

https://bugzilla.redhat.com/show_bug.cgi?id=1395175
@miq-bot
Copy link
Member

miq-bot commented Nov 21, 2016

Checked commit lfu@3569ecf with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
4 files checked, 3 offenses detected

lib/miq_automation_engine/service_models/miq_ae_service_vm.rb

spec/lib/miq_automation_engine/service_methods/miq_ae_service_manageiq-providers-vmware-infra_manager-vm_spec.rb

@gmcculloug
Copy link
Member

Thanks @durandom. That seems outside of the scope of this PR so I am going to merge this and we can look into adding individual snapshot method checks later.

@gmcculloug gmcculloug merged commit eef6493 into ManageIQ:master Nov 21, 2016
@gmcculloug gmcculloug added this to the Sprint 50 Ending Dec 5, 2016 milestone Nov 21, 2016
simaishi pushed a commit that referenced this pull request Jan 6, 2017
Move snapshot code to Vm in service model.
(cherry picked from commit eef6493)

https://bugzilla.redhat.com/show_bug.cgi?id=1399207
@simaishi
Copy link
Contributor

simaishi commented Jan 6, 2017

Euwe backport details:

$ git log -1
commit 4547649a00d3b6d2e49cb31aac957548c01def32
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Mon Nov 21 12:17:23 2016 -0500

    Merge pull request #12726 from lfu/ae_snapshot_1395175
    
    Move snapshot code to Vm in service model.
    (cherry picked from commit eef649356ad7bf8742b3494ea69f71fba5d82a73)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1399207

@lfu lfu deleted the ae_snapshot_1395175 branch March 2, 2017 22:29
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.

5 participants