Implement backblaze-b2 gateway support #5002

harshavardhana · 2017-10-02T17:15:09Z

Description

Implement backblaze-b2 gateway support

Motivation and Context

Fixes #4072

How Has This Been Tested?

Manually testing and mint.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

codecov · 2017-10-02T17:27:12Z

Codecov Report

Merging #5002 into master will decrease coverage by 1.15%.
The diff coverage is 5.47%.

@@            Coverage Diff             @@
##           master    #5002      +/-   ##
==========================================
- Coverage   62.36%   61.21%   -1.16%     
==========================================
  Files         190      192       +2     
  Lines       27756    28320     +564     
==========================================
+ Hits        17310    17336      +26     
- Misses       9209     9740     +531     
- Partials     1237     1244       +7

Impacted Files	Coverage Δ
cmd/gateway-gcs.go	`6.13% <ø> (ø)`	⬆️
cmd/globals.go	`100% <ø> (ø)`	⬆️
cmd/gateway-b2.go	`0% <0%> (ø)`
cmd/gateway-main.go	`12.88% <0%> (-1.01%)`	⬇️
cmd/gateway-unsupported.go	`0% <0%> (ø)`	⬆️
cmd/gateway-b2-anonymous.go	`37.33% <37.33%> (ø)`
cmd/api-headers.go	`87.5% <50%> (-5.36%)`	⬇️
cmd/fs-v1-rwpool.go	`64.34% <0%> (-4.35%)`	⬇️

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 3d0dced...274ca16. Read the comment docs.

harshavardhana · 2017-10-03T02:30:54Z

Consistent unrelated failure on appveyor ignore windows error for now.

go test -v -timeout 17m -coverprofile=c.txt -covermode=atomic github.com/minio/minio/cmd
go build github.com/minio/minio/cmd: c:\go18\pkg\tool\windows_amd64\compile.exe: fork/exec c:\go18\pkg\tool\windows_amd64\compile.exe: The filename or extension is too long.

Talking to appveyor support at the moment.

harshavardhana · 2017-10-03T02:54:52Z

Looks like with this PR we are expanding beyond 32K limit on windows shell PATH_MAX - golang/go#18468

Work-around is i will try to merge the files reducing the overall length and lets see if it fixes.

harshavardhana · 2017-10-03T07:05:47Z

Looks like with this PR we are expanding beyond 32K limit on windows shell PATH_MAX - golang/go#18468

Work-around is i will try to merge the files reducing the overall length and lets see if it fixes.

Was able to fix it by merging some files.

krishnasrinivas · 2017-10-03T18:26:15Z

cmd/gateway-s3.go

@@ -134,8 +135,9 @@ func newS3Gateway(host string) (GatewayLayer, error) {
 	anonClient.SetCustomTransport(newCustomHTTPTransport())

 	return &s3Objects{
-		Client:     client,
-		anonClient: anonClient,
+		unsupportedAPIs: unsupportedAPIs{},


unsupportedAPIs: unsupportedAPIs{}, can be left out

krishnasrinivas · 2017-10-03T18:29:20Z

cmd/gateway-unsupported.go

@@ -16,27 +16,30 @@

 package cmd

+type unsupportedAPIs struct{}


can be renamed to more specific gatewayUnsupported

krishnasrinivas · 2017-10-03T19:32:25Z

cmd/gateway-b2.go

+	}
+	// startPartNumber must be in the range 1 - 10000 for B2.
+	if partNumberMarker == 0 {
+		partNumberMarker = 1


this will cause the list response to always skip the first part

It won't @krishnasrinivas . Not in case of Backblaze.

startPartNumber includes 1 and later, it is not the same style as S3.

in that case we need to do partNumberMarker++ always because:

krishna@escape:~/dev/minio-tile-2$ miniodebug multipart listparts --bucket krishna1980 --object testobject --uploadid 4_z1efea6cee041ea395aeb021f_f205a55e6b4215c82_d20171003_m202115_c001_v0001093_t0023 --partmarker 2 { "Bucket": "krishna1980", "Key": "testobject", "UploadID": "4_z1efea6cee041ea395aeb021f_f205a55e6b4215c82_d20171003_m202115_c001_v0001093_t0023", "Initiator": { "ID": "02d6176db174dc93cb1b899f7c6078f08654445fe8cf1b6ce98d8855f66bdbf4", "DisplayName": "" }, "Owner": { "DisplayName": "", "ID": "02d6176db174dc93cb1b899f7c6078f08654445fe8cf1b6ce98d8855f66bdbf4" }, "StorageClass": "STANDARD", "PartNumberMarker": 2, "NextPartNumberMarker": 0, "MaxParts": 1000, "IsTruncated": false, "ObjectParts": [ { "PartNumber": 2, "LastModified": "0001-01-01T00:00:00Z", "ETag": "\"5da54579c335464b6ba52090bf57d063384bb1e9\"", "Size": 2461 }, { "PartNumber": 3, "LastModified": "0001-01-01T00:00:00Z", "ETag": "\"5da54579c335464b6ba52090bf57d063384bb1e9\"", "Size": 2461 } ], "EncodingType": "" }

Oh yes you are right. @krishnasrinivas

krisis

These are some initial comments.

krisis · 2017-10-04T20:56:09Z

cmd/gateway-b2-anonymous.go

+	return fmt.Sprintf("%s%d-%d", byteRangePrefix, offset, offset+size-1)
+}
+
+// AnonGetObject - Get object anonymously


This comment doesn't say anything more than the name of the function. The comment could be AnonGetObject - Get object using a plain GET request. or something to that effect.

krisis · 2017-10-04T21:02:19Z

cmd/gateway-b2-anonymous.go

+// Converts http Header into ObjectInfo. This function looks for all the
+// standard Backblaze B2 headers to convert into ObjectInfo.
+//
+// Content-Length is converted to Size.


nit: we could order the headers in the comment such that X-Bz-... appear together.

They are written in the order they are handled inside the code..

krisis · 2017-10-04T21:07:08Z

cmd/gateway-b2-anonymous.go

+		return objInfo, err
+	}
+
+	info := make(map[string]string)


