Re: [Tarantool-patches] [PATCH 1/2] feedback: determine runtime platform info

["Илья Конюхов" <runsfor@gmail.com>]

Hi, thanks for your review!

> On 7 Jun 2020, at 19:45, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> 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.

Got it.

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

Done. I’ve adjusted ifs and simplified function code a bit.

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

Thanks for a suggestion about caching. I’ve splitted the function into two parts and added caching part. Now file reading will be invoked only once.

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

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
 
-local function fill_in_feedback(feedback)
+local function detect_docker_environment_impl()
+    local fh = 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 = 4096
+    local s = 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 == nil then
+        cached_detect_docker_env = detect_docker_environment_impl()
+    end
+
+    return cached_detect_docker_env
+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()
+end
+
+local function fill_in_feedback(feedback)
+    fill_in_base_info(feedback)
+    fill_in_platform_info(feedback)
+
     return feedback
 end