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

New rpc service to attach/detach a controller to a namespace. #237

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

New rpc service to attach/detach a controller to a namespace. #237

wants to merge 1 commit into from

Conversation

VaibhavJMarvell
Copy link

Added attach/detach controller to cater to use-case of an existing customer.

Signed-off-by: Vaibhav Jain [email protected]
Signed-off-by: Michal Kalderon [email protected]

@VaibhavJMarvell VaibhavJMarvell requested a review from a team as a code owner January 25, 2023 18:18
@glimchb glimchb requested a review from a team January 25, 2023 18:43
@glimchb glimchb added the Storage APIs or code related to storage area label Jan 25, 2023
glimchb
glimchb requested changes Jan 25, 2023
View reviewed changes
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.

how this si different from CreateNVMeNamespace ?

@VaibhavJMarvell
Copy link
Author

VaibhavJMarvell commented Jan 25, 2023 •
edited by glimchb
Loading

how this si different from CreateNVMeNamespace ?

We have an existing customer whose use-case requires attaching/detaching specific controllers to a namespace. In the current scenario of CreateNVMeNamespace, all the controllers are attached to the newly created namespace.

@glimchb
Copy link
Member

glimchb commented Jan 25, 2023 •
edited
Loading

how this si different from CreateNVMeNamespace ?

We have an existing customer whose use-case requires attaching/detaching specific controllers to a namespace. In the current scenario of 'CreateNVMeNamespace', all the controllers are attached to the newly created namespace.

so maybe just add field ControllerId to existing CreateNVMeNamespace ? instead of new RPCs? see ca2c25b

jainvipin
jainvipin suggested changes Jan 26, 2023
View reviewed changes
Copy link
Contributor

@jainvipin jainvipin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

+1 on the need for this API i.e. the need to explicitly enable/disable the namespace within a controller.
a few minor comments inline @VaibhavJMarvell

storage/v1alpha1/frontend_nvme_pcie.proto Outdated
(google.api.field_behavior) = REQUIRED
];
string controller = 2 [(google.api.field_behavior) = REQUIRED];
string nv_me_namespace_id = 3 [(google.api.field_behavior) = REQUIRED];
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be common.v1.ObjectKey which bytes all ids are like that. Same for controller?

@jainvipin
Copy link
Contributor

jainvipin commented Jan 26, 2023 •
edited
Loading

how this si different from CreateNVMeNamespace ?

We have an existing customer whose use-case requires attaching/detaching specific controllers to a namespace. In the current scenario of 'CreateNVMeNamespace', all the controllers are attached to the newly created namespace.

so maybe just add field ControllerId to existing CreateNVMeNamespace ? instead of new RPCs? see ca2c25b

we could do that to, but this API would then become explicit enable/disable API to toggle namespace on controller.

@VaibhavJMarvell
New rpc service to attach/detach a controller to a namespace.
1699976 
Added attach/detach controller to cater to use-case of an existing customer.

Signed-off-by: Vaibhav Jain <[email protected]>
Signed-off-by: Michal Kalderon <[email protected]>
VaibhavJMarvell
VaibhavJMarvell commented Jan 26, 2023
View reviewed changes
Copy link
Author

@VaibhavJMarvell VaibhavJMarvell left a comment

Choose a reason for hiding this comment

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

I have changed the controller and namespace types to ObjectKey.

@glimchb
Copy link
Member

glimchb commented Jan 26, 2023

how this si different from CreateNVMeNamespace ?

We have an existing customer whose use-case requires attaching/detaching specific controllers to a namespace. In the current scenario of 'CreateNVMeNamespace', all the controllers are attached to the newly created namespace.

so maybe just add field ControllerId to existing CreateNVMeNamespace ? instead of new RPCs? see ca2c25b

we could do that to, but this API would then become explicit enable/disable API to toggle namespace on controller.

seems to me like we should use UpdateNVMeNamespace if we want to toggle enable/disable bit... instead of new RPC

@jainvipin
Copy link
Contributor

how this si different from CreateNVMeNamespace ?

We have an existing customer whose use-case requires attaching/detaching specific controllers to a namespace. In the current scenario of 'CreateNVMeNamespace', all the controllers are attached to the newly created namespace.

so maybe just add field ControllerId to existing CreateNVMeNamespace ? instead of new RPCs? see ca2c25b

we could do that to, but this API would then become explicit enable/disable API to toggle namespace on controller.

seems to me like we should use UpdateNVMeNamespace if we want to toggle enable/disable bit... instead of new RPC

we need to add enable/disable in NameSpaceSpec if we do it via namespace object; that'd work too, but having an admin know to enable a namespace on a subsystem/controller is the ask here.

@glimchb
Copy link
Member

glimchb commented Jan 26, 2023

we need to add enable/disable in NameSpaceSpec if we do it via namespace object; that'd work too, but having an admin know to enable a namespace on a subsystem/controller is the ask here.

agree, let's add Enable/Disable to NameSpaceSpec and then using Update RPC we can toggle it

@glimchb
Copy link
Member

glimchb commented Mar 17, 2023

@VaibhavJMarvell @jainvipin can you please continue this ? it is open for a long time

@glimchb glimchb linked an issue Sep 12, 2023 that may be closed by this pull request
[storage]: Front-end Attach / Detach Namespace Proto-Bufs #177
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glimchb glimchb glimchb requested changes

@jainvipin jainvipin jainvipin requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Storage APIs or code related to storage area
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[storage]: Front-end Attach / Detach Namespace Proto-Bufs
3 participants
@VaibhavJMarvell @glimchb @jainvipin