Adding configurator feature to dynamically inject configuration changes by mankan1 · Pull Request #374 · futurewei-cloud/chogori-platform

mankan1 · 2023-01-26T22:58:30Z

Adding configurator feature to dynamically inject configuration changes, added a server and client.

src/k2/configurator/Configurator.cpp

jfunston · 2023-01-30T17:27:45Z

src/k2/configurator/Configurator.cpp

+/*
+MIT License
+
+Copyright(c) 2020 Futurewei Cloud


Copyright date here and in other new files should be 2023

jfunston · 2023-01-30T17:28:42Z

src/k2/configurator/Configurator.h

+
+
+private :
+    ConfigVar<String> _key{"key"};


Are these variables used anywhere?

jfunston · 2023-01-30T17:30:13Z

src/k2/configurator/ConfiguratorDTO.h

+};
+
+struct DELETE_ConfiguratorResponse {
+    String key;


Does the key need to be in the response for any of these?

jfunston · 2023-01-30T17:30:50Z

src/k2/transport/RPCTypes.h

-    MAX_VERB = 250,  // something we can use to prevent override of internal verbs.
+    CONFGURATOR_GET = 250,  // used to configure get.
+    CONFGURATOR_SET = 251,  // used to configure set.
+    CONFGURATOR_DELETE = 252,  // used to configure clear messages.


I'm not sure this is where we want these defined, maybe Ivan has a suggestion.

jfunston · 2023-01-30T17:35:44Z

test/configurator/ConfiguratorTest.cpp

+            K2LOG_I(log::configuratortest, "Registering message handlers");
+            auto ep = RPC().getServerEndpoint(TCPRPCProtocol::proto);
+
+            _singleTimer.setCallback([ep, this] {


I don't think you need a timer for this. You can assign the result from the make_ready_future call below to a member variable

jfunston · 2023-01-30T17:36:45Z

test/integration/test_configurator.sh

+tso_child_pid=$!
+
+# start CPO
+./build/src/k2/cmd/controlPlaneOracle/cpo_main ${COMMON_ARGS} -c1 --tcp_endpoints ${CPO} --data_dir ${CPODIR} --txn_heartbeat_deadline=10s --prometheus_port 63000 --assignment_timeout=1s --nodepool_endpoints ${EPS[@]} --tso_endpoints ${TSO} --tso_error_bound=100us --persistence_endpoints ${PERSISTENCE} &


Your configurator test is self-contained, you don't need to start any of the k2 components

iafuture · 2023-01-27T18:05:08Z

src/k2/configurator/Configurator.cpp

+/*
+MIT License
+
+Copyright(c) 2020 Futurewei Cloud


iafuture · 2023-01-27T18:08:55Z

src/k2/transport/RPCTypes.h

 // Verbs used by K2 internally
 enum InternalVerbs : k2::Verb {
    LIST_ENDPOINTS = 249,  // used to discover the endpoints of a node
+    CONFGURATOR_GET = 250,  // used to configure get.


You should rename these verbs to CONFIG_SET/GET/DELETE since they manage the config, not the configurator. Also, this enum has the MAX_VERB sentinel value of 250. You cannot repeat it. Since we already have a value for 249, You should list your verbs back from 249, so:
CONFIG_DELETE=246
CONFIG_GET=247
CONFIG_SET=248

iafuture · 2023-01-27T18:10:50Z

src/k2/configurator/ConfiguratorDTO.h

+
+namespace k2 {
+
+struct DELETE_ConfiguratorRequest {


Let's make the structs match the verb naming. So instead of DELETE_ConfiguratorRequest it should be ConfigDeleteRequest. Same for the rest

iafuture · 2023-01-27T18:11:43Z

src/k2/configurator/ConfiguratorDTO.h

+struct DELETE_ConfiguratorRequest {
+    String key;
+    String value;
+    bool applyToAll;


We supply values for all primitive types in structs. This should be applyToAll{false}. Same for the rest

iafuture · 2023-01-27T18:12:07Z

src/k2/configurator/ConfiguratorDTO.h

+};
+
+struct DELETE_ConfiguratorResponse {
+    String key;


no need to return the key.

iafuture · 2023-01-27T18:15:33Z

src/k2/configurator/Configurator.cpp

+namespace k2 {
+
+namespace log {
+inline thread_local k2::logging::Logger configurator("k2::configurator_server");


I would just call the name "k2::config". This name shows up in the logs so something shorter would be better.

iafuture · 2023-01-27T18:17:22Z

src/k2/configurator/Configurator.cpp

+    K2LOG_I(log::configurator, "Registering message handlers");
+
+    RPC().registerRPCObserver<SET_ConfiguratorRequest, SET_ConfiguratorResponse>(InternalVerbs::CONFGURATOR_SET, [this](SET_ConfiguratorRequest&& request) {
+                K2LOG_I(log::configurator, "Received set for key: {}", request.key);


log the whole request

iafuture · 2023-01-27T18:17:30Z

src/k2/configurator/Configurator.cpp

+            });
+
+    RPC().registerRPCObserver<GET_ConfiguratorRequest, GET_ConfiguratorResponse>(InternalVerbs::CONFGURATOR_GET, [this](GET_ConfiguratorRequest&& request) {
+                K2LOG_I(log::configurator, "Received get for key: {}", request.key);


log the whole request

iafuture · 2023-01-27T18:17:38Z

src/k2/configurator/Configurator.cpp

+            });
+
+    RPC().registerRPCObserver<DELETE_ConfiguratorRequest, DELETE_ConfiguratorResponse>(InternalVerbs::CONFGURATOR_DELETE, [this](DELETE_ConfiguratorRequest&& request) {
+                K2LOG_I(log::configurator, "Received delete for key: {}", request.key);


log the whole request

iafuture · 2023-01-30T18:14:24Z

src/k2/configurator/Configurator.cpp

+                K2LOG_I(log::configurator, "Received set for key: {}", request.key);
+                auto key = request.key;
+                auto value = request.value;
+


We probably need to validate the incoming value here before storing it in the config. you should check the boost map API to see if it does any config-related validation. If not, we should do it ourselves and return the appropriate status back to the RPC caller.

For log-related config, we need to apply the new value(for both set/delete) to the log system. The logic for that is here: https://github.com/futurewei-cloud/chogori-platform/blob/master/src/k2/appbase/Appbase.cpp#L81 and we may have to refactor it to reuse it.

You can also avoid code duplication like so:

return [request=std::move(request)] { auto applier = [request] (auto& config) { config.emplace(key, boost::program_options::variable_value(value, "")); boost::program_options::notify(config); return seastar::make_ready_future(); }; if (request.applyToAll) { return ConfigDist().invoke_on_all(applier); } return applier(); }() .then([] { return RPCResponse(Statuses::S201_Created("ConfigSet accepted"), ConfigSetResponse{}); });

iafuture · 2023-01-30T18:35:55Z

src/k2/configurator/Configurator.cpp

+                auto key = request.key;
+                auto value = request.value;
+
+                if (request.applyToAll) {


On a further thought, I think the validation and application to config should be delegated to the config here: https://github.com/futurewei-cloud/chogori-platform/blob/master/src/k2/config/Config.h

We should probably add the set/delete methods there which can also hide the all/local dispatch

Adding configurator server and client side

e5280cf

mankan1 requested review from iafuture and jfunston January 26, 2023 22:58

Manoj Kandlikar added 6 commits January 27, 2023 13:38

added integration test

97e0f33

removed configurator_client.cp from cmd/demo

081ceeb

increased MAX_VERB to 253

38a5f28

fixes build errors

5fe378d

changed permission on test_configurator.sh to 755

2fa712f

corrected path in shell script for configurator

6042f86

jfunston suggested changes Jan 30, 2023

View reviewed changes

iafuture reviewed Jan 30, 2023

View reviewed changes

iafuture suggested changes Jan 30, 2023

View reviewed changes


		namespace k2 {

		struct DELETE_ConfiguratorRequest {

Conversation

mankan1 commented Jan 26, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants