S3FS Fuse Driver Support #409

akutz · 2017-02-07T20:13:55Z

This patch introduces support for mounting S3FS buckets as Linux file systems. This initial version of the S3FS storage driver does not yet support volume (bucket) creation/removal.

This patch also updates the AWS SDK dependency that is shared by the EBS and EFS storage drivers to v1.6.18, released on 2017/01/27.

Finally, this patch also refactors the VolumeRemove, and its related signatures, to use the new VolumeRemoveOpts struct instead of a bare Options type. All drivers affected by this change have been updated as part of this patch. For people compiling REX-Ray against this branch, you will need the following patch in order to do so. The linked patch updates REX-Ray's two sources affected by this change to libStorage.

This PR replaces the existing S3FS PR #397. Thank you @Andrey-mp and @alexey-mr for your work on this, we could not have done this without you.

codecov-io · 2017-02-07T20:26:20Z

Codecov Report

Merging #409 into master will increase coverage by 0.02%.

@@            Coverage Diff             @@
##           master     #409      +/-   ##
==========================================
+ Coverage   30.47%   30.49%   +0.02%     
==========================================
  Files          29       29              
  Lines        1739     1741       +2     
==========================================
+ Hits          530      531       +1     
- Misses       1151     1152       +1     
  Partials       58       58

Impacted Files	Coverage Δ
api/types/types_drivers_storage.go	`50% <ø> (ø)`	✅
api/types/types_clients.go	`0% <ø> (ø)`	✅
api/types/types_model.go	`0% <ø> (ø)`	✅
api/types/types_drivers_executor.go	`0% <ø> (ø)`	✅
api/types/types_localdevices.go	`79.24% <ø> (+1.88%)`	✅

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 f0aa0a1...3c24fe3. Read the comment docs.

codenrhoden · 2017-02-07T20:33:19Z

drivers/storage/s3fs/executor/s3fs_executor.go

+		for _, o := range d.opts {
+			args = append(args, fmt.Sprintf("-o%s", o))
+		}
+	} else if d.szOpts != "" {


I'm confused by the difference between opts and szOpts. I mean, I can see that the former is a string, and the latter is a slice of strings, but why are the two necessary? The docs say that options is supposed to be a list of options, and that -o should not be included in the config value. So shouldn't only one of these be necessary?

Perhaps I'm not familiar with gofig, but I don't know why, in the Init function, both GetString and GetStringSlice must be used. But if they do have to be used, would it be possible to just have one "opts" field in the driver stuct, as a string slice, and put in the single entry rather than having two different opts fields?

What does "sz" in szOpts mean here, Size? that's how I read it. But I also don't get why the -o isn't required.

Hi @codenrhoden,

So, szOpts stands for string of characters terminated by null character ... Opts. The sz prefix is often found in use by C or C++ programmers to denote a null-terminated string. I will sometimes use it to indicate a variable is a string if there is a similarly named variable of a different type.

The properties in the config file don't really have types per se. It's more how they're interpreted upon being consumed. Thus while it's cleaner to specify the options as an array of arg=value or flag items, maybe I also want to read it as a string? The above code first attempts to parse the s3fs.options value as an array of strings. If it isn't it reads it as a string. That way a user can determine how to put in the options.

codenrhoden · 2017-02-07T20:36:36Z

drivers/storage/s3fs/s3fs.go

+		"",
+		DefaultMaxRetries,
+		"Max number of times to retry failed operations",
+		ConfigS3FSMaxRetries)


The hostname, tag, and maxretries options are not mentioned in the docs.

Hi @codenrhoden,

They're really developer options for overriding the instance ID value and for debugging.

codenrhoden · 2017-02-07T20:53:09Z

drivers/storage/s3fs/storage/s3fs_storage.go

