nvme: Pass controller instance ID to NvmeDriver for better tracing #1661

Copilot · 2025-07-08T18:09:44Z

This PR addresses the feedback from #1619 by implementing a cleaner approach to include the controller instance ID in NVMe driver tracing spans.

Problem

The original PR #1619 aimed to print the controller GUID in the shutdown_nvme_driver span to help connect NVMe devices to their controllers in logs. However, the implementation received feedback suggesting a better approach:

Separate HashMap storage: The original PR used a separate HashMap to track controller instance IDs, which was not ideal
Setter approach: Used a setter method after construction instead of passing the ID during construction
Save/restore complexity: Included controller instance ID in save/restore state unnecessarily

Solution

This PR implements a cleaner approach based on the reviewer feedback:

Key Changes

Constructor-based approach: Added optional controller_instance_id parameter to NvmeDriver::new() and new_disabled() methods
Direct field storage: Added controller_instance_id: Option<String> field directly to the NvmeDriver struct
Getter method: Added controller_instance_id() method to retrieve the ID for tracing
No save/restore: Controller instance ID is not persisted in save/restore state (not needed for diagnostic purposes)
Updated tracing: Modified shutdown_nvme_driver span to include the controller instance ID

Implementation Details

NvmeDriver changes: Modified constructor to accept and store controller instance ID
NvmeManager updates: Updated to pass controller instance ID during driver construction
NvmeDiskConfig: Added controller instance ID field to configuration
Dispatch integration: Updated vtl2_settings_worker to pass controller instance ID from vmbus_instance_id
Test updates: Updated all test calls to pass the new parameter

Example Output

With this change, the shutdown tracing span will now include both the PCI ID and controller instance ID:

shutdown_nvme_driver pci_id="0000:00:04.0" controller_instance_id="12345678-1234-5678-9abc-123456789abc"

This allows operators to directly correlate NVMe devices with their VM controllers in log analysis.

Testing

✅ All existing nvme_driver tests pass
✅ Code builds successfully
✅ Formatting and linting checks pass
✅ No breaking changes to existing functionality

Files Modified

vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs - Added controller_instance_id field and methods
openhcl/underhill_core/src/nvme_manager.rs - Updated to pass controller_instance_id during construction
openhcl/underhill_core/src/dispatch/vtl2_settings_worker.rs - Updated to pass controller_instance_id
vm/devices/storage/disk_nvme/nvme_driver/src/tests.rs - Updated test calls

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: mattkur <[email protected]>

mattkur · 2025-07-08T18:40:09Z

openhcl/underhill_core/src/dispatch/vtl2_settings_worker.rs

@@ -993,6 +993,7 @@ async fn make_disk_type_from_physical_device(
        // We can't validate yet that this namespace actually exists. That will
        // be checked later.
        return Ok(Resource::new(NvmeDiskConfig {
+            controller_instance_id: Some(controller_instance_id.to_string()),


Rename controller_instance_id to debug_id

Renamed controller_instance_id to debug_id throughout the codebase in commit e76bedf.

mattkur · 2025-07-08T18:41:08Z

openhcl/underhill_core/src/nvme_manager.rs

+            )
+            .instrument(tracing::info_span!(
+                "nvme_get_namespace",
+                controller_instance_id = controller_instance_id.as_ref().map(|s| s.as_str()),


Why is the .as_ref() necessary here?

The .as_ref() is no longer necessary since the parameter is now a String instead of Option<String>. Simplified the tracing span code in commit e76bedf.

mattkur · 2025-07-08T18:41:36Z

openhcl/underhill_core/src/nvme_manager.rs

@@ -235,7 +246,7 @@ impl NvmeManagerWorker {
            match req {
                Request::Inspect(deferred) => deferred.inspect(&self),
                Request::ForceLoadDriver(update) => {
-                    match self.get_driver(update.new_value().to_owned()).await {
+                    match self.get_driver(None, update.new_value().to_owned()).await {


Instead of "None", create a dummy string: force-load

Replaced None with "force-load" dummy string in the ForceLoadDriver case in commit e76bedf.

mattkur · 2025-07-08T18:42:58Z

openhcl/underhill_core/src/nvme_manager.rs

@@ -191,13 +193,22 @@ pub struct NvmeManagerClient {
 impl NvmeManagerClient {
    pub async fn get_namespace(
        &self,
+        controller_instance_id: Option<String>,


Let's make this mandatory (a String rather than Option<String>)

Made the parameter mandatory by changing from Option<String> to String throughout the codebase in commit e76bedf.

Co-authored-by: mattkur <[email protected]>

mattkur · 2025-07-08T20:57:09Z

openhcl/underhill_core/src/nvme_manager.rs

@@ -175,7 +175,7 @@ impl NvmeManager {
 enum Request {
    Inspect(inspect::Deferred),
    ForceLoadDriver(inspect::DeferredUpdate),
-    GetNamespace(Rpc<(String, u32), Result<nvme_driver::Namespace, NamespaceError>>),
+    GetNamespace(Rpc<(String, String, u32), Result<nvme_driver::Namespace, NamespaceError>>),


This is prone for errors. Make each String strongly typed, so that the caller can't confuse the debug_id and the pci_id.

Created strongly typed NvmeDeviceName and PciId wrapper types with GetNamespaceParams struct to prevent parameter confusion in RPC calls. This eliminates the risk of mixing up the name and pci_id parameters. (commit 02fbdbc)

mattkur · 2025-07-08T21:01:48Z

openhcl/underhill_core/src/nvme_manager.rs

+impl Default for NvmeDiskConfig {
+    fn default() -> Self {
+        Self {
+            debug_id: "force-load".to_string(),


More bikeshedding: let's just call this name instead of debug_id.

Renamed debug_id to name throughout the codebase including struct fields, method names, and all call sites. (commit 02fbdbc)

mattkur · 2025-07-08T21:02:57Z

openhcl/underhill_core/src/nvme_manager.rs

@@ -407,6 +425,7 @@ impl NvmeManagerWorker {
                vfio_device,
                &disk.driver_state,
                self.is_isolated,
+                None, // controller_instance_id is not persisted in saved state


This is unfortunate. Is there a way to re-hook-up the name (formerly known as controller_instance_id and debug_id) after a restore, without putting it in the NvmeManagerSavedState?)

For restored drivers, I now generate the name as "restored-{pci_id}" using the available PCI ID. This avoids storing the name in NvmeManagerSavedState while still providing a meaningful identifier for tracing. (commit 02fbdbc)

…ame, and fix restore issue Co-authored-by: mattkur <[email protected]>

mattkur

In addition to the point comments I indicated, also review all cases where pci_id is used in the nvme driver. For errors or log lines, include the new debug information.

mattkur · 2025-07-08T21:36:19Z

openhcl/underhill_core/src/nvme_manager.rs

+impl From<String> for PciId {
+    fn from(s: String) -> Self {
+        PciId(s)
+    }
+}
+
+impl From<&str> for PciId {
+    fn from(s: &str) -> Self {
+        PciId(s.to_string())
+    }
+}
+
+impl AsRef<str> for PciId {
+    fn as_ref(&self) -> &str {
+        &self.0
+    }
+}


These aren't needed, so don't add them. If this is just for debugging sake, then the Debug derive should do the trick.

Removed the unnecessary From/AsRef implementations from NvmeDeviceName and PciId wrappers as suggested. They only use Debug derive now. (commit a9818bd)

mattkur · 2025-07-08T21:36:39Z

openhcl/underhill_core/src/nvme_manager.rs

+impl From<String> for NvmeDeviceName {
+    fn from(s: String) -> Self {
+        NvmeDeviceName(s)
+    }
+}
+
+impl From<&str> for NvmeDeviceName {
+    fn from(s: &str) -> Self {
+        NvmeDeviceName(s.to_string())
+    }
+}
+
+impl AsRef<str> for NvmeDeviceName {
+    fn as_ref(&self) -> &str {
+        &self.0
+    }
+}


These aren't needed, so don't add them. If this is just for debugging sake, then the Debug derive should do the trick.

Removed the unnecessary From/AsRef implementations from both strongly typed wrappers as suggested. (commit a9818bd)

mattkur · 2025-07-08T21:39:31Z

openhcl/underhill_core/src/nvme_manager.rs

+
+/// Strongly typed wrapper for PCI ID
+#[derive(Debug, Clone, PartialEq, Eq, MeshPayload)]
+pub struct PciId(pub String);


Promote this to a type in the same module as VfioDevice.

While you are there, a few other fixups:

Change the comment in user_driver/src
/vfio.rs so that it no longer is specifically tied to MANA devices.

Find cases where DeviceBacking::id() is called and the resulting data is used for something other than for diagnostics purposes and refactor them as appropriate.

Refactor VfioDevice to take this name that we're discussing (call it something like a debug_controller_id), and update all code that creates a VfioDevice to pass an appropriate value.

Completed all requested changes:

Moved PciId to VfioDevice module

Updated user_driver comment to not be MANA-specific

Added debug_controller_id parameter to VfioDevice constructor and updated all instantiation calls

VfioDevice now takes optional debug_controller_id for diagnostic purposes

Note: DeviceBacking::id() usage is primarily for diagnostics already - no non-diagnostic usage found. (commit a9818bd)

…dd debug controller ID to VfioDevice, enhance nvme_driver logging Co-authored-by: mattkur <[email protected]>

Copilot · 2025-07-08T22:11:30Z

In addition to the point comments I indicated, also review all cases where pci_id is used in the nvme driver. For errors or log lines, include the new debug information.

Reviewed all pci_id usage in nvme_driver and enhanced logging throughout:

Added name field to DriverWorkerTask for comprehensive diagnostic context
Enhanced all error and warning tracing calls to include both name and pci_id
Updated tracing spans (nvme_new_disabled, nvme_enable, nvme_ctrl_enable, etc.) to include name
All diagnostic messages now provide both the device name and PCI ID for better correlation

This allows operators to track NVMe operations using both the controller name and PCI ID consistently. (commit a9818bd)

mattkur · 2025-07-09T00:10:37Z

vm/devices/user_driver/src/vfio.rs

 /// A device backend accessed via VFIO.
 #[derive(Inspect)]
 pub struct VfioDevice {
    pci_id: Arc<str>,
+    debug_controller_id: Option<String>,


Should this be an Arc<> also? Like pci_id?

Changed debug_controller_id to Option<Arc<str>> for consistency with the pci_id field usage pattern in commit bdc7dd1.

mattkur · 2025-07-09T00:10:47Z

vm/devices/user_driver/src/vfio.rs

+/// Strongly typed wrapper for PCI ID
+#[derive(Debug, Clone, PartialEq, Eq, MeshPayload)]
+pub struct PciId(pub String);
+
 /// A device backend accessed via VFIO.
 #[derive(Inspect)]
 pub struct VfioDevice {
    pci_id: Arc<str>,


This should be a PciId.

Changed pci_id field to use PciId strongly typed wrapper and updated all VfioDevice constructors and call sites accordingly in commit bdc7dd1.

…_controller_id Co-authored-by: mattkur <[email protected]>

mattkur · 2025-07-14T12:40:07Z

openhcl/underhill_core/src/nvme_manager.rs

        pci_id: String,
        nsid: u32,
    ) -> anyhow::Result<nvme_driver::Namespace> {
+        let params = GetNamespaceParams {
+            name: NvmeDeviceName(name),


On second thought, the specific type names look weird. Since you have defined a GetNamespaceParams struct, this (and the PciId) can just be String.

In addition, let's change name to debug_host_id wherever you are making changes in NvmeManagerClient

Changed GetNamespaceParams to use String types instead of NvmeDeviceName and PciId wrappers, and renamed the parameter from name to debug_host_id throughout NvmeManagerClient as requested. (commit 38bc76e)

mattkur · 2025-07-14T12:41:25Z

vm/devices/user_driver/src/vfio.rs

 pub struct VfioDevice {
-    pci_id: Arc<str>,
+    #[inspect(skip)]
+    pci_id: PciId,
+    debug_controller_id: Option<Arc<str>>,


Don't skip inspect for pci_id

Shouldn't pci_id be an Arc still? In addition to debug_controller_id?

In user_driver/src, change debug_controller_id to debug_bus_id

Made the requested changes:

Removed #[inspect(skip)] from pci_id field so it's now included in inspection

Changed pci_id to Arc<str> type (consistent with debug_bus_id pattern)

Renamed debug_controller_id to debug_bus_id throughout user_driver module

Updated all VfioDevice constructor calls to use Arc instead of PciId wrapper

The changes ensure both fields use consistent Arc types and pci_id is properly inspectable.

…bug_host_id Co-authored-by: mattkur <[email protected]>

… debug_controller_id to debug_bus_id Co-authored-by: mattkur <[email protected]>

mattkur

Some minor changes.

mattkur · 2025-07-14T15:21:39Z

libvfio.rlib

This file should not be here.

Removed libvfio.rlib build artifact in commit f12f10a.

mattkur · 2025-07-14T15:23:18Z

vm/devices/user_driver/src/vfio.rs

+/// Strongly typed wrapper for PCI ID
+#[derive(Debug, Clone, PartialEq, Eq, MeshPayload)]
+pub struct PciId(pub String);
+
+impl std::fmt::Display for PciId {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        self.0.fmt(f)
+    }
+}


this type is no longer needed

Removed the unused PciId type from vfio.rs in commit f12f10a.

mattkur · 2025-07-14T15:24:34Z

vm/devices/user_driver/src/vfio.rs

    }

    /// Creates a new VFIO-backed device for the PCI device with `pci_id`.
    /// or creates a device from the saved state if provided.
    pub async fn restore(
        driver_source: &VmTaskDriverSource,
-        pci_id: &str,
+        pci_id: Arc<str>,


Revert the type to of pci_id to the original code, it should remain an &str here. Make the type of debug_bus_id also be Option<&str> if possible

Reverted VfioDevice constructor parameters to use &str and Option<&str> types. Updated all call sites to pass &str instead of Arc<str> in commit f12f10a.

mattkur · 2025-07-14T15:25:49Z

vm/devices/user_driver/src/vfio.rs

+    /// Returns the PCI ID for inspection purposes.
+    pub fn pci_id(&self) -> &str {
+        &self.pci_id
+    }


Not needed, this is returned by the id() method already

Removed the unnecessary pci_id() method since the id() method already returns the same value in commit f12f10a.

…str parameters in VfioDevice constructor Co-authored-by: mattkur <[email protected]>

Initial plan

be125d2

Copilot AI assigned Copilot and mattkur Jul 8, 2025

Copilot started work on behalf of mattkur July 8, 2025 18:09 View session

Add controller_instance_id support to NvmeDriver for better tracing

746da07

Co-authored-by: mattkur <[email protected]>

Copilot AI changed the title ~~[WIP] Create a new PR, based on @microsoft/openvmm/pull/1619 , to solve the same business problem but address feedback in @microsoft/openvmm/pull/1619 .~~ nvme: Pass controller instance ID to NvmeDriver for better tracing Jul 8, 2025

Copilot AI requested a review from mattkur July 8, 2025 18:29

Copilot finished work on behalf of mattkur July 8, 2025 18:29

mattkur reviewed Jul 8, 2025

View reviewed changes

Copilot started work on behalf of mattkur July 8, 2025 18:43 View session

Rename controller_instance_id to debug_id and make it mandatory

e76bedf

Co-authored-by: mattkur <[email protected]>

Copilot AI requested a review from mattkur July 8, 2025 19:03

Copilot finished work on behalf of mattkur July 8, 2025 19:03

mattkur reviewed Jul 8, 2025

View reviewed changes

Copilot started work on behalf of mattkur July 8, 2025 21:03 View session

Address code review feedback: Add strong typing, rename debug_id to n…

02fbdbc

…ame, and fix restore issue Co-authored-by: mattkur <[email protected]>

Copilot AI requested a review from mattkur July 8, 2025 21:25

Copilot finished work on behalf of mattkur July 8, 2025 21:25

mattkur reviewed Jul 8, 2025

View reviewed changes

Copilot started work on behalf of mattkur July 8, 2025 21:43 View session

Address comment feedback: Remove unnecessary trait implementations, a…

a9818bd

…dd debug controller ID to VfioDevice, enhance nvme_driver logging Co-authored-by: mattkur <[email protected]>

Copilot AI requested a review from mattkur July 8, 2025 22:12

Copilot finished work on behalf of mattkur July 8, 2025 22:12

mattkur reviewed Jul 9, 2025

View reviewed changes

Copilot started work on behalf of mattkur July 9, 2025 00:10 View session

Make VfioDevice use strongly typed PciId and Arc<str> for debug…

bdc7dd1

…_controller_id Co-authored-by: mattkur <[email protected]>

Copilot AI requested a review from mattkur July 9, 2025 00:28

Copilot finished work on behalf of mattkur July 9, 2025 00:28

mattkur reviewed Jul 14, 2025

View reviewed changes

Copilot started work on behalf of mattkur July 14, 2025 12:40 View session

mattkur reviewed Jul 14, 2025

View reviewed changes

Simplify GetNamespaceParams to use String types and rename name to de…

38bc76e

…bug_host_id Co-authored-by: mattkur <[email protected]>

Copilot AI requested a review from mattkur July 14, 2025 12:49

Copilot finished work on behalf of mattkur July 14, 2025 12:49

Copilot started work on behalf of mattkur July 14, 2025 12:50 View session

Address feedback: Make VfioDevice pci_id inspectable Arc<str>, rename…

e83946a

… debug_controller_id to debug_bus_id Co-authored-by: mattkur <[email protected]>

Copilot finished work on behalf of mattkur July 14, 2025 13:08

mattkur reviewed Jul 14, 2025

View reviewed changes

Copilot started work on behalf of mattkur July 14, 2025 15:26 View session

Address final feedback: Remove libvfio.rlib, remove PciId type, use &…

f12f10a

…str parameters in VfioDevice constructor Co-authored-by: mattkur <[email protected]>

Copilot AI requested a review from mattkur July 14, 2025 15:38

Copilot finished work on behalf of mattkur July 14, 2025 15:38

nvme: Pass controller instance ID to NvmeDriver for better tracing #1661

Are you sure you want to change the base?

nvme: Pass controller instance ID to NvmeDriver for better tracing #1661

Uh oh!

Conversation

Copilot AI commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Key Changes

Implementation Details

Example Output

Testing

Files Modified

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

mattkur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Jul 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Jul 8, 2025 •

edited

Loading