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

[Vladislav Shpilevoy <v.shpilevoy@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.