Skip to content

Fixes #460 - Verify VMID existence before creating VM #461

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.

Sign up for GitHub

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

Open
jakduch wants to merge 2 commits into
base: master
Choose a base branch
from
+41 −1
Open

Fixes #460 - Verify VMID existence before creating VM #461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a55380d
Fixes #460 - verify VMID existence before creating VM
jakduch Mar 6, 2026
8b70de5
Fix rescue scope in vmid_exists? and update test mocks
jakduch Mar 9, 2026
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
19 changes: 18 additions & 1 deletion app/models/foreman_fog_proxmox/proxmox_vm_commands.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,24 @@ def create_vm(args = {})
end

def assign_vmid(vmid, node)
(vmid < 1) ? node.servers.next_id : vmid
vmid = node.servers.next_id if vmid < 1
max_retries = 5
retries = 0
while vmid_exists?(vmid) && retries < max_retries
logger.warn("assign_vmid: VMID #{vmid} already exists, requesting new one (attempt #{retries + 1})")
vmid = node.servers.next_id
retries += 1
end
raise ::Foreman::Exception, N_('Unable to find a free VMID after %s attempts') % max_retries if vmid_exists?(vmid)
vmid
end

def vmid_exists?(vmid)
nodes.any? do |search_node|
search_node.servers.get(vmid) || search_node.containers.get(vmid)
rescue Fog::Errors::NotFound
false
end
end

def compute_clone_attributes(args, container, type)
Expand Down
8 changes: 8 additions & 0 deletions test/factories/foreman_fog_proxmox/proxmox_node_mock_factory.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,18 @@

module ForemanFogProxmox
module ProxmoxNodeMockFactory
def mock_empty_collection
col = mock('collection')
col.stubs(:get).returns(nil)
col
end

def mock_node_servers(cr, servers)
node = mock('node')
nodes = mock('nodes')
node.stubs(:node).returns('proxmox')
node.stubs(:servers).returns(servers)
node.stubs(:containers).returns(mock_empty_collection)
nodes.stubs(:get).returns(node)
nodes.stubs(:all).returns([node])
client = mock('client')
Expand All @@ -37,6 +44,7 @@ def mock_node_containers(cr, containers)
nodes = mock('nodes')
node.stubs(:node).returns('proxmox')
node.stubs(:containers).returns(containers)
node.stubs(:servers).returns(mock_empty_collection)
nodes.stubs(:get).returns(node)
nodes.stubs(:all).returns([node])
client = mock('client')
Expand Down
8 changes: 8 additions & 0 deletions test/unit/foreman_fog_proxmox/proxmox_vm_commands_container_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,9 @@ class ProxmoxVMCommandsContainerTest < ActiveSupport::TestCase
args = { vmid: '100', type: 'lxc', node_id: 'proxmox', start_after_create: '0' }
servers = mock('servers')
servers.stubs(:id_valid?).returns(true)
servers.stubs(:get).returns(nil)
containers = mock('containers')
containers.stubs(:get).returns(nil)
containers.stubs(:create).with(args)
cr = mock_node_servers_containers(ForemanFogProxmox::Proxmox.new, servers, containers)
cr.stubs(:parse_typed_vm).with(args, 'lxc').returns(args)
Expand All @@ -269,7 +271,9 @@ class ProxmoxVMCommandsContainerTest < ActiveSupport::TestCase
args = { vmid: '100', type: 'lxc', node_id: 'proxmox', start_after_create: '1' }
servers = mock('servers')
servers.stubs(:id_valid?).returns(true)
servers.stubs(:get).returns(nil)
containers = mock('containers')
containers.stubs(:get).returns(nil)
vm = mock('vm')
containers.stubs(:create).with(args).returns(vm)
cr = mock_node_servers_containers(ForemanFogProxmox::Proxmox.new, servers, containers)
Expand All @@ -283,7 +287,9 @@ class ProxmoxVMCommandsContainerTest < ActiveSupport::TestCase
args = { vmid: '100', type: 'lxc', node_id: 'proxmox', start_after_create: '0', pool: 'pool1' }
servers = mock('servers')
servers.stubs(:id_valid?).returns(true)
servers.stubs(:get).returns(nil)
containers = mock('containers')
containers.stubs(:get).returns(nil)
vm = mock('vm')
containers.stubs(:create).with(args).returns(vm)
cr = mock_node_servers_containers(ForemanFogProxmox::Proxmox.new, servers, containers)
Expand All @@ -296,7 +302,9 @@ class ProxmoxVMCommandsContainerTest < ActiveSupport::TestCase
args = { vmid: '100', type: 'lxc', image_id: '999', name: 'name' }
servers = mock('servers')
servers.stubs(:id_valid?).returns(true)
servers.stubs(:get).returns(nil)
containers = mock('containers')
containers.stubs(:get).returns(nil)
cr = mock_node_servers_containers(ForemanFogProxmox::Proxmox.new, servers, containers)
vm = mock('vm')
cr.expects(:clone_from_image).with('999', 100).returns(vm)
Expand Down
7 changes: 7 additions & 0 deletions test/unit/foreman_fog_proxmox/proxmox_vm_commands_server_create_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ class ProxmoxVMCommandsServerCreateTest < ActiveSupport::TestCase
args = { vmid: '100' }
servers = mock('servers')
servers.stubs(:id_valid?).returns(false)
servers.stubs(:get).returns(nil)
cr = mock_node_servers(ForemanFogProxmox::Proxmox.new, servers)
err = assert_raises Foreman::Exception do
cr.create_vm(args)
Expand All @@ -47,6 +48,7 @@ class ProxmoxVMCommandsServerCreateTest < ActiveSupport::TestCase
servers = mock('servers')
servers.stubs(:id_valid?).returns(true)
servers.stubs(:next_id).returns('101')
servers.stubs(:get).returns(nil)
cr = mock_node_servers(ForemanFogProxmox::Proxmox.new, servers)
cr.stubs(:parse_typed_vm).with(args, 'qemu').returns(args)
servers.stubs(:create).with(args)
Expand All @@ -59,6 +61,7 @@ class ProxmoxVMCommandsServerCreateTest < ActiveSupport::TestCase
args = { vmid: '100', type: 'qemu', node_id: 'proxmox', start_after_create: '0' }
servers = mock('servers')
servers.stubs(:id_valid?).returns(true)
servers.stubs(:get).returns(nil)
cr = mock_node_servers(ForemanFogProxmox::Proxmox.new, servers)
cr.stubs(:parse_typed_vm).with(args, 'qemu').returns(args)
servers.stubs(:create).with(args)
Expand All @@ -71,6 +74,7 @@ class ProxmoxVMCommandsServerCreateTest < ActiveSupport::TestCase
args = { vmid: '100', type: 'qemu', node_id: 'proxmox', start_after_create: '1' }
servers = mock('servers')
servers.stubs(:id_valid?).returns(true)
servers.stubs(:get).returns(nil)
cr = mock_node_servers(ForemanFogProxmox::Proxmox.new, servers)
cr.stubs(:parse_typed_vm).with(args, 'qemu').returns(args)
vm = mock('vm')
Expand All @@ -84,6 +88,7 @@ class ProxmoxVMCommandsServerCreateTest < ActiveSupport::TestCase
args = { vmid: '100', type: 'qemu', node_id: 'proxmox', start_after_create: '0', pool: 'pool1' }
servers = mock('servers')
servers.stubs(:id_valid?).returns(true)
servers.stubs(:get).returns(nil)
cr = mock_node_servers(ForemanFogProxmox::Proxmox.new, servers)
cr.stubs(:parse_typed_vm).with(args, 'qemu').returns(args)
vm = mock('vm')
Expand All @@ -98,6 +103,8 @@ class ProxmoxVMCommandsServerCreateTest < ActiveSupport::TestCase
servers = mock('servers')
containers = mock('containers')
servers.stubs(:id_valid?).returns(true)
servers.stubs(:get).returns(nil)
containers.stubs(:get).returns(nil)
cr = mock_node_servers_containers(ForemanFogProxmox::Proxmox.new, servers, containers)
vm = mock('vm')
cr.expects(:clone_from_image).with('999', 100).returns(vm)
Expand Down