bhouse-nexthop commented on code in PR #14188:
URL: https://github.com/apache/cloudstack/pull/14188#discussion_r4217708593
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java:
##########
@@ -696,6 +709,74 @@ public KVMPhysicalDisk getPhysicalDisk(String volumeUuid,
KVMStoragePool pool) {
}
}
+ /**
+ * Looks an RBD volume up directly through librbd.
+ *
+ * The size reported by librbd is used for both the size and the virtual
size of
+ * the disk, matching what this class already does after converting an
image into
+ * an RBD volume.
+ */
+ private KVMPhysicalDisk getRbdPhysicalDisk(String volumeUuid,
LibvirtStoragePool pool) {
+ Rados r = new Rados(pool.getAuthUserName());
+ try {
+ r.confSet("mon_host", pool.getSourceHost() + ":" +
pool.getSourcePort());
+ /*
+ * The secret is null when the pool has no cephx user, and librados
+ * aborts the process rather than returning an error if it is
handed a
+ * null value here.
+ */
+ if (pool.getAuthUserName() != null) {
+ r.confSet("key", pool.getAuthSecret());
+ } else {
+ r.confSet("auth_client_required", "none");
+ }
Review Comment:
Checked both parts against the code; leaving this as is.
- `new Rados(null)` is only reached for a pool with no cephx user, and
librados' `rados_create()` accepts a NULL id (it falls back to the default
client). It is the same constructor call the five other RADOS connections in
this class make.
- The username and secret are set together in `getStoragePool()`: the secret
is read from the libvirt secret for that user, so it is never null when a
username is present. Checking the username is therefore the same condition as
checking the secret, and it is the one the comment describes (no cephx user
means no secret). This guard is also the only one in the class: the other five
call sites pass the key unconditionally.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java:
##########
@@ -696,6 +709,74 @@ public KVMPhysicalDisk getPhysicalDisk(String volumeUuid,
KVMStoragePool pool) {
}
}
+ /**
+ * Looks an RBD volume up directly through librbd.
+ *
+ * The size reported by librbd is used for both the size and the virtual
size of
+ * the disk, matching what this class already does after converting an
image into
+ * an RBD volume.
+ */
+ private KVMPhysicalDisk getRbdPhysicalDisk(String volumeUuid,
LibvirtStoragePool pool) {
+ Rados r = new Rados(pool.getAuthUserName());
+ try {
+ r.confSet("mon_host", pool.getSourceHost() + ":" +
pool.getSourcePort());
+ /*
+ * The secret is null when the pool has no cephx user, and librados
+ * aborts the process rather than returning an error if it is
handed a
+ * null value here.
+ */
+ if (pool.getAuthUserName() != null) {
+ r.confSet("key", pool.getAuthSecret());
+ } else {
+ r.confSet("auth_client_required", "none");
+ }
+ r.confSet("client_mount_timeout", "30");
Review Comment:
`client_mount_timeout` = 30 is what the five other RADOS connections in this
class already set. Pulling it into a constant, or making it configurable, would
be worth doing for all six together in a separate change; here I kept it
consistent with them.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java:
##########
@@ -696,6 +709,74 @@ public KVMPhysicalDisk getPhysicalDisk(String volumeUuid,
KVMStoragePool pool) {
}
}
+ /**
+ * Looks an RBD volume up directly through librbd.
+ *
+ * The size reported by librbd is used for both the size and the virtual
size of
+ * the disk, matching what this class already does after converting an
image into
+ * an RBD volume.
+ */
+ private KVMPhysicalDisk getRbdPhysicalDisk(String volumeUuid,
LibvirtStoragePool pool) {
+ Rados r = new Rados(pool.getAuthUserName());
+ try {
+ r.confSet("mon_host", pool.getSourceHost() + ":" +
pool.getSourcePort());
+ /*
+ * The secret is null when the pool has no cephx user, and librados
+ * aborts the process rather than returning an error if it is
handed a
+ * null value here.
+ */
+ if (pool.getAuthUserName() != null) {
+ r.confSet("key", pool.getAuthSecret());
+ } else {
+ r.confSet("auth_client_required", "none");
+ }
+ r.confSet("client_mount_timeout", "30");
+ r.connect();
+
+ IoCTX io = r.ioCtxCreate(pool.getSourceDir());
+ try {
+ Rbd rbd = new Rbd(io);
+ // The image is only stat'ed, so it is opened read only and
cannot take the exclusive lock.
+ RbdImage image = rbd.openReadOnly(volumeUuid);
+ try {
+ RbdImageInfo rbdInfo = image.stat();
+ KVMPhysicalDisk disk = new
KVMPhysicalDisk(pool.getSourceDir() + "/" + volumeUuid, volumeUuid, pool);
+ disk.setFormat(PhysicalDiskFormat.RAW);
+ disk.setSize(rbdInfo.size);
+ disk.setVirtualSize(rbdInfo.size);
+ return disk;
+ } finally {
+ closeRbdImage(rbd, image, volumeUuid);
+ }
+ } finally {
+ r.ioCtxDestroy(io);
+ }
+ } catch (RadosException e) {
+ logger.error("A Ceph RADOS operation failed (" +
e.getReturnValue() + "). The error was: " + e.getMessage()
+ + " - " + ErrorCode.getErrorMessage(e.getReturnValue()));
+ throw new CloudRuntimeException(e.toString(), e);
+ } catch (RbdException e) {
+ logger.error("A Ceph RBD operation failed (" + e.getReturnValue()
+ "). The error was: " + e.getMessage()
+ + " - " + ErrorCode.getErrorMessage(e.getReturnValue()));
+ throw new CloudRuntimeException(e.toString(), e);
Review Comment:
This uses the same message format as the existing RADOS/RBD error handlers
in this class, and the exception is not dropped: it is chained as the cause of
the `CloudRuntimeException` that is thrown. Leaving it consistent with the
surrounding code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
