[GH-ISSUE #412] ChainTable的version有可能回退 #236

New issue

Closed

opened 2026-10-26 09:47:36 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented 2026-10-26 09:47:36 +00:00

Owner

Copy link

Originally created by @Captain32 on GitHub (May 4, 2026).
Original GitHub issue: https://github.com/deepseek-ai/3FS/issues/412

问题描述

看代码的时候发现，admin_cli处理upload-chain-table命令的时候，会向Mgmtd Server发起setChainTable这个RPC调用，但是Mgmtd Server处理的时候，没有考虑只改变了desc的情况：

CoTryTask<SetChainTableRsp> SetChainTableOperation::handle(MgmtdState &state) {
  ...
  auto tableId = req.chainTableId;
  /* 新构建的ChainTable对象，version默认为1 */
  auto tableVersion = flat::ChainTableVersion(1);
  auto newChainTable = flat::ChainTable::create(tableId, tableVersion, std::move(req.chains), std::move(req.desc));

  auto handler = [&]() -> CoTryTask<SetChainTableRsp> {
    auto writerLock = co_await state.coScopedLock<"SetChainTable">();
    {
      ...
      auto ctit = dataPtr->routingInfo.chainTables.find(tableId);
      if (ctit != dataPtr->routingInfo.chainTables.end()) { // 已经存在ChainTable具有这个ID，所以这是一次修改操作
        const auto &m = ctit->second;
        const auto &current = m.rbegin()->second;
        if (newChainTable.chains != current.chains) { // Bug点：只有chains发生改变，才会递进version，否则就沿用刚才的1！
          newChainTable.chainTableVersion = nextVersion(current.chainTableVersion);
        }
        if (newChainTable.desc.empty()) {
          newChainTable.desc = current.desc;
        }
        if (newChainTable == current) { // 因为desc不同，所以判断不通过，还是会走到后续真正修改的逻辑，新version用的1！
          // same as the latest version, do not add new version
          co_return SetChainTableRsp::create(current.chainTableVersion);
        }
      }
    }
    ...
  }
  ...
}

复现Case

在tests/mgmtd/TestMgmtdOperator.cc中加了一个测试样例，其中Step 3就是只修改了desc：

TEST_F(MgmtdOperatorTest, testSetChainTableVersionMonotonicity) {
  folly::coro::blockingWait([&]() -> CoTask<void> {
    MgmtdOperator mgmtd(env_, defaultConfig_);
    co_await MgmtdTestHelper(mgmtd).extendLease();

    // Prepare two Chains
    co_await MgmtdTestHelper(mgmtd).setRoutingInfo([](flat::RoutingInfo &ri) {
      for (auto cid : {flat::ChainId(1), flat::ChainId(2)}) {
        flat::ChainInfo chain;
        chain.chainId = cid;
        chain.chainVersion = flat::ChainVersion(1);
        flat::ChainTargetInfo cti;
        cti.targetId = flat::TargetId(100 + (uint32_t)cid);
        cti.publicState = flat::PublicTargetState::SERVING;
        chain.targets.push_back(cti);
        ri.chains[cid] = chain;
      }
    });

    // Step 1: upload ChainTable, with version 1
    {
      mgmtd::SetChainTableReq req;
      req.clusterId = "clusterId";
      req.chainTableId = flat::ChainTableId(1);
      req.chains = {flat::ChainId(1)};
      req.desc = "old_desc";
      auto ret = co_await mgmtd.setChainTable(req, {});
      CO_ASSERT_OK(ret);
      CO_ASSERT_EQ(ret->chainTableVersion, flat::ChainTableVersion(1));
    }

    // Step 2: update chains of ChainTable, increase version to 2
    {
      mgmtd::SetChainTableReq req;
      req.clusterId = "clusterId";
      req.chainTableId = flat::ChainTableId(1);
      req.chains = {flat::ChainId(1), flat::ChainId(2)};
      req.desc = "old_desc";
      auto ret = co_await mgmtd.setChainTable(req, {});
      CO_ASSERT_OK(ret);
      CO_ASSERT_EQ(ret->chainTableVersion, flat::ChainTableVersion(2));
    }

    // Step 3: update desc of ChainTable, the version should be 3
    {
      mgmtd::SetChainTableReq req;
      req.clusterId = "clusterId";
      req.chainTableId = flat::ChainTableId(1);
      req.chains = {flat::ChainId(1), flat::ChainId(2)};  // keep same
      req.desc = "new_desc";                              // only desc changed
      auto ret = co_await mgmtd.setChainTable(req, {});
      CO_ASSERT_OK(ret);
      // the new version should be 3, but return 1!
      CO_ASSERT_EQ(ret->chainTableVersion, flat::ChainTableVersion(3));
    }
  }());
}

运行测试可以发现，Step 3过后，ChainTable的version回退到了1，其实相当于把原本的version 1覆盖了：

build/tests/test_mgmtd --gtest_filter="*VersionMonotonicity*"
Note: Google Test filter = *VersionMonotonicity*
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from MgmtdOperatorTest
[ RUN      ] MgmtdOperatorTest.testSetChainTableVersionMonotonicity
./tests/mgmtd/TestMgmtdOperator.cc:683: Failure
Expected equality of these values:
  ret->chainTableVersion
    Which is: 1
  flat::ChainTableVersion(3)
    Which is: 3
[  FAILED  ] MgmtdOperatorTest.testSetChainTableVersionMonotonicity (21 ms)
[----------] 1 test from MgmtdOperatorTest (21 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (21 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] MgmtdOperatorTest.testSetChainTableVersionMonotonicity

 1 FAILED TEST

修复方案

其实ChainTable到这一步能发生修改的就chains或desc，令desc发生修改的时候也推进version就行。并且感觉不应该用newChainTable == current判断是否发生修改，因为newChainTable的version默认为1，和current很可能不一样，只需要关注chains和desc就行：

diff --git a/src/mgmtd/ops/SetChainTableOperation.cc b/src/mgmtd/ops/SetChainTableOperation.cc
index 889ed17..3e3e8ad 100644
--- a/src/mgmtd/ops/SetChainTableOperation.cc
+++ b/src/mgmtd/ops/SetChainTableOperation.cc
@@ -53,13 +53,12 @@ CoTryTask<SetChainTableRsp> SetChainTableOperation::handle(MgmtdState &state) {
         const auto &m = ctit->second;
         XLOGF_IF(FATAL, m.empty(), "{} has no versions", tableId);
         const auto &current = m.rbegin()->second;
-        if (newChainTable.chains != current.chains) {
-          newChainTable.chainTableVersion = nextVersion(current.chainTableVersion);
-        }
         if (newChainTable.desc.empty()) {
           newChainTable.desc = current.desc;
         }
-        if (newChainTable == current) {
+        if (newChainTable.chains != current.chains || newChainTable.desc != current.desc) {
+          newChainTable.chainTableVersion = nextVersion(current.chainTableVersion);
+        } else {
           // same as the latest version, do not add new version
           co_return SetChainTableRsp::create(current.chainTableVersion);
         }

修复后可以通过test_mgmtd内所有测试样例，外加新增的这个：

build/tests/test_mgmtd
...
[==========] 26 tests from 4 test suites ran. (3197 ms total)
[  PASSED  ] 26 tests.

Originally created by @Captain32 on GitHub (May 4, 2026). Original GitHub issue: https://github.com/deepseek-ai/3FS/issues/412 ## 问题描述 看代码的时候发现，admin_cli处理`upload-chain-table`命令的时候，会向Mgmtd Server发起`setChainTable`这个RPC调用，但是Mgmtd Server处理的时候，没有考虑只改变了`desc`的情况： ```cpp CoTryTask<SetChainTableRsp> SetChainTableOperation::handle(MgmtdState &state) { ... auto tableId = req.chainTableId; /* 新构建的ChainTable对象，version默认为1 */ auto tableVersion = flat::ChainTableVersion(1); auto newChainTable = flat::ChainTable::create(tableId, tableVersion, std::move(req.chains), std::move(req.desc)); auto handler = [&]() -> CoTryTask<SetChainTableRsp> { auto writerLock = co_await state.coScopedLock<"SetChainTable">(); { ... auto ctit = dataPtr->routingInfo.chainTables.find(tableId); if (ctit != dataPtr->routingInfo.chainTables.end()) { // 已经存在ChainTable具有这个ID，所以这是一次修改操作 const auto &m = ctit->second; const auto &current = m.rbegin()->second; if (newChainTable.chains != current.chains) { // Bug点：只有chains发生改变，才会递进version，否则就沿用刚才的1！ newChainTable.chainTableVersion = nextVersion(current.chainTableVersion); } if (newChainTable.desc.empty()) { newChainTable.desc = current.desc; } if (newChainTable == current) { // 因为desc不同，所以判断不通过，还是会走到后续真正修改的逻辑，新version用的1！ // same as the latest version, do not add new version co_return SetChainTableRsp::create(current.chainTableVersion); } } } ... } ... } ``` ## 复现Case 在`tests/mgmtd/TestMgmtdOperator.cc`中加了一个测试样例，其中`Step 3`就是只修改了`desc`： ```cpp TEST_F(MgmtdOperatorTest, testSetChainTableVersionMonotonicity) { folly::coro::blockingWait([&]() -> CoTask<void> { MgmtdOperator mgmtd(env_, defaultConfig_); co_await MgmtdTestHelper(mgmtd).extendLease(); // Prepare two Chains co_await MgmtdTestHelper(mgmtd).setRoutingInfo([](flat::RoutingInfo &ri) { for (auto cid : {flat::ChainId(1), flat::ChainId(2)}) { flat::ChainInfo chain; chain.chainId = cid; chain.chainVersion = flat::ChainVersion(1); flat::ChainTargetInfo cti; cti.targetId = flat::TargetId(100 + (uint32_t)cid); cti.publicState = flat::PublicTargetState::SERVING; chain.targets.push_back(cti); ri.chains[cid] = chain; } }); // Step 1: upload ChainTable, with version 1 { mgmtd::SetChainTableReq req; req.clusterId = "clusterId"; req.chainTableId = flat::ChainTableId(1); req.chains = {flat::ChainId(1)}; req.desc = "old_desc"; auto ret = co_await mgmtd.setChainTable(req, {}); CO_ASSERT_OK(ret); CO_ASSERT_EQ(ret->chainTableVersion, flat::ChainTableVersion(1)); } // Step 2: update chains of ChainTable, increase version to 2 { mgmtd::SetChainTableReq req; req.clusterId = "clusterId"; req.chainTableId = flat::ChainTableId(1); req.chains = {flat::ChainId(1), flat::ChainId(2)}; req.desc = "old_desc"; auto ret = co_await mgmtd.setChainTable(req, {}); CO_ASSERT_OK(ret); CO_ASSERT_EQ(ret->chainTableVersion, flat::ChainTableVersion(2)); } // Step 3: update desc of ChainTable, the version should be 3 { mgmtd::SetChainTableReq req; req.clusterId = "clusterId"; req.chainTableId = flat::ChainTableId(1); req.chains = {flat::ChainId(1), flat::ChainId(2)}; // keep same req.desc = "new_desc"; // only desc changed auto ret = co_await mgmtd.setChainTable(req, {}); CO_ASSERT_OK(ret); // the new version should be 3, but return 1! CO_ASSERT_EQ(ret->chainTableVersion, flat::ChainTableVersion(3)); } }()); } ``` 运行测试可以发现，`Step 3`过后，`ChainTable`的version回退到了1，其实相当于把原本的version 1覆盖了： ```shell build/tests/test_mgmtd --gtest_filter="*VersionMonotonicity*" Note: Google Test filter = *VersionMonotonicity* [==========] Running 1 test from 1 test suite. [----------] Global test environment set-up. [----------] 1 test from MgmtdOperatorTest [ RUN ] MgmtdOperatorTest.testSetChainTableVersionMonotonicity ./tests/mgmtd/TestMgmtdOperator.cc:683: Failure Expected equality of these values: ret->chainTableVersion Which is: 1 flat::ChainTableVersion(3) Which is: 3 [ FAILED ] MgmtdOperatorTest.testSetChainTableVersionMonotonicity (21 ms) [----------] 1 test from MgmtdOperatorTest (21 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test suite ran. (21 ms total) [ PASSED ] 0 tests. [ FAILED ] 1 test, listed below: [ FAILED ] MgmtdOperatorTest.testSetChainTableVersionMonotonicity 1 FAILED TEST ``` **修复方案** 其实`ChainTable`到这一步能发生修改的就`chains`或`desc`，令`desc`发生修改的时候也推进version就行。并且感觉不应该用`newChainTable == current`判断是否发生修改，因为`newChainTable`的version默认为1，和current很可能不一样，只需要关注`chains`和`desc`就行： ``` diff --git a/src/mgmtd/ops/SetChainTableOperation.cc b/src/mgmtd/ops/SetChainTableOperation.cc index 889ed17..3e3e8ad 100644 --- a/src/mgmtd/ops/SetChainTableOperation.cc +++ b/src/mgmtd/ops/SetChainTableOperation.cc @@ -53,13 +53,12 @@ CoTryTask<SetChainTableRsp> SetChainTableOperation::handle(MgmtdState &state) { const auto &m = ctit->second; XLOGF_IF(FATAL, m.empty(), "{} has no versions", tableId); const auto &current = m.rbegin()->second; - if (newChainTable.chains != current.chains) { - newChainTable.chainTableVersion = nextVersion(current.chainTableVersion); - } if (newChainTable.desc.empty()) { newChainTable.desc = current.desc; } - if (newChainTable == current) { + if (newChainTable.chains != current.chains || newChainTable.desc != current.desc) { + newChainTable.chainTableVersion = nextVersion(current.chainTableVersion); + } else { // same as the latest version, do not add new version co_return SetChainTableRsp::create(current.chainTableVersion); } ``` 修复后可以通过`test_mgmtd`内所有测试样例，外加新增的这个： ```shell build/tests/test_mgmtd ... [==========] 26 tests from 4 test suites ran. (3197 ms total) [ PASSED ] 26 tests. ```

bsakaguchi closed this issue 2026-10-26 09:47:36 +00:00

bsakaguchi commented 2026-10-26 09:47:36 +00:00

Author

Owner

Copy link

@SF-Zhou commented on GitHub (May 4, 2026):

LGTM. @KuribohG take a look?

<!-- gh-comment-id:4369884957 --> @SF-Zhou commented on GitHub (May 4, 2026): LGTM. @KuribohG take a look?

Sign in to join this conversation.

No Branch/Tag specified

Branches Tags

main

SF-Zhou-patch-1

No results found.

Labels

Clear labels   

pull-request

Mirrored from GitHub Pull Request

No labels

pull-request

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

deepseek-ai/3FS#236

No description provided.