From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <alexander.turenko@tarantool.org>
Received: from smtp5.mail.ru (smtp5.mail.ru [94.100.179.24])
 (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 23E4C469719
 for <tarantool-patches@dev.tarantool.org>;
 Wed, 19 Feb 2020 00:41:11 +0300 (MSK)
Date: Wed, 19 Feb 2020 00:41:30 +0300
From: Alexander Turenko <alexander.turenko@tarantool.org>
Message-ID: <20200218214130.h2wilrsmf2zaku25@tkn_work_nb>
References: <c7d3c933ea4a41ac07dd27fc3c59e28ec1a8bc4e.1581947950.git.avtikhon@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <c7d3c933ea4a41ac07dd27fc3c59e28ec1a8bc4e.1581947950.git.avtikhon@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v4] Implement perf testing at
	gitlab-ci
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: "Alexander V. Tikhonov" <avtikhon@tarantool.org>
Cc: Oleg Piskunov <o.piskunov@tarantool.org>, tarantool-patches@dev.tarantool.org

I still think that details might be better, but okay, I see: you need
some base now to proceed further. Don't want to block it anymore.

I commented the patch below, but didn't perform any changes except a bit
reworded commit message.

Pushed to master. CCed Kirill.

Don't sure how it should look at other release branches:

- Whether something need to be changed for 2.3/2.2?
  - perf_only_template should have "2.3" / "2.2" branch instead of
    master, that I understood.
  - Should IMAGE_PERF be tagged as "perf_2.3" / "perf_2.2" instead of
    "perf_master"? Are benchmarks should be adjusted for those versions
    and should this lead to such separation base images (or will be
    handled at runtime with bench-run scripts?).
- Whether something need to be changed for 1.10?
  - At least SQL benchmarks will not work. Should it be handled here or
    they will be skipped on bench-run side?
  - Same question re IMAGE_PERF as above.

Let's elaborate those questions. After this we can push it downward.

WBR, Alexander Turenko.

> Implement perf testing at gitlab-ci

Changed to: 'gitlab-ci: enable performance testing'.

>
> Enabled Tarantool performance testing on Gitlab-CI
> for release/master branches and "*-perf" named branches.
> For this purpose 'perf' and 'cleanup' stages were added
> into Gitlab-CI pipeline.
> 
> Performance testing support next benchmarks:
> - cbench
> - linkbench
> - nosqlbench (hash and tree Tarantool run modes)
> - sysbench
> - tpcc
> - ycsb (hash and tree Tarantool run modes)
> 
> Benchmarks use scripts from repository:
>   http://gitlab.com/tarantool/bench-run

Dead link. Changed gitlab.com to github.com.

> 
> Perfomance testing uses docker images, built

Fixed typo: 'Perfomance'.

> with docker files from bench-run repository:
> - perf/ubuntu-bionic:perf_master
>     parent image with benchmarks only
> - perf_tmp/ubuntu-bionic:perf_<commit_SHA>
>     child images used for testing Tarantool sources

Formatted a bit (to fit 72 symbols, but not much less).

> Github: https://github.com/tarantool/tarantool/tree/avtikhon/gitlab-ci-perf

> +.perf_only_template: &perf_only_definition
> +  only:
> +    - master
> +    - /^.*-perf$/
> +  variables: &perf_vars_definition
> +    IMAGE_PERF: "${CI_REGISTRY}/${CI_PROJECT_PATH}/perf/ubuntu-bionic:perf_master"
> +    IMAGE_PERF_BUILT: "${CI_REGISTRY}/${CI_PROJECT_PATH}/perf_tmp/ubuntu-bionic:perf_${CI_COMMIT_SHORT_SHA}"
> +

The resulting bench-run API looks strage for me:

* It expects that a caller will set CI_REGISTRY, CI_REGISTRY_USER,
  CI_REGISTRY_PASSWORD environment variables, which come from GitLab-CI,
  but can be set manually.
* However it does not use CI_REGISTRY, CI_PROJECT_PATH,
  CI_COMMIT_SHORT_SHA to choose images name on its own, but expect
  IMAGE_PERF and IMAGE_PERF_BUILT from a caller.
* All those variables have prefix CI_*, not, say, BENCH_RUN_*.

> +# Pre-testing part
> +
> +perf_bootstrap:
> +  <<: *perf_only_definition
> +  stage: test
> +  tags:
> +    - perf
> +  script:
> +    - ${GITLAB_MAKE} perf_prepare

There is no reason to use two terms for the same thing: bootstrap and
prepare.

Also I don't see a reason to extract such one-two-liners into a
gitlab.mk.

> +# Post-testing part
> +
> +remove_images:
> +  <<: *perf_only_definition
> +  stage: cleanup
> +  when: always
> +  tags:
> +    - perf
> +  script:
> +    - ${GITLAB_MAKE} perf_cleanup
> +

Same as above: there is no reason to name it both 'remove_images' and
'perf_cleanup'.