+
+// Type returns the type of storage the driver provides.
+func (d *driver) Type(ctx types.Context) (types.StorageType, error) {
+	return types.Object, nil


Is this true? Even though the backing storage is object, you end up with a file system, so I would think the type is File.

Hi @codenrhoden,

This value refers to the backing storage type.

codenrhoden · 2017-02-07T20:55:03Z

drivers/storage/s3fs/storage/s3fs_storage.go

+
+// VolumeAttach attaches a volume and provides a token clients can use
+// to validate that device has appeared locally.
+func (d *driver) VolumeAttach(


Can we add a comment that for s3fs, the attach is not performed server-side, but is left to the executor?

Hi @codenrhoden,

The client performs a mount, not an attach.

codenrhoden · 2017-02-07T20:58:29Z

drivers/storage/s3fs/storage/s3fs_storage.go

+			DeviceName: bucket,
+		}
+		if iid, iidOK := context.InstanceID(ctx); iidOK {
+			vatt.InstanceID = iid


I know things were weird with this driver, but isn't this going to list every single bucket as attached to the instance if a client provides the IID header?

Hi @codenrhoden,

Ah, you're right. I should make it so all volumes are available if the instance ID is not provided. Thanks.

codenrhoden · 2017-02-07T21:04:01Z

drivers/storage/s3fs/utils/utils.go

+	if config == nil {
+		hostName = config.GetString(s3fs.ConfigS3FSHostName)
+	} else {
+		hostName, _ = os.Hostname()


It wasn't until I looked at this code that it dawned on me that this driver is useful outside of AWS. Of course it is, but I just hadn't thought about it. Might be something worth pointing out in the docs, because it is really useful?

Would there be any merit it trying to make sure there is a more uniqueness to the IID, by appending a MAC address or anything? Probably not, but I think once you jump out of the cloud provider, you are much more likely to run into hostname collisions.

HI @codenrhoden,

Yes, the S3FS driver is not at all required to be used on EC2 instances. I'll add that to the documentation if you think it is important to call it out. I figured anyone wanting to use S3 would understand that, but there's no harm in calling it out.

The hostname and instance ID are not used by the storage driver except for logging. Even there it could be useful for them to be unique I suppose.

akutz · 2017-02-07T21:13:43Z

HI @codenrhoden,

I'm going to call you to discuss many of your comments due to the unique nature of this driver.

This patch introduces support for mounting S3FS buckets as Linux file systems. This initial version of the S3FS storage driver does not yet support volume (bucket) creation/removal. This patch also updates the AWS SDK dependency that is shared by the EBS and EFS storage drivers to v1.6.18, released on 2017/01/27. This patch now includes support for force deleting a non-empty bucket. All of the bucket's objects and their versions will be removed if VolumeRemove is invoked with a truthy force value.

akutz added in progress s3fs storagedrivers labels Feb 7, 2017

akutz added this to the 0.5.0 milestone Feb 7, 2017

akutz self-assigned this Feb 7, 2017

akutz requested review from codenrhoden and cduchesne February 7, 2017 20:13

akutz mentioned this pull request Feb 7, 2017

S3FS storage driver #397

Closed

3 tasks

akutz force-pushed the feature/s3fs branch 2 times, most recently from 9e307ec to 65f29af Compare February 7, 2017 20:22

codenrhoden reviewed Feb 7, 2017

View reviewed changes

akutz force-pushed the feature/s3fs branch 17 times, most recently from 5f164c7 to 017bc7f Compare February 8, 2017 07:22

akutz force-pushed the feature/s3fs branch 2 times, most recently from 4e0932a to 3c24fe3 Compare February 8, 2017 20:00

cduchesne mentioned this pull request Feb 9, 2017

S3FS storage driver rexray/rexray#709

Closed

akutz force-pushed the feature/s3fs branch 5 times, most recently from 14baad8 to eaac7a5 Compare February 9, 2017 23:09

akutz changed the base branch from master to release/0.5.0 February 9, 2017 23:14

akutz force-pushed the feature/s3fs branch from eaac7a5 to 3322a7c Compare February 9, 2017 23:21

akutz force-pushed the feature/s3fs branch from 3322a7c to 47d0ff2 Compare February 9, 2017 23:31

akutz merged commit ad5dbe0 into thecodeteam:release/0.5.0 Feb 9, 2017

akutz deleted the feature/s3fs branch February 9, 2017 23:53

akutz removed the in progress label Feb 9, 2017

akutz mentioned this pull request Feb 16, 2017

FittedCloud storage driver #408

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

S3FS Fuse Driver Support #409

S3FS Fuse Driver Support #409

akutz commented Feb 7, 2017 •

edited

Loading

codecov-io commented Feb 7, 2017 •

edited

Loading

codenrhoden Feb 7, 2017

akutz Feb 7, 2017

codenrhoden Feb 7, 2017

akutz Feb 7, 2017

codenrhoden Feb 7, 2017

akutz Feb 7, 2017

codenrhoden Feb 7, 2017

akutz Feb 7, 2017

codenrhoden Feb 7, 2017

akutz Feb 7, 2017

codenrhoden Feb 7, 2017

akutz Feb 7, 2017

akutz commented Feb 7, 2017

S3FS Fuse Driver Support #409

S3FS Fuse Driver Support #409

Conversation

akutz commented Feb 7, 2017 • edited Loading

codecov-io commented Feb 7, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akutz commented Feb 7, 2017

akutz commented Feb 7, 2017 •

edited

Loading

codecov-io commented Feb 7, 2017 •

edited

Loading