Re: gitlab containers are broken

[Daniel P . Berrangé]

On Thu, Feb 04, 2021 at 08:27:00AM -1000, Richard Henderson wrote:
> On 2/4/21 7:37 AM, Daniel P. Berrangé wrote:
> > On Thu, Feb 04, 2021 at 07:36:19AM -1000, Richard Henderson wrote:
> >> On 2/4/21 12:00 AM, Daniel P. Berrangé wrote:
> >>>>> Hmm.  Is there any way to get the full output of the container build?  
> >>>>> At
> >>>>> present it's being truncated:
> >>>>>
> >>>>> #7 [4/5] RUN yum install -y bzip2     bzip2-devel     ccache     
> >>>>> csnappy-de...
> >>>>>
> >>>>>
> >>>>> In particular, I'm trying to add a new test, and I have added 
> >>>>> libffi-devel.i686
> >>>>> to the fedora-i386-cross.docker file, but then the actual build fails 
> >>>>> because
> >>>>> the libffi header file is missing.
> >> ...
> >>> Alternatively just make your changes to the dockerfiles and thne push
> >>> the branch to gitlab. Gitlab will run the build and you can pull down
> >>> the docker image from your fork's docker registry
> >>
> >> That's what I did, with the results as described above.
> > 
> > Ah, if you can point me to the gitlab pipeline / branch I can have a
> > look at diagnosing it.
> 
> One that failed is
> 
> https://gitlab.com/rth7680/qemu/-/pipelines/250583359
> 
> where the cross-i386-tci job fails.  It takes some digging to see that all of
> the correct bits are in place: from the head (84f9ac62) up to the ffi patch
> (7bdae288) which modifies the docker files, up to the gitlab patch (a1d93694)
> which adds the cross test.  I'll note that for this particular push, gitlab 
> has
> failed to rebuild the containers, and I don't know why.

> Irritatingly, I re-pushed a combined cross-test+ffi patch on top of my current
> tci branch (since I had removed both patches shown above), and it seems to be
> working.  Possibly because this time gitlab decided to rebuild the containers:
> 
> https://gitlab.com/rth7680/qemu/-/pipelines/251596167

I'm inclined to blame our gitlab CI rules. We have a condition for
the container builds:

  rules:
    - changes:
      - .gitlab-ci.d/containers.yml
      - tests/docker/*
      - tests/docker/dockerfiles/*
    - if: '$CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH'
    - if: '$CI_COMMIT_REF_NAME == "testing/next"'


The intent here is that containers are only built if a patch touches
these files.

The question is though *how* gitlab identifies that changes have been
made, when running a job for a branch. ie what set of commits does it
look at ?

The docs say it looks at the commits "in the push event".

IOW, if you have a git branch which has a delta of 40 commits vs
current git master, and you force push to that branch, it is not
guaranteed to look at all 40 commits.  I suspect it only looks
at commits which have changed since the previous push on that
branch.

This is a problem if you are pushing to multiple different
branches.

The containers are always tagged with ":latest", which means
that every time a build runs on a branch it will replace
containers created from a different branch.

So consider you

 - Push 40 commits to tci-next with a dockerfile change in the 4th commit.
   This triggers a build of the containers.

   Containers reflect your tci-next branch content.

 - Now push to master to catch up with upstream. This triggers a rebuild
   becuase of if: '$CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH'

   Containers reflect your master branch content.

 - Push 40 commits to tci-next, but only the last 5 in the branch are
   different from your previous commit.  The container build won't
   trigger, despite the branch containing a dockerfile change, because
   the commit with the dockerfile wasn't changed from previous push


I've not dug further back into your pipeline history to verify if this
is what happened, but I've convinced myself that it is plausible, and
thus I believe our rules for container rebuilds are likely fundamentally
broken.

When I first introduced the container build stuff to gitlab, I used
the :latest tag because I don't want to pollute the container registry
with an ever growing number of tags for throwaway branches. This was
safe because my original patches *always* ran the container builds,
and told docker to use the existing image as a cache. This meant that
despite running in every pipeline, the container builds were reasonably
fast as they'd hit docker layer cache most of the time.

The addition of the rules to make container builds conditional has
broken the assumption that it is safe to use :latest for the docker
tag.

I think we need to remove the rules condition

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|