On Tue, Aug 25, 2020 at 2:55 AM Han Han <hhan@redhat.com> wrote:
>
>
>
> On Mon, Aug 24, 2020 at 11:09 PM Jason Dillaman <jdillama@redhat.com> wrote:
>>
>> On Mon, Aug 24, 2020 at 10:52 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>> >
>> > On Mon, Aug 24, 2020 at 10:19:59PM +0800, Han Han wrote:
>> > > On Fri, Aug 21, 2020 at 8:01 PM Jason Dillaman <jdillama@redhat.com> wrote:
>> > >
>> > > > On Fri, Aug 7, 2020 at 5:50 AM Han Han <hhan@redhat.com> wrote:
>> > > > >
>> > > > > Diff from v3:
>> > > > > - add the check for capability of rbd namespace
>> > > > > - rename the item of rbd namespace in disk source struct
>> > > > > - combine the commit of doc into the commit of patch
>> > > > > - remove the code for -drive
>> > > > >
>> > > > > gitlab branch:
>> > > > > https://gitlab.com/hhan2/libvirt/-/commits/rbd-namespace-v4
>> > > > >
>> > > > > Han Han (4):
>> > > > >   qemu_capabilities: Add QEMU_CAPS_RBD_NAMESPACE
>> > > > >   conf: Support to parse rbd namespace attribute
>> > > > >   qemu: Implement rbd namespace attribute
>> > > > >   news: qemu: Support rbd namespace
>> > > > >
>> > > > >  NEWS.rst                                      |  6 +++
>> > > > >  docs/formatdomain.rst                         |  5 ++-
>> > > > >  docs/schemas/domaincommon.rng                 |  3 ++
>> > > > >  src/conf/domain_conf.c                        |  4 ++
>> > > > >  src/qemu/qemu_block.c                         |  1 +
>> > > > >  src/qemu/qemu_capabilities.c                  |  4 ++
>> > > > >  src/qemu/qemu_capabilities.h                  |  3 ++
>> > > > >  src/qemu/qemu_domain.c                        |  8 ++++
>> > > > >  src/util/virstoragefile.h                     |  1 +
>> > > > >  .../caps_5.0.0.aarch64.xml                    |  1 +
>> > > > >  .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml |  1 +
>> > > > >  .../caps_5.0.0.riscv64.xml                    |  1 +
>> > > > >  .../caps_5.0.0.x86_64.xml                     |  1 +
>> > > > >  .../caps_5.1.0.x86_64.xml                     |  1 +
>> > > > >  ...k-network-rbd-namespace.x86_64-latest.args | 41 +++++++++++++++++++
>> > > > >  .../disk-network-rbd-namespace.xml            | 33 +++++++++++++++
>> > > > >  tests/qemuxml2argvtest.c                      |  1 +
>> > > > >  ...sk-network-rbd-namespace.x86_64-latest.xml | 41 +++++++++++++++++++
>> > > > >  tests/qemuxml2xmltest.c                       |  1 +
>> > > > >  19 files changed, 156 insertions(+), 1 deletion(-)
>> > > > >  create mode 100644
>> > > > tests/qemuxml2argvdata/disk-network-rbd-namespace.x86_64-latest.args
>> > > > >  create mode 100644 tests/qemuxml2argvdata/disk-network-rbd-namespace.xml
>> > > > >  create mode 100644https://www.spinics.net/linux/fedora/libvir/msg201067.html
>> > > > tests/qemuxml2xmloutdata/disk-network-rbd-namespace.x86_64-latest.xml
>> > > > >
>> > > > > --
>> > > > > 2.27.0
>> > > > >
>> > > >
>> > > > Hopefully you still plan to add a "pool" attribute in a future series
>> > > > to help split-up the overloaded "pool/image" name attribute.
>> > > >
>> > > >From my opinions, I think it's ok to keep "pool/image" in the name
>> > > attribute if the meaning of this attribute
>> > > is clarified in libvirt docs.
>> > > Currently I have no plan to split the "pool/image".
>>
>>
>> The problem is that having separate "<pool>/<image>" and "<pool-name>"
>
> <pool-name> ? I am confused here. Do you mean the pool-namespace?

Yes

>>
>> attributes is semi-nonsensicle. At that point, you might as well just
>
> I think the only separated namespace is sensible here. Because there are 3 ways
> to express the rbd image path with namespace:

Except "pool-namespace" is not a standalone property, it's tied to the
pool that you are hiding in the "name". A "pool-namespace" of "ns1" in
pool "pool1" is not the same as the namespace "ns1" in pool "pool2".
Like I said, you seem to be developing your own unique RBD image-spec
format.

> 1. <pool>/<namespace>/<image>
> e.g: the rbd info comand and the qemu-img comand with legacy rbd path:
> ➜  ~ rbd -c ~/.ceph/ceph.conf -k ~/.ceph/ceph.client.admin.keyring info rbd/hhan/1
> rbd image '1':
>         size 100 MiB in 25 objects

$ rbd namespace create rbd/ns1
$ rbd create --size 1G rbd/ns1/image1
$ rbd info rbd/ns1/image1
rbd image 'image1':
    size 1 GiB in 256 objects
... snip ...

The rbd CLI will always treat that middle section as a namespace so
for your example it would be pool rbd, namespace hhan, and image name
1.

>  ...
> ➜  ~ qemu-img info rbd:rbd/hhan/1:conf=/home/hhan/.ceph/ceph.conf:id=admin:key=XXXXXXX
> image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "1", "conf": "/home/hhan/.ceph/ceph.conf", "driver": "rbd", "user": "admin"}}
> file format: raw
> ...
> Note that the missing namespace attribute in image json is caused by https://bugzilla.redhat.com/show_bug.cgi?id=1821528
>
> 2. only separated namespace: <pool>/><image> and namespace attribute or option
> e.g: the rbd command with namespace option
> ➜  ~ rbd -c ~/.ceph/ceph.conf -k ~/.ceph/ceph.client.admin.keyring info rbd/1 --namespace hhan
> rbd image '1':
>         size 100 MiB in 25 objects
>         order 22 (4 MiB objects)
> ...
>
> 3. separated pool, namespace, image
> e.g: qemu blockdev options of rbd: https://github.com/qemu/qemu/blob/30aa19446d82358a30eac3b556b4d6641e00b7c1/qapi/block-core.json#L3585
> ➜  ~ qemu-img info 'json:{"driver": "raw", "file": {"pool": "rbd", "image": "1", "conf": "/home/hhan/.ceph/ceph.conf", "driver": "rbd", "user": "admin", "namespace":"hhan"}}'
> image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "1", "conf": "/home/hhan/.ceph/ceph.conf", "driver": "rbd", "user": "admin"}}
> file format: raw
> ...
>
>
> From these precedents, I think it is no problem to use the 2nd pattern in libvirt XML.
> I reject the 1st pattern because of compat issues.
> Suppose the 1st pattern is used, it will cause problems in the following case.
> Since rbd allows the image name contains '/'

That's not true, RBD has not permitted "/" in pool nor image names for
years -- and it's definitely not allowed when creating images in
namespaces:

$ rbd create --size 1G rbd/ns1/image1/
rbd: invalid spec 'rbd/ns1/image1/'
$ rbd create --size 1G --pool rbd --namespace ns1 --image "image1/"
rbd: invalid spec 'image1/'

Is this a bug in "qemu-img" that allowed you to create "hhan/2" below?
Does it allow you to create that image in a namespace or does your
example no longer work? QEMU should also parse that middle section as
the namespace [1].

> ➜  ~ rbd -c ~/.ceph/ceph.conf -k ~/.ceph/ceph.client.admin.keyring -p rbd ls
> attach-new
> copy
> hhan/2
>
> If I used 'hhan/2' image in libvirt XML at the previous libvirt and then I updated libvirt to the version support 1st pattern,
> the new libvirt parse the name='rbd/hhan/2' as pool 'rbd', namespace 'hhan', image '2',instead of the pool 'rbd', image
> 'hhan/2', default namespace.
>
>
> For the 3rd pattern, separating all the attributes, the xml will look like
> <source protocol="rbd" pool="POOL" image="IMG" namespace="NS">
>
> However it cannot replace the old attribute name='<pool>/<image>' because of the compatibility.
> What about keeping the old attribute the adapting this new pattern?
> Well, it looks weird only rbd protocol adapts this new pattern because all the network protocols in libvirt
> use the old xml pattern <source protocol="PROTO" name="XX"> (seeing the examples in https://libvirt.org/formatdomain.html#hard-drives-floppy-disks-cdroms)
> How about adapting this new pattern to all the network protocols?
> Considering the effort and the benifits of that, I think it is not worthwhile.
>
>> drop the "pool_namespace" attribute" and parse the image name just
>> like the rest of Ceph/RBD code does as
>> "[<pool-name>/[<pool_namespace>/]]<image-name>". Why should libvirt
>> invent its own custom way to describe the image location?
>>
>> See this thread here [1] from back in April where you said you would
>> split it out.
>>
> Yes.
> The above is why I changed my mind and decided to use the only separated attribute namespace.
>>
>> > That would create back compat issues too, so I can't see us splitting
>> > that.
>>
>> Yes, I understand the backwards compatibility concerns so you would
>> need to continue to support "<pool>/<image>", but you could at least
>> force the new format if a "<pool-namespace>" was specified.
>>
>>
>>
>> > 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 :|
>> >
>>
>> [1] https://www.spinics.net/linux/fedora/libvir/msg201067.html
>>
>> --
>> Jason
>>
>
>
> --
> Best regards,
> -----------------------------------
> Han Han
> Senior Quality Engineer
> Redhat.
>
> Email: hhan@redhat.com
> Phone: +861065339333

[1] https://github.com/qemu/qemu/blob/master/block/rbd.c#L150

--
Jason