You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kvrocks.apache.org by tw...@apache.org on 2023/10/15 15:11:58 UTC
[kvrocks] branch unstable updated: Fix server cannot start without the configuration file (#1804)
This is an automated email from the ASF dual-hosted git repository.
twice pushed a commit to branch unstable
in repository https://gitbox.apache.org/repos/asf/kvrocks.git
The following commit(s) were added to refs/heads/unstable by this push:
new 2b954568 Fix server cannot start without the configuration file (#1804)
2b954568 is described below
commit 2b9545688142df85048bc8bc04c0589cb96f9cc1
Author: hulk <hu...@gmail.com>
AuthorDate: Sun Oct 15 23:11:53 2023 +0800
Fix server cannot start without the configuration file (#1804)
Co-authored-by: Twice <tw...@gmail.com>
---
src/config/config.cc | 2 +-
src/config/config.h | 1 +
src/server/namespace.cc | 23 +++++++++++++---
src/server/namespace.h | 1 +
.../integration/cli_options/cli_options_test.go | 2 +-
tests/gocase/unit/config/config_test.go | 12 +++++++++
tests/gocase/unit/namespace/namespace_test.go | 30 +++++++++++++++++++++
tests/gocase/util/server.go | 31 +++++++++++++++-------
8 files changed, 87 insertions(+), 15 deletions(-)
diff --git a/src/config/config.cc b/src/config/config.cc
index ad922dd6..d3549e59 100644
--- a/src/config/config.cc
+++ b/src/config/config.cc
@@ -844,7 +844,7 @@ Status Config::Set(Server *svr, std::string key, const std::string &value) {
}
Status Config::Rewrite(const std::map<std::string, std::string> &tokens) {
- if (path_.empty()) {
+ if (!HasConfigFile()) {
return {Status::NotOK, "the server is running without a config file"};
}
diff --git a/src/config/config.h b/src/config/config.h
index d79d1527..d540906f 100644
--- a/src/config/config.h
+++ b/src/config/config.h
@@ -222,6 +222,7 @@ struct Config {
void SetMaster(const std::string &host, uint32_t port);
void ClearMaster();
bool IsSlave() const { return !master_host.empty(); }
+ bool HasConfigFile() const { return !path_.empty(); }
private:
std::string path_;
diff --git a/src/server/namespace.cc b/src/server/namespace.cc
index a0e8d05a..01a44dcf 100644
--- a/src/server/namespace.cc
+++ b/src/server/namespace.cc
@@ -31,6 +31,8 @@ constexpr const char* kErrClusterModeEnabled = "forbidden to add namespace when
constexpr const char* kErrDeleteDefaultNamespace = "forbidden to delete the default namespace";
constexpr const char* kErrAddDefaultNamespace = "forbidden to add the default namespace";
constexpr const char* kErrInvalidToken = "the token is duplicated with requirepass or masterauth";
+constexpr const char* kErrCantModifyNamespace =
+ "modify namespace requires the server is running with a configuration file or enabled namespace replication";
Status IsNamespaceLegal(const std::string& ns) {
if (ns.size() > UINT8_MAX) {
@@ -43,6 +45,12 @@ Status IsNamespaceLegal(const std::string& ns) {
return Status::OK();
}
+bool Namespace::IsAllowModify() const {
+ auto config = storage_->GetConfig();
+
+ return config->HasConfigFile() || config->repl_namespace_enabled;
+}
+
Status Namespace::LoadAndRewrite() {
auto config = storage_->GetConfig();
// Load from the configuration file first
@@ -100,6 +108,9 @@ Status Namespace::Set(const std::string& ns, const std::string& token) {
if (config->cluster_enabled) {
return {Status::NotOK, kErrClusterModeEnabled};
}
+ if (!IsAllowModify()) {
+ return {Status::NotOK, kErrCantModifyNamespace};
+ }
if (ns == kDefaultNamespace) {
return {Status::NotOK, kErrAddDefaultNamespace};
}
@@ -142,6 +153,9 @@ Status Namespace::Del(const std::string& ns) {
if (ns == kDefaultNamespace) {
return {Status::NotOK, kErrDeleteDefaultNamespace};
}
+ if (!IsAllowModify()) {
+ return {Status::NotOK, kErrCantModifyNamespace};
+ }
for (const auto& iter : tokens_) {
if (iter.second == ns) {
@@ -159,9 +173,12 @@ Status Namespace::Del(const std::string& ns) {
Status Namespace::Rewrite() {
auto config = storage_->GetConfig();
- auto s = config->Rewrite(tokens_);
- if (!s.IsOK()) {
- return s;
+ // Rewrite the configuration file only if it's running with the configuration file
+ if (config->HasConfigFile()) {
+ auto s = config->Rewrite(tokens_);
+ if (!s.IsOK()) {
+ return s;
+ }
}
// Don't propagate write to DB if its role is slave to prevent from
diff --git a/src/server/namespace.h b/src/server/namespace.h
index cccf46e0..943a3ece 100644
--- a/src/server/namespace.h
+++ b/src/server/namespace.h
@@ -42,6 +42,7 @@ class Namespace {
Status Del(const std::string &ns);
const std::map<std::string, std::string> &List() const { return tokens_; }
Status Rewrite();
+ bool IsAllowModify() const;
private:
engine::Storage *storage_;
diff --git a/tests/gocase/integration/cli_options/cli_options_test.go b/tests/gocase/integration/cli_options/cli_options_test.go
index 767679dd..43f3cf77 100644
--- a/tests/gocase/integration/cli_options/cli_options_test.go
+++ b/tests/gocase/integration/cli_options/cli_options_test.go
@@ -28,7 +28,7 @@ import (
)
func TestCLIOptions(t *testing.T) {
- srv := util.StartServerWithCLIOptions(t, map[string]string{}, []string{"--maxclients", "23333"})
+ srv := util.StartServerWithCLIOptions(t, true, map[string]string{}, []string{"--maxclients", "23333"})
defer srv.Close()
ctx := context.Background()
diff --git a/tests/gocase/unit/config/config_test.go b/tests/gocase/unit/config/config_test.go
index 2bca988a..c6c9b682 100644
--- a/tests/gocase/unit/config/config_test.go
+++ b/tests/gocase/unit/config/config_test.go
@@ -129,3 +129,15 @@ func TestConfigSetCompression(t *testing.T) {
}
require.ErrorContains(t, rdb.ConfigSet(ctx, configKey, "unsupported").Err(), "invalid enum option")
}
+
+func TestStartWithoutConfigurationFile(t *testing.T) {
+ srv := util.StartServerWithCLIOptions(t, false, map[string]string{}, []string{})
+ defer srv.Close()
+
+ ctx := context.Background()
+ rdb := srv.NewClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ require.NoError(t, rdb.Do(ctx, "SET", "foo", "bar").Err())
+ require.Equal(t, "bar", rdb.Do(ctx, "GET", "foo").Val())
+}
diff --git a/tests/gocase/unit/namespace/namespace_test.go b/tests/gocase/unit/namespace/namespace_test.go
index bc04a7c3..1b80e5b9 100644
--- a/tests/gocase/unit/namespace/namespace_test.go
+++ b/tests/gocase/unit/namespace/namespace_test.go
@@ -241,3 +241,33 @@ func TestNamespaceReplicate(t *testing.T) {
util.ErrorRegexp(t, masterRdb.ConfigSet(ctx, "repl-namespace-enabled", "no").Err(), ".*cannot switch off repl_namespace_enabled when namespaces exist in db.*")
})
}
+
+func TestNamespaceRewrite(t *testing.T) {
+ password := "pwd"
+ srv := util.StartServerWithCLIOptions(t, false, map[string]string{
+ "requirepass": password,
+ }, []string{})
+ defer srv.Close()
+
+ rdb := srv.NewClientWithOption(&redis.Options{
+ Password: password,
+ })
+ defer func() { require.NoError(t, rdb.Close()) }()
+ ctx := context.Background()
+
+ t.Run("Cannot modify namespace if running without the configuration", func(t *testing.T) {
+ errMsg := ".*ERR modify namespace requires the server is running with a configuration file or enabled namespace replication.*"
+ r := rdb.Do(ctx, "NAMESPACE", "ADD", "ns1", "token1")
+ util.ErrorRegexp(t, r.Err(), errMsg)
+ r = rdb.Do(ctx, "NAMESPACE", "SET", "ns1", "token1")
+ util.ErrorRegexp(t, r.Err(), errMsg)
+ r = rdb.Do(ctx, "NAMESPACE", "DEL", "ns1")
+ util.ErrorRegexp(t, r.Err(), errMsg)
+
+ // Good to go after enabling namespace replication
+ require.NoError(t, rdb.ConfigSet(ctx, "repl-namespace-enabled", "yes").Err())
+ require.NoError(t, rdb.Do(ctx, "NAMESPACE", "ADD", "ns1", "token1").Err())
+ require.NoError(t, rdb.Do(ctx, "NAMESPACE", "SET", "ns1", "token2").Err())
+ require.NoError(t, rdb.Do(ctx, "NAMESPACE", "DEL", "ns1").Err())
+ })
+}
diff --git a/tests/gocase/util/server.go b/tests/gocase/util/server.go
index 5d190750..a3f4314e 100644
--- a/tests/gocase/util/server.go
+++ b/tests/gocase/util/server.go
@@ -194,10 +194,16 @@ func StartTLSServer(t testing.TB, configs map[string]string) *KvrocksServer {
}
func StartServer(t testing.TB, configs map[string]string) *KvrocksServer {
- return StartServerWithCLIOptions(t, configs, []string{})
+ return StartServerWithCLIOptions(t, true, configs, []string{})
}
-func StartServerWithCLIOptions(t testing.TB, configs map[string]string, options []string) *KvrocksServer {
+func StartServerWithCLIOptions(
+ t testing.TB,
+ withConfigFile bool,
+ configs map[string]string,
+ options []string,
+) *KvrocksServer {
+
b := *binPath
require.NotEmpty(t, b, "please set the binary path by `-binPath`")
cmd := exec.Command(b)
@@ -215,16 +221,21 @@ func StartServerWithCLIOptions(t testing.TB, configs map[string]string, options
require.NoError(t, err)
configs["dir"] = dir
- f, err := os.Create(filepath.Join(dir, "kvrocks.conf"))
- require.NoError(t, err)
- defer func() { require.NoError(t, f.Close()) }()
-
- for k := range configs {
- _, err := f.WriteString(fmt.Sprintf("%s %s\n", k, configs[k]))
+ if withConfigFile {
+ f, err := os.Create(filepath.Join(dir, "kvrocks.conf"))
require.NoError(t, err)
- }
+ defer func() { require.NoError(t, f.Close()) }()
- cmd.Args = append(cmd.Args, "-c", f.Name())
+ for k := range configs {
+ _, err := f.WriteString(fmt.Sprintf("%s %s\n", k, configs[k]))
+ require.NoError(t, err)
+ }
+ cmd.Args = append(cmd.Args, "-c", f.Name())
+ } else {
+ for k := range configs {
+ cmd.Args = append(cmd.Args, fmt.Sprintf("--%s", k), configs[k])
+ }
+ }
cmd.Args = append(cmd.Args, options...)
stdout, err := os.Create(filepath.Join(dir, "stdout"))