From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 8892B469710 for ; Sun, 7 Jun 2020 19:45:41 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: <36bc5d2e-1981-54ed-befa-095debab6805@tarantool.org> Date: Sun, 7 Jun 2020 18:45:39 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 1/2] feedback: determine runtime platform info List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ilya Konyukhov , tarantool-patches@dev.tarantool.org Cc: alexander.turenko@tarantool.org Hi! Thanks for the patch! See 4 comments below. On 05/06/2020 10:35, Ilya Konyukhov wrote: > This patch detect which platform instance is running on. > It uses luajit `jit` module to get OS name and architecture. > Se more in [docs page](https://luajit.org/ext_jit.html). > > Also it tries to figure out whether instance is running > inside docker container or not. It's difficult know > accurately but one of the most stable and simple ways > at the same time is to look in > [`/proc/1/cgroup`](https://stackoverflow.com/a/20012536/1881632) > file. > > Closes #3608 > Related to #4943 > --- > cmake/os.cmake | 2 +- > src/box/lua/feedback_daemon.lua | 29 ++++++++++++++++++++++++++- > test/box-tap/feedback_daemon.test.lua | 3 +-- > 3 files changed, 30 insertions(+), 4 deletions(-) > > diff --git a/cmake/os.cmake b/cmake/os.cmake > index 905be61df..462bdccc3 100644 > --- a/cmake/os.cmake > +++ b/cmake/os.cmake > @@ -78,7 +78,7 @@ elseif (${CMAKE_SYSTEM_NAME} STREQUAL "Darwin") > > # > # Default build type is None, which uses depends by Apple > -# command line tools. Also supportting install with MacPorts. > +# command line tools. Also supporting install with MacPorts. 1. Lets better avoid unnecessary diff, not related to the patch subject. > # > if (NOT DARWIN_BUILD_TYPE) > set(DARWIN_BUILD_TYPE None CACHE STRING > diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua > index 95130d696..2ce49fb22 100644 > --- a/src/box/lua/feedback_daemon.lua > +++ b/src/box/lua/feedback_daemon.lua > @@ -26,13 +26,40 @@ local function get_fiber_id(f) > return fid > end > > -local function fill_in_feedback(feedback) > +local function detect_docker_environment() > + local fh = fio.open('/proc/1/cgroup', {'O_RDONLY'}) > + if not fh then return false end 2. Please, write conditions and their bodies on separate lines. In the second patch too. > + > + -- fh:read() doesn't read empty files > + local big_enough_chunk = 4096 > + local ok = fh:read(big_enough_chunk) > + fh:close() > + if not ok then return false end > + > + if not string.find(ok, 'docker') then return false end > + > + return true > +end > + > +local function fill_in_base_info(feedback) > if box.info.status ~= "running" then > return nil, "not running" > end > feedback.tarantool_version = box.info.version > feedback.server_id = box.info.uuid > feedback.cluster_id = box.info.cluster.uuid > +end > + > +local function fill_in_platform_info(feedback) > + feedback.os = jit.os > + feedback.arch = jit.arch > + feedback.is_docker = detect_docker_environment() 3. detect_docker_environment() involves file reading, up to 4KB. On my machine it was 1KB. It would be better to avoid calling it multiple times. I suggest you to call this function only once, and then re-use the result. It can't change anyway. > +end > + > +local function fill_in_feedback(feedback) > + fill_in_base_info(feedback) > + fill_in_platform_info(feedback) > + > return feedback > end > > diff --git a/test/box-tap/feedback_daemon.test.lua b/test/box-tap/feedback_daemon.test.lua > index d4adb71f1..c36b2a694 100755 > --- a/test/box-tap/feedback_daemon.test.lua > +++ b/test/box-tap/feedback_daemon.test.lua > @@ -131,5 +131,4 @@ daemon.start() > daemon.send_test() > daemon.stop() > > -test:check() > -os.exit(0) > +os.exit(test:check() and 0 or 1) 4. Unnecessary diff, not related to the issue.