From 998f38e7a3d5c4ea208f2c22037fb2236c54d9c7 Mon Sep 17 00:00:00 2001 From: ti-srebot <66930949+ti-srebot@users.noreply.github.com> Date: Wed, 13 Apr 2022 18:36:36 +0800 Subject: [PATCH] lightning: Add source dir existence check for s3 (#30674) (#30713) close pingcap/tidb#28031, close pingcap/tidb#30709 --- br/pkg/backup/client_test.go | 2 -- br/pkg/lightning/lightning.go | 13 +++++++++++++ br/pkg/storage/gcs.go | 8 -------- br/pkg/storage/s3.go | 8 -------- br/pkg/storage/s3_test.go | 3 +-- br/pkg/storage/storage.go | 13 ------------- br/pkg/task/backup.go | 1 - br/pkg/task/backup_raw.go | 1 - br/pkg/task/common.go | 5 ++++- br/pkg/task/restore.go | 1 - br/pkg/task/restore_log.go | 1 - br/tests/lightning_s3/run.sh | 14 ++++++++++++++ 12 files changed, 32 insertions(+), 38 deletions(-) diff --git a/br/pkg/backup/client_test.go b/br/pkg/backup/client_test.go index 6a76baa7cd34f..1c193f8c5cc65 100644 --- a/br/pkg/backup/client_test.go +++ b/br/pkg/backup/client_test.go @@ -241,7 +241,6 @@ func (r *testBackup) TestSendCreds(c *C) { c.Assert(err, IsNil) opts := &storage.ExternalStorageOptions{ SendCredentials: true, - SkipCheckPath: true, } _, err = storage.New(r.ctx, backend, opts) c.Assert(err, IsNil) @@ -260,7 +259,6 @@ func (r *testBackup) TestSendCreds(c *C) { c.Assert(err, IsNil) opts = &storage.ExternalStorageOptions{ SendCredentials: false, - SkipCheckPath: true, } _, err = storage.New(r.ctx, backend, opts) c.Assert(err, IsNil) diff --git a/br/pkg/lightning/lightning.go b/br/pkg/lightning/lightning.go index 8031627fe7021..93804359e3b1f 100644 --- a/br/pkg/lightning/lightning.go +++ b/br/pkg/lightning/lightning.go @@ -277,6 +277,19 @@ func (l *Lightning) run(taskCtx context.Context, taskCfg *config.Config, g glue. return errors.Annotate(err, "create storage failed") } + // return expectedErr means at least meet one file + expectedErr := errors.New("Stop Iter") + walkErr := s.WalkDir(ctx, &storage.WalkOption{ListCount: 1}, func(string, int64) error { + // return an error when meet the first regular file to break the walk loop + return expectedErr + }) + if !errors.ErrorEqual(walkErr, expectedErr) { + if walkErr == nil { + return errors.Errorf("data-source-dir '%s' doesn't exist or contains no files", taskCfg.Mydumper.SourceDir) + } + return errors.Annotatef(walkErr, "visit data-source-dir '%s' failed", taskCfg.Mydumper.SourceDir) + } + loadTask := log.L().Begin(zap.InfoLevel, "load data source") var mdl *mydump.MDLoader mdl, err = mydump.NewMyDumpLoaderWithStore(ctx, taskCfg, s) diff --git a/br/pkg/storage/gcs.go b/br/pkg/storage/gcs.go index e4835e0eb6111..c54141b8ee560 100644 --- a/br/pkg/storage/gcs.go +++ b/br/pkg/storage/gcs.go @@ -276,14 +276,6 @@ func newGCSStorage(ctx context.Context, gcs *backuppb.GCS, opts *ExternalStorage // so we need find sst in slash directory gcs.Prefix += "//" } - // TODO remove it after BR remove cfg skip-check-path - if !opts.SkipCheckPath { - // check bucket exists - _, err = bucket.Attrs(ctx) - if err != nil { - return nil, errors.Annotatef(err, "gcs://%s/%s", gcs.Bucket, gcs.Prefix) - } - } return &gcsStorage{gcs: gcs, bucket: bucket}, nil } diff --git a/br/pkg/storage/s3.go b/br/pkg/storage/s3.go index 8fe1b6b3ae24c..27d25b69aa7dc 100644 --- a/br/pkg/storage/s3.go +++ b/br/pkg/storage/s3.go @@ -283,14 +283,6 @@ func newS3Storage(backend *backuppb.S3, opts *ExternalStorageOptions) (*S3Storag } c := s3.New(ses) - // TODO remove it after BR remove cfg skip-check-path - if !opts.SkipCheckPath { - err = checkS3Bucket(c, &qs) - if err != nil { - return nil, errors.Annotatef(berrors.ErrStorageInvalidConfig, "Bucket %s is not accessible: %v", qs.Bucket, err) - } - } - if len(qs.Prefix) > 0 && !strings.HasSuffix(qs.Prefix, "/") { qs.Prefix += "/" } diff --git a/br/pkg/storage/s3_test.go b/br/pkg/storage/s3_test.go index f1a4e42221afa..97ee9b58f54be 100644 --- a/br/pkg/storage/s3_test.go +++ b/br/pkg/storage/s3_test.go @@ -288,7 +288,6 @@ func (s *s3Suite) TestS3Storage(c *C) { _, err := New(ctx, s3, &ExternalStorageOptions{ SendCredentials: test.sendCredential, CheckPermissions: test.hackPermission, - SkipCheckPath: true, }) if test.errReturn { c.Assert(err, NotNil) @@ -414,7 +413,7 @@ func (s *s3Suite) TestS3Storage(c *C) { func (s *s3Suite) TestS3URI(c *C) { backend, err := ParseBackend("s3://bucket/prefix/", nil) c.Assert(err, IsNil) - storage, err := New(context.Background(), backend, &ExternalStorageOptions{SkipCheckPath: true}) + storage, err := New(context.Background(), backend, &ExternalStorageOptions{}) c.Assert(err, IsNil) c.Assert(storage.URI(), Equals, "s3://bucket/prefix/") } diff --git a/br/pkg/storage/storage.go b/br/pkg/storage/storage.go index 427e625795533..0743fb2cd4a7d 100644 --- a/br/pkg/storage/storage.go +++ b/br/pkg/storage/storage.go @@ -121,18 +121,6 @@ type ExternalStorageOptions struct { // NoCredentials means that no cloud credentials are supplied to BR NoCredentials bool - // SkipCheckPath marks whether to skip checking path's existence. - // - // This should only be set to true in testing, to avoid interacting with the - // real world. - // When this field is false (i.e. path checking is enabled), the New() - // function will ensure the path referred by the backend exists by - // recursively creating the folders. This will also throw an error if such - // operation is impossible (e.g. when the bucket storing the path is missing). - - // deprecated: use checkPermissions and specify the checkPermission instead. - SkipCheckPath bool - // HTTPClient to use. The created storage may ignore this field if it is not // directly using HTTP (e.g. the local storage). HTTPClient *http.Client @@ -148,7 +136,6 @@ type ExternalStorageOptions struct { func Create(ctx context.Context, backend *backuppb.StorageBackend, sendCreds bool) (ExternalStorage, error) { return New(ctx, backend, &ExternalStorageOptions{ SendCredentials: sendCreds, - SkipCheckPath: false, HTTPClient: nil, }) } diff --git a/br/pkg/task/backup.go b/br/pkg/task/backup.go index 08eef6eeb8515..9ccf162a09164 100644 --- a/br/pkg/task/backup.go +++ b/br/pkg/task/backup.go @@ -257,7 +257,6 @@ func RunBackup(c context.Context, g glue.Glue, cmdName string, cfg *BackupConfig opts := storage.ExternalStorageOptions{ NoCredentials: cfg.NoCreds, SendCredentials: cfg.SendCreds, - SkipCheckPath: cfg.SkipCheckPath, } if err = client.SetStorage(ctx, u, &opts); err != nil { return errors.Trace(err) diff --git a/br/pkg/task/backup_raw.go b/br/pkg/task/backup_raw.go index 6810a40308dc8..9657b9ff915b5 100644 --- a/br/pkg/task/backup_raw.go +++ b/br/pkg/task/backup_raw.go @@ -150,7 +150,6 @@ func RunBackupRaw(c context.Context, g glue.Glue, cmdName string, cfg *RawKvConf opts := storage.ExternalStorageOptions{ NoCredentials: cfg.NoCreds, SendCredentials: cfg.SendCreds, - SkipCheckPath: cfg.SkipCheckPath, } if err = client.SetStorage(ctx, u, &opts); err != nil { return errors.Trace(err) diff --git a/br/pkg/task/common.go b/br/pkg/task/common.go index 1a2d0b8fa4f85..0ffc1a5368db9 100644 --- a/br/pkg/task/common.go +++ b/br/pkg/task/common.go @@ -367,6 +367,10 @@ func (cfg *Config) ParseFromFlags(flags *pflag.FlagSet) error { if cfg.SkipCheckPath, err = flags.GetBool(flagSkipCheckPath); err != nil { return errors.Trace(err) } + if cfg.SkipCheckPath { + log.L().Info("--skip-check-path is deprecated, need explicitly set it anymore") + } + return cfg.normalizePDURLs() } @@ -431,7 +435,6 @@ func storageOpts(cfg *Config) *storage.ExternalStorageOptions { return &storage.ExternalStorageOptions{ NoCredentials: cfg.NoCreds, SendCredentials: cfg.SendCreds, - SkipCheckPath: cfg.SkipCheckPath, } } diff --git a/br/pkg/task/restore.go b/br/pkg/task/restore.go index e8b82614063a9..feb416caecf1f 100644 --- a/br/pkg/task/restore.go +++ b/br/pkg/task/restore.go @@ -221,7 +221,6 @@ func RunRestore(c context.Context, g glue.Glue, cmdName string, cfg *RestoreConf opts := storage.ExternalStorageOptions{ NoCredentials: cfg.NoCreds, SendCredentials: cfg.SendCreds, - SkipCheckPath: cfg.SkipCheckPath, } if err = client.SetStorage(ctx, u, &opts); err != nil { return errors.Trace(err) diff --git a/br/pkg/task/restore_log.go b/br/pkg/task/restore_log.go index 26a8bdae0add0..45486c4f8e577 100644 --- a/br/pkg/task/restore_log.go +++ b/br/pkg/task/restore_log.go @@ -121,7 +121,6 @@ func RunLogRestore(c context.Context, g glue.Glue, cfg *LogRestoreConfig) error opts := storage.ExternalStorageOptions{ NoCredentials: cfg.NoCreds, SendCredentials: cfg.SendCreds, - SkipCheckPath: cfg.SkipCheckPath, } if err = client.SetStorage(ctx, u, &opts); err != nil { return errors.Trace(err) diff --git a/br/tests/lightning_s3/run.sh b/br/tests/lightning_s3/run.sh index edec1a5690218..2fac39a555205 100755 --- a/br/tests/lightning_s3/run.sh +++ b/br/tests/lightning_s3/run.sh @@ -61,6 +61,20 @@ _EOF_ run_sql "DROP DATABASE IF EXISTS $DB;" run_sql "DROP TABLE IF EXISTS $DB.$TABLE;" +# test not exist path +rm -f $TEST_DIR/lightning.log +SOURCE_DIR="s3://$BUCKET/not-exist-path?endpoint=http%3A//127.0.0.1%3A9900&access_key=$MINIO_ACCESS_KEY&secret_access_key=$MINIO_SECRET_KEY&force_path_style=true" +! run_lightning -d $SOURCE_DIR --backend local 2> /dev/null +grep -Eq "data-source-dir .* doesn't exist or contains no files" $TEST_DIR/lightning.log + +# test empty dir +rm -f $TEST_DIR/lightning.log +emptyPath=empty-bucket/empty-path +mkdir -p $DBPATH/$emptyPath +SOURCE_DIR="s3://$emptyPath/not-exist-path?endpoint=http%3A//127.0.0.1%3A9900&access_key=$MINIO_ACCESS_KEY&secret_access_key=$MINIO_SECRET_KEY&force_path_style=true" +! run_lightning -d $SOURCE_DIR --backend local 2> /dev/null +grep -Eq "data-source-dir .* doesn't exist or contains no files" $TEST_DIR/lightning.log + SOURCE_DIR="s3://$BUCKET/?endpoint=http%3A//127.0.0.1%3A9900&access_key=$MINIO_ACCESS_KEY&secret_access_key=$MINIO_SECRET_KEY&force_path_style=true" run_lightning -d $SOURCE_DIR --backend local 2> /dev/null run_sql "SELECT count(*), sum(i) FROM \`$DB\`.$TABLE"