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

Projects
None yet
5 participants
@lfu
Copy link
Member

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

This comment has been minimized.

Copy link
Member

gmcculloug commented Nov 17, 2016

@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 lfu:ae_snapshot_1395175 branch 2 times, most recently Nov 18, 2016

@lfu lfu changed the title Move snapshot code to VmOrTemplate in service model. Move snapshot code to Vm in service model. Nov 18, 2016

lib/miq_automation_engine/service_models/miq_ae_service_vm.rb Outdated
end

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

This comment has been minimized.

Copy link
@gmcculloug

gmcculloug Nov 18, 2016

Member

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

@lfu lfu force-pushed the lfu:ae_snapshot_1395175 branch Nov 18, 2016

@durandom

This comment has been minimized.

Copy link
Member

durandom commented Nov 21, 2016

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

This comment has been minimized.

Copy link
Member

gmcculloug commented Nov 21, 2016

@lfu Please review failing tests

@lfu lfu force-pushed the lfu:ae_snapshot_1395175 branch Nov 21, 2016

@lfu

This comment has been minimized.

Copy link
Member Author

lfu commented Nov 21, 2016

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

Move snapshot code to Vm in service model.
The snapshot code in automate was moved from VmOrTemplate to VMware specific class by PR #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

@lfu lfu force-pushed the lfu:ae_snapshot_1395175 branch to 3569ecf Nov 21, 2016

@miq-bot

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

gmcculloug commented Nov 21, 2016

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

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.0002%) to 34.899%
Details

simaishi added a commit that referenced this pull request Jan 6, 2017

Merge pull request #12726 from lfu/ae_snapshot_1395175
Move snapshot code to Vm in service model.
(cherry picked from commit eef6493)

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

This comment has been minimized.

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

@simaishi simaishi added euwe/backported and removed euwe/yes labels Jan 6, 2017

@lfu lfu deleted the lfu:ae_snapshot_1395175 branch Mar 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.