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

Support publish VM #95

Merged
merged 1 commit into from
Oct 2, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,8 @@ def api4_supported_features
:migrate,
:quick_stats,
:reconfigure_disks,
:snapshots
:snapshots,
:publish
]
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,14 @@ def clone_completed?(args)
phase_context = args[:phase_context]
logger = args[:logger]

if source.template?
vm_clone_completed?(logger, phase_context, source)
else
template_clone_completed?(logger, phase_context, source)
end
end

def vm_clone_completed?(logger, phase_context, source)
source.with_provider_connection(VERSION_HASH) do |connection|
vm = vm_service_by_href(phase_context[:new_vm_ems_ref], connection).get
status = vm.status
Expand All @@ -44,6 +52,15 @@ def clone_completed?(args)
end
end

def template_clone_completed?(logger, phase_context, source)
source.with_provider_connection(VERSION_HASH) do |connection|
template = template_service_by_href(phase_context[:new_vm_ems_ref], connection).get
status = template.status
logger.info("The status of the template being cloned is #{status}")
status == OvirtSDK4::TemplateStatus::OK
end
end

def destination_image_locked?(vm)
vm.with_provider_object(VERSION_HASH) do |vm_proxy|
vm_proxy.get.status == OvirtSDK4::VmStatus::IMAGE_LOCKED
Expand Down Expand Up @@ -170,7 +187,7 @@ def get_template_proxy(template, connection)
end

def get_vm_proxy(vm, connection)
VmProxyDecorator.new(connection.system_service.vms_service.vm_service(vm.uid_ems), self)
VmProxyDecorator.new(connection.system_service.vms_service.vm_service(vm.uid_ems), connection, self)
end

def collect_disks_by_hrefs(disks)
Expand Down Expand Up @@ -205,6 +222,13 @@ def start_clone(source, clone_options, phase_context)
end
end

def make_template(source, clone_options, phase_context)
source.with_provider_object(VERSION_HASH) do |rhevm_vm|
template = rhevm_vm.make_template(clone_options)
populate_phase_context(phase_context, template)
end
end

def vm_start(vm, cloud_init)
opts = {}
vm.with_provider_object(VERSION_HASH) do |rhevm_vm|
Expand Down Expand Up @@ -298,11 +322,13 @@ def host_deactivate(host)
end

class VmProxyDecorator < SimpleDelegator
attr_reader :service
def initialize(vm, service)
@obj = vm
@service = service
super(vm)
attr_reader :ovirt_services, :connection

def initialize(vm_service, connection, ovirt_services)
@obj = vm_service
@connection = connection
@ovirt_services = ovirt_services
super(vm_service)
end

def update_memory_reserve!(memory_reserve_size)
Expand All @@ -326,7 +352,7 @@ def update_memory!(memory, limit)

def update_host_affinity!(dest_host_ems_ref)
vm = get
vm.placement_policy.hosts = [OvirtSDK4::Host.new(:id => service.uuid_from_href(dest_host_ems_ref))]
vm.placement_policy.hosts = [OvirtSDK4::Host.new(:id => ovirt_services.uuid_from_href(dest_host_ems_ref))]
update(vm)
end

Expand Down Expand Up @@ -405,6 +431,33 @@ def update_cloud_init!(content)
)
)
end

def make_template(options)
templates_service = connection.system_service.templates_service
cluster = ovirt_services.cluster_from_href(options[:cluster], connection)
if options[:storage]
storage = ovirt_services.storage_from_href(options[:storage], connection)
end
vm = get
vm.disk_attachments = connection.follow_link(vm.disk_attachments)
template = build_template_from_hash(:name => options[:name],
:vm => vm,
:description => options[:description],
:cluster => cluster,
:storage => storage)
templates_service.add(template)
end

def build_template_from_hash(args)
options = {
:name => args[:name],
:description => args[:description],
:vm => args[:vm],
:cluster => args[:cluster],
:storage_domain => args[:storage] && {:id => args[:storage].id}
}.compact
OvirtSDK4::Template.new(options)
end
end

