From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f193.google.com (mail-lj1-f193.google.com [209.85.208.193]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 92B3A469710 for ; Wed, 10 Jun 2020 02:05:51 +0300 (MSK) Received: by mail-lj1-f193.google.com with SMTP id q19so142191lji.2 for ; Tue, 09 Jun 2020 16:05:51 -0700 (PDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\)) From: =?utf-8?B?0JjQu9GM0Y8g0JrQvtC90Y7RhdC+0LI=?= In-Reply-To: <36bc5d2e-1981-54ed-befa-095debab6805@tarantool.org> Date: Wed, 10 Jun 2020 02:05:44 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <36bc5d2e-1981-54ed-befa-095debab6805@tarantool.org> 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org, alexander.turenko@tarantool.org Hi, thanks for your review! > On 7 Jun 2020, at 19:45, Vladislav Shpilevoy = wrote: >=20 > Hi! Thanks for the patch! >=20 > See 4 comments below. >=20 > 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). >>=20 >> 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. >>=20 >> 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(-) >>=20 >> 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") >>=20 >> # >> # 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. >=20 > 1. Lets better avoid unnecessary diff, not related to the patch > subject. Got it. >=20 >> # >> 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 >>=20 >> -local function fill_in_feedback(feedback) >> +local function detect_docker_environment() >> + local fh =3D fio.open('/proc/1/cgroup', {'O_RDONLY'}) >> + if not fh then return false end >=20 > 2. Please, write conditions and their bodies on > separate lines. In the second patch too. Done. I=E2=80=99ve adjusted ifs and simplified function code a bit. >=20 >> + >> + -- fh:read() doesn't read empty files >> + local big_enough_chunk =3D 4096 >> + local ok =3D 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 ~=3D "running" then >> return nil, "not running" >> end >> feedback.tarantool_version =3D box.info.version >> feedback.server_id =3D box.info.uuid >> feedback.cluster_id =3D box.info.cluster.uuid >> +end >> + >> +local function fill_in_platform_info(feedback) >> + feedback.os =3D jit.os >> + feedback.arch =3D jit.arch >> + feedback.is_docker =3D detect_docker_environment() >=20 > 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. Thanks for a suggestion about caching. I=E2=80=99ve splitted the = function into two parts and added caching part. Now file reading will be = invoked only once. >=20 >> +end >> + >> +local function fill_in_feedback(feedback) >> + fill_in_base_info(feedback) >> + fill_in_platform_info(feedback) >> + >> return feedback >> end >>=20 >> 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() >>=20 >> -test:check() >> -os.exit(0) >> +os.exit(test:check() and 0 or 1) >=20 > 4. Unnecessary diff, not related to the issue. Reverted back diff --git a/src/box/lua/feedback_daemon.lua = b/src/box/lua/feedback_daemon.lua index 95130d696..21e69d511 100644 --- a/src/box/lua/feedback_daemon.lua +++ b/src/box/lua/feedback_daemon.lua @@ -26,13 +26,49 @@ local function get_fiber_id(f) return fid end =20 -local function fill_in_feedback(feedback) +local function detect_docker_environment_impl() + local fh =3D fio.open('/proc/1/cgroup', {'O_RDONLY'}) + if not fh then + return false + end + + -- fh:read() doesn't read empty "proc" files + local big_enough_chunk =3D 4096 + local s =3D fh:read(big_enough_chunk) + fh:close() + + return s and s:find('docker') and true or false +end + +local cached_detect_docker_env + +local function detect_docker_environment() + if cached_detect_docker_env =3D=3D nil then + cached_detect_docker_env =3D detect_docker_environment_impl() + end + + return cached_detect_docker_env +end + +local function fill_in_base_info(feedback) if box.info.status ~=3D "running" then return nil, "not running" end feedback.tarantool_version =3D box.info.version feedback.server_id =3D box.info.uuid feedback.cluster_id =3D box.info.cluster.uuid +end + +local function fill_in_platform_info(feedback) + feedback.os =3D jit.os + feedback.arch =3D jit.arch + feedback.is_docker =3D detect_docker_environment() +end + +local function fill_in_feedback(feedback) + fill_in_base_info(feedback) + fill_in_platform_info(feedback) + return feedback end =20