we could call this userDefinedHdrs instead.

krisis · 2017-10-04T22:09:20Z

cmd/gateway-b2-anonymous.go

+		Bucket:      bucket,
+		Name:        object,
+		ContentType: header.Get("Content-Type"),
+		ModTime:     time.Unix(timeStamp/1000, timeStamp%1000*1e6),


ModTime can be idiomatically computed like time.Unix(0, 0).Add(timeStamp * time.Millisecond).

Yes but why is your approach more idiomatic? @krisis .

It is idiomatic since it uses standard Go types like time.Duration to work with milliseconds. To work with milliseconds available as string in header, we could do the following,

timeStamp, err := time.ParseDuration(header.Get("X-Bz-Upload-Timestamp")) if err != nil { } modTime := time.Unix(0, 0).Add(timeStamp)

This way we use the appropriate Go types to work with time in milliseconds instead of dropping down to low-level types like int64, int.

krisis · 2017-10-04T22:14:13Z

cmd/gateway-b2-anonymous.go

+		return objInfo, err
+	}
+	defer resp.Body.Close()
+	if resp.StatusCode != 200 && resp.StatusCode != 206 {


do we expect partial content for a HEAD request?

Yes if you set the Range header.

we aren't setting range headers here. should we still check?

we don't need then

krisis · 2017-10-04T22:21:18Z

cmd/gateway-b2.go

+		uploadID = params[2]
+	}
+
+	// Following code is a non-exhaustive list.


nit: Following code is a non-exhaustive list is out of place since there is no list of codes below.

krisis · 2017-10-04T22:24:01Z

cmd/gateway-b2.go

+// MakeBucket creates a new container on B2 backend.
+func (l *b2Objects) MakeBucketWithLocation(bucket, location string) error {
+	info := make(map[string]string)
+	info["x-minio-bucket-createTime"] = UTCNow().Format(time.RFC1123)


"x-minio-bucket-createTime" should be a const.

krisis · 2017-10-04T22:26:08Z

cmd/gateway-b2.go

+	return b2ToObjectError(traceError(err), bucket)
+}
+
+// Bucket - bucket


this comment can be expanded.

krisis · 2017-10-04T22:29:08Z

cmd/gateway-b2.go

+}
+
+// Looks for x-minio-bucket-createTime as part of bucket metadata, if key
+// is absent then returns current UTC time instead. Returns error if


