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

Allow adding disks to vm provision via api and automation #13318

Merged
merged 1 commit into from Jan 17, 2017

Conversation

@masayag
Contributor

masayag commented Dec 26, 2016

Restful AP:

Provisioning of RHV vm via provision restful request is extended
to support disks creation.

The request format is detailed below:
diskscsi##=<controller_#>:<controller_position_#>:<size_in_mb> - The controller number and position are ignored for RHV.
diskscsi##.datastore=<datastore name>

Optional:
diskscsi##.filename = <filename>
diskscsi##.bootable = <true/false> Default: false
diskscsi##.backing.thinProvisioned = <true/false> Default: false
diskscsi##.interface = <VIRTIO/IDE/VIRTIO_SCSI/SPAPR_VSCSI> Default: VIRTIO

An example of the disks section as part of the provision request:

"vm_fields" : {
...
"diskscsi1" : "0:0:200",
"diskscsi1.datastore" : "data-40",
"diskscsi1.backing.thinProvisioned" : "true",
"diskscsi1.bootable" : "true",
"diskscsi1.filename" : "prov_disk_1",
"diskscsi1.interface": "IDE",
}

The result is creating a bootable disk named 'prov_disk_1' on datastore 'data-40'
with size of 200MB, thinly provisioned and controlled via 'IDE' driver.

Automation:

Same support is added for the automation as well to allow adding disks
to vm.

The signature of adding disk to a vm is exposed via
MiqAeServiceManageIQ_Providers_Redhat_InfraManager_Vm#add_disk(disk_name, disk_size_mb, options = {})

Where the 'options' expects:
thin_provisioned - <true/false> Default: false
bootable - <true/false> Default: false
datastore - the storage name to create the disk on. If not specified, the VM's storage is used.
interface - <VIRTIO/IDE/VIRTIO_SCSI/SPAPR_VSCSI> Default: VIRTIO

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

@masayag

This comment has been minimized.

Contributor

masayag commented Dec 26, 2016

@miq-bot add_label providers/rhevm

@masayag

This comment has been minimized.

Contributor

masayag commented Dec 26, 2016

@miq-bot add_label automate

@masayag

This comment has been minimized.

Contributor

masayag commented Dec 26, 2016

@borod108 please review

@miq-bot miq-bot added the automate label Dec 26, 2016

@borod108

I think its good, nice test coverage. Please look at the builder comment, except for that I think its good to go.

app/models/manageiq/providers/redhat/infra_manager.rb Outdated
}
# prepare disk attachment request payload of adding disk for reconfigure vm
def prepare_disk(disk_spec, storage)
disk_attachment_builder = DiskAttachmentBuilder.new

This comment has been minimized.

@borod108

borod108 Dec 26, 2016

Contributor

I think you should just pass options to the attachment builder (in the initializer) and encapsulate dealing with them there

This comment has been minimized.

@masayag

masayag Dec 26, 2016

Contributor

done - you're right. what's the point on having options in c'tor if not using them...

app/models/manageiq/providers/redhat/infra_manager.rb Outdated
storage = options[:datastore] || vm.storage
raise _("Data Store does not exist, unable to add disk") unless storage
disk_attachment_builder = DiskAttachmentBuilder.new

This comment has been minimized.

@borod108

borod108 Dec 26, 2016

Contributor

I think you should just pass options to the attachment builder (in the initializer) and encapsulate dealing with them there

This comment has been minimized.

@masayag

masayag Dec 26, 2016

Contributor

done

app/models/manageiq/providers/redhat/infra_manager/provision/disk.rb Outdated
raise MiqException::MiqProvisionError, "Unable to find storage: <#{storage_name}> for disk: <#{disk_spec.inspect}>"
end
disk_attachment_builder = ManageIQ::Providers::Redhat::InfraManager::DiskAttachmentBuilder.new

This comment has been minimized.

@borod108

borod108 Dec 26, 2016

Contributor

same

This comment has been minimized.

@masayag

masayag Dec 26, 2016

Contributor

done

@masayag

This comment has been minimized.

Contributor

masayag commented Dec 27, 2016

@durandom @gmcculloug could you review ?

@durandom

