Provide initial MEV NVMe implementation. #15
Conversation
Codecov Report
@@ Coverage Diff @@
## main #15 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 1 2 +1
Lines 258 117 -141
======================================
+ Misses 258 117 -141
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
artek-koltun
force-pushed
the
feat-initial-mev-nvme-implementation
branch
2 times, most recently
from
2890632
to
b32acfe
Compare
README.md
Outdated
| @@ -20,6 +20,12 @@ See [CONTRIBUTING](https://github.com/opiproject/opi/blob/main/CONTRIBUTING.md) | |||
|
|
|||
| ## Getting started | |||
|
|
|||
| a special transport should be created on IPU side by | |||
There was a problem hiding this comment.
do you expect user to create this manually or gRPC handles it? I think some small clarification needed here
There was a problem hiding this comment.
remove - more detailed instructions can be adjusted later
There was a problem hiding this comment.
It is not clear, how the whole flow is going to be used from the user POV, probably we can just remove this note. What would you say @intelfisz ?
| } | ||
|
|
||
| // CreateNVMeController creates an NVMe Controller | ||
| func (s *Server) CreateNVMeController(ctx context.Context, in *pb.CreateNVMeControllerRequest) (*pb.NVMeController, error) { |
There was a problem hiding this comment.
what is different about this function VS op-spdk-bridge impl ?
There was a problem hiding this comment.
opi-spdk-bridge works with tcp transport only at the moment. For this bridge we use another transport, and another format for the arguments.
The interface of the call is the same as the one in SPDK
There was a problem hiding this comment.
I would try to override Trtype and Traddr , then we can re-use all the code...
There was a problem hiding this comment.
Just wanted to note that by design, the RPC interfaces are exactly the same as regular SPDK NVMe-oF target and we treat it as a new transport type. So it really is just overriding trtype/traddr.
There was a problem hiding this comment.
Yep, what I'm saying, we can maybe place trtype/traddr in the server struct and then replacement will be easier without re-writing the functions, just init the server with proper arguments...
README.md
Outdated
| @@ -20,6 +20,12 @@ See [CONTRIBUTING](https://github.com/opiproject/opi/blob/main/CONTRIBUTING.md) | |||
|
|
|||
| ## Getting started | |||
|
|
|||
| a special transport should be created on IPU side by | |||
There was a problem hiding this comment.
remove - more detailed instructions can be adjusted later
artek-koltun
force-pushed
the
feat-initial-mev-nvme-implementation
branch
from
b32acfe
to
b727593
Compare
| msg := fmt.Sprintf("unable to find subsystem with id %v for controller %v", | ||
| controller.Spec.SubsystemId.Value, | ||
| in.Name) | ||
| log.Printf("error: %v", msg) |
There was a problem hiding this comment.
same comment here about idempotency: why instead of error, not to return the success here if controller already deleted ?
There was a problem hiding this comment.
I don't want to hold you from merging (especially that you are maintainer) but few notes:
- I see a lot of dup with
opi-spdk-bridgeimplementation. if we can just overrideTrtypeandTraddr, then we can re-use all the code instead of duplication. - I left few comments inside about idempotency.
artek-koltun
force-pushed
the
feat-initial-mev-nvme-implementation
branch
from
b727593
to
91b7f5a
Compare
This patch provides support for NVMe device creation/deletion for MEV and leverage the existing functionality of opi-spdk-bridge to handle subsystem/namespace related requests. Signed-off-by: Artsiom Koltun <[email protected]>
This patch provides support for NVMe device creation/deletion for MEV and leverage the existing functionality of opi-spdk-bridge to handle subsystem/namespace related requests.