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(block): adding raw block volume support for LVM #14

Merged
merged 3 commits into from
Jan 30, 2021

Conversation

pawanpraka1
Copy link
Contributor

Signed-off-by: Pawan [email protected]

Why is this PR required? What issue does it fix?:

adding raw block volume support for LVM

Does this PR require any upgrade changes?:

No

If the changes in this PR are manually verified, list down the scenarios covered::

tested with the sample fio yaml added in this PR

@pawanpraka1 pawanpraka1 added the kind/feature Categorizes issue or PR as related to a new feature label Jan 25, 2021
@pawanpraka1 pawanpraka1 requested a review from kmova January 25, 2021 13:24
@@ -115,7 +115,8 @@ func (ns *node) NodePublishVolume(
// If the access type is block, do nothing for stage
Copy link
Contributor

Choose a reason for hiding this comment

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

Didnt get this comment. This is the publish request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not a vaild comment here, this is for nodestagevolume grpc call.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pawanpraka1 Can you remove this comment,

volume := vol.Spec.VolGroup + "/" + vol.Name
devicePath := DevPath + volume

mountopt := []string{"bind"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to bind mount the device at a path during nodepublish. Is it a requirement from CSI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, we need to moount the device to the path where kubelet wants us to.

Copy link
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

given a comment regarding workflow.

Copy link
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

LGTM. Gave a suggestion on removing the comment.

Signed-off-by: Pawan <[email protected]>
@kmova kmova merged commit 7de9b55 into openebs:master Jan 30, 2021
@pawanpraka1 pawanpraka1 added this to the v0.2 milestone Feb 15, 2021
@pawanpraka1 pawanpraka1 deleted the block branch February 18, 2021 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants