storage: fix s3 walk directory #750

glorv · 2021-02-24T03:39:48Z

What problem does this PR solve?

close #749

What is changed and how it works?

#713 tried to fix the s3 WalkDir prefix issue in the wrong way and introduced another bug.

The original issue is that if prefix and subDir are both empty, the prefix parameter in the ListObject request then is "/", which causes s3 to return empty results.

The PR avoids adding the "/" postfix for the s3Storage object if the original prefix is an empty string.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Code changes

Side effects

Related changes

Need to cherry-pick to the release branch
Need to update the documentation

Release Note

overvenus · 2021-02-24T04:40:01Z

pkg/storage/s3.go

@@ -279,8 +279,10 @@ func newS3Storage(backend *backup.S3, opts *ExternalStorageOptions) (*S3Storage,
 			return nil, errors.Annotatef(berrors.ErrStorageInvalidConfig, "Bucket %s is not accessible: %v", qs.Bucket, err)
 		}
 	}
+	if len(qs.Prefix) > 0 {
+		qs.Prefix += "/"


What if there is a '/' already?

what if we just path.Clean it 🤷

The Prefix is parsed from url it can't contains the "/" suffix. See:

br/pkg/storage/parse.go

Lines 69 to 70 in 578be7f

prefix := strings.Trim(u.Path, "/")

s3 := &backup.S3{Bucket: u.Host, Prefix: prefix}

We just not want the bare "/" prefix. for other non-empty prefixes, the suffix "/" is needed.
BTW, maybe we shouldn't depend on the Prefix filed to contain the "/" suffix, but in this way, we need to change more code.

What if the user wants to load all data from one bucket?

The original pr is try to resolve this issue then. If the input path is the bucket name, we should leave the prefix parameter empty in the ListObject request, otherwise, s3 will return empty result.

if len(prefix) > 0 && !strings.HasSuffix(prefix, "/") {

What about checking suffix too? In case the API is called from other projects.

overvenus · 2021-02-24T04:41:00Z

pkg/storage/s3.go

-		if len(prefix) > 0 && !strings.HasSuffix(prefix, "/") {
-			prefix += "/"
-		}
+	prefix := rs.options.Prefix + opt.SubDir


What about path.Join?

path.Join cannot work on windows since it separator is not "/". And options.Prefix already contains the suffix "/"

@glorv path.Join always use /. you're thinking of filepath.Join 😛

kennytm · 2021-02-24T11:15:28Z

https://internal.pingcap.net/idc-jenkins/blue/organizations/jenkins/br_ghpr_unit_and_integration_test/detail/br_ghpr_unit_and_integration_test/4889/pipeline

[2021-02-24T03:59:46.545Z] TEST: [br_full_ddl] fail due to stats are not equal

😥

kennytm · 2021-02-24T11:15:50Z

/run-integration-test

3pointer · 2021-02-24T11:25:53Z

https://internal.pingcap.net/idc-jenkins/blue/organizations/jenkins/br_ghpr_unit_and_integration_test/detail/br_ghpr_unit_and_integration_test/4889/pipeline
[2021-02-24T03:59:46.545Z] TEST: [br_full_ddl] fail due to stats are not equal
😥

#690 fix this test, we can merge it first

lichunzhu · 2021-02-24T11:23:22Z

pkg/storage/s3_test.go

@@ -881,6 +881,31 @@ func (s *s3Suite) TestWalkDir(c *C) {
 			}, nil
 		}).
 		After(secondCall)
+	forthCall := s.s3.EXPECT().


typo?

Suggested change

forthCall := s.s3.EXPECT().

fourthCall := s.s3.EXPECT().

lichunzhu · 2021-02-24T11:23:57Z

pkg/storage/s3_test.go

+				Contents:    contents[4:],
+			}, nil
+		}).
+		After(forthCall)


Suggested change

After(forthCall)

After(fourthCall)

lichunzhu · 2021-02-24T11:52:15Z

pkg/storage/s3.go

@@ -279,8 +279,10 @@ func newS3Storage(backend *backup.S3, opts *ExternalStorageOptions) (*S3Storage,
 			return nil, errors.Annotatef(berrors.ErrStorageInvalidConfig, "Bucket %s is not accessible: %v", qs.Bucket, err)
 		}
 	}
+	if len(qs.Prefix) > 0 {
+		qs.Prefix += "/"


What if the user wants to load all data from one bucket?

lichunzhu

/lgtm

overvenus

Rest LGTM

overvenus · 2021-02-25T06:41:20Z

pkg/storage/s3.go

@@ -279,8 +279,10 @@ func newS3Storage(backend *backup.S3, opts *ExternalStorageOptions) (*S3Storage,
 			return nil, errors.Annotatef(berrors.ErrStorageInvalidConfig, "Bucket %s is not accessible: %v", qs.Bucket, err)
 		}
 	}
