Skip to content

Add bootdisk support#188

Open
sbernhard wants to merge 1 commit intofog:masterfrom
ATIX-AG:add_bootdisk_support
Open

Add bootdisk support#188
sbernhard wants to merge 1 commit intofog:masterfrom
ATIX-AG:add_bootdisk_support

Conversation

@sbernhard
Copy link

No description provided.

Copy link
Contributor

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I haven't reviewed all the details, this is just a superficial review.

@sbernhard sbernhard force-pushed the add_bootdisk_support branch from 36bba07 to 0402e3f Compare February 23, 2026 17:36
@sbernhard sbernhard force-pushed the add_bootdisk_support branch from 0402e3f to f5495f3 Compare February 24, 2026 15:34
Copy link
Contributor

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I'm wondering about the design so I'm making various comments inline.

Rather than implementing separate attach and detach ISO requests, should it have a requests to reconfigure the cdrom device where the source file is an optional parameter?


private

def attach_cdrom_xml(iso_path, target_dev, bus)
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't actually attach anything and is only the CDROM XML so naming wise I'd suggest.

Suggested change
def attach_cdrom_xml(iso_path, target_dev, bus)
def cdrom_xml(iso_path, target_dev, bus)

private

def attach_cdrom_xml(iso_path, target_dev, bus)
Nokogiri::XML::Builder.new do |x|
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a different implementation than what's default when a host is created:

xml.disk(:type => "file", :device => "cdrom") do
xml.driver(:name => "qemu", :type => "raw")
xml.source(:file => "#{iso_dir}/#{iso_file}")
xml.target(:dev => "sda", :bus => "scsi")
xml.readonly
xml.address(:type => "drive", :controller => 0, :bus => 0, :unit => 0)
end

If the host already has a cdrom drive, shouldn't it reuse that?

module Fog
module Libvirt
module Util
DEFAULT_CDROM_TARGET_DEV = "sdc".freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

When a host is created with a cdrom it uses sda as the default:

xml.disk(:type => "file", :device => "cdrom") do
xml.driver(:name => "qemu", :type => "raw")
xml.source(:file => "#{iso_dir}/#{iso_file}")
xml.target(:dev => "sda", :bus => "scsi")
xml.readonly
xml.address(:type => "drive", :controller => 0, :bus => 0, :unit => 0)
end

return flags unless domain_active

flags | ::Libvirt::Domain::AFFECT_LIVE
rescue ::Libvirt::Error
Copy link
Contributor

Choose a reason for hiding this comment

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

When is this raised? I don't see API calls here so what could raise an exception?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants