curvefs metaserver supports external services #974

SeanHai · 2022-01-11T11:21:32Z

What problem does this PR solve?

Issue Number: close #xxx

#973

Problem Summary:

What is changed and how it works?

What's Changed:

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

YunhuiChen · 2022-01-12T10:57:11Z

recheck

SeanHai · 2022-01-13T01:47:23Z

recheck

cw123 · 2022-01-13T02:08:03Z

curvefs/src/metaserver/register.h

@@ -36,7 +36,8 @@ struct RegisterOptions {
    std::string mdsListenAddr;
    std::string metaserverInternalIp;
    std::string metaserverExternalIp;
-    int metaserverPort;
+    uint32_t metaserverPort;


metaserverInternalPort?

cw123 · 2022-01-13T02:09:43Z

curvefs/src/metaserver/metaserver.cpp

@@ -87,6 +89,8 @@ void Metaserver::InitRegisterOptions() {
    conf_->GetValueFatalIfFail("global.external_ip",
                               &registerOptions_.metaserverExternalIp);
    conf_->GetValueFatalIfFail("global.port", &registerOptions_.metaserverPort);
+    conf_->GetValueFatalIfFail("global.external_port",


conf->GetBoolValue("global.enable_external_server", &options_.enableExternalServer);

if enable is false, can you still get external port?

conf->GetBoolValue("global.enable_external_server", &options_.enableExternalServer);

if enable is false, can you still get external port?

Default the switch is off and external ip and port is same as internal ip and port. The metaserve.conf and topology.json created by curveamd will support and I have reached a consensus with @Wine93.
If you want to turn on this switch, you need set enable to true and available external ip and port in topology.yaml(curveamd).

cw123 · 2022-01-13T02:10:39Z

curvefs/src/metaserver/metaserver.cpp

+        << "start internal brpc server error";
+
+    // add external server
+    externalServer_ = absl::make_unique<brpc::Server>();


move this in if ?

move this in if ?

fixed.

wu-hanqing · 2022-01-13T03:01:56Z

curvefs/src/client/rpcclient/mds_client.cpp

+                uint16_t internalPort = csl.port();
                std::string internalIp = csl.hostip();
                csinfo.peerID = csl.metaserverid();
+                uint16_t externalPort = internalPort;
                std::string externalIp = internalIp;
+                if (csl.has_externalport()) {
+                    externalPort = csl.externalport();
+                }
                if (csl.has_externalip()) {
                    externalIp = csl.externalip();
                }

                butil::EndPoint internal;
-                butil::str2endpoint(internalIp.c_str(), port, &internal);
+                butil::str2endpoint(internalIp.c_str(), internalPort,
+                    &internal);
                butil::EndPoint external;
-                butil::str2endpoint(externalIp.c_str(), port, &external);
+                butil::str2endpoint(externalIp.c_str(), externalPort,
+                    &external);


the logic is almost the same as above, try to extract this into a single function.

the logic is almost the same as above, try to extract this into a single function.

fixed

wu-hanqing · 2022-01-13T03:04:52Z

curvefs/src/metaserver/metaserver.cpp

@@ -64,6 +64,8 @@ void Metaserver::InitOptions(std::shared_ptr<Configuration> conf) {
    conf_ = conf;
    conf_->GetValueFatalIfFail("global.ip", &options_.ip);
    conf_->GetValueFatalIfFail("global.port", &options_.port);
+    conf->GetBoolValue("global.enable_external_server",


use conf_

use conf_

fixed

wu-hanqing · 2022-01-13T03:34:29Z

curvefs/src/metaserver/metaserver.cpp

+        butil::EndPoint listenAddr(ip, registerOptions_.metaserverExternalPort);
+
+        // add raft-related service
+        copysetNodeManager_->AddService(externalServer_.get(), listenAddr);


raft-related services should only be added to the internal server.

previous ChunkServer implementation adds raft-related service because it has compatible problem, there're lots of old clients that can upgrade.

and in CopysetNodeManager::AddService,

curve/curvefs/src/metaserver/copyset/copyset_node_manager.cpp

Line 221 in 895fa84

void CopysetNodeManager::AddService(brpc::Server* server,

you should add RaftCliService2 into external server.

raft-related services should only be added to the internal server.

previous ChunkServer implementation adds raft-related service because it has compatible problem, there're lots of old clients that can upgrade.

and in CopysetNodeManager::AddService,

curve/curvefs/src/metaserver/copyset/copyset_node_manager.cpp

Line 221 in 895fa84

void CopysetNodeManager::AddService(brpc::Server* server,

you should add RaftCliService2 into external server.

fixed

wu-hanqing · 2022-01-13T03:34:47Z

curvefs/src/metaserver/metaserver.cpp

+        brpc::ServerOptions option;
+        if (options_.bthreadWorkerCount != -1) {
+            option.num_threads = options_.bthreadWorkerCount;
+        }


use above option is enough.

use above option is enough.

done

wu-hanqing · 2022-01-13T03:36:15Z

curvefs/src/metaserver/metaserver.cpp

+        if (options_.bthreadWorkerCount != -1) {
+            option.num_threads = options_.bthreadWorkerCount;
+        }
+        LOG_IF(FATAL, externalServer_->Start(listenAddr, &option) != 0)


you didn't stop external server when Stop.

you didn't stop external server when Stop.

fixed

cw123 · 2022-01-13T07:51:53Z

curvefs/proto/topology.proto

@@ -328,6 +328,7 @@ message MetaServerLocation {
    required string hostIp = 2;


here change it to internal ip, internal port

here change it to internal ip, internal port

Change to internalIp and internalPort on all the related interfces.

cw123 · 2022-01-13T07:52:30Z

curvefs/src/mds/topology/topology_manager.cpp

@@ -810,6 +810,7 @@ void TopologyManager::GetMetaServerListInCopysets(
                    location->set_hostip(metaserver.GetInternalHostIp());


here

fixed

wu-hanqing · 2022-01-14T02:26:26Z

curvefs/proto/topology.proto

+    required string InternalIp = 3;
+    required uint32 InternalPort = 4;


variable name should starts with lowercase

variable name should starts with lowercase

fixed

wu-hanqing · 2022-01-14T02:26:35Z

curvefs/proto/topology.proto

+    required string InternalIp = 2;
+    required uint32 InternalPort = 3;


wu-hanqing · 2022-01-14T02:26:42Z

curvefs/proto/topology.proto

+    required string InternalIp = 2;
+    required uint32 Internalport = 3;
    optional string externalIp = 4;
+    optional uint32 externalPort = 5;


wu-hanqing · 2022-01-14T02:28:21Z

curvefs/src/client/rpcclient/mds_client.h

+    template<typename T>
+    void GetEndPoint(const T &info, butil::EndPoint *internal,
+        butil::EndPoint *external);


no need to define it as member functions, define in cpp file and as free template function is enough.

no need to define it as member functions, define in cpp file and as free template function is enough.

fixed

wu-hanqing · 2022-01-14T02:43:41Z

curvefs/src/metaserver/metaserver.cpp

+            brpc::SERVER_DOESNT_OWN_SERVICE) != 0)
+            << "add raftCliService2 error";
+        LOG_IF(FATAL, externalServer_->AddService(&raftStatService,
+            brpc::SERVER_DOESNT_OWN_SERVICE) != 0)


externalServer_->AddService(new braft::RaftStatImpl{}, brpc::SERVER_OWN_SERVICE)

new braft::RaftStatImpl{}

fixed

wu-hanqing · 2022-01-14T02:44:33Z

curvefs/src/metaserver/metaserver.cpp

@@ -222,6 +256,8 @@ void Metaserver::Stop() {

    LOG(INFO) << "MetaServer is going to quit";

+    externalServer_->Stop(0);


you should check whether external ip is enabled otherwise, externalServer_ is null, this can cause segment fault.

you should check whether external ip is enabled otherwise, externalServer_ is null, this can cause segment fault.

fixed

wu-hanqing · 2022-01-14T02:48:06Z

curvefs/src/client/rpcclient/mds_client.cpp

+    std::string internalIp = info.internalip();
+    std::string externalIp = internalIp;
+    if (info.has_externalip()) {
+        externalIp = info.externalip();
+    }


const std::string& internalIp = info.internalip(); const std::string externalIp = [&info]() { if (info.has_externalip()) { return info.externapIp(); } else { return info.internalIp(); } }();

const std::string& internalIp = info.internalip(); const std::string externalIp = [&info]() { if (info.has_externalip()) { return info.externapIp(); } else { return info.internalIp(); } }();

fixed

wu-hanqing · 2022-01-14T02:48:18Z

curvefs/src/client/rpcclient/mds_client.cpp

+    uint32_t internalPort = info.internalport();
+    uint32_t externalPort = internalPort;
+    if (info.has_externalport()) {
+        externalPort = info.externalport();
+    }


…cluster communicate itself and some external services support for client and management tools. If all the services start on the same ip+port will affect the communication within the cluster when the heavy stress from client. Start the services needed by outsied on another ip+port as a external server.

SeanHai · 2022-01-21T06:01:47Z

recheck

SeanHai changed the title ~~curvefs: The curvefs cluster has some internal service which support …~~ curvefs metaserver supports external services Jan 11, 2022

SeanHai linked an issue Jan 11, 2022 that may be closed by this pull request

curvefs metaserver supports external services #973

Closed

SeanHai force-pushed the externalIp branch from fd42be2 to 6073b01 Compare January 12, 2022 09:26

cw123 reviewed Jan 13, 2022

View reviewed changes

wu-hanqing reviewed Jan 13, 2022

View reviewed changes

SeanHai force-pushed the externalIp branch 2 times, most recently from 9e79c77 to 2f698f9 Compare January 13, 2022 07:40

cw123 reviewed Jan 13, 2022

View reviewed changes

SeanHai force-pushed the externalIp branch 2 times, most recently from f6f1d97 to 86a17a1 Compare January 14, 2022 02:15

wu-hanqing reviewed Jan 14, 2022

View reviewed changes

SeanHai force-pushed the externalIp branch from 86a17a1 to 26078e7 Compare January 14, 2022 03:26

wu-hanqing approved these changes Jan 17, 2022

View reviewed changes

cw123 approved these changes Jan 17, 2022

View reviewed changes

SeanHai force-pushed the externalIp branch 3 times, most recently from d7c568e to ac2c254 Compare January 20, 2022 07:48

SeanHai force-pushed the externalIp branch from ac2c254 to 059cb07 Compare January 20, 2022 11:23

SeanHai closed this Jan 21, 2022

SeanHai mentioned this pull request Jan 21, 2022

curvefs metaserver supports external services #1047

Merged

2 tasks

		@@ -328,6 +328,7 @@ message MetaServerLocation {
		required string hostIp = 2;

		@@ -810,6 +810,7 @@ void TopologyManager::GetMetaServerListInCopysets(
		location->set_hostip(metaserver.GetInternalHostIp());

		required string InternalIp = 3;
		required uint32 InternalPort = 4;

		required string InternalIp = 2;
		required uint32 InternalPort = 3;

		@@ -222,6 +256,8 @@ void Metaserver::Stop() {

		LOG(INFO) << "MetaServer is going to quit";

		externalServer_->Stop(0);

curvefs metaserver supports external services #974

curvefs metaserver supports external services #974

Conversation

SeanHai commented Jan 11, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

YunhuiChen commented Jan 12, 2022

SeanHai commented Jan 13, 2022

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

SeanHai commented Jan 21, 2022

SeanHai commented Jan 11, 2022 •

edited

Loading