Project

General

Profile

Bug #9568

Testsuite fails if TailsToasterStorage already exists with libvirt from Debian Stretch

Added by kytv over 4 years ago. Updated about 4 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
Test suite
Target version:
Start date:
06/12/2015
Due date:
% Done:

100%

Feature Branch:
kytv:bugfix/9568-tailstoasterstorage-already-exists
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Description

After recent upgrades, running the test suite fails from Debian Stretch.

esting ISO image: tails-i386-feature_7724-sandbox-i2p-1.4.1-20150607.iso
Call to virStoragePoolBuild failed: failed to create directory '/home/kytv/git/tails/tmp/TailsToasterStorage': Directory exists (Libvirt::Error)
/home/kytv/git/tails/features/support/helpers/storage_helper.rb:31:in `build'
/home/kytv/git/tails/features/support/helpers/storage_helper.rb:31:in `initialize'
/home/kytv/git/tails/features/support/hooks.rb:72:in `new'
/home/kytv/git/tails/features/support/hooks.rb:72:in `block in <top (required)>'
/home/kytv/git/tails/features/support/extra_hooks.rb:13:in `call'
/home/kytv/git/tails/features/support/extra_hooks.rb:13:in `invoke'
/home/kytv/git/tails/features/support/extra_hooks.rb:46:in `block in before_feature'
/home/kytv/git/tails/features/support/extra_hooks.rb:45:in `each'
/home/kytv/git/tails/features/support/extra_hooks.rb:45:in `before_feature'
/usr/lib/ruby/vendor_ruby/cucumber/ast/tree_walker.rb:181:in `block in send_to_all'
/usr/lib/ruby/vendor_ruby/cucumber/ast/tree_walker.rb:179:in `each'
/usr/lib/ruby/vendor_ruby/cucumber/ast/tree_walker.rb:179:in `send_to_all'
/usr/lib/ruby/vendor_ruby/cucumber/ast/tree_walker.rb:169:in `broadcast'
/usr/lib/ruby/vendor_ruby/cucumber/ast/tree_walker.rb:26:in `visit_feature'
/usr/lib/ruby/vendor_ruby/cucumber/ast/features.rb:28:in `block in accept'
/usr/lib/ruby/vendor_ruby/cucumber/ast/features.rb:17:in `each'
/usr/lib/ruby/vendor_ruby/cucumber/ast/features.rb:17:in `each'
/usr/lib/ruby/vendor_ruby/cucumber/ast/features.rb:27:in `accept'
/usr/lib/ruby/vendor_ruby/cucumber/ast/tree_walker.rb:21:in `block in visit_features'
/usr/lib/ruby/vendor_ruby/cucumber/ast/tree_walker.rb:170:in `broadcast'
/usr/lib/ruby/vendor_ruby/cucumber/ast/tree_walker.rb:20:in `visit_features'
/usr/lib/ruby/vendor_ruby/cucumber/runtime.rb:49:in `run!'
/usr/lib/ruby/vendor_ruby/cucumber/cli/main.rb:42:in `execute!'
/usr/bin/cucumber:13:in `<main>'

Associated revisions

Revision 9ffa149c
Added by intrigeri about 4 years ago

Merge remote-tracking branch 'kytv/bugfix/9568-tailstoasterstorage-already-exists' into stable

Fix-committed: #9568

History

#1 Updated by kytv over 4 years ago

  • Status changed from New to In Progress
  • Assignee changed from kytv to anonym
  • QA Check set to Ready for QA
  • Feature Branch set to kytv:bugfix/9568-tailstoasterstorage-already-exists

#2 Updated by intrigeri over 4 years ago

Wow :) I say: as long as it's cheap to have our test suite support running in testing/sid environment, yes, let's do it. This will save us some time later on the way, and may allow me to run it directly on bare metal rather than in nested virt.

#3 Updated by intrigeri over 4 years ago

  • Assignee changed from anonym to kytv
  • % Done changed from 0 to 30
  • QA Check changed from Ready for QA to Info Needed

I can reproduce the bug. However, it seems to be that your changes somewhat break abstraction boundaries, in that it adds logics to our own code that depend on internals of libvirt, and if the build method some day does more than just creating the directory, then we'll be skipping some code that might be necessary. On https://libvirt.org/html/libvirt-libvirt-storage.html#virStoragePoolBuild, I notice that there's a VIR_STORAGE_POOL_BUILD_NO_OVERWRITE flag available. If the Ruby bindings allow us to pass that flag, maybe this would solve the bug and the design problem I'm raising?

