[Tarantool-patches] [PATCH 4/4] feedback: add cmake option to disable the daemon
Serge Petrenko
sergepetrenko at tarantool.org
Wed Apr 15 20:28:31 MSK 2020
Hi! Thanks for the patchset!
All LGTM except one comment below.
> 12 апр. 2020 г., в 23:13, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> написал(а):
>
> There is a complaint that the feedback daemon is a 'spying' tool
> and because of that can't be used on Gentoo. Its default disabled
> option also is not acceptable, the daemon should be eliminated
> completely.
>
> The patch introduces cmake option ENABLE_FEEDBACK_DAEMON. It is
> ON by default. When set to OFF, all feedback daemon's code is not
> included into the binary, its configuration options disappear.
>
> Closes #3308
> ---
> src/box/CMakeLists.txt | 10 ++++++++-
> src/box/lua/init.c | 9 +++++++++
> src/box/lua/load_cfg.lua | 29 +++++++++++++++++++++------
> src/trivia/config.h.cmake | 2 ++
> test/box-tap/feedback_daemon.test.lua | 23 +++++++++++++++++----
> test/box/misc.result | 19 ++++++++++++++++--
> test/box/misc.test.lua | 15 +++++++++++++-
> 7 files changed, 93 insertions(+), 14 deletions(-)
>
> diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
> index 56758bd2f..4827e786e 100644
> --- a/src/box/CMakeLists.txt
> +++ b/src/box/CMakeLists.txt
> @@ -1,5 +1,11 @@
> file(MAKE_DIRECTORY ${CMAKE_BINARY_DIR}/src/box/lua)
>
> +# Sometimes 'spying' code is not acceptable even if it would be
> +# disabled by default. That option allows to remove the feedback
> +# daemon from the build completely.
> +option(ENABLE_FEEDBACK_DAEMON "Feedback daemon which reports debug data to \
> + the Tarantool team" ON)
> +
> add_subdirectory(sql)
>
> set(lua_sources)
> @@ -7,7 +13,9 @@ lua_source(lua_sources lua/load_cfg.lua)
> lua_source(lua_sources lua/schema.lua)
> lua_source(lua_sources lua/tuple.lua)
> lua_source(lua_sources lua/session.lua)
> -lua_source(lua_sources lua/feedback_daemon.lua)
> +if (ENABLE_FEEDBACK_DAEMON)
> + lua_source(lua_sources lua/feedback_daemon.lua)
> +endif()
> lua_source(lua_sources lua/net_box.lua)
> lua_source(lua_sources lua/upgrade.lua)
> lua_source(lua_sources lua/console.lua)
> diff --git a/src/box/lua/init.c b/src/box/lua/init.c
> index 9db740de6..29217c21e 100644
> --- a/src/box/lua/init.c
> +++ b/src/box/lua/init.c
> @@ -71,7 +71,9 @@ extern char session_lua[],
> schema_lua[],
> load_cfg_lua[],
> xlog_lua[],
> +#if ENABLE_FEEDBACK_DAEMON
> feedback_daemon_lua[],
> +#endif
> net_box_lua[],
> upgrade_lua[],
> console_lua[],
> @@ -81,7 +83,14 @@ static const char *lua_sources[] = {
> "box/session", session_lua,
> "box/tuple", tuple_lua,
> "box/schema", schema_lua,
> +#if ENABLE_FEEDBACK_DAEMON
> + /*
> + * It is important to initialize the daemon before
> + * load_cfg, because the latter peeks up some values
Peeks -> Picks ?
> + * from the feedback daemon.
> + */
> "box/feedback_daemon", feedback_daemon_lua,
> +#endif
> "box/upgrade", upgrade_lua,
> "box/net_box", net_box_lua,
> "box/console", console_lua,
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index b671eb7a2..5d818addf 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -21,6 +21,20 @@ local function locked(f)
> end
> end
>
> +--
> +-- When feedback is disabled, every single mentioning of it should
> +-- be eliminated. Even box.cfg{} should not accept any
> +-- 'feedback_*' parameters as valid. This is why they are set to
> +-- nil, when the daemon does not exist.
> +--
> +local function ifdef_feedback(value)
> + return private.feedback_daemon ~= nil and value or nil
> +end
> +
> +local ifdef_feedback_set_params =
> + private.feedback_daemon ~= nil and
> + private.feedback_daemon.set_feedback_params or nil
> +
> -- all available options
> local default_cfg = {
> listen = nil,
> @@ -143,9 +157,9 @@ local template_cfg = {
> replication_connect_quorum = 'number',
> replication_skip_conflict = 'boolean',
> replication_anon = 'boolean',
> - feedback_enabled = 'boolean',
> - feedback_host = 'string',
> - feedback_interval = 'number',
> + feedback_enabled = ifdef_feedback('boolean'),
> + feedback_host = ifdef_feedback('string'),
> + feedback_interval = ifdef_feedback('number'),
> net_msg_max = 'number',
> sql_cache_size = 'number',
> }
> @@ -236,9 +250,9 @@ local dynamic_cfg = {
> checkpoint_interval = private.cfg_set_checkpoint_interval,
> checkpoint_wal_threshold = private.cfg_set_checkpoint_wal_threshold,
> worker_pool_threads = private.cfg_set_worker_pool_threads,
> - feedback_enabled = private.feedback_daemon.set_feedback_params,
> - feedback_host = private.feedback_daemon.set_feedback_params,
> - feedback_interval = private.feedback_daemon.set_feedback_params,
> + feedback_enabled = ifdef_feedback_set_params,
> + feedback_host = ifdef_feedback_set_params,
> + feedback_interval = ifdef_feedback_set_params,
> -- do nothing, affects new replicas, which query this value on start
> wal_dir_rescan_delay = function() end,
> custom_proc_title = function()
> @@ -258,6 +272,9 @@ local dynamic_cfg = {
> sql_cache_size = private.cfg_set_sql_cache_size,
> }
>
> +ifdef_feedback = nil
> +ifdef_feedback_set_params = nil
> +
> --
> -- For some options it is important in which order they are set.
> -- For example, setting 'replication', including self, before
> diff --git a/src/trivia/config.h.cmake b/src/trivia/config.h.cmake
> index 226f83128..89e0d39c6 100644
> --- a/src/trivia/config.h.cmake
> +++ b/src/trivia/config.h.cmake
> @@ -251,6 +251,8 @@
> /* Cacheline size to calculate alignments */
> #define CACHELINE_SIZE 64
>
> +#cmakedefine ENABLE_FEEDBACK_DAEMON 1
> +
> /*
> * vim: syntax=c
> */
> diff --git a/test/box-tap/feedback_daemon.test.lua b/test/box-tap/feedback_daemon.test.lua
> index 9e6c59efc..d4adb71f1 100755
> --- a/test/box-tap/feedback_daemon.test.lua
> +++ b/test/box-tap/feedback_daemon.test.lua
> @@ -8,8 +8,6 @@ local fiber = require('fiber')
> local test = tap.test('feedback_daemon')
> local socket = require('socket')
>
> -test:plan(11)
> -
> box.cfg{log = 'report.log', log_level = 6}
>
> local feedback_save = nil
> @@ -49,10 +47,27 @@ local server = socket.tcp_server("127.0.0.1", 0, http_handle)
> local port = server:name().port
>
> local interval = 0.01
> -box.cfg{
> +local ok, err = pcall(box.cfg, {
> feedback_host = string.format("127.0.0.1:%i", port),
> feedback_interval = interval,
> -}
> +})
> +
> +if not ok then
> + --
> + -- gh-3308: feedback daemon is an optional pre-compile time
> + -- defined feature, depending on CMake flags. It is not
> + -- present always. When it is disabled, is should not be
> + -- visible anywhere.
> + --
> + test:plan(2)
> + test:diag('Feedback daemon is supposed to be disabled')
> + test:like(err, 'unexpected option', 'Feedback options are undefined')
> + test:isnil(box.feedback, 'No feedback daemon - no its API')
> + test:check()
> + os.exit(0)
> +end
> +
> +test:plan(11)
>
> local function check(message)
> while feedback_count < 1 do
> diff --git a/test/box/misc.result b/test/box/misc.result
> index 9b76d7665..10699adc2 100644
> --- a/test/box/misc.result
> +++ b/test/box/misc.result
> @@ -49,8 +49,24 @@ index = space:create_index('primary', { type = 'hash' })
> ---
> - ' lua says: hello'
> ...
> +--
> +-- gh-3308: feedback daemon is an optional pre-compile time
> +-- defined feature, depending on CMake flags. It is not present
> +-- always.
> +--
> +optional = {feedback = true}
> +---
> +...
> -- # What's in the box?
> -t = {} for n in pairs(box) do table.insert(t, tostring(n)) end table.sort(t)
> +t = {}
> +---
> +...
> +for n in pairs(box) do \
> + if not optional[n] then \
> + table.insert(t, tostring(n)) \
> + end \
> +end \
> +table.sort(t)
> ---
> ...
> t
> @@ -64,7 +80,6 @@ t
> - ctl
> - error
> - execute
> - - feedback
> - func
> - index
> - info
> diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
> index 350f9f75d..4d7fb0a16 100644
> --- a/test/box/misc.test.lua
> +++ b/test/box/misc.test.lua
> @@ -17,8 +17,21 @@ index = space:create_index('primary', { type = 'hash' })
> -- Test Lua from admin console. Whenever producing output,
> -- make sure it's a valid YAML.
> ' lua says: hello'
> +
> +--
> +-- gh-3308: feedback daemon is an optional pre-compile time
> +-- defined feature, depending on CMake flags. It is not present
> +-- always.
> +--
> +optional = {feedback = true}
> -- # What's in the box?
> -t = {} for n in pairs(box) do table.insert(t, tostring(n)) end table.sort(t)
> +t = {}
> +for n in pairs(box) do \
> + if not optional[n] then \
> + table.insert(t, tostring(n)) \
> + end \
> +end \
> +table.sort(t)
> t
> t = nil
>
> --
> 2.21.1 (Apple Git-122.3)
>
--
Serge Petrenko
sergepetrenko at tarantool.org
More information about the Tarantool-patches
mailing list