When creation time is unavailable we should use a sentinel value like `time.Unix(0,0) to indicate that we couldn't determine the actual time of creation. Returning current time may be misleading.

time.Unix(0,0) is not a good idea because of the tests that expect CreatTime to be always within some acceptable boundary of time. The reason to use currentTime here is to provide lastAccessedTime instead.

time.Unix(0,0) is not a good idea because of the tests that expect CreatTime to be always within some acceptable boundary of time

Which tests are you referring to? If it is unit tests we can (and should) change it as fit. This suggestion is to avoid any confusion to the user, who knows that the bucket was created much before the time we might show (i.e, last accessed time like you mention above).

I am talking about mint tests.. Setting time.Unix(0, 0) will be more confusing to user IMO. I would prefer not make this change and keep it the way it is. We can even discuss about this but i would like to avoid epoch date.

For the most part users using buckets from minio gateway will not have any problems, B2 users aren't expecting this value anyways. So i choose to set this value to something more meaningful than Epoch time.

As a matter of fact perhaps i can remove the x-minio-bucket-createTime and avoid such intelligence in the first place.

Setting time.Unix(0, 0) will be more confusing to user IMO. I would prefer not make this

The idea behind time.Unix(0,0) is that it is a constant and doesn't change with time like time.Now(). We could choose a different constant time.Time value if that's the only issue.

As a matter of fact perhaps i can remove the x-minio-bucket-createTime and avoid such intelligence in the first place.

what would InitiatedTime value for BucketInfo be?

Nothing just time.Unix(0, 0) there is no such info available from B2 API so we don't return anything.

harshavardhana · 2017-10-05T01:53:59Z

New change in this PR is regular PutObject() is doing multipart if the input stream size is bigger than MinPartSize . This is to ensure that we don't corrupt the namespace if there were errors during data transfer.

krishnasrinivas · 2017-10-06T18:02:45Z

cmd/gateway-b2.go

+	}
+}
+
+type hexEndReader struct {


could be renamed to b2Hasher as hexEndReader would be a generic name

krishnasrinivas · 2017-10-06T18:03:42Z

cmd/gateway-b2.go

+	}
+
+	hr := newHexDigitsEndReader(data, data.Size())
+	sha1, err := fc.UploadPart(l.ctx, hr, "hex_digits_at_end", hr.Len(), partID)


"hex_digits_at_end" should be a const

krishnasrinivas · 2017-10-06T18:27:15Z

cmd/gateway-b2-anonymous.go

+		ContentType: header.Get("Content-Type"),
+		ModTime:     time.Unix(0, 0).Add(time.Duration(timeStamp) * time.Millisecond),
+		Size:        clen,
+		ETag:        header.Get("X-Bz-Content-Sha1"),


Looks like fileID is the right candidate for ETag, because the functionality of If-Match is supported by b2_download_file_by_id and b2_get_file_info both of which take fileID, there is no SHA1 support for these calls

krisis · 2017-10-10T23:29:57Z

cmd/gateway-b2-anonymous.go

+// Content-Type is converted to ContentType.
+// X-Bz-Content-Sha1 is converted to ETag.
+func headerToObjectInfo(bucket, object string, header http.Header) (objInfo ObjectInfo, err error) {
+	clen, err := strconv.ParseInt(header.Get("Content-Length"), 10, 64)


Needs a comment mentioning what is going on here. E.g, // Converting upload timestamp in milliseconds to a time.Time value for ObjectInfo.ModTime.

krisis · 2017-10-10T23:49:46Z

cmd/gateway-b2-anonymous.go

+		return objInfo, err
+	}
+
+	userMetadata := make(map[string]string)


A comment explaining why were skipping headers that don't start with X-Bz-Info is needed.

krisis · 2017-10-11T00:25:38Z

cmd/gateway-b2-anonymous.go

+		return objInfo, err
+	}
+	defer resp.Body.Close()
+	if resp.StatusCode != 200 {


why are we returning a new error value, i.e errors.New(resp.Status) while returning the error as is in AnonGetObject?

what is the error that you would return as is?

krisis · 2017-10-11T00:29:12Z

cmd/gateway-b2.go

+	return b2ToObjectError(traceError(err), bucket)
+}
+
+// Bucket - bucket


Could we name this function similar to other functions like GetBucket? Is this function required to be exported? Please add a comment too.

krisis · 2017-10-11T00:32:48Z

cmd/gateway-b2.go

+				Name:        file.Name,
+				ModTime:     file.Timestamp,
+				Size:        file.Size,
+				ETag:        file.Info.SHA1,


Shouldn't file.FileID be returned for ETag?

krisis · 2017-10-11T00:33:33Z

cmd/gateway-b2.go

+				Name:        file.Name,
+				ModTime:     file.Timestamp,
+				Size:        file.Size,
+				ETag:        file.Info.SHA1,


Shouldn't file.FileID be returned for ETag?

krisis · 2017-10-11T00:38:32Z

cmd/gateway-b2.go

+type B2Reader struct {
+	r    *HashReader
+	size int64
+	hsh  hash.Hash


we can call it sha1Sum instead of hsh.

krisis · 2017-10-11T00:42:07Z

cmd/gateway-b2.go

+	if err != nil {
+		return b2ToObjectError(traceError(err), bucket, object)
+	}
+	io.Copy(ioutil.Discard, reader)


can we not Close reader without copying?

krisis · 2017-10-11T00:55:35Z

cmd/gateway-b2.go

+	return lpi, nil
+}
+
+// AbortMultipartUpload aborts a ongoing multipart upload, uses B2's LargeFile upload API.


typo: s/a ongoing/an ongoing/

krisis · 2017-10-11T01:05:47Z

cmd/gateway-b2.go

+	}
+	_, err = bkt.File(uploadID, object).CompileParts(0, hashes).FinishLargeFile(l.ctx)
+	if err != nil {
+		return oi, err


shouldn't we call b2ToObjectError(traceError(err), ...)?

krisis · 2017-10-11T21:53:13Z

cmd/gateway-b2.go

+	"sync"
+	"time"
+
+	b2 "github.com/minio/blazer/base"


why are we not using "github.com/minio/blazer/b2" client, which has reauthorization built into it?

We need low level API @krisis

Fixes minio#4072

harshavardhana force-pushed the backblaze-b2 branch from 26f2f35 to 6634afa Compare October 2, 2017 17:17

deekoder requested review from krisis and krishnasrinivas October 2, 2017 20:15

harshavardhana force-pushed the backblaze-b2 branch 4 times, most recently from 54fc3aa to a2bf27b Compare October 3, 2017 01:53

harshavardhana force-pushed the backblaze-b2 branch from a2bf27b to 3b19f12 Compare October 3, 2017 02:31

harshavardhana force-pushed the backblaze-b2 branch 2 times, most recently from 482a29f to 21c1275 Compare October 3, 2017 06:34

harshavardhana force-pushed the backblaze-b2 branch 2 times, most recently from dec5bb7 to ef3ff30 Compare October 3, 2017 07:31

krishnasrinivas reviewed Oct 3, 2017

View reviewed changes

harshavardhana force-pushed the backblaze-b2 branch from ef3ff30 to 4815c9a Compare October 3, 2017 18:51

krishnasrinivas reviewed Oct 3, 2017

View reviewed changes

harshavardhana force-pushed the backblaze-b2 branch 2 times, most recently from fa10005 to 07211fb Compare October 3, 2017 23:02

krisis requested changes Oct 5, 2017

View reviewed changes

harshavardhana force-pushed the backblaze-b2 branch 3 times, most recently from 3fb3748 to 9e1aeb1 Compare October 5, 2017 01:52

harshavardhana force-pushed the backblaze-b2 branch 3 times, most recently from 51e141f to 6043296 Compare October 5, 2017 02:11

harshavardhana force-pushed the backblaze-b2 branch 2 times, most recently from ac1bf8e to 36994a0 Compare October 6, 2017 00:49

krishnasrinivas reviewed Oct 6, 2017

View reviewed changes

harshavardhana force-pushed the backblaze-b2 branch from 36994a0 to c6bff39 Compare October 6, 2017 18:24

krishnasrinivas reviewed Oct 6, 2017

View reviewed changes

harshavardhana force-pushed the backblaze-b2 branch from 6fa8891 to 7939c51 Compare October 6, 2017 22:18

krishnasrinivas approved these changes Oct 6, 2017

View reviewed changes

harshavardhana force-pushed the backblaze-b2 branch 7 times, most recently from 87524ff to 2672aea Compare October 10, 2017 16:44

krisis requested changes Oct 11, 2017

View reviewed changes

harshavardhana force-pushed the backblaze-b2 branch 4 times, most recently from f26bdf4 to 14e577c Compare October 11, 2017 19:03

krisis reviewed Oct 11, 2017

View reviewed changes

harshavardhana force-pushed the backblaze-b2 branch 2 times, most recently from f1c3ac9 to cb814ad Compare October 13, 2017 00:30

krisis approved these changes Oct 13, 2017

View reviewed changes

harshavardhana force-pushed the backblaze-b2 branch 2 times, most recently from d379d42 to de8231a Compare October 13, 2017 10:02

Implement backblaze-b2 gateway support

274ca16

Fixes minio#4072

harshavardhana force-pushed the backblaze-b2 branch from de8231a to 274ca16 Compare October 13, 2017 10:02

nitisht merged commit 0c0d1e4 into minio:master Oct 13, 2017

harshavardhana deleted the backblaze-b2 branch October 13, 2017 10:56

Implement backblaze-b2 gateway support #5002

Implement backblaze-b2 gateway support #5002

Conversation

harshavardhana commented Oct 2, 2017 • edited

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

codecov bot commented Oct 2, 2017 • edited

Codecov Report

harshavardhana commented Oct 3, 2017 • edited

harshavardhana commented Oct 3, 2017

harshavardhana commented Oct 3, 2017

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

krisis left a 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

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

harshavardhana Oct 5, 2017 • edited

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

harshavardhana commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krishnasrinivas Oct 6, 2017 • edited

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

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

harshavardhana commented Oct 2, 2017 •

edited

codecov bot commented Oct 2, 2017 •

edited

harshavardhana commented Oct 3, 2017 •

edited

harshavardhana Oct 5, 2017 •

edited

krishnasrinivas Oct 6, 2017 •

edited