Re: [Tarantool-patches] [PATCH v4] Implement perf testing at gitlab-ci

[Alexander Turenko <alexander.turenko@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'.