[Tarantool-patches] [PATCH 0/4] CMake option to remove feedback daemon

[Tarantool-patches] [PATCH 0/4] CMake option to remove feedback daemon

[Vladislav Shpilevoy]

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 patchset 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.

The issue is closed in the last commit, but there appeared some
side tasks necessary to make it work, these are first 3 commits.

Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-3308-cmake-remove-feedback
Issue: https://github.com/tarantool/tarantool/issues/3308

Vladislav Shpilevoy (4):
  box: improve built-in module load panic message
  feedback: move feedback code to the single file
  box: yield after initial box_cfg() is finished
  feedback: add cmake option to disable the daemon

 src/box/CMakeLists.txt                | 10 ++++++++-
 src/box/box.cc                        | 15 +++++++++++++-
 src/box/lua/feedback_daemon.lua       | 16 +++++++++++++++
 src/box/lua/init.c                    | 13 ++++++++++--
 src/box/lua/load_cfg.lua              | 29 +++++++++++++++++++++------
 src/box/lua/schema.lua                | 17 ----------------
 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 +++++++++++++-
 10 files changed, 125 insertions(+), 34 deletions(-)

-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH 1/4] box: improve built-in module load panic message

[Vladislav Shpilevoy]

Box built-in modules, such as session, tuple, schema, etc, were
loaded using luaL_loadbuffer() + lua_call(). Error of the former
call was handled properly with a panic message describing the
problem.

But if lua_call() failed, it resulted into 'unknown exception' in
main.cc. Not very helpful. Now it is lua_pcall(), and the error
message is included into the panic() message. That helps in debug,
when something is being changed in the box modules.
---
 src/box/lua/init.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index 63e8b8216..9db740de6 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -432,10 +432,10 @@ box_lua_init(struct lua_State *L)
 		const char *modsrc = *(s + 1);
 		const char *modfile = lua_pushfstring(L,
 			"@builtin/%s.lua", modname);
-		if (luaL_loadbuffer(L, modsrc, strlen(modsrc), modfile))
+		if (luaL_loadbuffer(L, modsrc, strlen(modsrc), modfile) != 0 ||
+		    lua_pcall(L, 0, 0, 0) != 0)
 			panic("Error loading Lua module %s...: %s",
 			      modname, lua_tostring(L, -1));
-		lua_call(L, 0, 0);
 		lua_pop(L, 1); /* modfile */
 	}
 
-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH 2/4] feedback: move feedback code to the single file

[Vladislav Shpilevoy]

Feedback daemon's code was located in two files:
box/lua/feedback_daemon.lua and box/lua/schema.lua. That makes
it harder to eliminate the daemon at cmake configuration time.

Now all its code is in one place, in feedback_daemon.lua. Disable
of the daemon's code now is a matter of excluding the Lua file
from source code.

Part of #3308
---
 src/box/lua/feedback_daemon.lua | 16 ++++++++++++++++
 src/box/lua/schema.lua          | 17 -----------------
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua
index ad71a3fe2..95130d696 100644
--- a/src/box/lua/feedback_daemon.lua
+++ b/src/box/lua/feedback_daemon.lua
@@ -4,6 +4,7 @@ local log   = require('log')
 local json  = require('json')
 local fiber = require('fiber')
 local http  = require('http.client')
+local fio = require('fio')
 
 local PREFIX = "feedback_daemon"
 
@@ -131,6 +132,21 @@ setmetatable(daemon, {
     }
 })
 
+box.feedback = {}
+box.feedback.save = function(file_name)
+    if type(file_name) ~= "string" then
+        error("Usage: box.feedback.save(path)")
+    end
+    local feedback = json.encode(daemon.generate_feedback())
+    local fh, err = fio.open(file_name, {'O_CREAT', 'O_RDWR', 'O_TRUNC'},
+                             tonumber('0777', 8))
+    if not fh then
+        error(err)
+    end
+    fh:write(feedback)
+    fh:close()
+end
+
 if box.internal == nil then
     box.internal = { [PREFIX] = daemon }
 else
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 85fcca562..10584494b 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -5,8 +5,6 @@ local msgpack = require('msgpack')
 local msgpackffi = require('msgpackffi')
 local fun = require('fun')
 local log = require('log')
-local fio = require('fio')
-local json = require('json')
 local buffer = require('buffer')
 local session = box.session
 local internal = require('box.internal')
@@ -2711,19 +2709,4 @@ end
 
 setmetatable(box.space, { __serialize = box_space_mt })
 
-box.feedback = {}
-box.feedback.save = function(file_name)
-    if type(file_name) ~= "string" then
-        error("Usage: box.feedback.save(path)")
-    end
-    local feedback = json.encode(box.internal.feedback_daemon.generate_feedback())
-    local fh, err = fio.open(file_name, {'O_CREAT', 'O_RDWR', 'O_TRUNC'},
-        tonumber('0777', 8))
-    if not fh then
-        error(err)
-    end
-    fh:write(feedback)
-    fh:close()
-end
-
 box.NULL = msgpack.NULL
-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH 3/4] box: yield after initial box_cfg() is finished

[Vladislav Shpilevoy]

box.cfg() works in two stages, when called first time - boot the
instance using box_cfg() C++ function, and then configure it.
During booting all non-dynamic parameters are read. Dynamic are
configured mostly afterwards.

Normally there should be a yield between box_cfg() C++ call and
dynamic parameters configuration. It is used by box.ctl.wait_ro()
and box.ctl.wait_rw() Lua calls to catch the instance in read-only
state always before read-write state.

In theory a user should be able to call box.ctl.wait_ro() and
box.ctl.wait_rw() in one fiber, box.cfg() in another, and these
waits would be unblocked one after another.

It works fine now, but only because of, surprisingly, the feedback
daemon. The daemon creates a yield after C++ box_cfg() is
finished, but dynamic parameters are still being applied in
load_cfg.lua. That gives time to catch box.ctl.wait_ro() event.

The thing is that dynamic parameters configuration includes the
daemon's options too. When 'feedback_enable' option is installed
to true, the daemon is started using fiber.create(). That creates
a yield, and gives time to box.ctl.wait_ro() fibers to handle the
event.

When the daemon is disabled or removed, like it is going to happen
in #3308, this trick does not work, and box.ctl.wait_ro() started
before box.cfg() is never triggered.

It could be tested on app-tap/cfg.test.lua with

    box.cfg{}

changed to

    box.cfg{feedback_enabled = false}

Then the test would hang. A test is not patched here, because the
feedback is going to be optionally removed in a next commit, and
the test would become flaky depending on build options.

Needed for #3308
---
 src/box/box.cc | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 0c15ba5e9..891289bf6 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2450,8 +2450,21 @@ box_cfg_xc(void)
 	if (!is_bootstrap_leader)
 		replicaset_sync();
 
-	/* If anyone is waiting for ro mode to go away */
+	/* box.cfg.read_only is not read yet. */
+	assert(box_is_ro());
+	/* If anyone is waiting for ro mode. */
 	fiber_cond_broadcast(&ro_cond);
+	/*
+	 * Yield to let ro condition waiters to handle the event.
+	 * Without yield it may happen there won't be a context
+	 * switch until the ro state is changed again, and as a
+	 * result, some ro waiters may sleep forever. For example,
+	 * when Tarantool is just started, it is expected it will
+	 * enter ro=true state, and then ro=false. Without the
+	 * yield the ro=true event may be lost. This affects
+	 * box.ctl.wait_ro() call.
+	 */
+	fiber_sleep(0);
 }
 
 void
-- 
2.21.1 (Apple Git-122.3)

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

[Vladislav Shpilevoy]

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
+	 * 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)

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

[Serge Petrenko]

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

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

[Vladislav Shpilevoy]

Hi! Thanks for the review!

>> 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
>> @@ -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 ?

Sure, that is a typo, thanks.

====================
diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index 29217c21e..0a65c3b56 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -86,7 +86,7 @@ static const char *lua_sources[] = {
 #if ENABLE_FEEDBACK_DAEMON
 	/*
 	 * It is important to initialize the daemon before
-	 * load_cfg, because the latter peeks up some values
+	 * load_cfg, because the latter picks up some values
 	 * from the feedback daemon.
 	 */
 	"box/feedback_daemon", feedback_daemon_lua,

====================

Also I found that the ENABLE_FEEDBACK_DAEMON option description
contains multiple whitespaces in a row, because I misunderstood
how multiline strings in CMake work. Here is a fix:

====================
diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index 71ab636b3..309248ed7 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -3,8 +3,8 @@ 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)
+option(ENABLE_FEEDBACK_DAEMON "Feedback daemon which reports debug data to \
+the Tarantool team" ON)
 
 add_subdirectory(sql)
 
====================

Re: [Tarantool-patches] [PATCH 0/4] CMake option to remove feedback daemon

[Vladislav Shpilevoy]

I wasn't sure whether a changelog is needed here, but just
in case:

@ChangeLog
- Using cmake option ENABLE_FEEDBACK_DAEMON it is possible to
  remove all the feedback daemon's code from the final executable
  completely, even configuration options disappear (gh-3308).

Re: [Tarantool-patches] [PATCH 0/4] CMake option to remove feedback daemon

[Kirill Yukhin]

Hello,

On 12 апр 22:13, Vladislav Shpilevoy wrote:
> 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 patchset 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.
> 
> The issue is closed in the last commit, but there appeared some
> side tasks necessary to make it work, these are first 3 commits.
> 
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-3308-cmake-remove-feedback
> Issue: https://github.com/tarantool/tarantool/issues/3308

I've checked your patchset into master.

--
Regards, Kirill Yukhin