Skip to content

Commit

Permalink
MB-51612: Ensure GetClusterConfig/NMVB sends config in mixed clusters
Browse files Browse the repository at this point in the history
In a mixed version cluster, ns_server may set a cluster config with
epoch -1.

get_cluster_config_executor tries to fetch the current config,
requesting anything "newer" than a default constructed config.

Similarly, Cookie::sendNotMyVBucket will only send configs "newer" than
any previously pushed config - or a default constucted config if
none has been pushed.

Unfortunately, a config with epoch -1 appears "older", and triggers
dedupe intended to avoid re-sending configs if the client already
has the current version.

Thus, in a mixed cluster, GetClusterConfig and sendNotMyVBucket may
erroneously consider the live config "older", and will refuse to send
it to clients which need it.

To resolve this, a default constructed config should start at epoch -1.
With this, any valid config set (and validated) by SetClusterConfig
will be "newer".

Change-Id: I470fe247c98c0c5e0d68667ddd40abe9691d8032
Reviewed-on: https://review.couchbase.org/c/kv_engine/+/172962
Well-Formed: Restriction Checker
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
  • Loading branch information
jameseh96 authored and daverigby committed Mar 29, 2022
1 parent cc160cb commit e703a5c
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 1 deletion.
2 changes: 1 addition & 1 deletion daemon/cluster_config.h
Expand Up @@ -21,7 +21,7 @@ class Connection;

class ClustermapVersion {
public:
ClustermapVersion() : epoch(0), revno(0) {
ClustermapVersion() : epoch(-1), revno(0) {
}
ClustermapVersion(int64_t epoch, int64_t revno)
: epoch(epoch), revno(revno){};
Expand Down
29 changes: 29 additions & 0 deletions tests/testapp/testapp_cluster_config.cc
Expand Up @@ -112,6 +112,35 @@ TEST_P(ClusterConfigTest, GetClusterConfig) {
{value.data(), value.size()}));
}

TEST_P(ClusterConfigTest, GetClusterConfig_ClusterCompat) {
// MB-51612: Test that GetClusterConfig works in a mixed version cluster

// set a config with an epoch of -1 - used by ns_server when in a mixed
// version cluster.
int64_t epoch = -1;
const std::string config{R"({"rev":100})"};
{
auto& conn = getAdminConnection();
ASSERT_TRUE(
conn.execute(BinprotSetClusterConfigCommand{token,
config,
epoch,
100 /*revision */,
"default"})
.isSuccess());
}

BinprotGenericCommand cmd{cb::mcbp::ClientOpcode::GetClusterConfig, "", ""};
auto& conn = getConnection();
const auto response = conn.execute(cmd);
EXPECT_TRUE(response.isSuccess()) << to_string(response.getStatus());
const auto value = response.getDataString();
EXPECT_EQ(config, value);
EXPECT_TRUE(hasCorrectDatatype(expectedJSONDatatype(),
cb::mcbp::Datatype(response.getDatatype()),
{value.data(), value.size()}));
}

TEST_P(ClusterConfigTest, test_MB_17506_no_dedupe) {
test_MB_17506(false);
}
Expand Down

0 comments on commit e703a5c

Please sign in to comment.