S3FS storage driver #397
S3FS storage driver #397
Conversation
| "github.com/codedellemc/libstorage/drivers/storage/s3fs" | ||
| ) | ||
|
|
||
| func Supported(ctx types.Context) (bool, error) { |
There was a problem hiding this comment.
exported function Supported should have comment or be unexported
| "github.com/codedellemc/libstorage/drivers/storage/s3fs" | ||
| ) | ||
|
|
||
| func Supported(ctx types.Context) (bool, error) { |
There was a problem hiding this comment.
exported function Supported should have comment or be unexported
| // Name is the provider's name. | ||
| Name = "s3fs" | ||
|
|
||
| // Name of the cmd utility |
There was a problem hiding this comment.
comment on exported const CmdName should be of the form "CmdName ..."
| // Name is the provider's name. | ||
| Name = "s3fs" | ||
|
|
||
| // Name of the cmd utility |
There was a problem hiding this comment.
comment on exported const CmdName should be of the form "CmdName ..."
| return isDeviceUriSupported(d.name, device) | ||
| } | ||
|
|
||
| func (d *s3fsDriver) getBucketFromUri(uri string) string { |
There was a problem hiding this comment.
func getBucketFromUri should be getBucketFromURI
| return isDeviceUriSupported(d.name, device) | ||
| } | ||
|
|
||
| func (d *s3fsDriver) getBucketFromUri(uri string) string { |
There was a problem hiding this comment.
func getBucketFromUri should be getBucketFromURI
| if mp == mountPoint { | ||
| // bucket is mounted to the required target => ok | ||
| return nil | ||
| } else { |
There was a problem hiding this comment.
if block ends with a return statement, so drop this else and outdent its block
| if mp == mountPoint { | ||
| // bucket is mounted to the required target => ok | ||
| return nil | ||
| } else { |
There was a problem hiding this comment.
if block ends with a return statement, so drop this else and outdent its block
| const ( | ||
| Name = "s3fs" | ||
| s3fsCmdName = "s3fs" | ||
| s3fsUriPrefix = Name + "://" |
There was a problem hiding this comment.
const s3fsUriPrefix should be s3fsURIPrefix
| const ( | ||
| Name = "s3fs" | ||
| s3fsCmdName = "s3fs" | ||
| s3fsUriPrefix = Name + "://" |
There was a problem hiding this comment.
const s3fsUriPrefix should be s3fsURIPrefix
| ) | ||
|
|
||
| const ( | ||
| Name = "s3fs" |
There was a problem hiding this comment.
exported const Name should have comment (or a comment on this block) or be unexported
| ) | ||
|
|
||
| const ( | ||
| Name = "s3fs" |
There was a problem hiding this comment.
exported const Name should have comment (or a comment on this block) or be unexported
| return strings.Split(device, ":")[0] | ||
| } | ||
|
|
||
| func makeDeviceUri(driver, device string) string { |
There was a problem hiding this comment.
func makeDeviceUri should be makeDeviceURI
| return strings.Split(device, ":")[0] | ||
| } | ||
|
|
||
| func makeDeviceUri(driver, device string) string { |
There was a problem hiding this comment.
func makeDeviceUri should be makeDeviceURI
| return strings.TrimPrefix(device, fmt.Sprintf("%s://", name)) | ||
| } | ||
|
|
||
| func getNameFromDeviceUri(device string) string { |
There was a problem hiding this comment.
func getNameFromDeviceUri should be getNameFromDeviceURI
| return strings.TrimPrefix(device, fmt.Sprintf("%s://", name)) | ||
| } | ||
|
|
||
| func getNameFromDeviceUri(device string) string { |
There was a problem hiding this comment.
func getNameFromDeviceUri should be getNameFromDeviceURI
| return strings.HasPrefix(device, fmt.Sprintf("%s://", name)) | ||
| } | ||
|
|
||
| func getPathFromDeviceUri(name, device string) string { |
There was a problem hiding this comment.
func getPathFromDeviceUri should be getPathFromDeviceURI
| return strings.HasPrefix(device, fmt.Sprintf("%s://", name)) | ||
| } | ||
|
|
||
| func getPathFromDeviceUri(name, device string) string { |
There was a problem hiding this comment.
func getPathFromDeviceUri should be getPathFromDeviceURI
| } | ||
|
|
||
| // Help functions to operate with device uri | ||
| func isDeviceUriSupported(name, device string) bool { |
There was a problem hiding this comment.
func isDeviceUriSupported should be isDeviceURISupported
| } | ||
|
|
||
| // Help functions to operate with device uri | ||
| func isDeviceUriSupported(name, device string) bool { |
There was a problem hiding this comment.
func isDeviceUriSupported should be isDeviceURISupported
| "github.com/codedellemc/libstorage/api/types" | ||
| ) | ||
|
|
||
| // Function to construct new MountDriver objects |
There was a problem hiding this comment.
comment on exported type NewMountDriver should be of the form "NewMountDriver ..." (with optional leading article)
| "github.com/codedellemc/libstorage/api/types" | ||
| ) | ||
|
|
||
| // Function to construct new MountDriver objects |
There was a problem hiding this comment.
comment on exported type NewMountDriver should be of the form "NewMountDriver ..." (with optional leading article)
Codecov Report@@ Coverage Diff @@
## master #397 +/- ##
=======================================
Coverage 30.47% 30.47%
=======================================
Files 29 29
Lines 1739 1739
=======================================
Hits 530 530
Misses 1151 1151
Partials 58 58Continue to review full report at Codecov.
|
|
|
|
Hi @alexey-mr, Thank you for this PR. I like the idea of a mount driver; in fact it's been something we've discussed a few times internally due to other driver requirements. Two notes about the way that it's been implemented in this PR:
If it's okay with you, I'd like to go ahead and rewrite the new mount driver for the Thoughts so far? Beyond my comments on the mount driver, I am currently reviewing the rest of the code in the PR. My only other immediate note is that I would like to see the PR's commits squashed and rebased. Thank you. |
| // so, it is workaround to patch Source info | ||
| // Another option - use moinPoint to filter mounts into | ||
| // the integration driver logic and remove this patching | ||
| d.patchMounts(ctx, &mounts) |
There was a problem hiding this comment.
Hi @codenrhoden,
Can you please take a look at the suggested PatchMountInfo function as well as the implementation of it in this PR and tell me if I'm crazy, but I think we should take the other option @alexey-mr suggests and relocate this to the integration driver here.
If I'm following all of the code correctly, it appears that the patching/lookup should really be occurring in the LocalDevices function of the executor so that the Libstorage-Localdevices map is initialized properly when sent to the server and thus the returned volumes have the mappings correctly set. That would mean the correct device name and source should be set when used in the integration driver.
Thoughts?
There was a problem hiding this comment.
@akutz I agree that the patching should happen in the executors LocalDevices(). I am not following, however, what would need to be added to the integration driver that is related to this... I feel like I should see it, but I don't.
With your work to add mount/unmount functionality capability to the executors, what would need to be added to the integration driver? I'm assuming a check to see if the volume we are working with needs to be mounted via executor, or one of our existing "standard" mount functions, but is there anything else related to the patching of mount info?
| return buckets, nil | ||
| } | ||
|
|
||
| func Mount( |
There was a problem hiding this comment.
exported function Mount should have comment or be unexported
| return nil, err | ||
| } | ||
| var buckets []string | ||
| for b, _ := range bucketsMap { |
There was a problem hiding this comment.
should omit 2nd value from range; this loop is equivalent to for b := range ...
s3fs allows to mount S3 buckets via FUSE. It can't create new buckets, so this first impl of the s3fs storage driver doesn't create/remove volumes actually. It uses list of buckets that could be used for mounting from config file. These buckets are returned as volumes and are available for mount/unmount operations.
|
Hi @akutz, I reworked it according updated executor's interface. Now patching of mountinfo.Source is moved in LocalDevices of s3fs executor... But if I understand correctly somethingelse should be done additionally to make it work, because Mounts() (in integration driver) doesnt call LocalDevices from storage driver executors. |
|
Hi @alexey-mr, Thank you again for your work on this. In light of recent changes and in order to close this out, I'm taking ownership of it at #409. Thank you again. |
This is the draft implementation, or proof-of-concent (POC), of the
s3fsstorage driver that enables the mounting of S3 buckets via FUSE.Restrictions
The current implementation has the following restrictions:
Next Steps