curvefs/Multiple s3 support #1132

Cyber-SiKu · 2022-03-03T12:15:43Z

What problem does this PR solve?

Issue Number: close #1119 #1120 #1037 #1038

Problem Summary:

What is changed and how it works?

What's Changed:

client get s3info from mds
metaserver get s3info from mds
mds check s3info before create fs

How it Works:

Side effects(Breaking backward compatibility? Performance regression?):

Check List

Relevant documentation/comments is changed or added
I acknowledge that all my contributions will be made under the project's license

wu-hanqing · 2022-03-04T06:00:59Z

curvefs/src/mds/fs_manager.cpp

@@ -182,6 +183,18 @@ FSStatusCode FsManager::CreateFs(const std::string& fsName, FSType fsType,
        }
    }

+    // check s3info
+    if (detail.has_s3info()) {
+        auto s3Info = detail.s3info();


const auto& s3Info = ...

const auto& s3Info = ...

fix

wu-hanqing · 2022-03-04T06:02:12Z

curvefs/src/mds/s3/mds_s3.h

+    S3ClientImpl() : S3Client() {}
+    virtual ~S3ClientImpl() {}
+
+    void SetAdaptor(std::shared_ptr<curve::common::S3Adapter> s3Adapter);


why not pass it in constructor ?

why not pass it in constructor ?

just do it like curvefs/src/client/s3/client_s3.h.
But I think it should be mocked for unit testing

wu-hanqing · 2022-03-04T06:07:22Z

curvefs/src/client/curve_fuse_op.cpp

 }

 void FuseOpInit(void *userdata, struct fuse_conn_info *conn) {
    CURVEFS_ERROR ret = g_ClientInstance->FuseOpInit(userdata, conn);
    if (ret != CURVEFS_ERROR::OK) {
-        LOG(FATAL) << "FuseOpInit failed, ret = " << ret;
+        LOG(ERROR) << "FuseOpInit failed, ret = " << ret;


why change log level to ERROR?

why change log level to ERROR?

fix

wu-hanqing · 2022-03-04T06:09:05Z

curvefs/src/client/curve_fuse_op.cpp

@@ -90,7 +94,30 @@ int InitGlog(const char *confPath, const char *argv0) {
    return 0;
 }

-int InitFuseClient(const char *confPath, const char *fsType) {
+int GetFsInfo(const char *fsName, std::shared_ptr<FsInfo> fsInfo) {


static or put this function into anonymous namespace

put this function into anonymous namespace

fix

wu-hanqing · 2022-03-04T07:01:37Z

curvefs/src/client/curve_fuse_op.cpp

+    } else if (fsTypeStr == "s3") {
        g_ClientInstance = new FuseS3Client();
+        // set fsS3Option
+        const auto& s3Info = fsInfo->detail().s3info();
+        ::curve::common::FsS3Option fsS3Option;
+        ::curvefs::client::common::S3Info2FsS3Option(s3Info, &fsS3Option);
+        SetFuseClientS3Option(g_fuseClientOption, fsS3Option);


can you just put these in fuse_s3_client ?

can you just put these in fuse_s3_client ?

good,idea! fix

wu-hanqing · 2022-03-04T07:07:13Z

curvefs/conf/mds.conf

+#### s3
+# the max size that fuse send
+s3.fuseMaxSize=131072
+s3.pagesize=65536
+# prefetch blocks that disk cache use
+s3.prefetchBlocks=1
+# prefetch threads
+s3.prefetchExecQueueNum=1
+# start sleep when mem cache use ratio is greater than nearfullRatio,
+# sleep time increase follow with mem cache use raito, baseSleepUs is baseline.
+s3.nearfullRatio=70
+s3.baseSleepUs=500


these fields are useless.

these fields are useless.

this for init but i will try for delete this

wu-hanqing · 2022-03-04T07:30:35Z

curvefs/src/metaserver/mdsclient/mds_client.h

+    ::curve::client::RPCExcutorRetryPolicy rpcexcutor_;
+    ::curve::client::MetaServerOption mdsOpt_;
+
+    MDSClientMetric mdsClientMetric_;


metric is also unnecessary

metric is also unnecessary

fix

wu-hanqing · 2022-03-04T07:34:45Z

curvefs/src/metaserver/s3/metaserver_s3.h

@@ -36,6 +36,9 @@ class S3Client {
    virtual void Init(const curve::common::S3AdapterOption& option) = 0;
    virtual int Delete(const std::string& name) = 0;
    virtual int DeleteBatch(const std::list<std::string>& nameList) = 0;
+    virtual void Reinit(const std::string& ak, const std::string& sk,


it's a superset of s3client in mds_s3.h, you should reduce duplication.

it's a superset of s3client in mds_s3.h, you should reduce duplication.

this is different from mds_s3, mds_s3 work for check s3 info available, this work for delete s3 object.
There is no inheritance relationship between these two s3.

wu-hanqing · 2022-03-04T07:38:58Z

src/common/s3_adapter.h

+struct FsS3Option {
+    // should get from mds
+    std::string ak;
+    std::string sk;
+    std::string s3Address;
+    std::string bucketName;
+    uint64_t blockSize;
+    uint64_t chunkSize;
+};


you shouldn't put this option in here, I think s3 adapter is just put object to s3 or get object from s3, it shouldn't care about what is fs option.

you shouldn't put this option in here, I think s3 adapter is just put object to s3 or get object from s3, it shouldn't care about what is fs option.

this just for init, maybe the name is not well ,it should be like S3InfoOption?

wu-hanqing · 2022-03-04T07:39:04Z

src/common/s3_adapter.h

+    uint64_t chunkSize;
+};
+
+void InitS3AdaptorOptionExceptFsS3Option(Configuration* conf,


SeanHai · 2022-03-04T07:25:43Z

curvefs/src/mds/fs_manager.cpp

+        s3Client_->Reinit(s3Info.ak(), s3Info.sk(), s3Info.endpoint(),
+            s3Info.bucketname());
+        if (!s3Client_->BucketExist()) {
+            LOG(ERROR) << "s3info for fs is not available, s3info is "


Output should be aligned and add fsName.

Output should be aligned and add fsName.

fix

SeanHai · 2022-03-04T07:29:13Z

curvefs/src/mds/mds.cpp

@@ -150,9 +151,17 @@ void MDS::Init() {
    FsManagerOption fsManagerOption;
    InitFsManagerOptions(&fsManagerOption);

+    auto s3Client = std::make_shared<S3ClientImpl>();
+    s3Client->SetAdaptor(std::make_shared<curve::common::S3Adapter>());
+    s3Client_ = s3Client;


auto s3Client = std::make_shared();
s3Client->SetAdaptor(std::make_sharedcurve::common::S3Adapter());
s3Client_ = s3Client;

s3Client_ = std::make_shared();
s3Client_->SetAdaptor(std::make_sharedcurve::common::S3Adapter());

auto s3Client = std::make_shared(); s3Client->SetAdaptor(std::make_sharedcurve::common::S3Adapter()); s3Client_ = s3Client;

s3Client_ = std::make_shared(); s3Client_->SetAdaptor(std::make_sharedcurve::common::S3Adapter());

fix

SeanHai · 2022-03-04T07:35:19Z

curvefs/src/mds/s3/mds_s3.h

+
+namespace curvefs {
+namespace mds {
+class S3Client {


The class S3Client in mds can use S3Adapter replace directly.

The class S3Client in mds can use S3Adapter replace directly.

This s3client doesn't need so many functions, just for check s3info available.

The class S3Client in mds can use S3Adapter replace directly.

This s3client doesn't need so many functions, just for check s3info available.

I means the S3Client doesn't need, it just a simple wrapper of S3Adaper.

The class S3Client in mds can use S3Adapter replace directly.

This s3client doesn't need so many functions, just for check s3info available.

I means the S3Client doesn't need, it just a simple wrapper of S3Adaper.

Compared with using the S3 Adaptor directly, the client seems to be able to better indicate the s3 functions used by mds, or the role of s3 in mds.

The class S3Client in mds can use S3Adapter replace directly.

This s3client doesn't need so many functions, just for check s3info available.

I means the S3Client doesn't need, it just a simple wrapper of S3Adaper.

fix

SeanHai · 2022-03-04T07:42:05Z

curvefs/src/metaserver/mdsclient/mds_client.h

+using ::curvefs::mds::GetFsInfoResponse;
+using ::curvefs::client::rpcclient::MDSBaseClient;
+
+class MdsClient {


Can reuse MdsClient in client, put the MdsClient to common or some other ways?

Can reuse MdsClient in client, put the MdsClient to common or some other ways?

This mdsclient doesn't need so many functions

Can reuse MdsClient in client, put the MdsClient to common or some other ways?

This mdsclient doesn't need so many functions

Yes, but mdsclient is everywhere in client, mds, metaserver. That is similar to S3 info is everywhere.

Can reuse MdsClient in client, put the MdsClient to common or some other ways?

This mdsclient doesn't need so many functions

Yes, but mdsclient is everywhere in client, mds, metaserver. That is similar to S3 info is everywhere.

ok, fix

wu-hanqing · 2022-03-05T03:32:47Z

curvefs/src/metaserver/s3/metaserver_s3.h

    virtual void Init(const curve::common::S3AdapterOption& option) = 0;
    virtual int Delete(const std::string& name) = 0;
    virtual int DeleteBatch(const std::list<std::string>& nameList) = 0;


I'm wondering why we have this S3Client because it's just a wrapper of S3Adapter, and what it does is forward the function call to S3Adapter.

I'm wondering why we have this S3Client because it's just a wrapper of S3Adapter, and what it does is forward the function call to S3Adapter.

yep

1. client get s3Info from mds by fsId, and use s3Info to set s3Adapter; 2. metaserver get s3info from mds by fsId, and use s3Info to set s3Adaptor; 3. mds check s3info available by check bucketName before create fs.

Cyber-SiKu force-pushed the curvefs/S3 branch 2 times, most recently from 67f336b to 87fe451 Compare March 3, 2022 14:43

Cyber-SiKu changed the title ~~curvefs/S3~~ curvefs/Multiple s3 support Mar 4, 2022

wu-hanqing reviewed Mar 4, 2022

View reviewed changes

Cyber-SiKu force-pushed the curvefs/S3 branch from 87fe451 to d573a0e Compare March 4, 2022 08:29

SeanHai reviewed Mar 4, 2022

View reviewed changes

Cyber-SiKu force-pushed the curvefs/S3 branch 3 times, most recently from bb1691e to 0856091 Compare March 4, 2022 15:17

wu-hanqing reviewed Mar 5, 2022

View reviewed changes

Cyber-SiKu force-pushed the curvefs/S3 branch from 0856091 to ac0042b Compare March 7, 2022 11:17

wu-hanqing approved these changes Mar 7, 2022

View reviewed changes

SeanHai approved these changes Mar 7, 2022

View reviewed changes

Cyber-SiKu force-pushed the curvefs/S3 branch 4 times, most recently from dd8dd50 to 377a147 Compare March 8, 2022 06:02

curvefs/S3

82da353

1. client get s3Info from mds by fsId, and use s3Info to set s3Adapter; 2. metaserver get s3info from mds by fsId, and use s3Info to set s3Adaptor; 3. mds check s3info available by check bucketName before create fs.

Cyber-SiKu force-pushed the curvefs/S3 branch from 377a147 to 82da353 Compare March 8, 2022 06:28

Cyber-SiKu merged commit 6a8b561 into opencurve:master Mar 9, 2022

Cyber-SiKu mentioned this pull request Apr 18, 2022

Configuration queries for client profiles and tools #988

Closed

Cyber-SiKu deleted the curvefs/S3 branch April 19, 2022 05:55

Cyber-SiKu mentioned this pull request Aug 2, 2023

[curvefs]: change s3info #2517

Open

curvefs/Multiple s3 support #1132

curvefs/Multiple s3 support #1132

Conversation

Cyber-SiKu commented Mar 3, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

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

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

Cyber-SiKu commented Mar 3, 2022 •

edited

Loading