class TemplateProxyDecorator < SimpleDelegator
Expand Down Expand Up @@ -437,6 +490,10 @@ def cluster_from_href(href, connection)
connection.system_service.clusters_service.cluster_service(uuid_from_href(href)).get
end

def storage_from_href(href, connection)
Copy link
Contributor

Choose a reason for hiding this comment

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

We already fetch storage domain in different place. I think that we start to have similar responsibilities in inventory and ovirt_services. In my opinion there is no need to have two distinct object having the same responsibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we'd might access RHV from different places of the provider. We can think of introducing a class that will be responsible for interacting between the provider to RHV via v4, I'm not sure this is the PR to introduce such.

We should first get into agreement if we'd prefer that method over letting each workflow/scenario interact independently using the engine's connection.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not saying that we should fix it in this PR. I am saying that we already have similar code in different place. I think it would be better to define responsibilities of the classes we use to talk to the engine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll track refactoring of ovirt_services and querying of engine's request by issue #100

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks

connection.system_service.storage_domains_service.storage_domain_service(uuid_from_href(href)).get
end

def uuid_from_href(ems_ref)
URI(ems_ref).path.split('/').last
end
Expand Down Expand Up @@ -476,6 +533,11 @@ def vm_service_by_href(href, connection)
connection.system_service.vms_service.vm_service(vm_uuid)
end

def template_service_by_href(href, connection)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

template_uuid = uuid_from_href(href)
connection.system_service.templates_service.template_service(template_uuid)
end

#
# Updates the amount memory of a virtual machine.
#
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,31 @@ def destination_image_locked?
end

def find_destination_in_vmdb(ems_ref)
ManageIQ::Providers::Redhat::InfraManager::Vm.find_by(:name => dest_name, :ems_ref => ems_ref)
if source.template?
ManageIQ::Providers::Redhat::InfraManager::Vm.find_by(:name => dest_name, :ems_ref => ems_ref)
else
ManageIQ::Providers::Redhat::InfraManager::Template.find_by(:name => dest_name, :ems_ref => ems_ref)
end
end

def prepare_for_clone_task
raise MiqException::MiqProvisionError, "Provision Request's Destination VM Name=[#{dest_name}] cannot be blank" if dest_name.blank?
raise MiqException::MiqProvisionError, "A VM with name: [#{dest_name}] already exists" if source.ext_management_system.vms.where(:name => dest_name).any?

prepare_clone_options
end

def prepare_clone_options
if source.template?
# return the options for creating a vm from the template
vm_clone_options
else
# return the options for creating a template from the vm
template_clone_options
end
end

