frontend proto changes #169

jainvipin · 2022-10-21T07:29:22Z

Frontend proto updates to incorporates comments in #133, and merges NvmeSubsystem, NvmeController and NvmeNamespace objects. A few notes

comments beginning with //?? are really going to be removed but kept for the context and discussions for now
have included id as an object key to each object, forward object referencing is so easy with that; this is by far the biggest semantic change in this PR
rearranged some enums, assuming no production/compliant implementations will run into compatibility issues.
fields are separated by a newline, but we can remove it if the coding guidelines needs to keep it next to each other

Signed-off-by: Vipin Jain jainvipin@gmail.com

Signed-off-by: Vipin Jain <jainvipin@gmail.com>

mkalderon · 2022-10-21T08:42:30Z

storage/v1/frontend_nvme_pcie.proto

+    string model_number = 4;
+
+    //?? do we need this in subsystem given that controller object would have it
+    //?? val change


we need this since there could be several controllers in subsystem.

storage/v1/frontend_nvme_pcie.proto

glimchb · 2022-10-21T11:06:17Z

storage/v1/frontend_nvme_pcie.proto

+    //?? replaces: int64 id = 1;
+    common.v1.Uuid id = 1;
+
+    //?? is this name needed if uuid is uniquely identifying the controller


I don't see use in the name field of the controller

fixed in updated commit

glimchb · 2022-10-21T11:06:39Z

storage/v1/frontend_nvme_pcie.proto

+    // replaces: int64 id = 1;
+    common.v1.Uuid id = 1;
+
+    //?? do we need this if NvmeNamespace is identified using unique uuid
    string name = 2;


I don't see use in the name field of the namespace

fixed in updated commit

glimchb · 2022-10-21T11:09:22Z

storage/v1/frontend_nvme_pcie.proto

-    string model_number = 3;
-    int64 max_ns = 4;
+    // object's unique identifier
+    common.v1.Uuid id = 1;


we wanted to use common.v1.ObjectKey instead of common.v1.Uuid as opaque object's unique identifier
common.v1.Uuid should be used only in places per NVME spec that defines true UUID/NGUID/EU64 for example...

yes a commit on this PR coming to move these all keys to reflect that.

glimchb · 2022-10-21T11:09:33Z

storage/v1/frontend_nvme_pcie.proto

-    int64 id = 1;
+    // object's unique identifier
+    //?? replaces: int64 id = 1;
+    common.v1.Uuid id = 1;


we wanted to use common.v1.ObjectKey instead of common.v1.Uuid as opaque object's unique identifier
common.v1.Uuid should be used only in places per NVME spec that defines true UUID/NGUID/EU64 for example...

glimchb · 2022-10-21T11:09:43Z

storage/v1/frontend_nvme_pcie.proto

-    int64 id = 1;
+    // namespace's unique key
+    // replaces: int64 id = 1;
+    common.v1.Uuid id = 1;


we wanted to use common.v1.ObjectKey instead of common.v1.Uuid as opaque object's unique identifier
common.v1.Uuid should be used only in places per NVME spec that defines true UUID/NGUID/EU64 for example...

yes, that should serve generically as well. So, I am good with an opaque string instead of uuid. let's hear from others if anyone wants to opine on it.

+1 from me. I'm not entirely convinced the server shouldn't be the one generating the handles, but I'm fine with moving forward in this direction for now.

storage/v1/frontend_nvme_pcie.proto

glimchb · 2022-10-21T11:11:32Z

storage/v1/frontend_nvme_pcie.proto

+
+    //?? can we take this out if controller is referring to a subsystem anyways
+    //?? replaces string subsystem_id = 3;
+    common.v1.Uuid subsystem_id = 3;


we wanted to use common.v1.ObjectKey instead of common.v1.Uuid as opaque object's unique identifier

A namespace goes in a subsystem, not in a controller. So this is correct to refer to subsystem. But with dispersed namespaces a namespace can be in multiple subsystems so this shouldn't be in the namespace object. It should just be in the call that adds this namespace to a subsystem.

why do you say that ? I think NS belongs to ctrl not subsystem

A namespace gets assigned to a subsystem in NVMe, not a controller. This is directly from the NVMe spec and that's how your diagram shows it too. The namespaces are in the orange box, not in the blue boxes.

so ctrl only have NSID inside ?

Unfortunately, that's complicated. There's actually two types of NSID - private and public. Public NSIDs are inside the subsystem, and all controllers see the same set. Private NSIDs are inside the controller. You can think of a private NSID as an extra layer of indirection that aliases the public NSID to a private value, although strictly speaking the private NSID does not have to also appear in the public list.

I've never seen any implementation ever use a private NSID. Certainly, lots of implementations change which public NSIDs are visible to hosts based on access control, but I've never seen an implementation use a private NSID. I don't believe they actually add any functionality, security, or value generally. I do not think we need to support private NSIDs in OPI.

fixed the object-id in updated commit.

glimchb · 2022-10-21T11:11:37Z

storage/v1/frontend_nvme_pcie.proto

+
+    // key of the PCIe controller object that will host this namespace.
+    //?? replaces int64 controller_id = 4;
+    common.v1.Uuid controller_id = 4;


we wanted to use common.v1.ObjectKey instead of common.v1.Uuid as opaque object's unique identifier

A namespace is not associated with 1 controller. Instead, controllers can view namespaces. Having this reference here is not correct.

NS is attached to 1 or more controllers

No. A namespace is placed in a subsystem.

will change - to rename this to objectKey instead.

glimchb · 2022-10-21T11:13:18Z

storage/v1/frontend_nvme_pcie.proto

    common.v1.Uuid uuid = 11;
+
+    //?? what is this for
    string multipath = 12;


no use for now, wanted to reserve for future, can remove now after discussion

Pulled from back/middle, not configured here. Remove this.

glimchb · 2022-10-21T11:13:22Z

storage/v1/frontend_nvme_pcie.proto

    string multipath = 12;
+
+    //?? what is this for
    string authentication = 13;


no use for now, wanted to reserve for future, can remove now after discussion

glimchb · 2022-10-21T11:16:14Z

storage/v1/frontend_nvme_pcie.proto

+    // for live migration
+    //?? replaces: int64 nsid = 5;
+    uint32 host_nsid = 5;
+
    string bdev = 6;


need to discuss this bdev as well... I think should be reference to either middle-end object (like encryption, compression, ...) or to the back-end object, like nvmeof bdev... wdyt ?

@glimchb do you mean replace bdev with objectKey ?

yeah, maybe... objectKey that points to a backend object

It needs to be replaced with however OPI references the middle/backend objects. Probably a volume handle/id/key of some sort.

yep, we should use https://github.com/opiproject/opi-api/blob/main/common/v1/object_key.proto

yes I am not sure about the use for it. if not we can remove/move it to the backend objects.

fixed in recent commit - all uuids are now objectKey.

glimchb · 2022-10-21T11:16:57Z

storage/v1/frontend_nvme_pcie.proto

    string authentication = 13;
+
+    // reference to encryption key for the data at rest encryption
+    common.v1.Uuid crypto_key_id = 14;


does this belong here ? I think should be reference to either middle-end object (like encryption, compression, ...) or to the back-end object, like nvmeof bdev... wdyt ?

Unless we're talking PCI bus encryption, this should be part of the back/middle.

backend is we talking about PSK's for TLS for example

middleend is we talking about data at rest

well, ultimately encryption granularity will need to be specified somewhere, so I put this in here. We can move this to ObjectKey instead, like in other objects; so this is really an attachment point for crypto, not the middleend layer, but a glue.

glimchb · 2022-10-21T11:21:41Z

@jkogutINTC @benlwalker @orendu @llabordehpe please re-review this as well

benlwalker · 2022-10-21T14:21:04Z

storage/v1/frontend_nvme_pcie.proto

+    // for live migration
+    //?? replaces: int64 nsid = 5;
+    uint32 host_nsid = 5;
+
    string bdev = 6;


It needs to be replaced with however OPI references the middle/backend objects. Probably a volume handle/id/key of some sort.

benlwalker · 2022-10-21T14:22:50Z

