nvme: Print controller GUID in shutdown_nvme_driver span #1619
Conversation
| @@ -1051,6 +1065,9 @@ pub mod save_restore { | |||
| /// NVMe driver worker task data. | |||
| #[mesh(4)] | |||
| pub worker_data: NvmeDriverWorkerSavedState, | |||
| /// Controller instance ID. | |||
| #[mesh(5)] | |||
| pub controller_instance_id: Option<String>, | |||
There was a problem hiding this comment.
Why would we need to save this?
|
I am not a big fan of this approach, because we're putting VPCI-specific stuff into our NVMe driver. It even infected the saved state (although I am hoping that's just an oversight). Is there a way we can instead provide a parent |
Ack your feedback about putting vpci-specific stuff into our NVMe driver. I don't fully agree with the push back: we have debug friendly identifiers in other places throughout the stack; I think it's reasonable to add a debug friendly identifier here. For now that can be a stringified guid that happens to be the vpci vmbus instance ID. While this change happens to only add the vsid to this particular span, I don't think that's far enough. I think we need to include in all cases where we currently log the How we end up getting these logs at scale make it difficult to operationalize a span parent/child walk. I admit that comment is disappointing, but adding a friendly name in this device will make it easier to debug and diagnose at scale. |
| @@ -336,6 +349,11 @@ impl NvmeManagerWorker { | |||
| .await | |||
| .map_err(InnerError::DeviceInitFailed)?; | |||
|
|
|||
| // Pass through controller instance ID if specified. | |||
| if let Some(controller_instance_id) = controller_instance_id { | |||
| driver.set_controller_instance_id(controller_instance_id); | |||
There was a problem hiding this comment.
Why do we need a setter, we just called new?
|
If this is going to be used in debugging/tracing scenarios only then it'd be nice to have a comment on the field saying so, or maybe include the words 'debug' or 'trace' in the name, or something. |
This allows us to directly connect the NVMe device to the controller attached to the VM in one log line.