Bug 1534396 - Type mismatch between QAPI schema and generated json:{} filenames
Summary: Type mismatch between QAPI schema and generated json:{} filenames
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Red Hat Enterprise Linux Advanced Virtualization
Classification: Red Hat
Component: qemu-kvm
Version: ---
Hardware: x86_64
OS: Linux
low
medium
Target Milestone: rc
: ---
Assignee: Virtualization Maintenance
QA Contact: zixchen
URL:
Whiteboard:
Depends On:
Blocks: 1540290
TreeView+ depends on / blocked
 
Reported: 2018-01-15 07:08 UTC by Meina Li
Modified: 2021-02-23 05:59 UTC (History)
15 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
: 1540290 (view as bug list)
Environment:
Last Closed: 2021-02-15 07:34:37 UTC
Type: Bug
Target Upstream Version:
Embargoed:


Attachments (Terms of Use)

Description Meina Li 2018-01-15 07:08:15 UTC
Description of problem:
The iscsi lun number in source name will be changed after guest restart with external snapshot.

Version-Release number of selected component (if applicable):
libvirt-3.9.0-7.el7.x86_64
qemu-kvm-rhev-2.10.0-15.el7.x86_64

How reproducible:
100%

Steps to Reproduce:
1. Prepare a guest with iscsi network disk.
# virsh dumpxml rhel7 | grep disk -a6
... <disk type='network' device='disk'>
     <driver name='qemu' type='qcow2'/>
     <source protocol='iscsi' name='iqn.2003-01.org.linux-iscsi.localhost.x8664:sn.9cba196611e6/2'>
       <host name='**IP**' port='3260'/>
       <auth username='redhat'>
         <secret type='iscsi' usage='libvirtiscsi'/>
       </auth>
     </source>
     <target dev='vda' bus='virtio'/>
     <alias name='virtio-disk0'/>
     <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
   </disk>...

2. Create external disk and check xml.
# for i in 1 2 3 4;do virsh snapshot-create-as rhel7 s$i --disk-only --diskspec vda,file=/tmp/rhel7.s$i;done
# virsh dumpxml rhel7 | grep disk -a12 
... <disk type='file' device='disk'>
     <driver name='qemu' type='qcow2'/>
     <source file='/tmp/rhel7.s4'/>
     <backingStore type='file' index='1'>
       <format type='qcow2'/>
       <source file='/tmp/rhel7.s3'/>
       <backingStore type='file' index='2'>
         <format type='qcow2'/>
         <source file='/tmp/rhel7.s2'/>
         <backingStore type='file' index='3'>
          <format type='qcow2'/>
           <source file='/tmp/rhel7.s1'/>
           <backingStore type='network' index='4'>
            <format type='qcow2'/>
             <source protocol='iscsi' name='iqn.2003-01.org.linux-iscsi.localhost.x8664:sn.9cba196611e6/2'>
               <host name='**IP**' port='3260'/>
             </source>
           </backingStore>...

3. Restart guest and check guest xml.
# virsh destroy rhel7
# virsh start rhel7
# virsh dumpxml rhel7 | grep disk -a12
... <disk type='file' device='disk'>
     <driver name='qemu' type='qcow2'/>
     <source file='/tmp/rhel7.s4'/>
         <backingStore type='file' index='2'>
          <format type='qcow2'/>
           <source file='/tmp/rhel7.s1'/>
           <backingStore type='network' index='3'>
            <format type='qcow2'/>
             <source protocol='iscsi' name='iqn.2003-01.org.linux-iscsi.localhost.x8664:sn.9cba196611e6/0'>     
               <host name='**IP**' port='3260'/>
             </source>                                                    --
           </backingStore>...

Actual results:
As above step 3.

Expected results:
# virsh dumpxml rhel7 | grep disk -a12
… <backingStore type='network' index='2'>
            <format type='qcow2'/>
             <source protocol='iscsi' name='iqn.2003-01.org.linux-iscsi.localhost.x8664:sn.9cba196611e6/2'>
               <host name='**IP**' port='3260'/>
             </source>
           </backingStore>...

Additional info:
After blockcommit/blockpull/blockcopy, it also will be changed.

Comment 2 Peter Krempa 2018-01-25 12:49:02 UTC
Could you please post output of 'qemu-img info /tmp/rhel7.s1' when that reproduces? it looks like we parse something wrong from the file on the disk

Comment 3 Meina Li 2018-01-26 02:26:37 UTC
After restart guest, check the info:
# qemu-img info /tmp/lmn.s1 -U  --backing-chain
image: /tmp/lmn.s1
file format: qcow2
virtual size: 5.0G (5368709120 bytes)
disk size: 196K
cluster_size: 65536
backing file: json:{"driver": "qcow2", "file": {"lun": "2", "portal": "**IP**:3260", "driver": "iscsi", "transport": "tcp", "target": "iqn.2003-01.org.linux-iscsi.localhost.x8664:sn.9cba196611e6"}}
backing file format: qcow2
Format specific information:
    compat: 1.1
    lazy refcounts: false
    refcount bits: 16
    corrupt: false

image: json:{"driver": "qcow2", "file": {"lun": "2", "portal": "**IP**:3260", "driver": "iscsi", "transport": "tcp", "target": "iqn.2003-01.org.linux-iscsi.localhost.x8664:sn.9cba196611e6"}}
file format: qcow2
virtual size: 5.0G (5368709120 bytes)
disk size: unavailable
cluster_size: 65536
Format specific information:
    compat: 1.1
    lazy refcounts: false
    refcount bits: 16
    corrupt: false