storage/v1/frontend_nvme_pcie.proto

    string bdev = 6;
+
+    // Block size in bytes, must be power of 2 and must be less than the max
+    // io size supported. Typically tested values are 512, and 4k.


I would still like to see size and capacity dropped from the front-end. Those are instead implied by the backing device. Even if we were to add support for emulating one block size on top of another that's most likely done in the middle end.

if those are optional fields that if left empty - taken from backend, I don;t mind leaving them for now here and re-consider again once we defined middle end. baby steps

Emulation is often much trickier than it seems, so my suggested baby step is just delete these two things for now.

agree with @benlwalker that these should be dropped. If not, then it needs to be clarified to determine with takes precedence - values from backend object or values from the api. Also for scalar types since the ‘ default/value not set’ would be 0 it could also be a valid value, so it's not entirely harmful. I think the comment is also relevant for the fields: optimal_write_size, write_granularity below

if @jainvipin have a use case for having them, I incline to leave them
Agree with @mkalderon that comment to clarify if those provided, they overpower the backend device properties and those are omitted (or negative values like -1) then properties are taken from the backend device

benlwalker · 2022-10-21T14:24:21Z

storage/v1/frontend_nvme_pcie.proto

    string authentication = 13;
+
+    // reference to encryption key for the data at rest encryption
+    common.v1.Uuid crypto_key_id = 14;


Unless we're talking PCI bus encryption, this should be part of the back/middle.

benlwalker · 2022-10-21T14:24:46Z

storage/v1/frontend_nvme_pcie.proto

+    // this to regulate IO size. Must be a multiple of the preferred write
+    // granularity. Must not exceed the controller maximum IO size value
+    // configured in the nvme agent config file.
+    uint32 optimal_write_size = 15;


Pulled from back/middle, not configured here. Remove this.

I think the intent here is what you present to the HOST over PCIe

benlwalker · 2022-10-21T14:25:19Z

storage/v1/frontend_nvme_pcie.proto

+    // preferred write granularity hint to the host driver. Host IO
+    // stack may use this to align IO sizes to the write granularity for
+    // optimum performance.
+    uint32 pref_write_granularity= 16;


Pulled from back/middle, not configured here. Remove this.

I think the intent here is what you present to the HOST over PCIe

It's not for the front-end, or even an administrator, to decide. It's based on the BE/ME device reporting this value.

but what if implementation inside DPU/IPU for NVMe emulation has constrains of it's own on top of BE/ME device reporting ?

Then those constraints do not need to be configured over OPI and can just be set up and reported by the device

benlwalker · 2022-10-21T15:59:56Z

storage/v1/frontend_nvme_pcie.proto

+
+    // key of the PCIe controller object that will host this namespace.
+    //?? replaces int64 controller_id = 4;
+    common.v1.Uuid controller_id = 4;


A namespace is not associated with 1 controller. Instead, controllers can view namespaces. Having this reference here is not correct.

benlwalker · 2022-10-21T16:04:20Z

storage/v1/frontend_nvme_pcie.proto

+    // maximum length for NQN field
+    NSV_NVME_SUBSYSTEM_NQN_LEN        = 256;
+}
+


As I've said elsewhere, there's likely no reason to expose a subsystem object in the front-end. An xPU will probably never expose more than 1 controller per subsystem, so the two objects can be grouped into a "device" object. But if someone really has a use case for this in an xPU then I'm listening.

we see use cases with more then 1 controller per subsystem

agree @benlwalker - but I kept it since this was already an object and so moved a few things in there. So I am good removing this all together from the front end objects - however some of the fields included in Subsystem object can be folded into the NvmeController object then.

benlwalker · 2022-10-21T16:04:51Z

storage/v1/frontend_nvme_pcie.proto

@@ -38,36 +39,146 @@ service NVMeNamespaceService {
    rpc NVMeNamespaceStats  (NVMeNamespaceStatsRequest)  returns (NVMeNamespaceStatsResponse)  {}
 }