+	if len(qs.Prefix) > 0 {
+		qs.Prefix += "/"


if len(prefix) > 0 && !strings.HasSuffix(prefix, "/") {

What about checking suffix too? In case the API is called from other projects.

overvenus · 2021-02-25T07:51:49Z

LGTM

glorv · 2021-02-25T08:48:31Z

/merge

ti-srebot · 2021-02-25T08:50:41Z

/run-all-tests

Signed-off-by: ti-srebot <[email protected]>

ti-srebot · 2021-02-25T09:06:32Z

cherry pick to release-4.0 in PR #756

Signed-off-by: ti-srebot <[email protected]>

glorv added the type/bug-fix label Feb 24, 2021

glorv force-pushed the fix-walk branch from e069dd5 to 8ffee17 Compare February 24, 2021 03:51

glorv requested review from kennytm, 3pointer and lichunzhu February 24, 2021 04:01

overvenus reviewed Feb 24, 2021

View reviewed changes

glorv added the component/external-storage label Feb 24, 2021

overvenus added this to the v4.0.12 milestone Feb 24, 2021

overvenus added the needs-cherry-pick-release-4.0 label Feb 24, 2021

glorv changed the title ~~fix s3 walk directory~~ storage: fix s3 walk directory Feb 24, 2021

fix s3 walk directory

f3f47a7

glorv force-pushed the fix-walk branch from 8ffee17 to f3f47a7 Compare February 24, 2021 11:15

lichunzhu reviewed Feb 24, 2021

View reviewed changes

overvenus and others added 3 commits February 24, 2021 20:08

Merge branch 'master' into fix-walk

ee60a3f

fix typo

aae24bd

Merge branch 'fix-walk' of https://github.com/glorv/br into fix-walk

484346f

lichunzhu reviewed Feb 24, 2021

View reviewed changes

ti-srebot added the status/LGT1 LGTM1 label Feb 24, 2021

overvenus reviewed Feb 25, 2021

View reviewed changes

also check suffix when append '/' to s3 prefix

c9d5a7c

ti-srebot removed the status/LGT1 LGTM1 label Feb 25, 2021

ti-srebot approved these changes Feb 25, 2021

View reviewed changes

ti-srebot added the status/LGT2 LGTM2 label Feb 25, 2021

ti-srebot added the status/can-merge label Feb 25, 2021

Merge branch 'master' into fix-walk

b5d761b

glorv added needs-cherry-pick-5.0-rc and removed needs-cherry-pick-5.0-rc labels Feb 25, 2021

ti-srebot merged commit 577dc24 into pingcap:master Feb 25, 2021

ti-srebot pushed a commit to ti-srebot/br that referenced this pull request Feb 25, 2021

cherry pick pingcap#750 to release-4.0

be6d631

Signed-off-by: ti-srebot <[email protected]>

ti-srebot mentioned this pull request Feb 25, 2021

storage: fix s3 walk directory (#750) #756

Merged

glorv deleted the fix-walk branch February 25, 2021 10:43

ti-srebot added a commit that referenced this pull request Mar 3, 2021

storage: fix s3 walk directory (#750) (#756)

3c4b0ab

Signed-off-by: ti-srebot <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: fix s3 walk directory #750

storage: fix s3 walk directory #750

glorv commented Feb 24, 2021

overvenus Feb 24, 2021

kennytm Feb 24, 2021

glorv Feb 24, 2021

glorv Feb 24, 2021

lichunzhu Feb 24, 2021

glorv Feb 24, 2021

overvenus Feb 25, 2021

glorv Feb 25, 2021

overvenus Feb 24, 2021

glorv Feb 24, 2021

kennytm Feb 24, 2021

kennytm commented Feb 24, 2021 •

edited

Loading

kennytm commented Feb 24, 2021

3pointer commented Feb 24, 2021

lichunzhu Feb 24, 2021

lichunzhu Feb 24, 2021

lichunzhu Feb 24, 2021

lichunzhu left a comment

overvenus left a comment

overvenus Feb 25, 2021

overvenus commented Feb 25, 2021

glorv commented Feb 25, 2021

ti-srebot commented Feb 25, 2021

ti-srebot commented Feb 25, 2021

	prefix := strings.Trim(u.Path, "/")
	s3 := &backup.S3{Bucket: u.Host, Prefix: prefix}

storage: fix s3 walk directory #750

storage: fix s3 walk directory #750

Conversation

glorv commented Feb 24, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

Release Note

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

kennytm commented Feb 24, 2021 • edited Loading

kennytm commented Feb 24, 2021

3pointer commented Feb 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lichunzhu left a comment

Choose a reason for hiding this comment

overvenus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overvenus commented Feb 25, 2021

glorv commented Feb 25, 2021

ti-srebot commented Feb 25, 2021

ti-srebot commented Feb 25, 2021

kennytm commented Feb 24, 2021 •

edited

Loading