Skip to content

Conversation

ekohl
Copy link
Contributor

@ekohl ekohl commented May 11, 2025

LVM volumes do not have a volume type and after a reload the value is set to nil. 0277e9b introduced that reload after saving. The mock driver doesn't properly show this behavior, which is why it was missed. A test is added to simulate the real world behavior.

Fixes: 0277e9b ("Correctly load volumes after creation and cloning")
Link: https://libvirt.org/formatstorage.html#storage-volume-target-elements
Link: https://libvirt.org/storage.html#valid-logical-volume-format-types

Fixes #169

LVM volumes do not have a volume type and after a reload the value is
set to nil. 0277e9b introduced that
reload after saving. The mock driver doesn't properly show this
behavior, which is why it was missed. A test is added to simulate the
real world behavior.

Fixes: 0277e9b ("Correctly load volumes after creation and cloning")
Link: https://libvirt.org/formatstorage.html#storage-volume-target-elements
Link: https://libvirt.org/storage.html#valid-logical-volume-format-types
@ekohl
Copy link
Contributor Author

ekohl commented May 17, 2025

@nofaralfasi mind taking a look? I take https://community.theforeman.org/t/invalid-libvirt-xml-unknown-driver-format-value/43081/10 as a confirmation that it works.

@ekohl ekohl requested a review from nofaralfasi May 17, 2025 10:30
@ekohl ekohl added the bug label May 17, 2025
@mxey
Copy link

mxey commented May 19, 2025

With this reproducer, the output does not change:

require 'fog/libvirt'

compute = Fog::Compute.new(provider: :libvirt, libvirt_uri: "qemu+ssh://s098209.rz.babiel.com/system")
vol = compute.volumes.new
puts "format_type before save: #{vol.format_type.inspect}"
vol.save
puts "format_type after save: #{vol.format_type.inspect}"

Gemfile:

source 'https://rubygems.org'

gem 'fog-libvirt', github: 'ekohl/fog-libvirt', ref: 'fix-volume-format_type-handling'
$ bundle install           
$ bundle show 
Gems included by the bundle:
  * builder (3.3.0)
  * bundler (2.6.9)
  * excon (1.2.5)
  * fog-core (2.6.0)
  * fog-json (1.2.0)
  * fog-libvirt (0.13.2 1614014)
  * fog-xml (0.1.5)
  * formatador (1.1.0)
  * json (2.12.0)
  * logger (1.7.0)
  * mime-types (3.7.0)
  * mime-types-data (3.2025.0514)
  * multi_json (1.15.0)
  * nokogiri (1.18.8)
  * racc (1.8.1)
  * ruby-libvirt (0.8.4)
$ bundle exec ruby repro.rb
format_type before save: "raw"
format_type after save: nil

Changing the Gemfile back to 0.12.0, it works:

fog-libvirt LVM Bug  % bundle show
Gems included by the bundle:
  * builder (3.3.0)
  * bundler (2.6.9)
  * excon (1.2.5)
  * fog-core (2.6.0)
  * fog-json (1.2.0)
  * fog-libvirt (0.12.0)
  * fog-xml (0.1.5)
  * formatador (1.1.0)
  * json (2.12.0)
  * logger (1.7.0)
  * mime-types (3.7.0)
  * mime-types-data (3.2025.0514)
  * multi_json (1.15.0)
  * nokogiri (1.18.8)
  * racc (1.8.1)
  * ruby-libvirt (0.8.4)
fog-libvirt LVM Bug  % bundle exec ruby repro.rb
format_type before save: "raw"
/opt/homebrew/lib/ruby/gems/3.2.0/gems/fog-libvirt-0.12.0/lib/fog/libvirt/models/compute/util/util.rb:24: warning: Passing safe_level with the 2nd argument of ERB.new is deprecated. Do not use it, and specify other arguments as keyword arguments.
/opt/homebrew/lib/ruby/gems/3.2.0/gems/fog-libvirt-0.12.0/lib/fog/libvirt/models/compute/util/util.rb:24: warning: Passing trim_mode with the 3rd argument of ERB.new is deprecated. Use keyword argument like ERB.new(str, trim_mode: ...) instead.
format_type after save: "raw"

@mxey
Copy link

mxey commented May 19, 2025

Handling a nil volume_type when creating the server might fix the issue with Foreman though, but it's IMO more of a workaround.

@ekohl
Copy link
Contributor Author

ekohl commented May 19, 2025

@mxey as indicated in the linked resources, this is mirroring what libvirt internally does. LVM doesn't have a format type so pretending it does doesn't make sense.

@mxey
Copy link

mxey commented May 19, 2025

@mxey as indicated in the linked resources, this is mirroring what libvirt internally does. LVM doesn't have a format type so pretending it does doesn't make sense.

OK, then the default format_type set to raw also does not make sense. This PR fixes the problem if used through Foreman, but it does not solve the bug as I filed it, which affects more API. I personally do not care about the other API, but maybe other users of the library do?

@ekohl
Copy link
Contributor Author

ekohl commented May 19, 2025

I debated dropping the default of raw. Might still revisit that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Volume loses its format_type when saved
2 participants