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

["Alexander Tikhonov" <avtikhon@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 5048 bytes --]

Alexander, thanks a lot for a deep review, I've answered the questions below.


>Среда, 19 февраля 2020, 0:41 +03:00 от 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. 
Right, thanks a lot.
>
>
>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. 
Right, that is the only change.
>
>  - 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?). 
No, there is no any need to have separate IMAGE_PERF images, due to it
has only built benchmarks w/o depends to Tarantool sources.
>
>- 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. 
Right, it needs special Dockerfile w/o SQL benchmarks, I already have
separate 'avtikhon/gitlab-ci_1.10-perf' branch for it, I'll update it with changes
from the current commit.
>
>
>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'. 
Ok.
>
>
>>
>> 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. 
Right, thanks.
>
>
>> 
>> Perfomance testing uses docker images, built
>
>Fixed typo: 'Perfomance'.
Right, thanks.
>
>> 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). 
Ok.
>
>
>> 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. 
These variables are in use for docker registry login at bench-run scripts,
these variables recreating each job run by gitlab-ci.
>
>* 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. 
Gitlab-ci initiates in .gitlab-ci.yml jobs running under docker images, so right
here the performance image IMAGE_PERF_BUILT must be set, IMAGE_PERF
is the part of the image IMAGE_PERF_BUILT and it's setup better to have at the
same place as IMAGE_PERF image.
>
>* All those variables have prefix CI_*, not, say, BENCH_RUN_*. 
CI_* are gitlab-ci variables and visible to the bench-run make targets - no need
to setup additional variables.
>
>
>> +# 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. 
I'm ok with your suggestion, let's do it on the next commit iteration.
>
>
>Also I don't see a reason to extract such one-two-liners into a
>gitlab.mk.
Previously we decided not to use .gitlab-ci.yml for code and to use it from
standalone make files, anyway I'm Ok with your suggestion, let's discuss
it a bit.
>
>> +# 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'.
Ok, the same answer as above.

-- 
Alexander Tikhonov

[-- Attachment #2: Type: text/html, Size: 9052 bytes --]