-
Notifications
You must be signed in to change notification settings - Fork 18
use dataVolumeTemplates to perform image-based provisioning #178
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
Changes from all commits
d9e318b
eb54a9c
799b83f
43f58ef
20b3aae
abfd1c7
ee62ff1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -183,13 +183,14 @@ def create_vm(args = {}) | |
| user_data = { "userData" => options[:user_data] } if options[:user_data].present? | ||
|
|
||
| begin | ||
| volumes = create_volumes_for_vm(options) | ||
| volumes, volume_templates = create_volumes_for_vm(options) | ||
| interfaces, networks = create_network_devices_for_vm(options, volumes) | ||
| client.vms.create(:vm_name => options[:name], | ||
| :cpus => options[:cpu_cores].to_i, | ||
| :memory_size => convert_memory(options[:memory] + "b", :mi).to_s, | ||
| :memory_unit => "Mi", | ||
| :volumes => volumes, | ||
| :volume_templates => volume_templates, | ||
| :cloudinit => user_data, | ||
| :networks => networks, | ||
| :interfaces => interfaces) | ||
|
|
@@ -221,7 +222,8 @@ def host_compute_attrs(host) | |
| def vm_instance_defaults | ||
| { | ||
| :memory => 1024.megabytes.to_s, | ||
| :cpu_cores => '1' | ||
| :cpu_cores => '1', | ||
| :volumes => [new_volume].compact, | ||
| } | ||
| end | ||
|
|
||
|
|
@@ -376,22 +378,42 @@ def verify_at_least_one_volume_provided(options) | |
| (volumes_attributes.present? || image) | ||
| end | ||
|
|
||
| def verify_booting_from_image_is_possible(volumes) | ||
| raise ::Foreman::Exception.new _('It is not possible to set a bootable volume and image based provisioning.') if | ||
| volumes&.any? { |_, v| v[:bootable] == "true" } | ||
| def add_volume_for_image_provision(options) | ||
| volume = Fog::Kubevirt::Compute::Volume.new | ||
| volume.config = { name: rootdisk_name(options) } | ||
| volume.boot_order = 1 | ||
| volume.name = 'rootdisk' | ||
| volume.type = 'dataVolume' | ||
| volume | ||
| end | ||
|
|
||
| def add_volume_for_image_provision(options) | ||
| def add_volume_template_for_image_provision(options) | ||
| image = options[:image_id] | ||
| raise ::Foreman::Exception.new _('VM should be created based on an image') unless image | ||
|
|
||
| verify_booting_from_image_is_possible(options[:volumes_attributes]) | ||
| namespace, name = image.split('/', 2) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When creating the image, I believe we should update the tooltip to include an option to specify whether to include a namespace. Also, if the token works in other workspaces that are not configured on the compute resource, it works there as well. This edge case could be mentioned in the docs only. The API help text will need to be updated as well.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the help in the UI. The API is not altered by the plugin (as the field comes from core), so can't be done there. |
||
| if name.nil? | ||
| name = namespace | ||
| namespace = nil | ||
| end | ||
|
|
||
| volume = Fog::Kubevirt::Compute::Volume.new | ||
| volume.info = image | ||
| volume.boot_order = 1 | ||
| volume.type = 'containerDisk' | ||
| volume | ||
| volumes_attributes = options.fetch(:volumes_attributes, {}) | ||
| _, boot_volume = volumes_attributes.find { |_, vol| vol[:bootable] == 'true' } | ||
| raise ::Foreman::Exception.new _('A bootable volume is required as a target for the image') unless boot_volume | ||
| capacity = boot_volume[:capacity] | ||
| capacity += "G" unless capacity.end_with? "G" | ||
| storage_class = boot_volume[:storage_class] | ||
| storage = { resources: { requests: { storage: capacity } } } | ||
| storage[:storageClassName] = storage_class if storage_class.present? | ||
|
|
||
| source_ref = { kind: 'DataSource', name: name, namespace: namespace }.compact | ||
| metadata = { name: rootdisk_name(options) } | ||
| { kind: 'DataVolume', metadata: metadata, spec: { sourceRef: source_ref, storage: storage } } | ||
| end | ||
|
|
||
| def rootdisk_name(options) | ||
| vm_name = options[:name].parameterize | ||
| "#{vm_name}-root" | ||
| end | ||
|
|
||
| def validate_volume_capacity(volumes_attributes) | ||
|
|
@@ -443,6 +465,8 @@ def add_volumes_based_on_pvcs(options, image_provision) | |
| volumes = [] | ||
| vm_name = options[:name].gsub(/[._]+/, '-') | ||
| volumes_attributes.each_with_index do |(_, v), index| | ||
| # skip if this is a boot volume for image provisioning | ||
| next if image_provision && v[:bootable] | ||
| # Add PVC as volumes to the virtual machine | ||
| pvc_name = vm_name + "-claim-" + (index + 1).to_s | ||
| capacity = v[:capacity] | ||
|
|
@@ -465,10 +489,15 @@ def create_volumes_for_vm(options) | |
|
|
||
| # Add image as volume to the virtual machine | ||
| volumes = [] | ||
| volume_templates = [] | ||
| image_provision = options[:provision_method] == "image" | ||
|
|
||
| volumes << add_volume_for_image_provision(options) if image_provision | ||
| volumes.concat(add_volumes_based_on_pvcs(options, image_provision)) | ||
|
|
||
| volume_templates << add_volume_template_for_image_provision(options) if image_provision | ||
|
|
||
| [volumes, volume_templates] | ||
| end | ||
|
|
||
| def create_pod_network_element | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| <%= text_f f, :username, :value => @image.username || "root", :help_inline => _("The user that is used to ssh into the instance, normally cloud-user, ec2-user, ubuntu, root etc") %> | ||
| <%= password_f f, :password, :help_inline => _("Password to authenticate with - used for SSH finish step.") %> | ||
| <%= checkbox_f f, :user_data, :help_inline => _("Does this image support user data input (e.g. via cloud-init)?") %> | ||
| <%= image_field(f, :label => _("Image"), :help_inline => _("The name of the image in the registry.")) %> | ||
| <%= image_field(f, :label => _("Image"), :help_inline => _("The name of the DataSource that contains the image. Use namespace/name notation to use a DataSource in a different namespace.")) %> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,23 @@ def new_kubevirt_vcr | |
| ::FactoryBot.build(:compute_resource_kubevirt) | ||
| end | ||
|
|
||
| def mocked_client | ||
| vms = stub | ||
| pvcs = stub | ||
| pvcs.stubs(:create) | ||
| pvcs.stubs(:delete) | ||
| servers = stub | ||
| servers.stubs(:get) | ||
| storageclasses = stub | ||
| storageclasses.stubs(:all).returns([{ 'name': 'local' }]) | ||
| client = stub | ||
| client.stubs(:vms).returns(vms) | ||
| client.stubs(:pvcs).returns(pvcs) | ||
| client.stubs(:servers).returns(servers) | ||
| client.stubs(:storageclasses).returns(storageclasses) | ||
| client | ||
| end | ||
|
|
||
| test "host_interfaces_attrs" do | ||
| record = new_kubevirt_vcr | ||
| host = ::FactoryBot.build(:host_kubevirt, :with_interfaces) | ||
|
|
@@ -23,26 +40,65 @@ def new_kubevirt_vcr | |
|
|
||
| describe "create_vm" do | ||
| test "uses sanitized NIC names" do | ||
| vms = stub | ||
| pvcs = stub | ||
| pvcs.stubs(:create) | ||
| pvcs.stubs(:delete) | ||
| servers = stub | ||
| servers.stubs(:get) | ||
| client = stub | ||
| client.stubs(:vms).returns(vms) | ||
| client.stubs(:pvcs).returns(pvcs) | ||
| client.stubs(:servers).returns(servers) | ||
| record = new_kubevirt_vcr | ||
| client = mocked_client | ||
| record.stubs(:client).returns(client) | ||
|
|
||
| expected_networks = [{ :name => "default-network", :multus => { :networkName => "default/network" } }] | ||
| expected_interfaces = [{ :bridge => {}, :name => "default-network" }] | ||
|
|
||
| vms.expects(:create).with(vm_name: anything, cpus: anything, memory_size: anything, memory_unit: anything, volumes: anything, cloudinit: anything, networks: expected_networks, interfaces: expected_interfaces) | ||
| client.vms.expects(:create).with do |args| | ||
| assert_equal expected_networks, args[:networks] | ||
| assert_equal expected_interfaces, args[:interfaces] | ||
| end | ||
|
|
||
| record.create_vm({ :name => "test", :volumes_attributes => { 0 => { :capacity => "5" } }, :interfaces_attributes => { "0" => { "cni_provider" => "multus", "network" => "default/network" } } }) | ||
| end | ||
|
|
||
| test "raises an error for image based provisioning without an explicit boot volume" do | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this duplicates the "create_vm image based without additional volumes should fail" test below, so probably can be dropped now? (in a previous iteration of this PR this checked that a VM actually gets created correctly)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this still a case? Can't find the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (in: |
||
| record = new_kubevirt_vcr | ||
|
|
||
| error = assert_raises(Foreman::Exception) do | ||
| record.create_vm({ :name => "test", :provision_method => 'image', :image_id => "default/template", :volumes_attributes => {}, :interfaces_attributes => { "0" => { "cni_provider" => "multus", "network" => "default/network" } } }) | ||
| end | ||
| assert_match(/A bootable volume is required as a target for the image/, error.message) | ||
| end | ||
|
|
||
| test "uses dataVolume for image based provisioning with an explicit boot volume" do | ||
| record = new_kubevirt_vcr | ||
| client = mocked_client | ||
| record.stubs(:client).returns(client) | ||
|
|
||
| client.vms.expects(:create).with do |args| | ||
| assert_equal 1, args[:volumes].length | ||
| assert_equal 1, args[:volume_templates].length | ||
|
|
||
| volume = args[:volumes].first | ||
| assert_equal 'dataVolume', volume.type | ||
| assert_equal 'test-root', volume.config[:name] | ||
| assert_equal 'rootdisk', volume.name | ||
| assert_equal 1, volume.boot_order | ||
|
|
||
| volume_template = args[:volume_templates].first | ||
| assert_equal 'DataVolume', volume_template[:kind] | ||
| assert_equal 'test-root', volume_template[:metadata][:name] | ||
| assert_equal 'DataSource', volume_template[:spec][:sourceRef][:kind] | ||
| assert_equal 'default', volume_template[:spec][:sourceRef][:namespace] | ||
| assert_equal 'template', volume_template[:spec][:sourceRef][:name] | ||
| assert_equal '10G', volume_template[:spec][:storage][:resources][:requests][:storage] | ||
| end | ||
|
|
||
| record.create_vm({ :name => "test", :provision_method => 'image', :image_id => "default/template", :volumes_attributes => { "0" => { :capacity => "10", :bootable => "true" } }, :interfaces_attributes => { "0" => { "cni_provider" => "multus", "network" => "default/network" } } }) | ||
| end | ||
|
|
||
| test "raises an error for image based provisioning with only an extra data volume" do | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this duplicates the "create_vm image based without additional volumes should fail" test below, so probably can be dropped now? (in a previous iteration of this PR this checked that a VM actually gets created correctly) |
||
| record = new_kubevirt_vcr | ||
|
|
||
| error = assert_raises(Foreman::Exception) do | ||
| record.create_vm({ :name => "test", :provision_method => 'image', :image_id => "default/template", :volumes_attributes => { "0" => { :capacity => "10" } }, :interfaces_attributes => { "0" => { "cni_provider" => "multus", "network" => "default/network" } } }) | ||
| end | ||
| assert_match(/A bootable volume is required as a target for the image/, error.message) | ||
| end | ||
| end | ||
|
|
||
| describe "create_network_element" do | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the UI, when provisioning a host from an image, I need to add a bootable storage disk.
But that's something users don't know they are required to do.
Should we include it in the UI as a tooltip or at least mention it in the docs?
cc @jafiala
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is that solved in other CRs? it's not like we're the first one to require a target when deploying something? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
answering myself,
vm_instance_defaults:foreman_kubevirt/app/models/foreman_kubevirt/kubevirt.rb
Lines 221 to 226 in 34e6a25
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding

:volumes => [new_volume].compact(stolen from the libvirt CR) here gives me:No idea if setting size and boot flags by default is a good idea here as I have no knowledge where else
vm_instance_defaultsare usedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would that make the storage class default for both image based and non image based right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other CRs taking example of VMware, the volume section is already added with some default size and other default fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it would make the storage section be populated with one default volume for both provisioning methods, but it's also kinda right -- you can't provision without a (target) disk after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah agree , we need at least one disk to be selected but also which storage class to be setted as default is a concern , as there is also a way to define default storage class in kuberneties/openshift https://kubernetes.io/docs/tasks/administer-cluster/change-default-storage-class/ and we can also check if a storage class is default or not in https://github.com/fog/fog-kubevirt/blob/master/lib/fog/kubevirt/compute/models/storageclass.rb#L33 while parsing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added the "empty" volume for now, we can enhance later