Repository navigation
utils,kvm: build every Ceph monitor list with RbdUtils - #14402
Draft
weizhouapache wants to merge 2 commits into
Draft
weizhouapache wants to merge 2 commits into
weizhouapache wants to merge 2 commits into
Conversation
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.
1 of 14 tasks
weizhouapache
changed the title
Member
Author
|
@blueorangutan package |
|
@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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14402 +/- ##
============================================
- Coverage 20.21% 20.20% -0.01%
- Complexity 20749 20753 +4
============================================
Files 6426 6427 +1
Lines 580181 580196 +15
Branches 71033 71038 +5
============================================
- Hits 117292 117257 -35
- Misses 450174 450238 +64
+ Partials 12715 12701 -14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19556 |
14 tasks
Contributor
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 findings
composeOptionForMonHostsescapes:in each monitor and uses an escaped\;as the join… · NewaddPortToRbdMonitoris a public API but has no Javadoc describing accepted input formats (e.g.,… · New
What changed in this PR
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
RbdUtilshelpers to normalize monitor lists (per-monitor port application, IPv6 bracketing) and compose the escapedrbd:mon_hostoption. - Replaces ad-hoc
mon_host/rbd:monitor formatting across KVM/RBD call sites withRbdUtils. - Adds a dedicated
RbdUtilsTestsuite 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.
|
|
||
| @Test | ||
| public void testComposeOptionForMonHostsWithoutAPortLeavesTheMonitorAlone() { | ||
| assertEquals("ceph-monitor", RbdUtils.composeOptionForMonHosts("ceph-monitor", 0)); |
Comment on lines
+49
to
+64
| public static String addPortToRbdMonitor(String monitor, int port) { | ||
| if (monitor.startsWith("[")) { | ||
| // IPv6 address in square brackets, which has a port if followed by ":<port>" | ||
| return monitor.contains("]:") ? monitor : monitor + ":" + port; | ||
| } | ||
| int colons = StringUtils.countMatches(monitor, ":"); | ||
| if (colons == 0) { | ||
| return monitor + ":" + port; | ||
| } | ||
| if (colons == 1) { | ||
| // IPv4 address or host name, with a port | ||
| return monitor; | ||
| } | ||
| // IPv6 address without square brackets, so without a port | ||
| return "[" + monitor + "]:" + port; | ||
| } |
Copilot stopped reviewing on behalf of
weizhouapache due to an error
October 15, 2026 10:59
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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, theceph.confwritten for therbdCLI, and themon_host=option of therbd:string a disk is attached by. Each of themspelled the rules out again, and they did not agree.
Two of those spellings are wrong today:
pool.getSourceHost() + ":" + pool.getSourcePort()appends the port to the host string as a whole, so a pool with monitors
a,b,cand port3300yields
a,b,c:3300: onlycis given 3300, whileaandbfall back to the default. A poolon the default port hides this; one on 3300 does not.
fd00::1with port 6789 becomesfd00::1:6789, in which the address' own colons cannot be told from the separator. Only anaddress 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:getRbdMonitors(host, port)— the list a client parses: each monitor gets the pool's portunless it names one, and a bare IPv6 address is bracketed first.
addPortToRbdMonitor(monitor, port)— the same decision for a single monitor.composeOptionForMonHosts(host, port)— that list again for therbd:option string, with:and;escaped, since those separate the options of that string and its monitors. It isbuilt on
getRbdMonitors, so it inherits the two fixes above.All callers then use it: 14 Ceph sites across
LibvirtStorageAdaptor,KVMStorageProcessorandthe backup, manage and revert snapshot wrappers, plus
KVMPhysicalDiskandRbdEncryption.The
iscsiadmportals inIscsiAdmStorageAdaptorread 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.
RbdUtilsTest, 17 cases, new in this PR: one monitor and several; with a port and without; IPv4,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.KVMPhysicalDiskTest, 4 cases, unchanged by this PR and still passing. These pin the wholerbd:string, including the three-monitor and bracketed-IPv6 shapes, so they show the diskstring callers see is byte-for-byte what it was.
plugins/hypervisors/kvmmodule: 918 tests, 0 failures, 0 errors.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 —
addPortToRbdMonitortells an IPv4host:portfrom 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
rbdCLI accept the strings nowproduced. The shapes are the documented ones (
[v6]:port, comma separated), and the disk stringis unchanged for the cases already pinned, but confirmation against a real multi-monitor and an
IPv6 cluster would be worth having before merge.