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.

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

feat(storage): set annotations for backend #330

Merged
merged 5 commits into from
Aug 29, 2023

Conversation

artek-koltun
Copy link
Contributor

@artek-koltun artek-koltun commented Aug 16, 2023

See #198 and AIP-203

First, let's try to discuss where we are going to keep functionality to connect to local Nvme PCIe drives for JBOF case, since it would affect how to annotate the fields properly. I see an empty separate backend_nvme_pcie.proto, but also I see NVME_TRANSPORT_PCIE enum value in NvmeTransportType in backend_nvme_tcp.proto file, which might imply reuse of the service for PCIe.

Some assumptions/considerations in the first patch:

  1. Assumed we can reuse NvmeRemoteControllerService for PCIe
  2. FabricsPath message was created, which is part of NvmePath. It allows to annotate some fields as REQUIRED for Nvme over fabrics transport types and have more consistent API. Otherwise, we need to keep them OPTIONAL, since we can use the same API to connect to local PCIe.
  3. TcpController was created to be consistent with previous item and isolate TCP related optional parameters. This message is a part of NvmeRemoteController.

Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
@artek-koltun artek-koltun added the Storage APIs or code related to storage area label Aug 16, 2023
@artek-koltun artek-koltun marked this pull request as ready for review August 16, 2023 07:57
@artek-koltun artek-koltun requested a review from a team as a code owner August 16, 2023 07:57
Copy link
Member

@glimchb glimchb left a comment

Choose a reason for hiding this comment

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

the annotations looks ok, but creating new message TcpController should be discussed and in a separate PR

@artek-koltun
Copy link
Contributor Author

the annotations looks ok, but creating new message TcpController should be discussed and in a separate PR

ok, I can remove TcpController for a while.
However, I want we all to agree on where we want to keep pcie related part: in the existing NvmeRemoteControllerService service or in a dedicated one within backend_nvme_pcie.proto
@jainvipin @sburla-marvell @GottliebNoam?

Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
@glimchb
Copy link
Member

glimchb commented Aug 28, 2023

@artek-koltun yes, please remove TcpController for now
and we will add have this discussion

Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Storage APIs or code related to storage area
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants