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

[storage] Add target to virtio-scsi, clean up virtio-blk #207

Merged
merged 1 commit into from
Nov 15, 2022

Conversation

benlwalker
Copy link

virtio-scsi has the same 3 level object system as NVMe

Signed-off-by: Ben Walker [email protected]

@benlwalker benlwalker requested a review from a team as a code owner November 14, 2022 19:43
@glimchb glimchb requested a review from a team November 14, 2022 20:03
@glimchb glimchb added Storage APIs or code related to storage area Merge Candidate in the open merge window, next candidate for merge labels Nov 14, 2022
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.

LGTM, thanks @benlwalker

@glimchb
Copy link
Member

glimchb commented Nov 14, 2022

@benlwalker you can add Intel Copyright to the files you changing...

Copy link
Contributor

@PWAlessi PWAlessi left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@jainvipin jainvipin left a comment

Choose a reason for hiding this comment

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

one minor comment otherwise looks good

service FrontendVirtioScsiService {
rpc CreateVirtioScsiTarget (CreateVirtioScsiTargetRequest) returns (VirtioScsiTarget) {
option (google.api.http) = {
post: "/v1/virtioscsitargets"
Copy link
Contributor

@jainvipin jainvipin Nov 14, 2022

Choose a reason for hiding this comment

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

URLs needs hierarchy - hopefully we can do it after this PR; the flattened url names are including the hierarchy in the names explicitly. So the URLs in this and in NVMe protos can be done separately.

Copy link
Member

Choose a reason for hiding this comment

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

I opened #188 to track the fix of all the URLs at once


message VirtioScsiTargetStatsResponse {
common.v1.ObjectKey id = 1;
string stats = 2;
Copy link
Contributor

@jainvipin jainvipin Nov 14, 2022

Choose a reason for hiding this comment

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

not sure if this makes any sense, returning a big string for stats; if we are unsure about stats protos, we can remove it from here and introduce if with a bit more structure/thinking later. I'd suggest removing statis req/response all together for now.

Copy link
Member

@glimchb glimchb Nov 14, 2022

Choose a reason for hiding this comment

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

we have now string stats defined in all our objects, so I'm personally OK with this now to align with what we have already and then fix stats later on. opened #209 to track

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, that would work.

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.

looks like straight forward alignment we doing already today for NVMe
opened separate issues to fix the review comments about URLs and STATs

Copy link
Contributor

@jainvipin jainvipin left a comment

Choose a reason for hiding this comment

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

LGTM

@glimchb glimchb merged commit 23fcac6 into opiproject:main Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge Candidate in the open merge window, next candidate for merge Storage APIs or code related to storage area
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants