Re: [Tarantool-patches] [PATCH v1] Correct cleanup gitlab-ci

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

Hi Sergey, thanks for the review, please check my comments bellow.

On Thu, Jun 11, 2020 at 01:50:57PM +0300, Sergey Bronnikov wrote:
> Hi,
> 
> thanks for the patch!
> 
> On 08:55 Thu 11 Jun , Alexander V. Tikhonov wrote:
> > Found the issue on regular testing hosts:
> >   https://gitlab.com/tarantool/tarantool/-/jobs/577884238#L7
> 
> Could you move all web links in a text to a single place below a text
> and refer to them by number? It would be easier to read in such case.
> For example:
> 
> bla-bla [1]
> 
> 1. https://google.com/
> 

Sure, changed.

> >   Fetching changes...
> >   00:04
> >    Reinitialized existing Git repository in
> >      /home/gitlab-runner/builds/zzyC6hh5/0/tarantool/tarantool/.git/
> >    Checking out 8ff7f32c as ...
> >    warning: failed to remove CMakeFiles/Makefile.cmake
> > 
> > Found the job that saved the directories with root permissions
> > https://gitlab.com/tarantool/tarantool/-/jobs/577768553
> > 
> > The issue appeared because the job that saved directories with root
> > permissions used the 'shell' runner to run docker container inside.
> > It caused the gitlab-runner to run the default workspace cleanup
> > outside the docker container. In opposite to it, when 'docker' runner
> > is used, the cleanup routine runs inside the docker container and no
> > fails ever exist, because the root permissions are used in the docker
> > container and this is the same root permissions for the host. As the
> > result using 'shell' runner, cleanup routine failed to remove files
> > created by root inside the docker container and which were shared to
> > global host with the same permissions, because gitlab-runner runs the
> > 'shell' runner by regular 'gitlab-runner' user, but not by root.
> > 
> > To fix the issue need to run docker containers using the gitlab-runner
> > only in RO mode with Out-Of-Source builds in it. Either use the
> > 'docker' runners when docker containers are needed. Anyway 'shell'
> > runner jobs with additional calls to docker containers can't be
> > control for the branches of developers, to avoid of it need to make
> > local cleanup routine instead of default to the working paths for each
> > job with 'shell' runners use.
> > 
> > Decided to setup gitlab-runner configuration as described in:
> > https://docs.gitlab.com/ce/ci/yaml/README.html#git-clean-flags
> > 
> > Also got the issue with left data from previous builds at the
> > submodule pathes, like here:
> >   https://gitlab.com/tarantool/tarantool/-/jobs/574199256#L3141
> > 
> >    Undefined symbols for architecture x86_64:
> >      "_u_isprint_66", referenced from:
> >          _yaml_emitter_is_printable in libyaml_static.a(emitter.c.o)
> >    ld: symbol(s) not found for architecture x86_64
> >    clang: error: linker command failed with exit code 1 (...)
> > 
> > Also got issues with left files from previously tested branches,
> > like here:
> >   https://gitlab.com/tarantool/tarantool/-/jobs/590573606#L3718
> > 
> >  [087] small/rlist.test
> >  [087] TAP13 parse failed (Missing plan in the TAP source).
> >  [087]
> >  [087] No result file (small/rlist.result) found.
> >  [087] Run the test with --update-result option to write the new result file.
> >  [087] [ fail ]
> > 
> >  [087] small/static.test
> >  [087] TAP13 parse failed (Missing plan in the TAP source).
> >  [087]
> >  [087] No result file (small/static.result) found.
> >  [087] Run the test with --update-result option to write the new result file.
> >  [087] [ fail ]
> > 
> > To fix it was added the command to clean all available git submodules:
> >   git submodule foreach git clean -ffdx
> 
> 2. Duplicate '-f' option (here and in a patch)?
> 

Right, this command was taken from gitlab-ci original documentation
  https://docs.gitlab.com/ee/raketasks/cleanup.html

I thought that it could be the mistake and rechecked it manually and
found that options '-fdx' and '-ffdx' have different results: '-ffdx'
options removed more files than '-fdx' before it.

> > 
> > Closes #5036
> > ---
> > 
> > Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-5036-gitlab-cleanup-1.10-full-ci
> > Issue: https://github.com/tarantool/tarantool/issues/5036
> > 
> >  .gitlab-ci.yml | 37 ++++++++++++++++++++++++++++---------
> >  1 file changed, 28 insertions(+), 9 deletions(-)
> > 
> > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> > index cf445c638..592163b08 100644
> > --- a/.gitlab-ci.yml
> > +++ b/.gitlab-ci.yml
> > @@ -5,6 +5,16 @@ stages:
> >  
> >  variables:
> >    GITLAB_MAKE: "make -f .gitlab.mk"
> > +  GIT_CLEAN_COMMAND: "git clean -ffdx -e packpack && git submodule foreach git clean -ffdx && git submodule foreach git status"
> > +  GIT_CLEAN_FLAGS: none
> > +
> > +.shell_before_script_template: &shell_cleanup_script
> > +  before_script:
> > +    - /bin/bash -c "${GIT_CLEAN_COMMAND}"
> 
> 3. Why do you execute docker by specifying binary name and execute bash
> with specifying full path to a binary? If "/bin" is absent in a PATH
> then we can add to it. If full path is mandatory then let's move
> '/bin/bash' to a variables.
> 

Right, we can set bash command w/o full path, but seems that we can't
move the bash call inside the environment, I've tried different variants
and it didn't work.

> > +
> > +.docker_before_script_template: &docker_cleanup_script
> > +  before_script:
> > +    - docker run -w /source -v ${PWD}:/source -i packpack/packpack:el-7 /bin/bash -c "${GIT_CLEAN_COMMAND}"
> >  
> >  # Jobs templates
> >  
> > @@ -43,18 +53,21 @@ variables:
> >    stage: test
> >    tags:
> >      - docker_test
> > +  <<: *shell_cleanup_script
> >  
> >  .docker_test_clang8_template: &docker_test_clang8_definition
> >    image: "${CI_REGISTRY}/${CI_PROJECT_PATH}/testing/debian-buster:latest"
> >    stage: test
> >    tags:
> >      - docker_test
> > +  <<: *shell_cleanup_script
> >  
> >  .pack_template: &pack_definition
> >    <<: *pack_only_definition
> >    stage: test
> >    tags:
> >      - deploy
> > +  <<: *docker_cleanup_script
> >    script:
> >      - ${GITLAB_MAKE} package
> >  
> > @@ -63,6 +76,7 @@ variables:
> >    stage: test
> >    tags:
> >      - deploy_test
> > +  <<: *docker_cleanup_script
> >    script:
> >      - ${GITLAB_MAKE} package
> >  
> > @@ -71,6 +85,7 @@ variables:
> >    stage: test
> >    tags:
> >      - deploy
> > +  <<: *docker_cleanup_script
> >    script:
> >      - ${GITLAB_MAKE} deploy
> >  
> > @@ -79,12 +94,20 @@ variables:
> >    stage: test
> >    tags:
> >      - deploy_test
> > +  <<: *docker_cleanup_script
> >    script:
> >      - ${GITLAB_MAKE} deploy
> >  
> > +.osx_template: &osx_definition
> > +  stage: test
> > +  <<: *shell_cleanup_script
> > +  script:
> > +    - ${GITLAB_MAKE} test_osx
> > +
> >  .vbox_template: &vbox_definition
> >    stage: test
> >    before_script:
> > +    - /bin/bash -c "${GIT_CLEAN_COMMAND}"
> >      - ${GITLAB_MAKE} vms_start
> >    after_script:
> >      - ${GITLAB_MAKE} vms_shutdown
> > @@ -98,6 +121,7 @@ variables:
> >      paths:
> >        - "*_result.txt"
> >        - "*_t_version.txt"
> > +  <<: *shell_cleanup_script
> >    script:
> >      - ${GITLAB_MAKE} perf_run
> >  
> > @@ -152,28 +176,22 @@ release_asan_clang8:
> >  
> >  osx_14_release:
> >    <<: *release_only_definition
> > -  stage: test
> >    tags:
> >      - osx_14
> > -  script:
> > -    - ${GITLAB_MAKE} test_osx
> > +  <<: *osx_definition
> >  
> >  osx_15_release:
> > -  stage: test
> >    tags:
> >      - osx_15
> > -  script:
> > -    - ${GITLAB_MAKE} test_osx
> > +  <<: *osx_definition
> >  
> >  osx_15_release_lto:
> >    <<: *release_only_definition
> > -  stage: test
> >    tags:
> >      - osx_15
> >    variables:
> >      EXTRA_ENV: 'export CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON ;'
> > -  script:
> > -    - ${GITLAB_MAKE} test_osx
> > +  <<: *osx_definition
> >  
> >  freebsd_12_release:
> >    <<: *vbox_definition
> > @@ -485,5 +503,6 @@ static_docker_build:
> >    stage: test
> >    tags:
> >      - deploy_test
> > +  <<: *docker_cleanup_script
> >    script:
> >      - ${GITLAB_MAKE} test_static_docker_build
> > -- 
> > 2.17.1
> >