Had this been in place earlier, I would have avoided the bugs in
commit 0baf6945 and 55c2ab3e. Writing the test required me to extend
the power of virsh - creating enough snapshots to cause fanout
requires enough input in a single session that adding comments and
markers makes it easier to check that output is correct. It's still a
bit odd that with test:///default, reverting to a snapshot changes the
domain from running to paused (possibly a bug in how the test driver
copied from the qemu driver) - but the important part is that the test
is reproducible, and any future tweaks we make to snapshot code have
less chance of breaking successful command sequences.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
tests/Makefile.am | 3 +-
tests/virsh-snapshot | 212 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 214 insertions(+), 1 deletion(-)
create mode 100755 tests/virsh-snapshot
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 29f1fe2d2a..d3cdbff8bb 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1,6 +1,6 @@
## Process this file with automake to produce Makefile.in
-## Copyright (C) 2005-2015 Red Hat, Inc.
+## Copyright (C) 2005-2019 Red Hat, Inc.
##
## This library is free software; you can redistribute it and/or
## modify it under the terms of the GNU Lesser General Public
@@ -410,6 +410,7 @@ libvirtd_test_scripts = \
virsh-schedinfo \
virsh-self-test \
virt-admin-self-test \
+ virsh-snapshot \
virsh-start \
virsh-undefine \
virsh-uriprecedence \
diff --git a/tests/virsh-snapshot b/tests/virsh-snapshot
new file mode 100755
index 0000000000..3e6ff39f5f
--- /dev/null
+++ b/tests/virsh-snapshot
@@ -0,0 +1,212 @@
+#!/bin/sh
+# simple testing of snapshot APIs on test driver
+
+# Copyright (C) 2019 Red Hat, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 2 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see
+# <http://www.gnu.org/licenses/>.
+
+. "$(dirname $0)/test-lib.sh"
+
+if test "$VERBOSE" = yes; then
+ set -x
+ $abs_top_builddir/tools/virsh --version
+fi
+
+fail=0
+
+# The test driver loses states between restarts, so we perform a script
+# with some convenient markers for later post-processing of output.
+$abs_top_builddir/tools/virsh --connect test:///default >out 2>err '
+ # Create a series of snapshots, with names that intentionally sort
+ # differently by topology than by name. Use revert to create fanout.
+ snapshot-create-as test s1
+ snapshot-create-as test s3
+ snapshot-create-as test s2
+ snapshot-revert test s3
+ snapshot-create-as test s6
+ snapshot-create-as test s5
+ snapshot-revert test s6
+ snapshot-create-as test s4
+ snapshot-revert test s1
+ snapshot-create-as test s7
+ snapshot-create-as test s8
+ # Checking tree view (siblings sorted alphabetically)
+ snapshot-list test --tree
+ # Current was last one created, but we can change that
+ snapshot-current test --name
+ snapshot-current test s1
+ snapshot-current test --name
+ # Deleting current root leads to multiple roots, demonstrate list filtering
+ snapshot-delete test --current
+ echo --err marker
+ snapshot-current test --name
+ echo --err marker
+ snapshot-list test --roots
+ snapshot-list test --leaves
+ snapshot-list test --parent --no-leaves
+ snapshot-list test --from s3
+ snapshot-list test --from s3 --descendants --name
+ # More fun with delete flags, current node moves up to remaining parent
+ snapshot-current test s4
+ snapshot-delete test --children-only s6
+ snapshot-current test --name
+ snapshot-delete test --children s7
+ snapshot-current test --name
+ snapshot-delete test s6
+ snapshot-current test --name
+ # Now the tree is linear, so we have an unambiguous topological order
+ snapshot-list test --name
+ snapshot-list test --name --topological
+ # Capture some XML for later redefine
+ echo "<!--MarkerA-->"
+ snapshot-dumpxml test s3
+ echo "<!--MarkerB-->"
+ snapshot-dumpxml test s2
+ echo "<!--MarkerC-->"
+ # All done
+' || fail=1
+
+# First part is expected output, --tree results in trailing spaces,
+# and --list produces timestamps
+sed 's/ *$//; s/[0-9-]\{10\} [0-9:.]* .[0-9]\{4\}/TIMESTAMP/;
+ /MarkerA/,/MarkerC/d' < out > out.cooked || fail=1
+# Second part holds domain XMLs
+sed -n '/MarkerA/,/MarkerB/p' < out > s3.xml || fail=1
+sed -n '/MarkerB/,/MarkerC/p' < out > s2.xml || fail=1
+
+cat <<\EOF > exp || fail=1
+Domain snapshot s1 created
+Domain snapshot s3 created
+Domain snapshot s2 created
+
+Domain snapshot s6 created
+Domain snapshot s5 created
+
+Domain snapshot s4 created
+
+Domain snapshot s7 created
+Domain snapshot s8 created
+s1
+ |
+ +- s3
+ | |
+ | +- s2
+ | +- s6
+ | |
+ | +- s4
+ | +- s5
+ |
+ +- s7
+ |
+ +- s8
+
+
+s8
+Snapshot s1 set as current
+s1
+Domain snapshot s1 deleted
+
+
+
+
+ Name Creation Time State
+---------------------------------------------
+ s3 TIMESTAMP running
+ s7 TIMESTAMP paused
+
+ Name Creation Time State
+---------------------------------------------
+ s2 TIMESTAMP running
+ s4 TIMESTAMP paused
+ s5 TIMESTAMP paused
+ s8 TIMESTAMP paused
+
+ Name Creation Time State Parent
+------------------------------------------------------
+ s3 TIMESTAMP running
+ s6 TIMESTAMP paused s3
+ s7 TIMESTAMP paused
+
+ Name Creation Time State
+---------------------------------------------
+ s2 TIMESTAMP running
+ s6 TIMESTAMP paused
+
+s2
+s4
+s5
+s6
+
+Snapshot s4 set as current
+Domain snapshot s6 children deleted
+
+s6
+Domain snapshot s7 deleted
+
+s6
+Domain snapshot s6 deleted
+
+s3
+s2
+s3
+
+s3
+s2
+
+EOF
+compare exp out.cooked || fail=1
+
+cat <<EOF > exp || fail=1
+error: marker
+error: domain 'test' has no current snapshot
+error: marker
+EOF
+compare exp err || fail=1
+
+# Restore state with redefine
+$abs_top_builddir/tools/virsh -c test:///default >out 2>err <<EOF || fail=1
+ # Redefine must be in topological order; this will fail
+ snapshot-create test --redefine s2.xml
+ echo --err marker
+ # This is the right order
+ snapshot-create test --redefine s3.xml
+ snapshot-create test --redefine s2.xml --current
+ snapshot-info test --current
+EOF
+
+cat <<\EOF > exp || fail=1
+
+
+Domain snapshot s3 created from 's3.xml'
+Domain snapshot s2 created from 's2.xml'
+Name: s2
+Domain: test
+Current: yes
+State: running
+Location: internal
+Parent: s3
+Children: 0
+Descendants: 0
+Metadata: yes
+
+EOF
+
+cat <<EOF > exp || fail=1
+error: invalid argument: parent s3 for snapshot s2 not found
+error: marker
+EOF
+compare exp err || fail=1
+
+(exit $fail); exit $fail
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Eric Blake wrote:
> Had this been in place earlier, I would have avoided the bugs in
> commit 0baf6945 and 55c2ab3e. Writing the test required me to extend
> the power of virsh - creating enough snapshots to cause fanout
> requires enough input in a single session that adding comments and
> markers makes it easier to check that output is correct. It's still a
> bit odd that with test:///default, reverting to a snapshot changes the
> domain from running to paused (possibly a bug in how the test driver
> copied from the qemu driver) - but the important part is that the test
> is reproducible, and any future tweaks we make to snapshot code have
> less chance of breaking successful command sequences.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> tests/Makefile.am | 3 +-
> tests/virsh-snapshot | 212 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 214 insertions(+), 1 deletion(-)
> create mode 100755 tests/virsh-snapshot
Hi,
I noticed this test is failing for me:
$ ./tests/virsh-snapshot
expr: not a decimal number: ' 29'
Bus error (core dumped)
cmp: EOF on out.cooked
exp err differ: char 8, line 1
$
(lldb) target create "tools/.libs/virsh" --core "/tmp/virsh_13727_0.core"
Core file '/tmp/virsh_13727_0.core' (x86_64) was loaded.
(lldb) bt
* thread #1, name = 'virsh', stop reason = signal SIGBUS
* frame #0: 0x00000008013984bf libvirt.so.0`virDomainMomentForEachChild(moment=0x0000000802aaea10, iter=(libvirt.so.0`virDomainMomentActOnDescendant at virdomainmomentobjlist.c:80), data=0x00007fffffffdb30) at virdomainmomentobjlist.c:62
frame #1: 0x000000080139854f libvirt.so.0`virDomainMomentForEachDescendant(moment=0x0000000802aaea10, iter=(libvirt.so.0`testDomainSnapshotDiscardAll at test_driver.c:6445), data=0x00007fffffffdcf0) at virdomainmomentobjlist.c:105
frame #2: 0x00000008013985c5 libvirt.so.0`virDomainMomentActOnDescendant(payload=0x0000000802aaea10, name=0x00000008010fe488, data=0x00007fffffffdc10) at virdomainmomentobjlist.c:85
frame #3: 0x00000008013984e2 libvirt.so.0`virDomainMomentForEachChild(moment=0x0000000802aae200, iter=(libvirt.so.0`virDomainMomentActOnDescendant at virdomainmomentobjlist.c:80), data=0x00007fffffffdc10) at virdomainmomentobjlist.c:63
frame #4: 0x000000080139854f libvirt.so.0`virDomainMomentForEachDescendant(moment=0x0000000802aae200, iter=(libvirt.so.0`testDomainSnapshotDiscardAll at test_driver.c:6445), data=0x00007fffffffdcf0) at virdomainmomentobjlist.c:105
frame #5: 0x000000080144bb2a libvirt.so.0`testDomainSnapshotDelete(snapshot=0x0000000802b46960, flags=4) at test_driver.c:6506
frame #6: 0x000000080156129e libvirt.so.0`virDomainSnapshotDelete(snapshot=0x0000000802b46960, flags=4) at libvirt-domain-snapshot.c:1047
frame #7: 0x00000000010a5c4d virsh`cmdSnapshotDelete(ctl=0x00007fffffffdf48, cmd=0x0000000802b39520) at virsh-snapshot.c:1921
frame #8: 0x00000000010af700 virsh`vshCommandRun(ctl=0x00007fffffffdf48, cmd=0x0000000802b39520) at vsh.c:1335
frame #9: 0x00000000010698b2 virsh`main(argc=4, argv=0x00007fffffffe088) at virsh.c:920
frame #10: 0x000000000106910b virsh`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1.c:76
(lldb) fr s 0
frame #0: 0x00000008013984bf libvirt.so.0`virDomainMomentForEachChild(moment=0x0000000802aaea10, iter=(libvirt.so.0`virDomainMomentActOnDescendant at virdomainmomentobjlist.c:80), data=0x00007fffffffdb30) at virdomainmomentobjlist.c:62
59 virDomainMomentObjPtr child = moment->first_child;
60
61 while (child) {
-> 62 virDomainMomentObjPtr next = child->sibling;
63 (iter)(child, child->def->name, data);
64 child = next;
65 }
(lldb) p child
(virDomainMomentObjPtr) $0 = 0x5a5a5a5a5a5a5a5a
(lldb) fr s 6
frame #6: 0x000000080156129e libvirt.so.0`virDomainSnapshotDelete(snapshot=0x0000000802b46960, flags=4) at libvirt-domain-snapshot.c:1047
1044 error);
1045
1046 if (conn->driver->domainSnapshotDelete) {
-> 1047 int ret = conn->driver->domainSnapshotDelete(snapshot, flags);
1048 if (ret < 0)
1049 goto error;
1050 return ret;
(lldb) p snapshot->name
(char *) $1 = 0x00000008010fe3e0 "s6"
(lldb)
I guess 0x5a5a5a5a5a5a5a5a is coming from jemalloc's junk option:
opt.junk (const char *) r- [--enable-fill]
Junk filling. If set to “alloc”, each byte of uninitialized
allocated memory will be initialized to 0xa5. If set to “free”, all
deallocated memory will be initialized to 0x5a. If set to “true”,
both allocated and deallocated memory will be initialized, and if
set to “false”, junk filling be disabled entirely. This is intended
for debugging and will impact performance negatively. This option
is “false” by default unless --enable-debug is specified during
configuration, in which case it is “true” by default.
(from jemalloc(3)).
Roman Bogorodskiy
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 4/7/19 6:16 AM, Roman Bogorodskiy wrote: > Eric Blake wrote: > >> Had this been in place earlier, I would have avoided the bugs in >> commit 0baf6945 and 55c2ab3e. Writing the test required me to extend >> the power of virsh - creating enough snapshots to cause fanout >> requires enough input in a single session that adding comments and >> markers makes it easier to check that output is correct. It's still a >> bit odd that with test:///default, reverting to a snapshot changes the >> domain from running to paused (possibly a bug in how the test driver >> copied from the qemu driver) - but the important part is that the test >> is reproducible, and any future tweaks we make to snapshot code have >> less chance of breaking successful command sequences. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> tests/Makefile.am | 3 +- >> tests/virsh-snapshot | 212 +++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 214 insertions(+), 1 deletion(-) >> create mode 100755 tests/virsh-snapshot > > Hi, > > I noticed this test is failing for me: Shoot, running under valgrind shows the same problem: +==54014== Invalid read of size 8 +==54014== at 0x540C802: virDomainMomentForEachChild (virdomainmomentobjlist.c:59) +==54014== by 0x540C914: virDomainMomentForEachDescendant (virdomainmomentobjlist.c:105) +==54014== by 0x540C8AB: virDomainMomentActOnDescendant (virdomainmomentobjlist.c:85) +==54014== by 0x540C832: virDomainMomentForEachChild (virdomainmomentobjlist.c:63) +==54014== by 0x540C914: virDomainMomentForEachDescendant (virdomainmomentobjlist.c:105) +==54014== by 0x54C7242: testDomainSnapshotDelete (test_driver.c:6506) +==54014== by 0x55D29BE: virDomainSnapshotDelete (libvirt-domain-snapshot.c:1047) +==54014== by 0x177CBE: cmdSnapshotDelete (virsh-snapshot.c:1921) +==54014== by 0x17EC2F: vshCommandRun (vsh.c:1335) +==54014== by 0x1347F6: main (virsh.c:920) +==54014== Address 0x102e1000 is 32 bytes inside a block of size 40 free'd +==54014== at 0x4C3013B: free (vg_replace_malloc.c:530) +==54014== by 0x52D0CB8: virFree (viralloc.c:581) +==54014== by 0x540CC62: virDomainMomentObjFree (virdomainmomentobjlist.c:210) +==54014== by 0x540CD94: virDomainMomentObjListDataFree (virdomainmomentobjlist.c:247) +==54014== by 0x53142CB: virHashRemoveEntry (virhash.c:533) +==54014== by 0x540D2E6: virDomainMomentObjListRemove (virdomainmomentobjlist.c:437) +==54014== by 0x540D95A: virDomainSnapshotObjListRemove (virdomainsnapshotobjlist.c:204) +==54014== by 0x54C702F: testDomainSnapshotDiscardAll (test_driver.c:6449) +==54014== by 0x540C88C: virDomainMomentActOnDescendant (virdomainmomentobjlist.c:84) +==54014== by 0x540C832: virDomainMomentForEachChild (virdomainmomentobjlist.c:63) +==54014== by 0x540C914: virDomainMomentForEachDescendant (virdomainmomentobjlist.c:105) +==54014== by 0x54C7242: testDomainSnapshotDelete (test_driver.c:6506) +==54014== Block was alloc'd at +==54014== at 0x4C30F26: calloc (vg_replace_malloc.c:711) +==54014== by 0x52D0483: virAlloc (viralloc.c:143) +==54014== by 0x540CB84: virDomainMomentObjNew (virdomainmomentobjlist.c:191) +==54014== by 0x540CCFF: virDomainMomentAssignDef (virdomainmomentobjlist.c:228) +==54014== by 0x540D577: virDomainSnapshotAssignDef (virdomainsnapshotobjlist.c:46) +==54014== by 0x54C6E3F: testDomainSnapshotCreateXML (test_driver.c:6396) +==54014== by 0x55D0952: virDomainSnapshotCreateXML (libvirt-domain-snapshot.c:241) +==54014== by 0x173680: virshSnapshotCreate (virsh-snapshot.c:51) +==54014== by 0x1743BC: cmdSnapshotCreateAs (virsh-snapshot.c:437) +==54014== by 0x17EC2F: vshCommandRun (vsh.c:1335) +==54014== by 0x1347F6: main (virsh.c:920) Looks like the problem is that while iterating over list we remove some elements and then call (some different) iterator over them. Eric? Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 4/7/19 1:32 AM, Michal Prívozník wrote: > On 4/7/19 6:16 AM, Roman Bogorodskiy wrote: >> Eric Blake wrote: >> >>> Had this been in place earlier, I would have avoided the bugs in >>> commit 0baf6945 and 55c2ab3e. Writing the test required me to extend >>> the power of virsh - creating enough snapshots to cause fanout >>> requires enough input in a single session that adding comments and >>> markers makes it easier to check that output is correct. It's still a >>> bit odd that with test:///default, reverting to a snapshot changes the >>> domain from running to paused (possibly a bug in how the test driver >>> copied from the qemu driver) - but the important part is that the test >>> is reproducible, and any future tweaks we make to snapshot code have >>> less chance of breaking successful command sequences. >>> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> --- >>> tests/Makefile.am | 3 +- >>> tests/virsh-snapshot | 212 +++++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 214 insertions(+), 1 deletion(-) >>> create mode 100755 tests/virsh-snapshot >> >> Hi, >> >> I noticed this test is failing for me: > > > Shoot, running under valgrind shows the same problem: > > > Looks like the problem is that while iterating over list we remove some elements and then call (some different) iterator over them. Eric? Looks like I have a use-after-free to debug; will get a patch out as soon as I can come up with the right fix. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Sat, Mar 23, 2019 at 11:02:03PM -0500, Eric Blake wrote: > Had this been in place earlier, I would have avoided the bugs in > commit 0baf6945 and 55c2ab3e. Writing the test required me to extend > the power of virsh - creating enough snapshots to cause fanout > requires enough input in a single session that adding comments and > markers makes it easier to check that output is correct. It's still a > bit odd that with test:///default, reverting to a snapshot changes the > domain from running to paused (possibly a bug in how the test driver > copied from the qemu driver) - but the important part is that the test > is reproducible, and any future tweaks we make to snapshot code have > less chance of breaking successful command sequences. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > tests/Makefile.am | 3 +- > tests/virsh-snapshot | 212 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 214 insertions(+), 1 deletion(-) > create mode 100755 tests/virsh-snapshot This has an unfortunate side-effect. When run it will try to create $HOME/.cache/libvirt/virsh This fails if $HOME is not writable - eg running in docker, where only the VPATH build dir is writable, nothing else: FAIL: virsh-snapshot ==================== --- exp 2019-03-27 13:27:56.956469010 +0000 +++ err 2019-03-27 13:27:56.952467010 +0000 @@ -1,2 +1,3 @@ error: invalid argument: parent s3 for snapshot s2 not found error: marker +error: Failed to create '/home/travis/.cache/libvirt/virsh': Permission denied FAIL virsh-snapshot (exit status: 1) I'm not sure the right answer for this - Require $HOME to be writable - Set a fake $HOME under $VPATH (but this won't match /etc/passwd) - Don't treat this as fatal Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 3/27/19 10:02 AM, Daniel P. Berrangé wrote: > On Sat, Mar 23, 2019 at 11:02:03PM -0500, Eric Blake wrote: >> Had this been in place earlier, I would have avoided the bugs in >> commit 0baf6945 and 55c2ab3e. Writing the test required me to extend >> the power of virsh - creating enough snapshots to cause fanout >> requires enough input in a single session that adding comments and >> markers makes it easier to check that output is correct. It's still a >> bit odd that with test:///default, reverting to a snapshot changes the >> domain from running to paused (possibly a bug in how the test driver >> copied from the qemu driver) - but the important part is that the test >> is reproducible, and any future tweaks we make to snapshot code have >> less chance of breaking successful command sequences. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> tests/Makefile.am | 3 +- >> tests/virsh-snapshot | 212 +++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 214 insertions(+), 1 deletion(-) >> create mode 100755 tests/virsh-snapshot > > This has an unfortunate side-effect. When run it will try to create > $HOME/.cache/libvirt/virsh > > This fails if $HOME is not writable - eg running in docker, where > only the VPATH build dir is writable, nothing else: > > FAIL: virsh-snapshot > ==================== > --- exp 2019-03-27 13:27:56.956469010 +0000 > +++ err 2019-03-27 13:27:56.952467010 +0000 > @@ -1,2 +1,3 @@ > error: invalid argument: parent s3 for snapshot s2 not found > error: marker > +error: Failed to create '/home/travis/.cache/libvirt/virsh': Permission denied > FAIL virsh-snapshot (exit status: 1) > > I'm not sure the right answer for this > > - Require $HOME to be writable I don't want to force this on our docker and other CI environments, but we can do it via: > - Set a fake $HOME under $VPATH (but this won't match /etc/passwd) This seems reasonable enough; could even be done as part of the virsh-snapshot test in isolation rather than globally in the Makefile for all tests. POSIX does not require HOME to stay constant, or to always match getent output (of course, when HOME and getent disagree, it's easier to get confused - but as long as we document it, we should be okay). > - Don't treat this as fatal This also seems nice - a cache is just that, after all; and failure to have a cache just means slower performance, but shouldn't mean extra noise. Since my test broke it, I'll play with ways to get rid of it before DV cuts the -rc2 build later this week. What is virsh trying to store in the cache, anyway? Is there a way to run virsh with a command line flag that says no caching is desired, so we don't even have to worry about where HOME points or whether it is writable? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.