-
Notifications
You must be signed in to change notification settings - Fork 172
Allow size as a percent for torusd. Fixes #175 #278
Conversation
2d56731
to
05a3806
Compare
fmt.Fprintf(os.Stderr, "error parsing size: %s\n", err) | ||
os.Exit(1) | ||
if strings.Contains(sizeStr, "%") { | ||
var stat syscall.Statfs_t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably should separate this into two different funcs: parsePercentage and getDiskSize.
for getDiskSize, do we want to be portable? current solution only works on unix i think.
not sure if it worth to introduce a dependency to handle it: https://github.com/StalkR/goircbot/tree/master/lib/disk
or we can use os.FileInfo and get os dependent field in Sys() https://golang.org/pkg/os/#FileInfo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xiang90 splitting it makes sense, I'll do that.
Yeah the current solution only would work on unix, not sure how wide spread torus is targeting for portability - the windows solution I saw was pretty shady, requiring linking the kernel32.dll.
I don't think we can use Sys() to get portability, looks like that was un-implemented for windows according to https://github.com/golang/go/issues/9611
, I could be wrong on that though, I don't have a windows machine to test at this moment.
I'll look a the dependency, if we want Windows portability it may be worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beh9540 At least it can build and probably just throwing unsupported on windows is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, the dependency is fine, and does the equivalent of dlopen()
on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's actually probably a better dependency here https://github.com/ricochet2200/go-disk-usage which does the same thing but brings less with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This is correct; though the Config struct should change slightly after this because there's a subtlety between storage-size presented to the cluster and storage size used (backends might need a little space for its own bookkeeping). Right now the flag is the former, and this refers to the latter. It's an easy calculation, and this can go in first, but it spawns a tiny followup issue. |
One thing I noticed while testing this is that it's really easy to get a percent of the drive that isn't an even multiple of the torus blocksize. It's easy enough to make the blocksize line up, but it seems like it's currently not easy to get the torus blocksize, as it seems to be set in two different places depending on which MDS is being used. I can hard code the values in, but that feels like a sub-optimal solution. Another idea would be to adjust the size in storage/mmap_file.go after the blocksize has been determined. Any thoughts on which way would be the better approach? |
f9c2e65
to
c75900b
Compare
c75900b
to
bdd79d3
Compare
Better approach is to always follow the blocksize from the MDS; if it's over, just shrink it a bit smaller to align to the blocksize. You have to initialize the MDS anyway before torusd can come up, so that's not a problem. |
@barakmich I added resizing to bring the volume down to the closet even multiple. That should complete this, assuming everything checks out okay. |
Merged! Sorry about the delay. |
No description provided.