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

Proxmox extention for foreman_bootdisk #90

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
50 changes: 50 additions & 0 deletions app/models/concerns/foreman_bootdisk/compute_resources/proxmox.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# frozen_string_literal: true

module ForemanBootdisk
module ComputeResources
module Proxmox
CDROM_VOLUME = 'ide2'

def capabilities
super + [:bootdisk]
end

def iso_upload(iso, vm_uuid)
server = find_vm_by_uuid(vm_uuid)
server.ssh_options = { password: fog_credentials[:proxmox_password] }
server.ssh_ip_address = proxmox_host
server.username = client.credentials[:current_user].split('@').first
server.scp_upload(iso, '/var/lib/vz/template/iso/')
lzap marked this conversation as resolved.
Show resolved Hide resolved
server.reload
storage = storages(server.node_id, 'iso')[0]
storage.volumes.any? { |v| v.volid.include? File.basename(iso) }
end

def iso_attach(iso, vm_uuid)
server = find_vm_by_uuid(vm_uuid)
storage = storages(server.node_id, 'iso')[0]
volume = storage.volumes.detect { |v| v.volid.include? File.basename(iso) }
disks = server.disks.map { |disk| disk.split(":")[0] }.join(";")
server.update({ ide2: "#{volume.volid},media=cdrom" })
server.update({ boot: "order=ide2;#{disks}" })
server.reboot
end

def iso_detach(vm_uuid)
server = find_vm_by_uuid(vm_uuid)

# get volid to delete iso after detaching from vm
volid = server.volumes.get(CDROM_VOLUME).volid
server.update({ ide2: "none,media=cdrom" })

# cdrom will be ejected on next power off
server.detach(CDROM_VOLUME)

# delete the iso file from proxmox server
storage = storages(server.node_id, 'iso')[0]
volume = storage.volumes.detect { |v| v.volid.include? volid }
Copy link
Member

Choose a reason for hiding this comment

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

When would this volume ID not be exactly equal? If the volume IDs are integers I'd imagine this also end up being 123'.include?('12') so

Suggested change
volume = storage.volumes.detect { |v| v.volid.include? volid }
volume = storage.volumes.detect { |v| v.volid == volid }

Also, previously you already used volid = server.volumes.get("ide2").volid so why can't you just do:

server.volumes.get('ide2').destroy

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 don't want to destroy the disk 'ide2' as the operation is not possible when vm is powered on. We just want to destroy the volume so that the iso files are not piled up in proxmox server.

Copy link
Member

Choose a reason for hiding this comment

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

So volume = storage.volumes.detect { |v| v.volid.include? volid } returns a different object than server.volumes.get("ide2"). That would surprise me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, server.volumes are disks of the server so it returns the disks, Fog::Proxmox::Compute::Disks and storage.volumes are volumes so it returns Fog::Proxmox::Compute::Volumes. You can remove the volumes without caring about server but you cannot remove the disk without powering off the server (hotplug issue).

volume.destroy
end
end
end
end
5 changes: 5 additions & 0 deletions lib/foreman_bootdisk/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ class Engine < ::Rails::Engine
Host::Managed.prepend ForemanBootdisk::HostExt
Host::Managed.include ForemanBootdisk::Orchestration::Compute
Foreman::Model::Vmware.prepend ForemanBootdisk::ComputeResources::Vmware if Foreman::Model::Vmware.available?
ForemanFogProxmox::Proxmox.prepend ForemanBootdisk::ComputeResources::Proxmox if ForemanBootdisk.with_proxmox?
rescue StandardError => e
Rails.logger.warn "#{ForemanBootdisk::ENGINE_NAME}: skipping engine hook (#{e})"
end
Expand All @@ -154,4 +155,8 @@ class Engine < ::Rails::Engine
def self.logger
Foreman::Logging.logger('foreman_bootdisk')
end

def self.with_proxmox?
Foreman::Plugin.installed?('foreman_fog_proxmox')
end
end
4 changes: 4 additions & 0 deletions test/test_plugin_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

require 'test_helper'

FactoryBot.definition_file_paths << File.join(ForemanFogProxmox::Engine.root, 'test', 'factories') if defined?(ForemanFogProxmox::Engine)
FactoryBot.definition_file_paths << File.join(__dir__, 'factories')
FactoryBot.reload
Manisha15 marked this conversation as resolved.
Show resolved Hide resolved

lzap marked this conversation as resolved.
Show resolved Hide resolved
module ForemanBootdiskTestHelper
def create_tempfile
file = Tempfile.new('bootdisk-test', '/tmp')
Expand Down
19 changes: 19 additions & 0 deletions test/unit/concerns/compute_resources/proxmox_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# frozen_string_literal: true

require 'test_plugin_helper'

module ForemanBootdisk
class ProxmoxTest < ActiveSupport::TestCase

describe '#capabilities' do
setup do
skip unless ForemanBootdisk.with_proxmox?
@cr = FactoryBot.build(:proxmox_cr)
end

test 'should include bootdisk' do
assert_includes @cr.capabilities, :bootdisk
end
end
end
end
70 changes: 70 additions & 0 deletions test/unit/concerns/orchestration/proxmox_compute_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
# frozen_string_literal: true

require 'test_plugin_helper'

module ForemanBootdisk
class OrchestrationProxmoxComputeTest < ActiveSupport::TestCase
setup do
disable_orchestration
skip unless ForemanBootdisk.with_proxmox?
@proxmox_cr = FactoryBot.build(:proxmox_cr)
@proxmox_host = FactoryBot.build(:host, :managed,
compute_resource: @proxmox_cr,
provision_method: 'bootdisk')
end

test 'provisioning a host with provision method bootdisk in proxmox should upload iso' do
@proxmox_cr.expects(:iso_upload)
@proxmox_host.send(:setIsoImage)
end

test 'provisioning a host with provision method bootdisk in proxmox should attach iso' do
@proxmox_cr.expects(:iso_attach)
@proxmox_host.send(:setAttachIsoImage)
end

test 'provisioning a host with provision method bootdisk in proxmox should detach iso' do
@proxmox_cr.expects(:iso_detach)
@proxmox_host.send(:setDetachIsoImage)
end

test 'provisioning a new proxmox host with provision method bootdisk should queue bootdisk tasks' do
@proxmox_host.stubs(:compute?).returns(true)
@proxmox_host.stubs(:build?).returns(true)
@proxmox_host.send(:queue_bootdisk_compute)
tasks = @proxmox_host.queue.all.map(&:name)
assert_includes tasks, "Generating ISO image for #{@proxmox_host.name}"
assert_includes tasks, "Upload ISO image to datastore for #{@proxmox_host.name}"
assert_includes tasks, "Attach ISO image to CDROM drive for #{@proxmox_host.name}"
assert_not_includes tasks, "Detach ISO image from CDROM drive for #{@proxmox_host.name}"
end

test 'rebuilding a proxmox host with provision method bootdisk should queue bootdisk tasks' do
@proxmox_host.stubs(:compute?).returns(true)
old = stub()
old.stubs(:build?).returns(false)
@proxmox_host.stubs(:old).returns(old)
@proxmox_host.stubs(:build?).returns(true)
@proxmox_host.send(:queue_bootdisk_compute)
tasks = @proxmox_host.queue.all.map(&:name)
assert_includes tasks, "Generating ISO image for #{@proxmox_host.name}"
assert_includes tasks, "Upload ISO image to datastore for #{@proxmox_host.name}"
assert_includes tasks, "Attach ISO image to CDROM drive for #{@proxmox_host.name}"
assert_not_includes tasks, "Detach ISO image from CDROM drive for #{@proxmox_host.name}"
end

test 'the iso should be detached when the proxmox host leaves build mode' do
@proxmox_host.stubs(:compute?).returns(true)
old = stub()
old.stubs(:build?).returns(true)
@proxmox_host.stubs(:old).returns(old)
@proxmox_host.stubs(:build?).returns(false)
@proxmox_host.send(:queue_bootdisk_compute)
tasks = @proxmox_host.queue.all.map(&:name)
assert_not_includes tasks, "Generating ISO image for #{@proxmox_host.name}"
assert_not_includes tasks, "Upload ISO image to datastore for #{@proxmox_host.name}"
assert_not_includes tasks, "Attach ISO image to CDROM drive for #{@proxmox_host.name}"
assert_includes tasks, "Detach ISO image from CDROM drive for #{@proxmox_host.name}"
end
end
end