【DNM】* *: support meta with s3 by DanielZhangQD · Pull Request #66 · pingcap/br

DanielZhangQD · 2019-11-25T06:00:35Z

Support storing backup meta with S3.

Here are the commands to use:

backup
- ./br backup full --pd 10.233.0.168:2379 --storage "s3://danz-backup6/backup11251525?access_key=aaa&secret_access_key=bbb&region=us-west-2&provider=aws&prefix=backup11251525"
checksum
- ./br meta checksum --storage "s3://danz-backup6/backup11251525?access_key=aaa&secret_access_key=bbb&region=us-west-2&provider=aws&prefix=backup11251525"
restore
- ./br restore full --pd 10.233.9.21:2379 --storage "s3://danz-backup6/backup11251525?access_key=aaa&secret_access_key=bbb&region=us-west-2&provider=aws&prefix=backup11251525"

codecov · 2019-11-25T06:20:49Z

Codecov Report

Merging #66 into master will decrease coverage by 3%.
The diff coverage is 4.13%.

@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
- Coverage    62.2%   59.19%   -3.01%     
==========================================
  Files          22       23       +1     
  Lines        2643     2779     +136     
==========================================
+ Hits         1644     1645       +1     
- Misses        768      901     +133     
- Partials      231      233       +2

Impacted Files	Coverage Δ
pkg/utils/remote_storage.go	`0% <0%> (ø)`
pkg/backup/client.go	`50.7% <20%> (-0.13%)`	⬇️
pkg/utils/storage.go	`65.21% <45.45%> (-6.22%)`	⬇️

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 76a40df...372503a. Read the comment docs.

pkg/utils/remote_storage.go

cmd/cmd.go

DanielZhangQD · 2019-11-26T09:50:23Z

@overvenus @tennix @onlymellb PTAL the new commit with aws-sdk-go.

go.mod

DanielZhangQD · 2019-11-27T13:42:23Z

@overvenus @kennytm @tennix @onlymellb PTAL the new commit with go cloud dev kit, thanks!

kennytm

Is it possible to add a test case? Use a S3 mock or minio or whatever...

pkg/utils/remote_storage.go

kennytm · 2019-11-27T15:09:41Z

pkg/utils/remote_storage.go

+		if aerr, ok := err.(awserr.Error); ok {
+			switch aerr.Code() {
+			case s3.ErrCodeNoSuchBucket, notFound:
+				return createS3Bucket(svc, bucket)


I'm not sure if it is desired to create a bucket by default

It's better that users create the bucket by themselves and br just uses it.

but here BR creates a bucket if it's missing

I think we can try to create the bucket if it's missing, but we'd add a document to tell users to create themselves. The minimal permission required is to read and write objects in the specified bucket.

This can avoid starting over again if users forgot to create the bucket.

Will make it clear in the document that:

Suggest users creating the bucket by themselves.

If not, BR will create it automatically with the default configuration, and users have to handle the ACLs and any other attributes they want to set by themselves.

kennytm · 2019-11-27T15:32:46Z

pkg/utils/remote_storage.go

+	prs, ok := qs[providerKey]
+	if ok && len(prs) >= 1 {
+		sqs.provider = prs[0]
+	}


Perhaps use the URL scheme oss://bucket for Aliyun OSS and s3://bucket for AWS S3 instead? Automatically set the endpoint to oss-$region.aliyuncs.com if the URL scheme is oss://.

Thanks for sharing this info, I'd like to test Aliyun and change the necessary code in another issue, currently, focus on AWS S3 and Ceph.

What I expect would be

Remove provider

One of region or endpoint must be supplied

For the internal protocol, I think using the unified URL as gocloud does should be OK. For user interface, I agree we can make it less verbose by adopting oss/s3/ceph/minio etc.

I don't see how provider can simplify config for Ceph or Minio.

I will update this if necessary when the interface between BR and TiKV is determined.

DanielZhangQD · 2019-11-28T03:26:09Z

@kennytm comments addressed, PTAL again, thanks!

kennytm · 2019-11-28T08:06:45Z

pkg/utils/remote_storage.go

+	sqs.bucket = u.Host
+
+	if u.Path != "" {
+		log.Info("path in storage arg is ignored", zap.String("path", u.Path))


Suggested change

log.Info("path in storage arg is ignored", zap.String("path", u.Path))

log.Warn("path in storage arg is ignored", zap.String("path", u.Path))

Let's make this a warning

kennytm · 2019-11-28T08:09:48Z

pkg/utils/remote_storage.go

+		if aerr, ok := err.(awserr.Error); ok {
+			switch aerr.Code() {
+			case s3.ErrCodeNoSuchBucket, notFound:
+				return createS3Bucket(svc, bucket)


but here BR creates a bucket if it's missing

kennytm · 2019-11-28T08:10:49Z

pkg/utils/remote_storage.go

+	prs, ok := qs[providerKey]
+	if ok && len(prs) >= 1 {
+		sqs.provider = prs[0]
+	}


What I expect would be

Remove provider

One of region or endpoint must be supplied

DanielZhangQD · 2019-12-09T03:44:12Z

#88 has updated the storage interface and reorganized the code structure, close this PR and will submit another one.

DanielZhangQD requested review from onlymellb, overvenus and tennix November 25, 2019 06:01

overvenus reviewed Nov 25, 2019

View reviewed changes

pkg/utils/remote_storage.go Outdated Show resolved Hide resolved

cmd/cmd.go Outdated Show resolved Hide resolved

tennix self-assigned this Nov 26, 2019

DanielZhangQD changed the title * *: support meta with s3 [WIP] * *: support meta with s3 Nov 26, 2019

DanielZhangQD changed the title ~~[WIP] * *: support meta with s3~~ * *: support meta with s3 Nov 26, 2019

kennytm reviewed Nov 26, 2019

View reviewed changes

go.mod Show resolved Hide resolved

DanielZhangQD added 2 commits November 27, 2019 17:04

* *: support storing metadata to s3

84b9618

* *: support storing metadata to s3

74ca7bf

kennytm reviewed Nov 27, 2019

View reviewed changes

address comments

372503a

kennytm reviewed Nov 29, 2019

View reviewed changes

IANTHEREAL changed the title * *: support meta with s3 【DMN】* *: support meta with s3 Dec 1, 2019

kennytm mentioned this pull request Dec 7, 2019

*: extract storage package, and update kvproto #88

Merged

tennix changed the title ~~【DMN】* *: support meta with s3~~ 【DNM】* *: support meta with s3 Dec 8, 2019

DanielZhangQD closed this Dec 9, 2019

	log.Info("path in storage arg is ignored", zap.String("path", u.Path))
	log.Warn("path in storage arg is ignored", zap.String("path", u.Path))

Conversation

DanielZhangQD commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

DanielZhangQD commented Nov 26, 2019

Uh oh!

Uh oh!

DanielZhangQD commented Nov 27, 2019

Uh oh!

kennytm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielZhangQD commented Nov 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielZhangQD commented Dec 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DanielZhangQD commented Nov 25, 2019 •

edited

Loading

codecov bot commented Nov 25, 2019 •

edited

Loading