feat: bringing up a bridge-networked hypervisor by VasilyStepanov · Pull Request #172 · exordos/gcl_sdk

VasilyStepanov · 2026-07-17T11:20:30Z

The libvirt-based machine pool driver lives natively in gcl_sdk — LibvirtPoolDriver, lightweight Machine/MachineVolume/Port/MachinePool models, and PoolAgentDriver/LocalPoolAgentDriver (a
MetaCoordinatorAgentDriver) moved in from exordos_core, so the universal-agent driver contract owns the pool/volume/machine lifecycle directly. libvirt-python is an optional gcl_sdk[libvirt] extra,
not a hard dependency.
exordos_local_hyper pools resolve correctly — registered under the gcl_sdk_machine_pool_driver entry point.
LocalPoolAgentDriver no longer crashes on its own marker capability — local_pool is scheduling-only, not an actualizable resource kind.
Re-registering agents actually reach ACTIVE — the update path (taken whenever an agent's uuid already exists, e.g. its caps_drivers changed) now sends status, not just capabilities/facts/name.
Ports resolve correctly regardless of hypervisor network type — both the boot port and the real/final port always connect through a libvirt network object by its logical name (type='network'),
never treating that name as a literal host bridge device; and when libvirt rewrites a bridge-forwarding network's type in the live domain XML, parsing prefers the preserved logical name so reported
state doesn't permanently diverge from the orchestrator's target state.

gemini-code-assist

Code Review

This pull request introduces a new coordinator driver architecture to manage related capability kinds (such as VM pools, volumes, and machines) that must be reconciled together. It implements a concrete LibvirtPoolDriver using libvirt-python alongside a base PoolAgentDriver coordinator, and includes corresponding documentation, entry points, and unit tests. The review feedback focuses on enhancing the robustness of the XML parsing in the libvirt driver by adding defensive checks against potential AttributeError and IndexError exceptions, correcting typos, replacing a single-iteration while loop with an if statement, and replacing a bare except: clause with except Exception: to adhere to PEP 8 guidelines.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

VasilyStepanov · 2026-07-17T16:18:06Z

+[project.entry-points."gcl_sdk_machine_pool_driver"]
+dummy = "gcl_sdk.agents.universal.drivers.pool:DummyPoolDriver"
+libvirt = "gcl_sdk.agents.universal.drivers.libvirt:LibvirtPoolDriver"
+exordos_local_hyper = "gcl_sdk.agents.universal.drivers.libvirt:LibvirtPoolDriver"


A separate runtime driver implementation for exordos_local_hyper isn't actually needed — it's literally the same LibvirtPoolDriver class in both cases. The two KIND values exist for a different reason,
at the control-plane level:

ExordosLocalHyperDriverSpec(LibvirtPoolDriverSpec) adds a required node field (the agent's UUID). KindModelSelectorType uses KIND as the discriminator to decide which spec class to validate a payload
against — so it knows node is required for this variant but not for a plain libvirt one.

The scheduler (compute/scheduler/service.py) and the uniqueness check in HypervisorsController._validate_driver_spec_uniqueness branch on kind: for libvirt, uniqueness is by connection_uri; for
exordos_local_hyper, it's by node — the pool is pinned to one specific agent.

The wrinkle is that MetaPool.load_driver() reuses that same driver_spec.KIND string as the lookup key for the runtime driver via entry points, not just as the spec discriminator. So even though the
driver class is identical, a second entry-point registration was still required — which is the one-line fix I just added, mirroring the existing dummy/libvirt pattern already in pyproject.toml.

VasilyStepanov · 2026-07-18T12:18:57Z

+                # hypervisor's own (possibly bridge-type) main network. The
+                # real port replaces it later via attach_port(), which does
+                # use self._spec.network_type.
+                iface_type="network",


LibvirtPoolDriver.create_machine() applied the hypervisor's own network_type (bridge/network) to the initial boot-network port. The boot network is always a plain libvirt virtual network regardless of the hypervisor's main network type, so on a bridge-type hypervisor libvirt tried to treat the boot network's name as a literal host bridge device name and rejected it as too long for IFNAMSIZ. attach_port(), used for the real port attached later, is unaffected and continues to honor network_type.

VasilyStepanov · 2026-07-19T12:11:39Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a coordinator driver framework (MetaCoordinatorAgentDriver) to manage related resources that must be reconciled together, implementing a concrete LibvirtPoolDriver to manage VM pools, volumes, and machines. It also adds corresponding documentation, unit tests, and optional libvirt package dependencies. The review feedback focuses on enhancing the robustness of the new drivers by recommending defensive checks against potential AttributeError, TypeError, and IndexError during XML parsing, replacing raw integer error codes with standard libvirt constants, avoiding bare except: clauses, and guarding against uninitialized properties.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

- _domain2machine: fall back to standard libvirt <vcpu>/<currentMemory>/ <memory> elements when a domain lacks Genesis metadata (e.g. a pre-existing/foreign VM on a shared hypervisor) instead of crashing pool listing for the whole hypervisor. New _memory_mib() handles the unit conversion, since a foreign domain's <memory> isn't guaranteed to be in MiB like Genesis's own tag. - Fix the `docement` -> `document` typo in document_set_tag/ document_meta_set_tag. - _find_attached_volume_element and friends: use `.findall(".//devices/disk")` instead of `.find("devices").findall("disk")`, consistent with the rest of the file, so a domain without a <devices> element doesn't crash with AttributeError. - resize_volume: replace a while-loop-as-one-shot-if with an actual `if`, and guard against a disk with no <target dev=...>. - create_machine: raise a clear error instead of AttributeError when the storage pool's XML has no <target><path>. - rename_machine: `except:` -> `except Exception:` (PEP 8). - Use libvirt.VIR_ERR_NO_SUPPORT instead of the bare int 3. Verified two other flagged spots are not actually reachable and left alone: Element.get(None) returns None rather than raising (checked empirically), and MetaMachine.port_info defaults to `dict` at the model level, so it's never None to begin with.

VasilyStepanov · 2026-07-19T15:05:05Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a coordinator driver framework (MetaCoordinatorAgentDriver) to manage related resources that must be reconciled together, implementing a concrete LibvirtPoolDriver for VM pools, volumes, and machines, alongside documentation and unit tests. Key feedback on these changes includes resolving potential DOM manipulation errors when removing XML tags, handling VIR_ERR_NO_DOMAIN gracefully to make machine deletion idempotent, correcting a multicast MAC address generation bug to prevent network issues, refining exception handling in volume deletion to avoid silencing unexpected errors, and ensuring that inactive storage pools do not crash the storage pool listing.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

gemini-code-assist · 2026-07-19T15:07:34Z

+        if virtual_machine:
+            return "52:54:00:%02x:%02x:%02x" % octets[2:]
+
+        return "a9:%02x:%02x:%02x:%02x:%02x" % octets


The first octet a9 (binary 10101001) has the least significant bit set to 1, which designates it as a multicast MAC address. Assigning a multicast MAC address to a unicast port/interface can cause severe network issues (e.g., switches flooding traffic, packets being dropped, or other hosts ignoring the packets).

To ensure it is a unicast, locally administered MAC address, the first octet should be even (least significant bit = 0) and have the second least significant bit set to 1 (locally administered). For example, a2, aa, or ae are valid unicast locally administered prefixes.

Suggested change

return "a9:%02x:%02x:%02x:%02x:%02x" % octets

return "ae:%02x:%02x:%02x:%02x:%02x" % octets

gemini-code-assist · 2026-07-19T15:07:34Z

+
+        # TODO(akremenetsky): Need to update information about volumes
+        for p in _pools:
+            pool_element = ET.fromstring(p.XMLDesc())
+            pool_type = pool_element.get("type")
+
+            pools.append(
+                pool_base.ThinStoragePool(
+                    uuid=sys_uuid.UUID(p.UUIDString()),
+                    name=p.name(),
+                    capacity_usable=p.info()[1] >> 30,  # GB
+                    available_actual=p.info()[3] >> 30,  # GB
+                    pool_type=pool_type,
+                )
+            )


If any storage pool on the host is inactive or has issues, calling p.info() or p.XMLDesc() might raise a libvirt.libvirtError. This will crash the entire list_storage_pools call, preventing other active pools from being listed.

We should check if the pool is active and wrap the info retrieval in a try...except block to skip problematic/inactive pools gracefully.

# TODO(akremenetsky): Need to update information about volumes for p in _pools: try: if not p.isActive(): continue pool_element = ET.fromstring(p.XMLDesc()) pool_type = pool_element.get("type") pool_info = p.info() except libvirt.libvirtError: LOG.warning("Failed to get info for storage pool %s", p.name()) continue pools.append( pool_base.ThinStoragePool( uuid=sys_uuid.UUID(p.UUIDString()), name=p.name(), capacity_usable=pool_info[1] >> 30, # GB available_actual=pool_info[3] >> 30, # GB pool_type=pool_type, ) )

- _domain2machine: fall back to standard libvirt <vcpu>/<currentMemory>/ <memory> elements when a domain lacks Genesis metadata (e.g. a pre-existing/foreign VM on a shared hypervisor) instead of crashing pool listing for the whole hypervisor. New _memory_mib() handles the unit conversion, since a foreign domain's <memory> isn't guaranteed to be in MiB like Genesis's own tag. - Fix the `docement` -> `document` typo in document_set_tag/ document_meta_set_tag. - _find_attached_volume_element and friends: use `.findall(".//devices/disk")` instead of `.find("devices").findall("disk")`, consistent with the rest of the file, so a domain without a <devices> element doesn't crash with AttributeError. - resize_volume: replace a while-loop-as-one-shot-if with an actual `if`, and guard against a disk with no <target dev=...>. - create_machine: raise a clear error instead of AttributeError when the storage pool's XML has no <target><path>. - rename_machine: `except:` -> `except Exception:` (PEP 8). - Use libvirt.VIR_ERR_NO_SUPPORT instead of the bare int 3. Verified two other flagged spots are not actually reachable and left alone: Element.get(None) returns None rather than raising (checked empirically), and MetaMachine.port_info defaults to `dict` at the model level, so it's never None to begin with.

fields_permissions={"status": {ra_c.ALL: RO}} also covered CREATE, which broke agent registration outright: a fresh agent's register payload always includes its own initial status (see UniversalAgent.from_system_uuid), so every agents_create() call was rejected with FieldPermissionError (403) before the agent row ever existed - the agent never reached ACTIVE, was invisible to have_capabilities(), and dependent machines/nodes stayed stuck at NEW. Scope the RO permission to ra_c.UPDATE only.

The client didn't need to send its own initial status either - only UPDATE was locked down, but a freshly-registering agent still declared its own status (e.g. ACTIVE, via from_system_uuid()'s default) on CREATE, unchecked. - status is now read-only over the API for both CREATE and UPDATE. - UniversalAgentsController.create() always starts the agent ACTIVE, mirroring update(). - UniversalAgentsClient.create() stops sending `status` in the create payload, since the server now rejects it outright if present. DatabaseOrchClient.agents_create() is untouched: it already always receives agents built via from_system_uuid() (ACTIVE by construction), and this in-process client bypasses the API layer entirely.

Rejecting a client-supplied status on CREATE broke every client that still sends its own initial value (e.g. exordos_seed's agent flashing, which isn't part of this codebase and shouldn't need updating). CREATE now accepts the field but the controller overrides it to ACTIVE regardless, same end result without breaking older callers. UPDATE stays strictly read-only, since nothing legitimately needs to send its own status there. Also drops UniversalAgentsClient.create()'s now-unnecessary override that stripped `status` from the payload - the server tolerates it now.

Move the libvirt-based machine pool driver from exordos_core into gcl_sdk as a first-class MetaCoordinatorAgentDriver, merging it with the bridge code that previously lived in exordos_core (compute/agents/universal/drivers/pool.py) so the universal-agent driver contract owns the pool/volume/machine lifecycle directly instead of going through a second abstraction layer. - gcl_sdk/agents/universal/drivers/pool.py: lightweight (non-ORM) Machine/ MachineVolume/Port/MachinePool models, driver specs, AbstractPoolDriver/ DummyPoolDriver, and the MetaPool/MetaVolume/MetaMachine coordinator models plus PoolAgentDriver/LocalPoolAgentDriver. - gcl_sdk/agents/universal/drivers/libvirt.py: LibvirtPoolDriver and its XML helpers, ported to the new lightweight models. - libvirt-python is now the optional `gcl_sdk[libvirt]` extra so gcl_sdk itself has no hard dependency on it. - Fix a pre-existing bug carried over from exordos_core: the dummy driver entry point was registered as "DummyPoolDriver" instead of "dummy", so it never matched DummyPoolDriverSpec.KIND and could never be loaded. - Add coordinator driver quick start doc and tests (create/get pool, volume-without-capacity error path, RootVolumeNotFound).

MetaPool.load_driver() looks up the runtime driver by the exact driver_spec KIND string, but only "libvirt" and "dummy" were registered under gcl_sdk_machine_pool_driver. ExordosLocalHyperDriverSpec is a LibvirtPoolDriverSpec subclass (same driver, plus a required node pin) and had no entry, so a local hypervisor pool failed to load its driver entirely.

…er capability LocalPoolAgentDriver advertises "local_pool" via get_capabilities() as a scheduling-only marker (matches the agent to exordos_local_hyper pools pinned to its node), but the generic capability-actualization loop treats every advertised capability as a real resource kind and calls list() on it. Since "local_pool" isn't in __model_map__, every actualization cycle crashed with TypeError before ever reaching the real pool/pool_volume/pool_machine capabilities.

LibvirtPoolDriver.create_machine() applied the hypervisor's own network_type (bridge/network) to the initial boot-network port. The boot network is always a plain libvirt virtual network regardless of the hypervisor's main network type, so on a bridge-type hypervisor libvirt tried to treat the boot network's name as a literal host bridge device name and rejected it as too long for IFNAMSIZ. attach_port(), used for the real port attached later, is unaffected and continues to honor network_type.

_domain2machine/_list_interfaces read a <source> element's attribute keyed by self._spec.network_type (the hypervisor's own bridge/network setting), but the boot-network port is always type='network' regardless of that setting (see the earlier create_machine fix). On a bridge-type hypervisor this made every boot-network interface look like it had no mac/source at all, breaking pool listing (and everything depending on it - pool_volume, pool_machine) for the entire hypervisor. Read the interface's own 'type' attribute instead of assuming it matches the hypervisor's network_type.

… source A type='network' interface backed by a libvirt network with <forward mode='bridge'/> (e.g. a boot network riding the same bridge as the main network) gets rewritten by libvirt to type='bridge' in the live domain XML, but its <source> element keeps both the original 'network' attribute and the underlying 'bridge' one. Reading by iface type alone picked up the bridge name, permanently diverging from the target state's logical network name and leaving update_on_dp() stuck logging "Unknown update action" every iteration since no tracked field actually looked different to it.

Same issue as create_machine's boot port: attach_port() used self._spec.network_type as the raw libvirt interface type, treating port.source as a literal host bridge device. port.source is always the logical libvirt network name the orchestrator tracks (e.g. "exordos-core-net") - on a bridge-type hypervisor that name must be resolved through a local libvirt network forwarding onto the real bridge, not used directly as a bridge device name.

- _domain2machine: fall back to standard libvirt <vcpu>/<currentMemory>/ <memory> elements when a domain lacks Genesis metadata (e.g. a pre-existing/foreign VM on a shared hypervisor) instead of crashing pool listing for the whole hypervisor. New _memory_mib() handles the unit conversion, since a foreign domain's <memory> isn't guaranteed to be in MiB like Genesis's own tag. - Fix the `docement` -> `document` typo in document_set_tag/ document_meta_set_tag. - _find_attached_volume_element and friends: use `.findall(".//devices/disk")` instead of `.find("devices").findall("disk")`, consistent with the rest of the file, so a domain without a <devices> element doesn't crash with AttributeError. - resize_volume: replace a while-loop-as-one-shot-if with an actual `if`, and guard against a disk with no <target dev=...>. - create_machine: raise a clear error instead of AttributeError when the storage pool's XML has no <target><path>. - rename_machine: `except:` -> `except Exception:` (PEP 8). - Use libvirt.VIR_ERR_NO_SUPPORT instead of the bare int 3. Verified two other flagged spots are not actually reachable and left alone: Element.get(None) returns None rather than raising (checked empirically), and MetaMachine.port_info defaults to `dict` at the model level, so it's never None to begin with.

…l_hyper Both were added without doc coverage: exordos_local_hyper is now a second gcl_sdk_machine_pool_driver entry alongside libvirt (same LibvirtPoolDriver class, distinguished at the control-plane level by ExordosLocalHyperDriverSpec's required node field), and LocalPoolAgentDriver adds the local_pool marker capability pattern for pinning a pool to the one agent running on its own node. Document the marker-capability pattern generally (advertise in get_capabilities(), short-circuit list()/etc. before they reach the coordinator machinery) since it's reusable beyond this one driver.

- document_set_tag/document_meta_set_tag used root.getElementsByTagName(tag).removeChild(node), but getElementsByTagName searches the whole subtree recursively; a match found deeper than a direct child makes removeChild raise xml.dom.NotFoundErr instead of removing anything (confirmed empirically). New _remove_direct_children() only removes actual direct children, shared by both methods. - delete_machine() called lookupByUUIDString unguarded, crashing with an uncaught libvirtError if the domain was already gone (e.g. a retry after an earlier delete destroyed/undefined it but failed later during volume cleanup). Now treats VIR_ERR_NO_DOMAIN as nothing-to-do and proceeds straight to volume cleanup. Tests use libvirt's built-in test:///default driver (real libvirt calls, no daemon/VMs needed) for delete_machine, and minidom directly for the child-removal regression.

Match the repo's existing convention (e.g. test_pool_driver.py's `pool as pool_driver`) instead of importing individual classes and functions directly from gcl_sdk.agents.universal.drivers.libvirt.

Adds agent_key_node to AbstractPoolDriverSpec (None by default), overridden by ExordosLocalHyperDriverSpec to return its node. Lets callers ensure a key generically instead of branching on isinstance(driver_spec, ExordosLocalHyperDriverSpec).

git rebase's auto-merge duplicated TestUniversalAgentsControllerFieldPermissions and its _req_for_method helper while replaying history already squashed into master. Same behavior, just de-duplicated.

phantomii

Could we add a CI job that installs the libvirt extra and runs the new libvirt tests? The current test environment installs only the test extra, so this test module is skipped when the bindings are unavailable and the new driver is not exercised by CI.

phantomii

Please add CI coverage that installs the libvirt extra and runs the new driver tests.

VasilyStepanov mentioned this pull request Jul 17, 2026

feat: move VM provisioning from exordos_core into gcl_sdk, and per-node agent encryption keys exordos/exordos_core#494

Open

gemini-code-assist Bot reviewed Jul 17, 2026

View reviewed changes

VasilyStepanov self-assigned this Jul 17, 2026

VasilyStepanov commented Jul 17, 2026

View reviewed changes

VasilyStepanov commented Jul 18, 2026

View reviewed changes

Comment thread gcl_sdk/agents/universal/clients/orch/http.py Outdated

VasilyStepanov commented Jul 18, 2026

View reviewed changes

Comment thread gcl_sdk/agents/universal/drivers/libvirt.py Outdated

VasilyStepanov changed the title ~~feat(agents): add native VM pool/libvirt coordinator driver~~ feat: bringing up a bridge-networked hypervisor Jul 19, 2026

VasilyStepanov marked this pull request as ready for review July 19, 2026 12:12

VasilyStepanov requested a review from a team as a code owner July 19, 2026 12:12

gemini-code-assist Bot reviewed Jul 19, 2026

View reviewed changes

VasilyStepanov force-pushed the add/pool branch from 0fc1fac to ebd6ca3 Compare July 22, 2026 14:36

VasilyStepanov changed the base branch from master to feat/agent-registration-key-issuance July 22, 2026 14:37

VasilyStepanov force-pushed the add/pool branch from ebd6ca3 to ec1488e Compare July 22, 2026 15:21

VasilyStepanov force-pushed the add/pool branch from c88c349 to 4b5c029 Compare July 22, 2026 15:50

VasilyStepanov force-pushed the add/pool branch from 4b5c029 to 96b2f67 Compare July 22, 2026 16:19

VasilyStepanov force-pushed the add/pool branch from 96b2f67 to f647f38 Compare July 22, 2026 18:16

VasilyStepanov force-pushed the add/pool branch from f647f38 to 14ea022 Compare July 22, 2026 18:24

VasilyStepanov force-pushed the add/pool branch from 14ea022 to 912b206 Compare July 22, 2026 20:22

VasilyStepanov force-pushed the add/pool branch from 1de5119 to 0a3d7d9 Compare July 27, 2026 13:36

VasilyStepanov force-pushed the add/pool branch from 0a3d7d9 to b3444b0 Compare July 27, 2026 14:01

VasilyStepanov force-pushed the add/pool branch from b3444b0 to 8058262 Compare July 27, 2026 14:14

VasilyStepanov force-pushed the add/pool branch from 8058262 to 454fd02 Compare July 28, 2026 10:37

VasilyStepanov force-pushed the add/pool branch from 454fd02 to 57c9c05 Compare July 28, 2026 14:34

Base automatically changed from fix/agent-status-read-only to master July 29, 2026 08:28

VasilyStepanov added 16 commits July 29, 2026 08:32

fix(tests): import the libvirt driver module, not names from it

71f088e

Match the repo's existing convention (e.g. test_pool_driver.py's `pool as pool_driver`) instead of importing individual classes and functions directly from gcl_sdk.agents.universal.drivers.libvirt.

test(agents): drop duplicate test class left over from rebase

0d7374c

git rebase's auto-merge duplicated TestUniversalAgentsControllerFieldPermissions and its _req_for_method helper while replaying history already squashed into master. Same behavior, just de-duplicated.

VasilyStepanov force-pushed the add/pool branch from 57c9c05 to 0d7374c Compare July 29, 2026 08:34

phantomii reviewed Jul 29, 2026

View reviewed changes

phantomii requested changes Jul 29, 2026

View reviewed changes

	return "a9:%02x:%02x:%02x:%02x:%02x" % octets
	return "ae:%02x:%02x:%02x:%02x:%02x" % octets

Uh oh!

Conversation

VasilyStepanov commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

VasilyStepanov Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

VasilyStepanov Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

VasilyStepanov commented Jul 19, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

VasilyStepanov commented Jul 19, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist Bot Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gemini-code-assist Bot Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

phantomii left a comment

Choose a reason for hiding this comment

Uh oh!

phantomii left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

VasilyStepanov commented Jul 17, 2026 •

edited

Loading