def vm_clone_options
clone_options = {
:name => dest_name,
:cluster => dest_cluster.ems_ref,
Expand All @@ -27,6 +45,18 @@ def prepare_for_clone_task
clone_options
end

def template_clone_options
clone_options = {
:name => dest_name,
:cluster => dest_cluster.ems_ref,
:description => get_option(:vm_description),
:seal => get_option(:seal)
}

clone_options[:storage] = dest_datastore.ems_ref unless dest_datastore.nil?
clone_options
end

def sparse_disk_value
case get_option(:disk_format)
when "preallocated" then false
Expand All @@ -42,13 +72,20 @@ def log_clone_options(clone_options)
_log.info("Destination VM Name: [#{clone_options[:name]}]")
_log.info("Destination Cluster: [#{dest_cluster.name} (#{dest_cluster.ems_ref})]")
_log.info("Destination Datastore: [#{dest_datastore.name} (#{dest_datastore.ems_ref})]") unless dest_datastore.nil?
_log.info("Seal: [#{clone_options[:seal]}]") unless source.template?

dump_obj(clone_options, "#{_log.prefix} Clone Options: ", $log, :info)
dump_obj(options, "#{_log.prefix} Prov Options: ", $log, :info, :protected => {:path => workflow_class.encrypted_options_field_regs})
end

def start_clone(clone_options)
ems.ovirt_services.start_clone(source, clone_options, phase_context)
if source.template?
# create a vm from the template
ems.ovirt_services.start_clone(source, clone_options, phase_context)
else
# create a template from the vm
ems.ovirt_services.make_template(source, clone_options, phase_context)
end
end

def ems
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,19 @@ def poll_clone_complete
end

def customize_destination
if destination_image_locked?
_log.info("Destination image locked; re-queuing")
requeue_phase
if source.template?
if destination_image_locked?
_log.info("Destination image locked; re-queuing")
requeue_phase
else
message = "Starting New #{destination_type} Customization"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we use capital letter for "new" and "customization"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

developer excuses #211: "it was like that before"

seems like other providers (vmware and microsoft) have the exact message (with the capitals)

_log.info("#{message} #{for_destination}")
update_and_notify_parent(:message => message)
configure_container
signal :configure_disks
end
else
message = "Starting New #{destination_type} Customization"
_log.info("#{message} #{for_destination}")
update_and_notify_parent(:message => message)
configure_container
signal :configure_disks
signal :post_provision
end
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,4 +135,13 @@ def validate_memory_limit(_field, values, dlg, fld, _value)
_("%{description} VM Memory is larger than Memory Limit") % {:description => required_description(dlg, fld)}
end
end

def validate_seal_template(_field, values, _dlg, _fld, _value)
seal = get_value(values[:seal])
return nil unless seal

if get_source_vm.platform == 'windows'
_("Template sealing is supported only for non-Windows OS.")
end
end
end
12 changes: 11 additions & 1 deletion app/models/manageiq/providers/redhat/infra_manager/vm.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,18 @@ class ManageIQ::Providers::Redhat::InfraManager::Vm < ManageIQ::Providers::Infra
end
end

supports_not :publish
supports_not :reset
supports :publish do
if blank? || orphaned? || archived?
unsupported_reason_add(:publish, _('Publish operation in not supported'))
elsif ext_management_system.blank?
unsupported_reason_add(:publish, _('The virtual machine is not associated with a provider'))
elsif !ext_management_system.supports_publish?
unsupported_reason_add(:publish, _('This feature is not supported by the api version of the provider'))
elsif power_state != "off"
unsupported_reason_add(:publish, _('The virtual machine must be down'))
end
end

POWER_STATES = {
'up' => 'on',
Expand Down
44 changes: 44 additions & 0 deletions spec/models/manageiq/providers/redhat/infra_manager/vm_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,50 @@
end
end

describe "#supports_publish?" do
context "when vm has no storage" do
let(:vm) { FactoryGirl.create(:vm_redhat, :storage => nil, :ext_management_system => nil) }

it "does not support publish" do
expect(vm.supports_publish?).to be_falsey
end
end

context "when vm has no ems" do
let(:storage) { FactoryGirl.create(:storage_nfs, :ems_ref => "http://example.com/storages/XYZ") }
let(:vm) { FactoryGirl.create(:vm_redhat, :storage => storage, :ext_management_system => nil) }

it "does not support publish" do
expect(vm.supports_publish?).to be_falsey
end
end

context "when vm is not in down state" do
let(:storage) { FactoryGirl.create(:storage_nfs, :ems_ref => "http://example.com/storages/XYZ") }
let(:ems) { FactoryGirl.create(:ems_redhat_with_authentication) }
let(:vm) { FactoryGirl.create(:vm_redhat, :ext_management_system => ems, :storage => storage) }

it "does not support publish" do
allow(vm).to receive(:power_state).and_return("on")

expect(vm.supports_publish?).to be_falsey
end
end

context "when vm is down" do
let(:storage) { FactoryGirl.create(:storage_nfs, :ems_ref => "http://example.com/storages/XYZ") }
let(:ems) { FactoryGirl.create(:ems_redhat_with_authentication) }
let(:vm) { FactoryGirl.create(:vm_redhat, :ext_management_system => ems, :storage => storage) }

it "does support publish" do
allow(ems).to receive(:supported_api_versions).and_return([4])
allow(vm).to receive(:power_state).and_return("off")

expect(vm.supports_publish?).to be_truthy
end
end
end

describe "#disconnect_storage" do
before(:each) do
_, _, zone = EvmSpecHelper.create_guid_miq_server_zone
Expand Down