Re: [Tarantool-patches] [PATCH 4/4] feedback: add cmake option to disable the daemon

[Serge Petrenko <sergepetrenko@tarantool.org>]

Hi! Thanks for the patchset!
All LGTM except one comment below.

> 12 апр. 2020 г., в 23:13, Vladislav Shpilevoy <v.shpilevoy@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@tarantool.org