Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feat backend separate transport specific messages #343

Merged
merged 3 commits into from
Sep 19, 2023
Merged

Feat backend separate transport specific messages #343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b8af9f5
feat(storage/backend): extract TcpController
artek-koltun Aug 30, 2023
fef19c9
feat(storage/backend): extract FabricsPath
artek-koltun Aug 30, 2023
4618e5f
chore(storage): auto-generated protos
artek-koltun Sep 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 41 additions & 9 deletions storage/v1alpha1/autogen.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
- [CreateNvmeRemoteControllerRequest](#opi_api-storage-v1-CreateNvmeRemoteControllerRequest)
- [DeleteNvmePathRequest](#opi_api-storage-v1-DeleteNvmePathRequest)
- [DeleteNvmeRemoteControllerRequest](#opi_api-storage-v1-DeleteNvmeRemoteControllerRequest)
- [FabricsPath](#opi_api-storage-v1-FabricsPath)
- [GetNvmePathRequest](#opi_api-storage-v1-GetNvmePathRequest)
- [GetNvmeRemoteControllerRequest](#opi_api-storage-v1-GetNvmeRemoteControllerRequest)
- [GetNvmeRemoteNamespaceRequest](#opi_api-storage-v1-GetNvmeRemoteNamespaceRequest)
Expand All @@ -53,6 +54,7 @@
- [StatsNvmePathResponse](#opi_api-storage-v1-StatsNvmePathResponse)
- [StatsNvmeRemoteControllerRequest](#opi_api-storage-v1-StatsNvmeRemoteControllerRequest)
- [StatsNvmeRemoteControllerResponse](#opi_api-storage-v1-StatsNvmeRemoteControllerResponse)
- [TcpController](#opi_api-storage-v1-TcpController)
- [UpdateNvmePathRequest](#opi_api-storage-v1-UpdateNvmePathRequest)
- [UpdateNvmeRemoteControllerRequest](#opi_api-storage-v1-UpdateNvmeRemoteControllerRequest)

Expand Down Expand Up @@ -657,6 +659,26 @@ Represents a request to delete an Nvme Remote Controller.



<a name="opi_api-storage-v1-FabricsPath"></a>

### FabricsPath



| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| trsvcid | [int64](#int64) | | Destination service id (e.g. Port) |
| subnqn | [string](#string) | | Subsystem NQN |
| adrfam | [NvmeAddressFamily](#opi_api-storage-v1-NvmeAddressFamily) | | |
| source_traddr | [string](#string) | | Source address (e.g. IP of local NIC) |
| source_trsvcid | [int64](#int64) | | Source port (e.g. Port of local NIC) |
| hostnqn | [string](#string) | | Host NQN |






<a name="opi_api-storage-v1-GetNvmePathRequest"></a>

### GetNvmePathRequest
Expand Down Expand Up @@ -812,13 +834,8 @@ Represents a response to list all Nvme Remote Namespaces.
| name | [string](#string) | | name is an opaque object handle that is not user settable. name will be returned with created object user can only set {resource}_id on the Create request object |
| controller_name_ref | [string](#string) | | |
| trtype | [NvmeTransportType](#opi_api-storage-v1-NvmeTransportType) | | |
| adrfam | [NvmeAddressFamily](#opi_api-storage-v1-NvmeAddressFamily) | | |
| traddr | [string](#string) | | Destination address (e.g. IP address, BDF for local PCIe) |
| trsvcid | [int64](#int64) | | Destination service id (e.g. Port) |
| subnqn | [string](#string) | | Subsystem NQN |
| source_traddr | [string](#string) | | Source address (e.g. IP of local NIC) |
| source_trsvcid | [int64](#int64) | | Source port (e.g. Port of local NIC) |
| hostnqn | [string](#string) | | Host NQN |
| fabrics | [FabricsPath](#opi_api-storage-v1-FabricsPath) | | Not applicable for local PCIe. Required for Nvme over fabrics transport types |



Expand All @@ -837,9 +854,7 @@ Represents a response to list all Nvme Remote Namespaces.
| multipath | [NvmeMultipath](#opi_api-storage-v1-NvmeMultipath) | | |
| io_queues_count | [int64](#int64) | | |
| queue_size | [int64](#int64) | | |
| hdgst | [bool](#bool) | | |
| ddgst | [bool](#bool) | | |
| psk | [bytes](#bytes) | | Nvme/TCP published secure channel specification (TP 8011) based on TLS 1.3 and PSK. Use PSK interchange format with base64 encoding as input. Also use information about hash function in interchange format for retained PSK generation. If no hash is selected, use configured PSK as retained PSK. Check the size of interchange PSK to determine cipher suite. Calculate CRC-32 bytes to ensure validity of PSK. Example: &#34;NVMeTLSkey-1:01:VRLbtnN9AQb2WXW3c9&#43;wEf/DRLz0QuLdbYvEhwtdWwNf9LrZ:&#34; if PSK field is empty, then unsecure connection Nvme/TCP without TLS will be made |
| tcp | [TcpController](#opi_api-storage-v1-TcpController) | | Nvme over TCP specific fields |



Expand Down Expand Up @@ -941,6 +956,23 @@ Represents a response to get an Nvme Remote Controller statistics.



<a name="opi_api-storage-v1-TcpController"></a>

### TcpController



| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| hdgst | [bool](#bool) | | |
| ddgst | [bool](#bool) | | |
| psk | [bytes](#bytes) | | Nvme/TCP published secure channel specification (TP 8011) based on TLS 1.3 and PSK. Use PSK interchange format with base64 encoding as input. Also use information about hash function in interchange format for retained PSK generation. If no hash is selected, use configured PSK as retained PSK. Check the size of interchange PSK to determine cipher suite. Calculate CRC-32 bytes to ensure validity of PSK. Example: &#34;NVMeTLSkey-1:01:VRLbtnN9AQb2WXW3c9&#43;wEf/DRLz0QuLdbYvEhwtdWwNf9LrZ:&#34; if PSK field is empty, then unsecure connection Nvme/TCP without TLS will be made |






<a name="opi_api-storage-v1-UpdateNvmePathRequest"></a>

### UpdateNvmePathRequest
Expand Down
31 changes: 21 additions & 10 deletions storage/v1alpha1/backend_nvme_tcp.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,13 @@ message NvmeRemoteController {
int64 io_queues_count = 3 [(google.api.field_behavior) = OPTIONAL];
int64 queue_size = 4 [(google.api.field_behavior) = OPTIONAL];

bool hdgst = 5 [(google.api.field_behavior) = OPTIONAL];
bool ddgst = 6 [(google.api.field_behavior) = OPTIONAL];
// Nvme over TCP specific fields
TcpController tcp = 5 [(google.api.field_behavior) = OPTIONAL];
}

message TcpController {
bool hdgst = 1 [(google.api.field_behavior) = OPTIONAL];
bool ddgst = 2 [(google.api.field_behavior) = OPTIONAL];

// Nvme/TCP published secure channel specification (TP 8011) based on TLS 1.3 and PSK.
// Use PSK interchange format with base64 encoding as input.
Expand All @@ -167,7 +172,7 @@ message NvmeRemoteController {
// Calculate CRC-32 bytes to ensure validity of PSK.
// Example: "NVMeTLSkey-1:01:VRLbtnN9AQb2WXW3c9+wEf/DRLz0QuLdbYvEhwtdWwNf9LrZ:"
// if PSK field is empty, then unsecure connection Nvme/TCP without TLS will be made
bytes psk = 7 [(google.api.field_behavior) = OPTIONAL];
bytes psk = 3 [(google.api.field_behavior) = OPTIONAL];
}

message NvmePath {
Expand All @@ -193,25 +198,31 @@ message NvmePath {
];

NvmeTransportType trtype = 3 [(google.api.field_behavior) = REQUIRED];
NvmeAddressFamily adrfam = 4 [(google.api.field_behavior) = OPTIONAL];

// Destination address (e.g. IP address, BDF for local PCIe)
string traddr = 5 [(google.api.field_behavior) = REQUIRED];
string traddr = 4 [(google.api.field_behavior) = REQUIRED];

// Not applicable for local PCIe. Required for Nvme over fabrics transport types
FabricsPath fabrics = 5 [(google.api.field_behavior) = OPTIONAL];
}

message FabricsPath {
Copy link
Member

@glimchb glimchb Sep 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like we have tuple: (addr, port, nqn) for target and again (addr, port, nqn) for source, no ?

maybe we can somehow capture this in it's own message ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The traddr for the target is in the common part (it's the BDF for PCIe), so it needs to be split across two objects here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benlwalker WDYT about struct spdk_nvme_transport_id ?
https://github.com/spdk/spdk/blob/ee9fb405bfa6f11b75788c38752b2494598bbbd8/include/spdk/nvme.h#L455-L501

I think it is pretty good abstraction what we want to adopt here, why not ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like that abstraction. To make that work here you'd need to have a FabricsPath with a source and target transport id, and then a MemoryBusPath with just a target transport id (or just traddr), and put the two into a OneOf.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My goal was to have the annotations as precise as possible, in order to provide a correct docs and benefit from some automatic validation tools like fieldbehavior.ValidateRequiredFields() call.
If we extract those members into a separate message, we probably need to mark nqn and port as OPTIONAL, since they are not applicable for pcie case, "lying" that they are actually required for fabrics. In the current patch, we clearly state that nqn and port are required by fabrics and not used by pcie

// Destination service id (e.g. Port)
int64 trsvcid = 6 [(google.api.field_behavior) = OPTIONAL];
int64 trsvcid = 1 [(google.api.field_behavior) = REQUIRED];

// Subsystem NQN
string subnqn = 7 [(google.api.field_behavior) = OPTIONAL];
string subnqn = 2 [(google.api.field_behavior) = REQUIRED];

NvmeAddressFamily adrfam = 3 [(google.api.field_behavior) = REQUIRED];

// Source address (e.g. IP of local NIC)
string source_traddr = 8 [(google.api.field_behavior) = OPTIONAL];
string source_traddr = 4 [(google.api.field_behavior) = OPTIONAL];

// Source port (e.g. Port of local NIC)
int64 source_trsvcid = 9 [(google.api.field_behavior) = OPTIONAL];
int64 source_trsvcid = 5 [(google.api.field_behavior) = OPTIONAL];

// Host NQN
string hostnqn = 10 [(google.api.field_behavior) = OPTIONAL];
string hostnqn = 6 [(google.api.field_behavior) = OPTIONAL];
}

message NvmeRemoteNamespace {
Expand Down
Loading
Loading