Skip to content

fix(pkg, mount): fetch all mount points for a given device#590

Merged
RealHarshThakur merged 1 commit intoopenebs-archive:masterfrom
z0marlin:multi-mps
May 20, 2021
Merged

fix(pkg, mount): fetch all mount points for a given device#590
RealHarshThakur merged 1 commit intoopenebs-archive:masterfrom
z0marlin:multi-mps

Conversation

@z0marlin
Copy link
Copy Markdown
Contributor

@z0marlin z0marlin commented May 10, 2021

Partitions can be mounted at multiple points in a given system.
Return a list of all the mount points of a given device as found in the
mounts file instead of just returning the first entry.

Signed-off-by: Aditya Jain aditya.jainadityajain.jain@gmail.com

Checklist:

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 10, 2021

Codecov Report

Merging #590 (9406b24) into master (2f01a37) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #590      +/-   ##
==========================================
+ Coverage   49.47%   49.56%   +0.09%     
==========================================
  Files          73       73              
  Lines        3359     3371      +12     
==========================================
+ Hits         1662     1671       +9     
- Misses       1541     1545       +4     
+ Partials      156      155       -1     
Impacted Files Coverage Δ
pkg/mount/mountinfo.go 0.00% <ø> (ø)
cmd/ndm_daemonset/probe/mountprobe.go 48.48% <100.00%> (ø)
pkg/mount/mountutil.go 74.45% <100.00%> (+2.45%) ⬆️
cmd/ndm_daemonset/probe/udevprobe.go 64.64% <0.00%> (-1.66%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f01a37...9406b24. Read the comment docs.

@z0marlin z0marlin changed the title [WIP] fix(pkg, mount): fetch all mount points for a given device fix(pkg, mount): fetch all mount points for a given device May 11, 2021
@z0marlin z0marlin marked this pull request as ready for review May 11, 2021 12:11
@akhilerm akhilerm self-requested a review May 11, 2021 13:05
@akhilerm
Copy link
Copy Markdown
Contributor

@z0marlin Can you check the DCO Sign Off?

@z0marlin
Copy link
Copy Markdown
Contributor Author

@z0marlin Can you check the DCO Sign Off?

The DCO check is passing. Have I missed something?

@akhilerm
Copy link
Copy Markdown
Contributor

The DCO check is passing. Have I missed something?

Nope. Its good now.

Comment thread pkg/mount/mountutil.go
Comment thread pkg/mount/mountutil_test.go
Comment thread pkg/mount/mountutil.go
}
if mountAttr, ok := fn(line); ok {
return mountAttr, nil
if lineMountAttr, ok := fn(line); ok {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here fn can either be either getPartitionName or getMountName. Can you test the new approach works in both cases and does not break the existing implementation

Comment thread pkg/mount/mountutil.go
Copy link
Copy Markdown
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 some comments.

@z0marlin z0marlin force-pushed the multi-mps branch 2 times, most recently from 2949f32 to 9406b24 Compare May 14, 2021 09:55
akhilerm
akhilerm previously approved these changes May 14, 2021
Copy link
Copy Markdown
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

@akhilerm akhilerm requested review from RealHarshThakur and kmova May 14, 2021 11:22
@RealHarshThakur
Copy link
Copy Markdown
Contributor

@z0marlin Changes we discussed in the issue related to adding field in CRD, would that be a separate PR ?

@z0marlin
Copy link
Copy Markdown
Contributor Author

I think yes. I'm still waiting on a confirmation to make the changes in CRD. Have broken down the issue into two tasks for the same reason. @RealHarshThakur

@akhilerm
Copy link
Copy Markdown
Contributor

@z0marlin Can you add the changelog to this PR?

@z0marlin
Copy link
Copy Markdown
Contributor Author

@z0marlin Can you add the changelog to this PR?

Sure. Also, I have opened #592 to add tests related to getPartitionName. If that can be merged first, we can verify that this PR doesn't break the existing code.

@akhilerm
Copy link
Copy Markdown
Contributor

@z0marlin Can you fix the conflicts in this PR?

partitions can be mounted at multiple points in a given system.
return a list of all the mount points of a given device as found in the
mounts file instead of just returning the first entry.

Signed-off-by: Aditya Jain <aditya.jainadityajain.jain@gmail.com>
Copy link
Copy Markdown
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

@RealHarshThakur RealHarshThakur merged commit 1a103cc into openebs-archive:master May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NDM BlockDevice should support multiple mount points

5 participants