Bug #9568
Testsuite fails if TailsToasterStorage already exists with libvirt from Debian Stretch
100%
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>'Subtasks
History
#1 Updated by kytv 2015-06-12 21:34:02
- 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 2015-06-13 05:04:27
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 2015-06-15 03:46:14
- 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 2015-06-15 06:15:10
- 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 2015-06-15 07:57:33
- 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 2015-06-16 05:10:08
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 2015-06-18 02:48:32
> 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 2015-06-21 06:23:36
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 2015-06-21 06:51:55
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 2015-06-21 10:54:43
- Assignee changed from kytv to intrigeri
- QA Check changed from Info Needed to Ready for QA
Upstream bug opened at https://bugzilla.redhat.com/show_bug.cgi?id=1234150
#11 Updated by intrigeri 2015-06-22 01:11:27
- 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!
#12 Updated by intrigeri 2015-06-28 07:54:12
Applied in changeset commit:9ffa149c1dec5661dd27dd5b028756632507ee4b.
#13 Updated by intrigeri 2015-07-03 03:35:28
- Status changed from Fix committed to Resolved