Copilot commented on code in PR #14188:
URL: https://github.com/apache/cloudstack/pull/14188#discussion_r4217186548
##########
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:
`"30"` is a newly introduced magic number for the Ceph client mount timeout,
and it’s not obvious why this value is appropriate or whether it should be
configurable. Suggested fix: extract this into a named constant (with a short
comment referencing the intended behavior) or source it from an existing
configuration setting used elsewhere in the KVM/Ceph integration so operators
can tune it without code changes.
##########
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:
`Rados` is constructed with `pool.getAuthUserName()` even when that value
can be `null` (the later branch explicitly handles "no cephx user"). Passing
`null` here can cause an immediate failure (NPE/exception) before reaching the
guarded configuration. Also, the guard for setting `"key"` checks
`getAuthUserName()` but the comment states the abort is triggered by a null
*key* value; if `getAuthSecret()` is null while a username is
present/misconfigured, this would still call `confSet("key", null)` and risk
aborting the process. Suggested fix: derive a non-null client id for the
`Rados` constructor (or use a no-arg/default constructor if available) and
guard the `"key"` confSet on `pool.getAuthSecret()` being non-null (and ideally
non-empty), while keeping the unauthenticated `"auth_client_required" = "none"`
path for the no-cephx case.
##########
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:
The error logging eagerly builds strings via concatenation and does not
include the exception as a parameter, which can drop the stack trace depending
on logger configuration and adds avoidable overhead. Suggested fix: use
parameterized logging and pass the exception (e.g., include `e` as the last
argument) and consider throwing `new CloudRuntimeException(message, e)` with a
message that preserves the return value and error text rather than
`e.toString()`.
--
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]
