On Tue, Aug 25, 2020 at 7:32 PM Jason Dillaman <jdill...@redhat.com> wrote:
> On Tue, Aug 25, 2020 at 2:55 AM Han Han <h...@redhat.com> wrote: > > > > > > > > On Mon, Aug 24, 2020 at 11:09 PM Jason Dillaman <jdill...@redhat.com> > wrote: > >> > >> On Mon, Aug 24, 2020 at 10:52 AM Daniel P. Berrangé < > berra...@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 <jdill...@redhat.com> > wrote: > >> > > > >> > > > On Fri, Aug 7, 2020 at 5:50 AM Han Han <h...@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 100644 > https://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/' > I am not sure how I created the image 'hhan/2' in the default namespace. It cannot be created by rbd command now. However it seems buggy on qemu-img(librbd1-12.2.7-9.el8.x86_64 qemu-img-5.1.0-3.module+el8.3.0+7708+740a1315.x86_64): ➜ ~ rbd -c ~/.ceph/ceph.conf -k ~/.ceph/ceph.client.admin.keyring namespace ls NAME hhan test ➜ ~ qemu-img create 'rbd:rbd/aa/new1:conf=/root/.ceph/ceph.conf:id=admin:key=AQBm9fldc9zhMhAAeDDedFhu55XjV1YhdqDOkQ==' 1M Formatting 'rbd:rbd/aa/new1:conf=/root/.ceph/ceph.conf:id=admin:key=AQBm9fldc9zhMhAAeDDedFhu55XjV1YhdqDOkQ==', fmt=raw size=1048576 ➜ ~ qemu-img create 'rbd:rbd/aa/new1:conf=/root/.ceph/ceph.conf:id=admin:key=AQBm9fldc9zhMhAAeDDedFhu55XjV1YhdqDOkQ==' 1M Formatting 'rbd:rbd/aa/new1:conf=/root/.ceph/ceph.conf:id=admin:key=AQBm9fldc9zhMhAAeDDedFhu55XjV1YhdqDOkQ==', fmt=raw size=1048576 qemu-img: rbd:rbd/aa/new1:conf=/root/.ceph/ceph.conf:id=admin:key=AQBm9fldc9zhMhAAeDDedFhu55XjV1YhdqDOkQ==: error rbd create: File exists ➜ ~ rbd -c ~/.ceph/ceph.conf -k ~/.ceph/ceph.client.admin.keyring namespace ls NAME hhan test ➜ ~ rbd -c ~/.ceph/ceph.conf -k ~/.ceph/ceph.client.admin.keyring -p rbd ls|grep new1 ➜ ~ qemu-img info rbd:rbd/aa/new1:conf=/root/.ceph/ceph.conf:id=admin:key=AQBm9fldc9zhMhAAeDDedFhu55XjV1YhdqDOkQ== image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "new1", "conf": "/root/.ceph/ceph.conf", "driver": "rbd", "user": "admin"}} file format: raw virtual size: 1 MiB (1048576 bytes) disk size: unavailable cluster_size: 4194304 Well, the image "rbd/aa/new1" could be found by qemu-img but cannot be listed by rbd. Very confusing here. Since the '/' in image name is invalid, the format of name attribute "<pool>/[pool-ns]<image>" is more sensible. I will adapt that in the next patch series. > 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: h...@redhat.com > > Phone: +861065339333 > > [1] https://github.com/qemu/qemu/blob/master/block/rbd.c#L150 > > -- > Jason > > -- Best regards, ----------------------------------- Han Han Senior Quality Engineer Redhat. Email: h...@redhat.com Phone: +861065339333