+enum NvmeFrontEndConstants {


Aren't these all spec defined values?

yep, what do you suggest ?
we can define them like this https://github.com/linux-nvme/libnvme/blob/master/src/nvme/types.h#L99-L129

I suggest dropping this enum since we know all of the values by spec.

I am not sure if they are, but even if they do, putting the here may serve explicitly for different current/future versions.

I suggest dropping this enum since we know all of the values by spec.

specifications also have versions, having this allows code to work on it via APIs, isn't it? Since these fields are exchanged via a versioned APIs, wouldn't it be better to keep it as part of API, instead of defining this in both client and server. Ultimately the client and server would need to have some checks on the length.

Are we expecting a single xPU to support different NVMe versions? Setting the version would be a clearer way to communicate that, rather than these lengths. Or are you suggesting some xPUs have stricter limits on the lengths of these values than the spec does, and this is for the xPU to report that back up to the client?

this brings up a question - do we need to have NMVE spec version xpu supports ? like 1.4 ?

perhaps- let's do in a separate PR then after more discussions/thought.

benlwalker · 2022-10-21T16:05:10Z

storage/v1/common.proto

    // SRIOV Virtual function within the Device and Physical function.
    // Set to 0 for Physical Function. Virtual Function numbering starts from 1
    uint32 virtual_function  = 4;
 }
+
+// Path Management Selection
+enum NvmePathMgmtType {


Can you explain what these do? Multipath is almost certainly a property of the backend and shouldn't be in this PR.

yes multipathing is a backend property (separate PR). however this is allowing selecting the path to be used by a given frontend Namespace object. So think of this as an association to a backend definition.

I don't think that makes any sense. You create the backend object, which includes the possible paths, and then you assign that object to the front-end. This should be removed from the front-end configuration.

benlwalker · 2022-10-21T16:07:31Z

storage/v1/frontend_nvme_pcie.proto

+    // maximum Number of namespaces that will be provisioned under
+    // the controller.
+    //?? val change
+    uint32 max_ns = 7;


This does not seem necessary. It's in the subsystem.

I was thinking we keep it here and take it out of the subsystem; it also depends on if we take out the subsystem object all together.

Signed-off-by: Vipin Jain <jainvipin@gmail.com>

storage/v1/frontend_nvme_pcie.proto

…ys in request/response messages Signed-off-by: Vipin Jain <jainvipin@gmail.com>

jainvipin · 2022-10-24T20:55:43Z

pushed the final set of changes - pls review @benlwalker @glimchb @mkalderon @orendu

glimchb · 2022-10-24T22:05:01Z

storage/v1/frontend_nvme_pcie.proto

+    common.v1.Uuid uuid = 10;
+
+    // reference to encryption key for the data at rest encryption
+    common.v1.ObjectKey crypto_key_id = 11;


to be more generic, this can be renamed to backend object id
type is ok here
once renamed, can remove the field bdev above

sure @glimchb - did you mean middle-end or the backend object-id? What we need to decide is if frontned objects will have reference to a generic bucket of middleend services (e.g. crypto, etc.) or one reference per frontend object say encryption granularity may make sense at a different level than compression or other middle-end services.

glimchb

Looks good now from my perspective
One last nit about crypto_key_id inline
Not clicking "approve" button only because want to give till the end of the week for other people to comment and if no objection, will merge on Thursday

storage/v1/frontend_nvme_pcie.proto

…message Signed-off-by: Vipin Jain <jainvipin@gmail.com>

mkalderon · 2022-10-26T10:28:44Z

storage/v1/frontend_nvme_pcie.proto

+    string nqn = 2;
+
+    // serial number must not exceed 'NSV_CTRLR_SERIAL_NO_LEN' bytes
+    string serial_number = 3;


is NSV_CTRLR_SERIAL_NO_LEN etc.. defined in the end somewhere?

we postponed NSV_CTRLR_SERIAL_NO_LEN to next PR

so should it still be part of comment ? not critical.. just made me look for it :-)

ah, ok... yes, need to remove the comment

mkalderon · 2022-10-26T10:29:13Z

storage/v1/frontend_nvme_pcie.proto

+    string firmware_revision  = 6;
+
+    // FRU identfier, 16bytes opaque identity for the type of unit
+    bytes fru_guid = 7;