#4 Updated by kytv over 4 years ago

  • Assignee changed from kytv to intrigeri

I can reproduce the bug

Woot, not just me. :)

Based on this page and line 269 I tried the following:

diff --git a/features/support/helpers/storage_helper.rb b/features/support/helpers/storage_helper.rb
index e6c6939..a971f9f 100644
--- a/features/support/helpers/storage_helper.rb
+++ b/features/support/helpers/storage_helper.rb
@@ -28,7 +28,7 @@ class VMStorage
     @pool_path = "#{$config["TMPDIR"]}/#{pool_name}" 
     pool_xml.elements['pool/target/path'].text = @pool_path
     @pool = @virt.define_storage_pool_xml(pool_xml.to_s)
-    @pool.build unless Dir.exists?(@pool_path)
+    @pool.build(Libvirt::StoragePool::BUILD_NO_OVERWRITE)
     @pool.create
     @pool.refresh
   end

...and the error persisted. I also tried BUILD_OVERWRITE and saw the same failure.

Is this upstream's bug (on the libvirt side) and there's nothing for "us" to actually do?

#5 Updated by intrigeri over 4 years ago

  • Assignee changed from intrigeri to kytv

I tried the following: [...]

Looks to me like flags= is a parameter that should be passed by name, as opposed to a positional parameter. Did you try this?
(Now, it may be me misunderstanding the bindings doc, I'm clearly no Ruby rock star :)

#6 Updated by anonym over 4 years ago

intrigeri wrote:

I tried the following: [...]

Looks to me like flags= is a parameter that should be passed by name, as opposed to a positional parameter. Did you try this?

This is not how it works in Ruby -- the current code is correct as far as Ruby is concenred. An assignment of a parameter in a function definition only sets a default value if that parameter (by place) is omitted in a call. Yes, this is pretty strange semantics; for e.g. def foo(bar = 1, baz = 2) there's no way to call it so you get the default value for bar and a non-default value for baz.

When it comes to a function call, = has no special meaning among the parameters so it's still a normal assignment; if you call e.g. foo(baz = 3, bar = 4) that till (1) assign 3 to baz and 4 to bar in the scope where foo() was called from, and (2) call foo(3, 4) (which probably wasn't what you intended, note the order) because the assignment operator = simply returns the assignment value. Both (1) and (2) can cause nasty bugs on their own, and that's why I am going into a bit of detail here. :)

For the "set parameter by name" functionality you expect (and how it work in e.g. python), a hack with a Hash is needed, e.g. try_for(42, :msg => "some error", :delay => 3).

#7 Updated by intrigeri over 4 years ago

This is not how it works in Ruby -- the current code is correct as far as Ruby is concenred.

OK, thanks. So, Kill Your TV: please file a bug against the Ruby bindings, add the URL here, and then I'll happily merge your branch :)

#8 Updated by kytv about 4 years ago

intrigeri wrote:

This is not how it works in Ruby -- the current code is correct as far as Ruby is concenred.

OK, thanks. So, Kill Your TV: please file a bug against the Ruby bindings, add the URL here, and then I'll happily merge your branch :)

I noticed there was another update to the ruby guestfs packages so I'll update and try to reproduce and/or file an upstream bug if it's still needed.

#9 Updated by kytv about 4 years ago

kytv wrote:

intrigeri wrote:

This is not how it works in Ruby -- the current code is correct as far as Ruby is concenred.

OK, thanks. So, Kill Your TV: please file a bug against the Ruby bindings, add the URL here, and then I'll happily merge your branch :)

I noticed there was another update to the ruby guestfs packages so I'll update and try to reproduce and/or file an upstream bug if it's still needed.

I meant ruby-libvirt, of course.

That said, it's still a problem with ruby-libvirt:amd64 0.5.1-3+b2.

#10 Updated by kytv about 4 years ago

  • Assignee changed from kytv to intrigeri
  • QA Check changed from Info Needed to Ready for QA

#11 Updated by intrigeri about 4 years ago

  • Status changed from In Progress to Fix committed
  • Assignee deleted (intrigeri)
  • % Done changed from 30 to 100
  • QA Check changed from Ready for QA to Pass

Merged!

#13 Updated by intrigeri about 4 years ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF