Fix the config parsing for S3GCManager connect to (#7682) (#7685)

close #7681

dbms/src/Server/RaftConfigParser.cpp

@@ -16,6 +16,7 @@
 #include <IO/WriteHelpers.h>
 #include <Poco/String.h>
 #include <Poco/StringTokenizer.h>
+#include <Poco/Util/AbstractConfiguration.h>
 #include <Poco/Util/LayeredConfiguration.h>
 #include <Server/RaftConfigParser.h>
 #include <Storages/MutableSupport.h>
@@ -33,8 +34,15 @@ extern const int INVALID_CONFIG_PARAMETER;
 TiFlashRaftConfig TiFlashRaftConfig::parseSettings(Poco::Util::AbstractConfiguration & config, const LoggerPtr & log)
 {
     TiFlashRaftConfig res;
+    // The ip/port that flash service bind to
     res.flash_server_addr = config.getString("flash.service_addr", "0.0.0.0:3930");
-    res.advertise_addr = config.getString("flash.proxy.advertise-engine-addr", res.flash_server_addr);
+    // The ip/port that other flash server connect to
+    res.advertise_engine_addr = config.getString(
+        "flash.proxy.advertise-engine-addr",
+        config.getString("flash.proxy.engine-addr", res.flash_server_addr));
+
+    // "addr" - The raft ip/port that raft service bind to
+    // "advertise-addr" - The raft ip/port that other raft server connect to
 
     {
         // Check by `raft` prefix instead of check by `config.has("raft")`,

dbms/src/Server/RaftConfigParser.h

@@ -19,32 +19,28 @@
 
 #include <unordered_set>
 
-namespace Poco
+namespace Poco::Util
 {
-class Logger;
-namespace Util
-{
-class AbstractConfiguration;
-}
-} // namespace Poco
+class LayeredConfiguration;
+} // namespace Poco::Util
 
 namespace DB
 {
 struct TiFlashRaftConfig
 {
-    const std::string engine_key = "engine";
-    const std::string engine_value = "tiflash";
     Strings pd_addrs;
     std::unordered_set<std::string> ignore_databases{"system"};
 
     // The addr that is bound for flash service
     // Actually its value is read from "flash.service_addr"
     std::string flash_server_addr;
-    // The addr that other nodes connect to this tiflash
-    // Actually its value is read from "flash.proxy.advertise-engine-addr".
-    // If "flash.proxy.advertise-engine-addr" is not set, it will fall
-    // back to be the same as `flash_server_addr`.
-    std::string advertise_addr;
+    // The addr that other TiFlash nodes connect to this tiflash. Its value is set by
+    // following configurations. The previous configuration will override the later
+    // items.
+    //   - "flash.proxy.advertise-engine-addr".
+    //   - "flash.proxy.engine-addr"
+    //   - "flash_server_addr"
+    std::string advertise_engine_addr;
 
     bool for_unit_test = false;
 

dbms/src/Server/Server.cpp

@@ -290,16 +290,14 @@ struct TiFlashProxyConfig
         auto disaggregated_mode = getDisaggregatedMode(config);
 
         // tiflash_compute doesn't need proxy.
-        // todo: remove after AutoScaler is stable.
         if (disaggregated_mode == DisaggregatedMode::Compute && useAutoScaler(config))
         {
-            LOG_WARNING(Logger::get(), "TiFlash Proxy will not start because AutoScale Disaggregated Compute Mode is specified.");
+            LOG_INFO(Logger::get(), "TiFlash Proxy will not start because AutoScale Disaggregated Compute Mode is specified.");
             return;
         }
 
         Poco::Util::AbstractConfiguration::Keys keys;
         config.keys("flash.proxy", keys);
-
         if (!config.has("raft.pd_addr"))
         {
             LOG_WARNING(Logger::get(), "TiFlash Proxy will not start because `raft.pd_addr` is not configured.");
@@ -339,11 +337,11 @@ struct TiFlashProxyConfig
     }
 };
 
-pingcap::ClusterConfig getClusterConfig(TiFlashSecurityConfigPtr security_config, const TiFlashRaftConfig & raft_config, const int api_version, const LoggerPtr & log)
+pingcap::ClusterConfig getClusterConfig(TiFlashSecurityConfigPtr security_config, const int api_version, const LoggerPtr & log)
 {
     pingcap::ClusterConfig config;
-    config.tiflash_engine_key = raft_config.engine_key;
-    config.tiflash_engine_value = raft_config.engine_value;
+    config.tiflash_engine_key = "engine";
+    config.tiflash_engine_value = DEF_PROXY_LABEL;
     auto [ca_path, cert_path, key_path] = security_config->getPaths();
     config.ca_path = ca_path;
     config.cert_path = cert_path;
@@ -1328,7 +1326,7 @@ int Server::main(const std::vector<std::string> & /*args*/)
                 if (updated)
                 {
                     auto raft_config = TiFlashRaftConfig::parseSettings(*config, log);
-                    auto cluster_config = getClusterConfig(global_context->getSecurityConfig(), raft_config, storage_config.api_version, log);
+                    auto cluster_config = getClusterConfig(global_context->getSecurityConfig(), storage_config.api_version, log);
                     global_context->getTMTContext().updateSecurityConfig(std::move(raft_config), std::move(cluster_config));
                     LOG_DEBUG(log, "TMTContext updated security config");
                 }
@@ -1401,7 +1399,7 @@ int Server::main(const std::vector<std::string> & /*args*/)
 
     {
         /// create TMTContext
-        auto cluster_config = getClusterConfig(global_context->getSecurityConfig(), raft_config, storage_config.api_version, log);
+        auto cluster_config = getClusterConfig(global_context->getSecurityConfig(), storage_config.api_version, log);
         global_context->createTMTContext(raft_config, std::move(cluster_config));
         if (store_ident)
         {

dbms/src/Server/StorageConfigParser.h

@@ -20,14 +20,10 @@
 #include <tuple>
 #include <vector>
 
-namespace Poco
-{
-class Logger;
-namespace Util
+namespace Poco::Util
 {
 class LayeredConfiguration;
-}
-} // namespace Poco
+} // namespace Poco::Util
 
 namespace DB
 {

dbms/src/Server/tests/gtest_raft_config.cpp

@@ -0,0 +1,93 @@
+// Copyright 2023 PingCAP, Ltd.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include <Common/Config/ConfigProcessor.h>
+#include <Common/Exception.h>
+#include <Poco/Environment.h>
+#include <Poco/Logger.h>
+#include <Server/RaftConfigParser.h>
+#include <TestUtils/ConfigTestUtils.h>
+#include <TestUtils/TiFlashTestBasic.h>
+#include <cpptoml.h>
+
+namespace DB::tests
+{
+
+class RaftConfigTest : public ::testing::Test
+{
+public:
+    RaftConfigTest()
+        : log(Logger::get())
+    {}
+
+    static void SetUpTestCase() {}
+
+protected:
+    LoggerPtr log;
+};
+
+TEST_F(RaftConfigTest, SimpleSinglePath)
+try
+{
+    Strings tests = {
+        R"(
+flash.service_addr = "0.0.0.0:3930"
+        )",
+        R"(
+flash.service_addr = "0.0.0.0:3930"
+flash.proxy.engine-addr = "172.16.5.85:3930"
+        )",
+        R"(
+flash.service_addr = "0.0.0.0:3930"
+flash.proxy.engine-addr = "172.16.5.85:3930"
+flash.proxy.advertise-engine-addr = "h85:3930"
+        )",
+    };
+
+    for (size_t i = 0; i < tests.size(); ++i)
+    {
+        const auto & test_case = tests[i];
+        auto config = loadConfigFromString(test_case);
+        LOG_INFO(log, "parsing, index={} content={}", i, test_case);
+
+        const TiFlashRaftConfig raft_config = TiFlashRaftConfig::parseSettings(*config, log);
+        switch (i)
+        {
+        case 0:
+        {
+            ASSERT_EQ(raft_config.flash_server_addr, "0.0.0.0:3930");
+            ASSERT_EQ(raft_config.advertise_engine_addr, "0.0.0.0:3930");
+            break;
+        }
+        case 1:
+        {
+            ASSERT_EQ(raft_config.flash_server_addr, "0.0.0.0:3930");
+            ASSERT_EQ(raft_config.advertise_engine_addr, "172.16.5.85:3930");
+            break;
+        }
+        case 2:
+        {
+            ASSERT_EQ(raft_config.flash_server_addr, "0.0.0.0:3930");
+            ASSERT_EQ(raft_config.advertise_engine_addr, "h85:3930");
+            break;
+        }
+        default:
+            RUNTIME_CHECK(false, test_case);
+        }
+    }
+}
+CATCH
+
+
+} // namespace DB::tests

dbms/src/Storages/Transaction/TMTContext.cpp

@@ -96,7 +96,7 @@ TMTContext::TMTContext(Context & context_, const TiFlashRaftConfig & raft_config
     if (!raft_config.pd_addrs.empty() && S3::ClientFactory::instance().isEnabled() && !context.getSharedContextDisagg()->isDisaggregatedComputeMode())
     {
         etcd_client = Etcd::Client::create(cluster->pd_client, cluster_config);
-        s3gc_owner = OwnerManager::createS3GCOwner(context, /*id*/ raft_config.advertise_addr, etcd_client);
+        s3gc_owner = OwnerManager::createS3GCOwner(context, /*id*/ raft_config.advertise_engine_addr, etcd_client);
         s3gc_owner->campaignOwner(); // start campaign
         s3lock_client = std::make_shared<S3::S3LockClient>(cluster.get(), s3gc_owner);