@jainvipin aren't the firmware_revision and fru_guid part of the controller properties and not the subsystem ?

mkalderon · 2022-10-26T10:30:25Z

storage/v1/frontend_nvme_pcie.proto

+    // maximum host IO queue pairs allowed, value will default to
+    // limits in PCI device configuration; if set to 0 or more it
+    // will default to maximum permitted per xPU's capability
+    uint32 max_io_qps = 5;


this limits the ability to define seperate CQ, SQ and attach several SQs to a single CQs as spec permits.
Perhaps should be split ?
also - perhaps we should add the max_queue_depth param ?

I suggest if you can please create new PR @mkalderon with both additions:

split CQ and SQ

add max_queue_depth

glimchb · 2022-10-27T16:44:54Z

Approved during the API meeting nobody objected. Merging now

jainvipin requested a review from a team as a code owner October 21, 2022 07:29

frontend proto changes

db563a0

Signed-off-by: Vipin Jain <jainvipin@gmail.com>

jainvipin force-pushed the frontend-protos branch from 7aa4471 to db563a0 Compare October 21, 2022 07:36

mkalderon reviewed Oct 21, 2022

View reviewed changes

glimchb reviewed Oct 21, 2022

View reviewed changes

storage/v1/frontend_nvme_pcie.proto Show resolved Hide resolved

glimchb requested review from PWAlessi, orendu, prasunkapoor, sburla-marvell and shafiqabedin October 21, 2022 11:01

glimchb reviewed Oct 21, 2022

View reviewed changes

storage/v1/frontend_nvme_pcie.proto Show resolved Hide resolved

glimchb reviewed Oct 21, 2022

View reviewed changes

storage/v1/frontend_nvme_pcie.proto Outdated Show resolved Hide resolved

glimchb reviewed Oct 21, 2022

View reviewed changes

benlwalker suggested changes Oct 21, 2022

View reviewed changes

jainvipin added 3 commits October 21, 2022 11:25

use object key vs uuid to identify an object

e66d4b0

Signed-off-by: Vipin Jain <jainvipin@gmail.com>

remove name from controller and namespace objects

e72afa9

Signed-off-by: Vipin Jain <jainvipin@gmail.com>

remove multipath and authentication fields from namespace object

d22fca9

Signed-off-by: Vipin Jain <jainvipin@gmail.com>

jainvipin commented Oct 21, 2022

View reviewed changes

storage/v1/frontend_nvme_pcie.proto Show resolved Hide resolved

glimchb added the Storage APIs or code related to storage area label Oct 21, 2022

remove NvmePathMgmtType and NvmeFrontEndConstants; also change the ke…

7ac9ec5

…ys in request/response messages Signed-off-by: Vipin Jain <jainvipin@gmail.com>

jainvipin force-pushed the frontend-protos branch from 99f5d4c to 7ac9ec5 Compare October 24, 2022 20:53

glimchb reviewed Oct 24, 2022

View reviewed changes

storage/v1/frontend_nvme_pcie.proto Outdated Show resolved Hide resolved

remove bdev from namespace, also fix controller_id field in response …

65e016a

…message Signed-off-by: Vipin Jain <jainvipin@gmail.com>

glimchb added the Merge Candidate in the open merge window, next candidate for merge label Oct 25, 2022

mkalderon reviewed Oct 26, 2022

View reviewed changes

glimchb mentioned this pull request Oct 26, 2022

front end additions from PR #169 #173

Closed

glimchb approved these changes Oct 27, 2022

View reviewed changes

glimchb merged commit c9df395 into opiproject:main Oct 27, 2022

frontend proto changes #169

frontend proto changes #169

Conversation

jainvipin commented Oct 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jainvipin Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jainvipin Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glimchb Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glimchb commented Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glimchb Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jainvipin Oct 21, 2022 •

edited

Loading

jainvipin Oct 21, 2022 •

edited

Loading

glimchb Oct 21, 2022 •

edited

Loading

glimchb commented Oct 21, 2022 •

edited

Loading

glimchb Oct 21, 2022 •

edited

Loading