Comment 4 Peter Krempa 2018-01-29 14:39:38 UTC
Thanks for the information.

In the backing json string the 'lun' field is clearly a string:

json:{"driver": "qcow2", "file": {"lun": "2", "portal": "**IP**:3260", "driver": "iscsi", "transport": "tcp", "target": "iqn.2003-01.org.linux-iscsi.localhost.x8664:sn.9cba196611e6"}}

while the documentation in qemu (qapi/block-core.json) clearly states that 'lun' is an int:

{ 'struct': 'BlockdevOptionsIscsi',
  'data': { 'transport': 'IscsiTransport',
            'portal': 'str',
            'target': 'str',
            '*lun': 'int',
            '*user': 'str',
            '*password-secret': 'str',
            '*initiator-name': 'str',
            '*header-digest': 'IscsiHeaderDigest',
            '*timeout': 'int' } }


Libvirt is parsing it as an int and thus fails to find it and uses the default (0).

Reassigning to qemu.

Comment 7 Kevin Wolf 2018-01-30 15:45:57 UTC
Well, nobody told libvirt to parse this string or that it matches the BlockdevOptionsIscsi structure... qemu accepts both strings and integers in json: strings because internally everything is converted into strings anyway (we're in the -drive legacy code path here, so no QAPI schema involved at all). Consequently, the generated json: strings treat all values as strings, too.

Maybe this can be improved by going through a real QAPI object using the keyval visitor, if we can assume that the content always actually contains everything that the schema requires. In that case you'd actually get JSON numbers for integers in the schema.

If that doesn't work and we actually need to use QAPI objects internally (which is something that we want, but it's really hard), don't hold your breath. It might be a few years then.

Either way, if libvirt parses the strings, it should probably accept both integers and strings as we won't be able to retroactively change all qemu versions from 2.1 where the feature was introduced.

Comment 8 Peter Krempa 2018-01-30 17:25:53 UTC
(In reply to Kevin Wolf from comment #7)
> Well, nobody told libvirt to parse this string or that it matches the
> BlockdevOptionsIscsi structure... qemu accepts both strings and integers in
> json: strings because internally everything is converted into strings anyway
> (we're in the -drive legacy code path here, so no QAPI schema involved at
> all). Consequently, the generated json: strings treat all values as strings,
> too.

Well the fact that we parse it is a result of qemu starting to put those strings into the backing store field in the qcow images. Libvirt uses those to reconstruct the backing chain and since there's no other documentation than the QAPI schema it makes sense that people will use it.

Okay I'll clone this BZ to fix the JSON parsers to accept both strings and numbers for the 'int' fields.

Comment 9 Kevin Wolf 2018-02-01 21:24:02 UTC
(In reply to Peter Krempa from comment #8)
> Libvirt uses those to reconstruct the backing chain

Do you still need to reconstruct the backing chain after the blockdev work?

Comment 10 Peter Krempa 2018-02-02 10:46:08 UTC
Yes that will still need to be used. The blockdev work will allow users to specify the full backing chain, but we really can't force them to specify it fully. This means that we'll still need to support the case when the user specifies only the top level image and libvirt will need to reconstruct the chain for filesystem labelling purposes.

Additionally we can't even record this into the persistent XML used for the next start since some usecases involve relatively linked images which can be accessed using multiple storage protocols and in such case we need to support users changing only the top level image path while the rest still needs to work.

Comment 11 Hanna Czenczek 2018-02-05 21:09:15 UTC
For the record, a simpler reproducer is:

$ qemu-img info --image-opts driver=null-co,size=512
image: json:{"driver": "null-co", "size": "512"}
[...]

@size is an int, so its value should be numeric, too.


Moving to 7.6 because libvirt seems to have its workaround, and qemu itself can cope with false strings in json:{} filenames.

Comment 12 Hanna Czenczek 2018-05-09 18:14:57 UTC
Sent an upstream series, although I’d say it’s in rather questionable state: http://lists.nongnu.org/archive/html/qemu-block/2018-05/msg00290.html

Comment 13 Ademar Reis 2020-02-05 22:46:28 UTC
QEMU has been recently split into sub-components and as a one-time operation to avoid breakage of tools, we are setting the QEMU sub-component of this BZ to "General". Please review and change the sub-component if necessary the next time you review this BZ. Thanks

Comment 19 RHEL Program Management 2021-02-15 07:34:37 UTC
After evaluating this issue, there are no plans to address it further or fix it in an upcoming release.  Therefore, it is being closed.  If plans change such that this issue will be fixed in an upcoming release, then the bug can be reopened.

Comment 20 zixchen 2021-02-23 03:58:52 UTC
Tested qemu-img-5.2.0-7.module+el8.4.0+9943+d64b3717.x86_64, 
# qemu-img info --image-opts driver=null-co,size=512
image: json:{"driver": "null-co", "size": "512"}
file format: null-co
virtual size: 512 B (512 bytes)
disk size: 0 B

Disk size is numberic, the issue is fixed, QE agrees to close this bug.

Comment 21 zixchen 2021-02-23 05:59:05 UTC
Correct my last comment, this issue still exits, but as libvirt has workaround, agrees to close it.


Note You need to log in before you can comment on or make changes to this bug.