@gmcculloug can you or somebody from your team have a look at the automate/statemachine part?

Besides some minor questions this LGTM, and 👍 for adding many specs.

Maybe you want to revisit them though and check if you can use FactoryGirl.build instead of create in some places to save some DB calls

app/models/manageiq/providers/redhat/infra_manager/provision/disk.rb Outdated
disks = system_service.vms_service.vm_service(destination.uid_ems).disk_attachments_service.list
disks.each do |disk|
fetched_disk = system_service.disks_service.disk_service(disk.id).get
return true unless fetched_disk&.status == "ok"

This comment has been minimized.

@durandom

durandom Jan 3, 2017

Member

please use fetched_disk.try(:status) until we made the switch to 2.3

This comment has been minimized.

@masayag

masayag Jan 4, 2017

Contributor

done

app/models/manageiq/providers/redhat/infra_manager.rb Outdated
# prepare disk attachment request payload of adding disk for reconfigure vm
def prepare_disk(disk_spec, storage)
da_options = {
:size_in_mb => disk_spec["disk_size_in_mb"],

This comment has been minimized.

@durandom

durandom Jan 3, 2017

Member

is disk_spec containing symbols and strings as keys? Maybe call symbolize_keys on it?

This comment has been minimized.

@masayag

masayag Jan 4, 2017

Contributor

done

@@ -103,24 +103,44 @@ def vm_reconfigure(vm, options = {})
end
def add_disks(add_disks_spec, vm)
ems_storage_uid = add_disks_spec["ems_storage_uid"]

This comment has been minimized.

@durandom

durandom Jan 3, 2017

Member

how about backwards compatablity of this method?
I'm not sure from where it get's called though

This comment has been minimized.

@masayag

masayag Jan 4, 2017

Contributor

This method was added recently for the sake of reconfigure vm - the disk spec is created internally only from app/models/manageiq/providers/redhat/infra_manager/vm/reconfigure.rb:

def build_config_spec(task_options)
    {
      "numCoresPerSocket" => (task_options[:cores_per_socket].to_i if task_options[:cores_per_socket]),
      "memoryMB"          => (task_options[:vm_memory].to_i if task_options[:vm_memory]),
      "numCPUs"           => (task_options[:number_of_cpus].to_i if task_options[:number_of_cpus]),
      "disksRemove"       => task_options[:disk_remove],
      "disksAdd"          => (spec_for_added_disks(task_options[:disk_add]) if task_options[:disk_add])
    }
  end

Therefore this is a legit spec change, without any risk to backward compatibility

app/models/manageiq/providers/redhat/infra_manager/disk_attachment_builder.rb Outdated
end
def self.true?(obj)
obj.to_s == "true"

This comment has been minimized.

@durandom

durandom Jan 3, 2017

Member

is this validating text input? This feels non ruby idiomatic to me.
The ? methods are mostly inquiring the object it is called upon. So something like @thin_provisioned.true?

Would present? work here?

This comment has been minimized.

@masayag

masayag Jan 4, 2017

Contributor

present? won't work since the value might be string 'true' or boolean true or false/'false'.
I'll suggest an alternative on the next patch to comply with @thin_provisioned.true? / @bootable.true?

app/models/manageiq/providers/redhat/infra_manager/disk_attachment_builder.rb Outdated
end
def self.mb_to_gb(size)
size.to_i * 1024 * 1024

This comment has been minimized.

@durandom

This comment has been minimized.

@masayag

masayag Jan 4, 2017

Contributor

done.

app/models/manageiq/providers/redhat/infra_manager/disk_attachment_builder.rb Outdated
size.to_i * 1024 * 1024
end
FILE_STORAGE_TYPE = %w(NFS GLUSTERFS VMFS).freeze

This comment has been minimized.

@durandom

durandom Jan 3, 2017

Member

if its just for lookup I tend to %w(A B).to_set.freeze

This comment has been minimized.

@masayag

masayag Jan 4, 2017

Contributor

done

app/models/manageiq/providers/redhat/infra_manager/provision/disk.rb Outdated
signal :customize_guest
else
add_disks(requested_disks)
signal :poll_add_disks_complete

This comment has been minimized.

@durandom

durandom Jan 3, 2017

Member

@gmcculloug not sure, but shouldnt the signals be consistent across providers? Or somehow defined at a base class?

This comment has been minimized.

@gmcculloug

gmcculloug Jan 4, 2017

Member

No, this does not need to be consistent across providers. VMware supports adding disks which happens when the initial config spec is built. Here it is happening as a post-creations step. If we add support for another provider we could possible refactor this logic then.

@masayag I have two suggests:

  1. Moving this method to the state_machine.rb file to keep the signal logic together.
  2. Mark most of the methods in this file as private.
...ion_engine/service_methods/miq_ae_service_manageiq-providers-redhat-infra_manager-vm_spec.rb Outdated
let(:service_vm) { MiqAeMethodService::MiqAeServiceManageIQ_Providers_Redhat_InfraManager_Vm.find(vm.id) }
before do
allow_any_instance_of(Vm).to receive(:my_zone).and_return('default')

This comment has been minimized.

@durandom

durandom Jan 3, 2017

Member

Can you get rid of allow_any_instance_of and only mock vm and service_vm if needed?

This comment has been minimized.

@masayag

masayag Jan 4, 2017

Contributor

done

@masayag

I replaced as much FactoryGirl.create with FactoryGirl.build as I could.
I've addressed the comments. Thanks for the review.

@@ -103,24 +103,44 @@ def vm_reconfigure(vm, options = {})
end
def add_disks(add_disks_spec, vm)
ems_storage_uid = add_disks_spec["ems_storage_uid"]

This comment has been minimized.

@masayag

masayag Jan 4, 2017

Contributor

This method was added recently for the sake of reconfigure vm - the disk spec is created internally only from app/models/manageiq/providers/redhat/infra_manager/vm/reconfigure.rb:

def build_config_spec(task_options)
    {
      "numCoresPerSocket" => (task_options[:cores_per_socket].to_i if task_options[:cores_per_socket]),
      "memoryMB"          => (task_options[:vm_memory].to_i if task_options[:vm_memory]),
      "numCPUs"           => (task_options[:number_of_cpus].to_i if task_options[:number_of_cpus]),
      "disksRemove"       => task_options[:disk_remove],
      "disksAdd"          => (spec_for_added_disks(task_options[:disk_add]) if task_options[:disk_add])
    }
  end

Therefore this is a legit spec change, without any risk to backward compatibility

app/models/manageiq/providers/redhat/infra_manager.rb Outdated
# prepare disk attachment request payload of adding disk for reconfigure vm
def prepare_disk(disk_spec, storage)
da_options = {
:size_in_mb => disk_spec["disk_size_in_mb"],

This comment has been minimized.

@masayag

masayag Jan 4, 2017

Contributor

done

app/models/manageiq/providers/redhat/infra_manager/disk_attachment_builder.rb Outdated
end
def self.true?(obj)
obj.to_s == "true"

This comment has been minimized.

@masayag

masayag Jan 4, 2017

Contributor

present? won't work since the value might be string 'true' or boolean true or false/'false'.
I'll suggest an alternative on the next patch to comply with @thin_provisioned.true? / @bootable.true?

app/models/manageiq/providers/redhat/infra_manager/disk_attachment_builder.rb Outdated
end
def self.mb_to_gb(size)
size.to_i * 1024 * 1024

This comment has been minimized.

@masayag

masayag Jan 4, 2017

Contributor

done.

app/models/manageiq/providers/redhat/infra_manager/disk_attachment_builder.rb Outdated
size.to_i * 1024 * 1024
end
FILE_STORAGE_TYPE = %w(NFS GLUSTERFS VMFS).freeze

This comment has been minimized.

@masayag

masayag Jan 4, 2017

Contributor

done

app/models/manageiq/providers/redhat/infra_manager/provision/disk.rb Outdated
disks = system_service.vms_service.vm_service(destination.uid_ems).disk_attachments_service.list
disks.each do |disk|
fetched_disk = system_service.disks_service.disk_service(disk.id).get
return true unless fetched_disk&.status == "ok"

This comment has been minimized.

@masayag

masayag Jan 4, 2017

Contributor

done

...ion_engine/service_methods/miq_ae_service_manageiq-providers-redhat-infra_manager-vm_spec.rb Outdated
let(:service_vm) { MiqAeMethodService::MiqAeServiceManageIQ_Providers_Redhat_InfraManager_Vm.find(vm.id) }
before do
allow_any_instance_of(Vm).to receive(:my_zone).and_return('default')

This comment has been minimized.

@masayag

masayag Jan 4, 2017

Contributor

done

app/models/manageiq/providers/redhat/infra_manager/provision/disk.rb Outdated
signal :customize_guest
else
add_disks(requested_disks)
signal :poll_add_disks_complete

This comment has been minimized.

@gmcculloug

gmcculloug Jan 4, 2017

Member

No, this does not need to be consistent across providers. VMware supports adding disks which happens when the initial config spec is built. Here it is happening as a post-creations step. If we add support for another provider we could possible refactor this logic then.

@masayag I have two suggests:

  1. Moving this method to the state_machine.rb file to keep the signal logic together.
  2. Mark most of the methods in this file as private.
app/models/manageiq/providers/redhat/infra_manager/provision/disk.rb Outdated
false
end
def poll_add_disks_complete

This comment has been minimized.

@gmcculloug

gmcculloug Jan 4, 2017

Member

This method should also move to the state_machine.rb file.

This comment has been minimized.

@masayag

masayag Jan 10, 2017

Contributor

done.

Allow adding disks to vm provision via api and automation
Restful AP:
----------
Provisioning of RHV vm via provision restful request is extended
to support disks creation.

The request format is detailed below:

diskscsi##=<controller_#>:<controller_position_#>:<size_in_mb>
 - The controller number and position are ignored for RHV.
diskscsi##.datastore=<datastore name>

Optional:
diskscsi##.filename = <filename>
diskscsi##.bootable = <true/false> Default: false
diskscsi##.backing.thinProvisioned = <true/false> Default: false
diskscsi##.interface = <VIRTIO/IDE/VIRTIO_SCSI/SPAPR_VSCSI> Default: VIRTIO

An example of the disks section as part of the provision request:
"vm_fields" : {
    ...
    "diskscsi1" : "0:0:200",
    "diskscsi1.datastore" : "data-40",
    "diskscsi1.backing.thinProvisioned" : "true",
    "diskscsi1.bootable" : "true",
    "diskscsi1.filename" : "prov_disk_1",
    "diskscsi1.interface": "IDE",
  }

The result is creating a bootable disk named 'prov_disk_1' on datastore 'data-40'
with size of 200MB, thinly provisioned and controlled via 'IDE' driver.

Automation:
----------
Same support is added for the automation as well to allow adding disks
to vm.

The signature of adding disk to a vm is exposed via
MiqAeServiceManageIQ_Providers_Redhat_InfraManager_Vm#add_disk(disk_name, disk_size_mb, options = {})

Where the 'options' expects:
thin_provisioned - <true/false> Default: false
bootable         - <true/false> Default: false
datastore        - the storage name to create the disk on. If not specified,
                   the VM's storage is used.
interface        - <VIRTIO/IDE/VIRTIO_SCSI/SPAPR_VSCSI> Default: VIRTIO
@miq-bot

This comment has been minimized.

Member

miq-bot commented Jan 10, 2017

Checked commit masayag@c07d1d3 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
15 files checked, 3 offenses detected

app/models/manageiq/providers/redhat/infra_manager/provision/disk.rb

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

@masayag

This comment has been minimized.

Contributor

masayag commented Jan 10, 2017

@gmcculloug I've incorporated your comments into the PR, could you have a look ?

I verified this via provision request, vm reconfigure via the UI and via automation request.

@durandom

looks good from the provider side

@gmcculloug merge at your discretion

@masayag

This comment has been minimized.

Contributor

masayag commented Jan 12, 2017

@miq-bot add_label euwe/yes

@miq-bot miq-bot added the euwe/yes label Jan 12, 2017

@gmcculloug gmcculloug merged commit b1ded2f into ManageIQ:master Jan 17, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 42.175%
Details
@simaishi

This comment has been minimized.

Contributor

simaishi commented Jan 18, 2017

@masayag Can you please create a BZ if it doesn't already exist?

@masayag

This comment has been minimized.

Contributor

masayag commented Jan 19, 2017

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

Merge pull request #13318 from masayag/add_disks_via_restapi_provisio…
…n_vm_for_rhevm

Allow adding disks to vm provision via api and automation
(cherry picked from commit b1ded2f)

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

This comment has been minimized.

Contributor

simaishi commented Jan 19, 2017

Euwe backport details:

$ git log -1
commit 49322437d9d7a24735c67c944496bc8e900a9d0e
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Tue Jan 17 09:53:30 2017 -0500

    Merge pull request #13318 from masayag/add_disks_via_restapi_provision_vm_for_rhevm
    
    Allow adding disks to vm provision via api and automation
    (cherry picked from commit b1ded2f1c7292627505555c8c4637c98eb67e76a)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1414893

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

masayag added a commit to masayag/manageiq that referenced this pull request Feb 14, 2017

Disks should be added as 'active' in RHV
PR ManageIQ#13318 introduced a new
class to handle disk attachments. A disk should be added to RHV as
'active' since there is no method to modify it afterward.

DiskAttachmentBuilder class will set the 'active' as the default
status of the new disk, so any scenario which adds disks (vm
reconfigure, automation request, provision request) will share the same
behavior.

https://bugzilla.redhat.com/show_bug.cgi?id=1422145
@psachin

This comment has been minimized.

psachin commented Mar 2, 2017

Hello @masayag

What am I missing here?

irb(main):001:0> s = Vm.find_by(:name => "psachin-ext")
irb(main):005:0> s.add_disk("disk2", 2 * 1024, {:thin_provisioned => true, :datastore => s.storage_name})
NoMethodError: undefined method `vm_add_disk' for #<ManageIQ::Providers::Redhat::InfraManager:0x0000000c84d030>
	from /opt/rubies/ruby-2.3.1/lib/ruby/gems/2.3.0/gems/activemodel-5.0.0.1/lib/active_model/attribute_methods.rb:433:in `method_missing'
	from /var/www/miq/vmdb/app/models/vm_or_template.rb:330:in `run_command_via_parent'
	from /var/www/miq/vmdb/app/models/vm_or_template/operations/configuration.rb:76:in `raw_add_disk'
	from /var/www/miq/vmdb/app/models/vm_or_template/operations/configuration.rb:81:in `add_disk'
	from (irb):5
	from /opt/rubies/ruby-2.3.1/lib/ruby/gems/2.3.0/gems/railties-5.0.0.1/lib/rails/commands/console.rb:65:in `start'
	from /opt/rubies/ruby-2.3.1/lib/ruby/gems/2.3.0/gems/railties-5.0.0.1/lib/rails/commands/console_helper.rb:9:in `start'
	from /opt/rubies/ruby-2.3.1/lib/ruby/gems/2.3.0/gems/railties-5.0.0.1/lib/rails/commands/commands_tasks.rb:78:in `console'
	from /opt/rubies/ruby-2.3.1/lib/ruby/gems/2.3.0/gems/railties-5.0.0.1/lib/rails/commands/commands_tasks.rb:49:in `run_command!'
	from /opt/rubies/ruby-2.3.1/lib/ruby/gems/2.3.0/gems/railties-5.0.0.1/lib/rails/commands.rb:18:in `<top (required)>'
	from bin/rails:4:in `require'
	from bin/rails:4:in `<main>'

@miq-bot miq-bot added the euwe/yes label Mar 2, 2017

@simaishi simaishi removed the euwe/yes label Mar 2, 2017

@gmcculloug

This comment has been minimized.

Member

gmcculloug commented Mar 2, 2017

@psachin My guess is you are not running with these PR changes. The vm_add_disk method was added to the ManageIQ::Providers::Redhat::InfraManager object in this PR.

@psachin

This comment has been minimized.

psachin commented Mar 3, 2017

@gmcculloug Thanks. You were right, the changes were not merged. This PR has made adding additional disk to RHV so much easy using button.

👍 @masayag
👍 @gmcculloug

@masayag masayag deleted the masayag:add_disks_via_restapi_provision_vm_for_rhevm branch Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment