cmake option to remove feedback daemon #3308
Comments
Totktonada
added a commit
to tarantool/gentoo-overlay
that referenced
this issue
Apr 3, 2018
The progress re adding cmake parameter for feedback daemon removing is tracked by [1]. [1]: tarantool/tarantool#3308
|
Please use a configuration time default, feedback_daemon=true/false, not a compile-time option. You can put a different config to /etc/default or /etc/tarantool/instances.available |
|
I’m against of delivering spying code to Gentoo even when it is disabled (one cannot statically check whether it is really so). Now several sed / rm commads removes the daemon on src_prepare stage. I’ll maintain that approach externally. |
Gerold103
added a commit
that referenced
this issue
Apr 12, 2020
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
Gerold103
added a commit
that referenced
this issue
Apr 12, 2020
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
Gerold103
added a commit
that referenced
this issue
Apr 12, 2020
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
Gerold103
added a commit
that referenced
this issue
Apr 12, 2020
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
Gerold103
added a commit
that referenced
this issue
Apr 12, 2020
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
Gerold103
added a commit
that referenced
this issue
Apr 12, 2020
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
Gerold103
added a commit
that referenced
this issue
Apr 15, 2020
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
Gerold103
added a commit
that referenced
this issue
Apr 15, 2020
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
Gerold103
added a commit
that referenced
this issue
Apr 15, 2020
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
kyukhin
pushed a commit
that referenced
this issue
Apr 17, 2020
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
kyukhin
pushed a commit
that referenced
this issue
Apr 17, 2020
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
avtikhon
added a commit
that referenced
this issue
Apr 17, 2020
Found that some package builds failed on the mistake in CMakeLists.txt file, the failed packages and test builds were: - CentOS 6 - CentOS 7 - Ubuntu 14.04 and static build based on Dockerfile. The core of the issue appeared to be single backslash instead of double at the comment in CMakeLists.txt file, which should pass the cmake syntax rules. Follow up #3308
Totktonada
added a commit
to tarantool/gentoo-overlay
that referenced
this issue
Apr 17, 2020
tarantool-2.4.0-231-g7b443650d changes src/box/CMakeLists.txt (see [1]), we should reflect it here. Now -DENABLE_FEEDBACK_DAEMON=[ON/OFF] is added (see [2]), but there are still versions with this option or without it. Keep the code for removing feedback daemon code for now. [1]: tarantool/tarantool@7b44365 [2]: tarantool/tarantool#3308
Totktonada
pushed a commit
that referenced
this issue
Apr 18, 2020
Found that some package builds failed on the mistake in CMakeLists.txt file, the failed packages and test builds were: - CentOS 6 - CentOS 7 - Ubuntu 14.04 and static build based on Dockerfile. The core of the issue is that CMake 2 does not support line continuation with backslash. The commit fixes the regression from 7b44365 ('feedback: add cmake option to disable the daemon'). Follow up #3308
|
We already have a global config file /etc/default/tarantool content--
-- System-wide settings for tarantoolctl and init scripts
--
-- This file was configured by the package maintainers and you probably
-- don't want to change it. Please complain about your custom configuration
-- directly to upstream's bug tracker rather than to your distro.
--
-- Settings below should be kept in sync with:
--
-- * logrotate configuration
-- * tarantool.service unit
-- * systemd-tmpfiles configuration
-- * directory structure and permissions
--
default_cfg = {
pid_file = "/var/run/tarantool", -- /var/run/tarantool/${INSTANCE}.pid
wal_dir = "/var/lib/tarantool", -- /var/lib/tarantool/${INSTANCE}/
memtx_dir = "/var/lib/tarantool", -- /var/lib/tarantool/${INSTANCE}
vinyl_dir = "/var/lib/tarantool", -- /var/lib/tarantool/${INSTANCE}
log = "/var/log/tarantool", -- /var/log/tarantool/${INSTANCE}.log
username = "tarantool",
language = "lua",
}
-- instances.available - all available instances
-- instances.enabled - instances to autostart by sysvinit
instance_dir = "/etc/tarantool/instances.enabled"
-- vim: set ft=lua :The file contains for example paths, languages, etc. So we could:
|
|
I vote for keeping the CMake option. Sooner or later I'll use it to exclude the feedback daemon code from the executable on Gentoo instead hacking with sed. As the maintainer of the Gentoo overlay I insist on excluding this code at all from the executable if a user don't set the Anyway, it does not block the runtime option in any way. I guess we should discuss the runtime option in #6395. In brief, the |
Totktonada
added a commit
to tarantool/gentoo-overlay
that referenced
this issue
Dec 12, 2021
Kept patching for versions older than 2.4.0.231. The main reason here is that it becomes hard to support this code working. The last time it fails after [1]. See [2] regarding -DENABLE_FEEDBACK_DAEMON=OFF. [1]: tarantool/tarantool@74e8c96 [2]: tarantool/tarantool#3308
Totktonada
added a commit
to tarantool/gentoo-overlay
that referenced
this issue
Dec 12, 2021
Kept patching for versions older than 2.4.0.231. The main reason here is that it becomes hard to support this code working. The last time it fails after [1]. See [2] regarding -DENABLE_FEEDBACK_DAEMON=OFF. [1]: tarantool/tarantool@74e8c96 [2]: tarantool/tarantool#3308
Totktonada
added a commit
to Totktonada/tarantool
that referenced
this issue
Aug 3, 2022
It is convenient to fast check, whether the option was enabled or disabled. Especially, when cmake is called indirectly, say, by a package manager on Gentoo. Follows up tarantool#3308 NO_DOC=quite minor build process change NO_TEST=has no relation to tarantool behavior NO_CHANGELOG=see NO_DOC
Totktonada
added a commit
to Totktonada/tarantool
that referenced
this issue
Aug 4, 2022
It is counter-intuitive to see options of a component that is disabled at build time. Especially, when the returned value means that the component is enabled (while it is not so). Before this patch (on `-DENABLE_FEEDBACK_DAEMON=OFF` build): ```yaml tarantool> box.cfg() tarantool> box.cfg.feedback_enabled --- - true ... ``` After this patch (on `-DENABLE_FEEDBACK_DAEMON=OFF` build): ```yaml tarantool> box.cfg() tarantool> box.cfg.feedback_enabled --- - null ... ``` NB: The following test cases in cartridge are failed with `-DENABLE_FEEDBACK_DAEMON=OFF` (as before as well as after the patch): * integration.feedback.test_feedback * integration.feedback.test_rocks Since they verify cartridge's additions for the feedback daemon, it is expected outcome of disabling the component entirely. Ideally we should conditionally disable those test cases, but it is out of scope here. Follows up tarantool#3308 NO_DOC=I think it is expected behavior and unlikely it requires any change in the documentation NO_TEST=a test would verify behavior of the particular build type, but we have no such configuration in CI, so the test would be pretty useless NO_CHANGELOG=seems too minor to highlight it for users
igormunkin
pushed a commit
that referenced
this issue
Aug 9, 2022
It is convenient to fast check, whether the option was enabled or disabled. Especially, when cmake is called indirectly, say, by a package manager on Gentoo. Follows up #3308 NO_DOC=quite minor build process change NO_TEST=has no relation to tarantool behavior NO_CHANGELOG=see NO_DOC
igormunkin
pushed a commit
that referenced
this issue
Aug 9, 2022
It is counter-intuitive to see options of a component that is disabled at build time. Especially, when the returned value means that the component is enabled (while it is not so). Before this patch (on `-DENABLE_FEEDBACK_DAEMON=OFF` build): ```yaml tarantool> box.cfg() tarantool> box.cfg.feedback_enabled --- - true ... ``` After this patch (on `-DENABLE_FEEDBACK_DAEMON=OFF` build): ```yaml tarantool> box.cfg() tarantool> box.cfg.feedback_enabled --- - null ... ``` NB: The following test cases in cartridge are failed with `-DENABLE_FEEDBACK_DAEMON=OFF` (as before as well as after the patch): * integration.feedback.test_feedback * integration.feedback.test_rocks Since they verify cartridge's additions for the feedback daemon, it is expected outcome of disabling the component entirely. Ideally we should conditionally disable those test cases, but it is out of scope here. Follows up #3308 NO_DOC=I think it is expected behavior and unlikely it requires any change in the documentation NO_TEST=a test would verify behavior of the particular build type, but we have no such configuration in CI, so the test would be pretty useless NO_CHANGELOG=seems too minor to highlight it for users
mkokryashkin
pushed a commit
to mkokryashkin/tarantool
that referenced
this issue
Sep 9, 2022
It is convenient to fast check, whether the option was enabled or disabled. Especially, when cmake is called indirectly, say, by a package manager on Gentoo. Follows up tarantool#3308 NO_DOC=quite minor build process change NO_TEST=has no relation to tarantool behavior NO_CHANGELOG=see NO_DOC
mkokryashkin
pushed a commit
to mkokryashkin/tarantool
that referenced
this issue
Sep 9, 2022
It is counter-intuitive to see options of a component that is disabled at build time. Especially, when the returned value means that the component is enabled (while it is not so). Before this patch (on `-DENABLE_FEEDBACK_DAEMON=OFF` build): ```yaml tarantool> box.cfg() tarantool> box.cfg.feedback_enabled --- - true ... ``` After this patch (on `-DENABLE_FEEDBACK_DAEMON=OFF` build): ```yaml tarantool> box.cfg() tarantool> box.cfg.feedback_enabled --- - null ... ``` NB: The following test cases in cartridge are failed with `-DENABLE_FEEDBACK_DAEMON=OFF` (as before as well as after the patch): * integration.feedback.test_feedback * integration.feedback.test_rocks Since they verify cartridge's additions for the feedback daemon, it is expected outcome of disabling the component entirely. Ideally we should conditionally disable those test cases, but it is out of scope here. Follows up tarantool#3308 NO_DOC=I think it is expected behavior and unlikely it requires any change in the documentation NO_TEST=a test would verify behavior of the particular build type, but we have no such configuration in CI, so the test would be pretty useless NO_CHANGELOG=seems too minor to highlight it for users
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
It would be better to pack tarantool for distros w/o external patches (because these patches will become broken from time to time).
Pack unconditionally with 'spying' stuff is not an option at least for Gentoo (but okay to enable it under a default-disabled flag). So, should I going to maintain patchset upward of tarantool or wait for the cmake flag?
The text was updated successfully, but these errors were encountered: