Repository navigation
utils,kvm: build every Ceph monitor list with RbdUtils - #14402
weizhouapache wants to merge 3 commits into
Conversation
Sorry, something went wrong.
Everything that talks to Ceph needs the pool's monitors as a list the client can parse, and the rules are the same wherever that list is built: give each monitor the pool's port unless it names one already, and put a bare IPv6 address in square brackets so its own colons stay distinguishable from the separator before the port. The mon_host option of a disk's rbd: string wants the same list again, with ':' and ';' escaped, because those two separate the options of that string and its monitors. None of it depends on a storage pool, so collect it in RbdUtils as static methods with their unit tests, rather than leaving each caller to grow a copy that is right in a different set of cases.
The agent assembled the monitor list in several places, each with its own idea of the rules. Opening a RADOS connection, or writing a ceph.conf for one, appended the pool's port to the host string as a whole, so with several monitors only the last was given it and a monitor that already named its port was handed a second. The rbd: option string of a disk escaped the colons of an IPv6 address only when it was bracketed already, which left a bare one reading as though it carried options of its own. Use the shared helpers for all of them. Two inputs therefore come out differently, both previously wrong: a monitor that names its port keeps just that one, and a bare IPv6 address is bracketed before anything is appended to it or escaped in it. The iscsiadm portals in IscsiAdmStorageAdaptor read the same way but are not monitor lists, so they keep their own formatting.
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 65.85366% with 14 lines in your changes missing coverage. Please review. @@ Coverage Diff @@
## main #14402 +/- ##
============================================
+ Coverage 20.21% 20.39% +0.18%
- Complexity 20749 21288 +539
============================================
Files 6426 6482 +56
Lines 580181 588055 +7874
Branches 71033 72026 +993
============================================
+ Hits 117292 119956 +2664
- Misses 450174 455070 +4896
- Partials 12715 13029 +314
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19556 |
Sorry, something went wrong.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open findingscomposeOptionForMonHosts escapes : in each monitor and uses an escaped \; as the join… · New
addPortToRbdMonitor is a public API but has no Javadoc describing accepted input formats (e.g.,… · New
Centralizes Ceph RBD monitor-list formatting into RbdUtils to fix incorrect port application across multiple monitors and ensure bare IPv6 monitors become parseable, then migrates KVM/Ceph callers to use the shared implementation.
Changes:
- Introduces RbdUtils helpers to normalize monitor lists (per-monitor port application, IPv6 bracketing) and compose the escaped rbd: mon_host option.
- Replaces ad-hoc mon_host/rbd: monitor formatting across KVM/RBD call sites with RbdUtils.
- Adds a dedicated RbdUtilsTest suite covering many IPv4/IPv6/port/escaping combinations.
| File | Description |
|---|---|
| utils/src/main/java/com/cloud/utils/RbdUtils.java | Adds centralized monitor list normalization and rbd: option escaping helpers. |
| utils/src/test/java/com/cloud/utils/RbdUtilsTest.java | Adds unit tests for monitor formatting, IPv6 bracketing, and rbd: escaping behavior. |
| plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/utils/rbd/RbdEncryption.java | Switches monitor spec generation to RbdUtils.getRbdMonitors. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java | Uses RbdUtils.getRbdMonitors for Rados.confSet("mon_host", ...). |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java | Uses RbdUtils.getRbdMonitors for Ceph mon_host configuration. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMPhysicalDisk.java | Replaces local mon_host option composer with RbdUtils.composeOptionForMonHosts. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java | Uses RbdUtils.getRbdMonitors when connecting to RBD pools. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtManageSnapshotCommandWrapper.java | Uses RbdUtils.getRbdMonitors for Rados connections. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtBackupSnapshotCommandWrapper.java | Uses RbdUtils.getRbdMonitors for Rados connections. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
composeOptionForMonHosts escapes every ':' of a monitor, because the colons separate the options of the rbd: string. That includes the colons of an IPv6 address, also when the pool has no port to add. Test that case, and make addPortToRbdMonitor, which is only used by RbdUtils itself, protected.
There was a problem hiding this comment.
🔵 Needs a closer look
Native Ceph acceptance of the changed multi-monitor and IPv6 strings remains unverified and warrants human validation before merge.
0 open findings
3 resolved since last review
🧠 Review effort: Balanced
Sorry, something went wrong.
Description
The agent builds the list of Ceph monitors of an RBD pool in about fifteen places: every
confSet("mon_host", ...) before opening a RADOS connection, the ceph.conf written for the
rbd CLI, and the mon_host= option of the rbd: string a disk is attached by. Each of them
spelled the rules out again, and they did not agree.
Two of those spellings are wrong today:
appends the port to the host string as a whole, so a pool with monitors a,b,c and port 3300
yields a,b,c:3300: only c is given 3300, while a and b fall back to the default. A pool
on the default port hides this; one on 3300 does not.
fd00::1:6789, in which the address' own colons cannot be told from the separator. Only an
address already written in square brackets was handled.
A third, smaller one: a monitor that already names its port was handed the pool's as well.
This PR puts the rules in one place, RbdUtils:
unless it names one, and a bare IPv6 address is bracketed first.
: and ; escaped, since those separate the options of that string and its monitors. It is
built on getRbdMonitors, so it inherits the two fixes above.
All callers then use it: 14 Ceph sites across LibvirtStorageAdaptor, KVMStorageProcessor and
the backup, manage and revert snapshot wrappers, plus KVMPhysicalDisk and RbdEncryption.
The iscsiadm portals in IscsiAdmStorageAdaptor read the same way but are not monitor lists,
so they keep their own formatting.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
How Has This Been Tested?
Unit tests only. This has not been exercised against a live Ceph cluster with several
monitors or with IPv6 monitors, which is where the two defects above show themselves.
bracketed IPv6, bare IPv6 and a mixed list; a monitor that already names its port; entries that
need trimming or are empty; and the escaped rbd: form of each of those.
rbd: string, including the three-monitor and bracketed-IPv6 shapes, so they show the disk
string callers see is byte-for-byte what it was.
How did you try to break this feature and the system with this change?
The inputs that decide the behaviour are the shape of the monitor string and whether a port is
present, so the tests drive those directly: an empty port, a port already on the monitor, an
address with no colons, one with a single colon (host and port), and one with several (IPv6).
The last distinction is the load-bearing one — addPortToRbdMonitor tells an IPv4 host:port
from a bare IPv6 address by counting colons — and it is covered from both sides.
What the unit tests cannot show is that librados and the rbd CLI accept the strings now
produced. The shapes are the documented ones ([v6]:port, comma separated), and the disk string
is unchanged for the cases already pinned, but confirmation against a real multi-monitor and an
IPv6 cluster would be worth having before merge.