<HTML><BODY>Alexander, thanks a lot for a deep review, I've answered the questions below.<br><br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Среда, 19 февраля 2020, 0:41 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:<br><br><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15820620700569992170_BODY">I still think that details might be better, but okay, I see: you need<br>
some base now to proceed further. Don't want to block it anymore.</div></div></div></div></blockquote>Right, thanks a lot.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15820620700569992170_BODY"><br><br>
I commented the patch below, but didn't perform any changes except a bit<br>
reworded commit message.<br><br>
Pushed to master. CCed Kirill.<br><br>
Don't sure how it should look at other release branches:<br><br>
- Whether something need to be changed for 2.3/2.2?<br>
- perf_only_template should have "2.3" / "2.2" branch instead of<br>
master, that I understood.</div></div></div></div></blockquote>Right, that is the only change.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15820620700569992170_BODY"><br>
- Should IMAGE_PERF be tagged as "perf_2.3" / "perf_2.2" instead of<br>
"perf_master"? Are benchmarks should be adjusted for those versions<br>
and should this lead to such separation base images (or will be<br>
handled at runtime with bench-run scripts?).</div></div></div></div></blockquote>No, there is no any need to have separate IMAGE_PERF images, due to it<br>has only built benchmarks w/o depends to Tarantool sources.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15820620700569992170_BODY"><br>
- Whether something need to be changed for 1.10?<br>
- At least SQL benchmarks will not work. Should it be handled here or<br>
they will be skipped on bench-run side?<br>
- Same question re IMAGE_PERF as above.</div></div></div></div></blockquote>Right, it needs special Dockerfile w/o SQL benchmarks, I already have<br>separate 'avtikhon/gitlab-ci_1.10-perf' branch for it, I'll update it with changes<br>from the current commit.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15820620700569992170_BODY"><br><br>
Let's elaborate those questions. After this we can push it downward.<br><br>
WBR, Alexander Turenko.<br><br>
> Implement perf testing at gitlab-ci<br><br>
Changed to: 'gitlab-ci: enable performance testing'.</div></div></div></div></blockquote>Ok.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15820620700569992170_BODY"><br><br>
><br>
> Enabled Tarantool performance testing on Gitlab-CI<br>
> for release/master branches and "*-perf" named branches.<br>
> For this purpose 'perf' and 'cleanup' stages were added<br>
> into Gitlab-CI pipeline.<br>
> <br>
> Performance testing support next benchmarks:<br>
> - cbench<br>
> - linkbench<br>
> - nosqlbench (hash and tree Tarantool run modes)<br>
> - sysbench<br>
> - tpcc<br>
> - ycsb (hash and tree Tarantool run modes)<br>
> <br>
> Benchmarks use scripts from repository:<br>
> <a href="http://gitlab.com/tarantool/bench-run" target="_blank">http://gitlab.com/tarantool/bench-run</a><br><br>
Dead link. Changed gitlab.com to github.com.</div></div></div></div></blockquote>Right, thanks.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15820620700569992170_BODY"><br><br>
> <br>
> Perfomance testing uses docker images, built<br><br>
Fixed typo: 'Perfomance'.<br></div></div></div></div></blockquote>Right, thanks.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15820620700569992170_BODY"><br>
> with docker files from bench-run repository:<br>
> - perf/ubuntu-bionic:perf_master<br>
> parent image with benchmarks only<br>
> - perf_tmp/ubuntu-bionic:perf_<commit_SHA><br>
> child images used for testing Tarantool sources<br><br>
Formatted a bit (to fit 72 symbols, but not much less).</div></div></div></div></blockquote>Ok.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15820620700569992170_BODY"><br><br>
> Github: <a href="https://github.com/tarantool/tarantool/tree/avtikhon/gitlab-ci-perf" target="_blank">https://github.com/tarantool/tarantool/tree/avtikhon/gitlab-ci-perf</a><br><br>
> +.perf_only_template: &perf_only_definition<br>
> + only:<br>
> + - master<br>
> + - /^.*-perf$/<br>
> + variables: &perf_vars_definition<br>
> + IMAGE_PERF: "${CI_REGISTRY}/${CI_PROJECT_PATH}/perf/ubuntu-bionic:perf_master"<br>
> + IMAGE_PERF_BUILT: "${CI_REGISTRY}/${CI_PROJECT_PATH}/perf_tmp/ubuntu-bionic:perf_${CI_COMMIT_SHORT_SHA}"<br>
> +<br><br>
The resulting bench-run API looks strage for me:<br><br>
* It expects that a caller will set CI_REGISTRY, CI_REGISTRY_USER,<br>
CI_REGISTRY_PASSWORD environment variables, which come from GitLab-CI,<br>
but can be set manually.</div></div></div></div></blockquote>These variables are in use for docker registry login at bench-run scripts,<br>these variables recreating each job run by gitlab-ci.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15820620700569992170_BODY"><br>
* However it does not use CI_REGISTRY, CI_PROJECT_PATH,<br>
CI_COMMIT_SHORT_SHA to choose images name on its own, but expect<br>
IMAGE_PERF and IMAGE_PERF_BUILT from a caller.</div></div></div></div></blockquote>Gitlab-ci initiates in .gitlab-ci.yml jobs running under docker images, so right<br>here the performance image IMAGE_PERF_BUILT must be set, IMAGE_PERF<br>is the part of the image IMAGE_PERF_BUILT and it's setup better to have at the<br>same place as IMAGE_PERF image.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15820620700569992170_BODY"><br>
* All those variables have prefix CI_*, not, say, BENCH_RUN_*.</div></div></div></div></blockquote>CI_* are gitlab-ci variables and visible to the bench-run make targets - no need<br>to setup additional variables.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15820620700569992170_BODY"><br><br>
> +# Pre-testing part<br>
> +<br>
> +perf_bootstrap:<br>
> + <<: *perf_only_definition<br>
> + stage: test<br>
> + tags:<br>
> + - perf<br>
> + script:<br>
> + - ${GITLAB_MAKE} perf_prepare<br><br>
There is no reason to use two terms for the same thing: bootstrap and<br>
prepare.</div></div></div></div></blockquote>I'm ok with your suggestion, let's do it on the next commit iteration.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15820620700569992170_BODY"><br><br>
Also I don't see a reason to extract such one-two-liners into a<br>
gitlab.mk.<br></div></div></div></div></blockquote>Previously we decided not to use .gitlab-ci.yml for code and to use it from<br>standalone make files, anyway I'm Ok with your suggestion, let's discuss<br>it a bit.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15820620700569992170_BODY"><br>
> +# Post-testing part<br>
> +<br>
> +remove_images:<br>
> + <<: *perf_only_definition<br>
> + stage: cleanup<br>
> + when: always<br>
> + tags:<br>
> + - perf<br>
> + script:<br>
> + - ${GITLAB_MAKE} perf_cleanup<br>
> +<br><br>
Same as above: there is no reason to name it both 'remove_images' and<br>
'perf_cleanup'.<br></div></div></div></div></blockquote>
Ok, the same answer as above.<br>
<br>-- <br>Alexander Tikhonov<br></BODY></HTML>