[PATCH 0/1] gitlab-ci: Only push docker images to mainstream registry from /master

[PATCH 0/1] gitlab-ci: Only push docker images to mainstream registry from /master

[Philippe Mathieu-Daudé]

Hi,

I guess I got very unlucky because I happened to pull the docker
images from the mainstream registry in a very short time frame
where they jumped back in time... Then I kept using them to run
my tests during 1 week trying to understand why I was having odd
build failures. Then I realize the Ubuntu docker images were out
of sync. I pulled again and it was working, so I searched for the
mainstream job producing the outdated images and found a pipeline
pushing the 'stable-6.0-staging' branch. This branch doesn't
contain the recent gitlab-ci and Dockerfile changes...

Similarly, this branch doesn't contain commit eafadbbbac0
("gitlab: only let pages be published from default branch") so
outdated documentation got pushed for a short time.

This patch won't fix branches pushed from the past, but at least
it should avoid to reproduce this problem in the future.

Any idea how to improve the GitLab infrastructure to avoid these
kind of problems in the future? Is it possible to enforce
restrictions from the project configuration, rather than the
repository YAML file?

Regards,

Phil.

Philippe Mathieu-Daudé (1):
  gitlab-ci: Only push docker images to registry from /master branch

 .gitlab-ci.d/container-template.yml | 11 ++++++++++-
 .gitlab-ci.d/edk2.yml               | 11 ++++++++++-
 .gitlab-ci.d/opensbi.yml            | 11 ++++++++++-
 3 files changed, 30 insertions(+), 3 deletions(-)

-- 
2.31.1

[PATCH 1/1] gitlab-ci: Only push docker images to registry from /master branch

[Philippe Mathieu-Daudé]

Users expect images pulled from registry.gitlab.com/qemu-project/qemu/
to be stable. QEMU repository workflow pushes merge candidates to
the /staging branch, and on success the same commit is pushed as
/master. If /staging fails, we do not want to push the built images
to the registry. Therefore limit the 'docker push' command to the
/master branch on the mainstream CI. The fork behavior is unchanged.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 .gitlab-ci.d/container-template.yml | 11 ++++++++++-
 .gitlab-ci.d/edk2.yml               | 11 ++++++++++-
 .gitlab-ci.d/opensbi.yml            | 11 ++++++++++-
 3 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/.gitlab-ci.d/container-template.yml b/.gitlab-ci.d/container-template.yml
index 1baecd94606..0a736644b22 100644
--- a/.gitlab-ci.d/container-template.yml
+++ b/.gitlab-ci.d/container-template.yml
@@ -16,6 +16,15 @@
           -t "qemu/$NAME" -f "tests/docker/dockerfiles/$NAME.docker"
           -r $CI_REGISTRY/qemu-project/qemu
     - docker tag "qemu/$NAME" "$TAG"
-    - docker push "$TAG"
+    # On mainstream CI, we only want to push images on the master branch,
+    # so skip the other cases (tag or non-master branch).
+    - if test "$CI_PROJECT_NAMESPACE" = "qemu-project" &&
+         test -n "$CI_COMMIT_TAG"
+              -o "$CI_COMMIT_BRANCH" != "$CI_DEFAULT_BRANCH";
+      then
+        :;
+      else
+        docker push "$TAG";
+      fi
   after_script:
     - docker logout
diff --git a/.gitlab-ci.d/edk2.yml b/.gitlab-ci.d/edk2.yml
index 13d0f8b019f..e15f80f4874 100644
--- a/.gitlab-ci.d/edk2.yml
+++ b/.gitlab-ci.d/edk2.yml
@@ -33,7 +33,16 @@ docker-edk2:
  - docker build --cache-from $IMAGE_TAG --tag $CI_REGISTRY_IMAGE:$CI_COMMIT_SHA
                                         --tag $IMAGE_TAG .gitlab-ci.d/edk2
  - docker push $CI_REGISTRY_IMAGE:$CI_COMMIT_SHA
- - docker push $IMAGE_TAG
+ # On mainstream CI, we only want to push images on the master branch,
+ # so skip the other cases (tag or non-master branch).
+ - if test "$CI_PROJECT_NAMESPACE" = "qemu-project" &&
+      test -n "$CI_COMMIT_TAG"
+           -o "$CI_COMMIT_BRANCH" != "$CI_DEFAULT_BRANCH";
+   then
+      :;
+   else
+      docker push "$IMAGE_TAG";
+   fi
 
 build-edk2:
  extends: .edk2_job_rules
diff --git a/.gitlab-ci.d/opensbi.yml b/.gitlab-ci.d/opensbi.yml
index 5e0a2477c5d..a38ccbe5baa 100644
--- a/.gitlab-ci.d/opensbi.yml
+++ b/.gitlab-ci.d/opensbi.yml
@@ -34,7 +34,16 @@ docker-opensbi:
  - docker build --cache-from $IMAGE_TAG --tag $CI_REGISTRY_IMAGE:$CI_COMMIT_SHA
                                         --tag $IMAGE_TAG .gitlab-ci.d/opensbi
  - docker push $CI_REGISTRY_IMAGE:$CI_COMMIT_SHA
- - docker push $IMAGE_TAG
+ # On mainstream CI, we only want to push images on the master branch,
+ # so skip the other cases (tag or non-master branch).
+ - if test "$CI_PROJECT_NAMESPACE" = "qemu-project" &&
+      test -n "$CI_COMMIT_TAG"
+           -o "$CI_COMMIT_BRANCH" != "$CI_DEFAULT_BRANCH";
+   then
+      :;
+   else
+      docker push "$IMAGE_TAG";
+   fi
 
 build-opensbi:
  extends: .opensbi_job_rules
-- 
2.31.1

Re: [PATCH 1/1] gitlab-ci: Only push docker images to registry from /master branch

[Willian Rampazzo]

On Tue, Oct 26, 2021 at 11:55 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Users expect images pulled from registry.gitlab.com/qemu-project/qemu/
> to be stable. QEMU repository workflow pushes merge candidates to
> the /staging branch, and on success the same commit is pushed as
> /master. If /staging fails, we do not want to push the built images
> to the registry. Therefore limit the 'docker push' command to the
> /master branch on the mainstream CI. The fork behavior is unchanged.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  .gitlab-ci.d/container-template.yml | 11 ++++++++++-
>  .gitlab-ci.d/edk2.yml               | 11 ++++++++++-
>  .gitlab-ci.d/opensbi.yml            | 11 ++++++++++-
>  3 files changed, 30 insertions(+), 3 deletions(-)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

Re: [PATCH 1/1] gitlab-ci: Only push docker images to registry from /master branch

[Thomas Huth]

On 26/10/2021 16.55, Philippe Mathieu-Daudé wrote:
> Users expect images pulled from registry.gitlab.com/qemu-project/qemu/
> to be stable. QEMU repository workflow pushes merge candidates to
> the /staging branch, and on success the same commit is pushed as
> /master. If /staging fails, we do not want to push the built images
> to the registry. Therefore limit the 'docker push' command to the
> /master branch on the mainstream CI. The fork behavior is unchanged.

Hmmm, what if I have a patch series that updates one of the containers and 
then also contains a new test that depends on the updated container? Won't 
that fail in the staging branch now and make me look bad?

  Thomas

Re: [PATCH 1/1] gitlab-ci: Only push docker images to registry from /master branch

[Daniel P. Berrangé]

On Wed, Oct 27, 2021 at 08:01:07AM +0200, Thomas Huth wrote:
> On 26/10/2021 16.55, Philippe Mathieu-Daudé wrote:
> > Users expect images pulled from registry.gitlab.com/qemu-project/qemu/
> > to be stable. QEMU repository workflow pushes merge candidates to
> > the /staging branch, and on success the same commit is pushed as
> > /master. If /staging fails, we do not want to push the built images
> > to the registry. Therefore limit the 'docker push' command to the
> > /master branch on the mainstream CI. The fork behavior is unchanged.
> 
> Hmmm, what if I have a patch series that updates one of the containers and
> then also contains a new test that depends on the updated container? Won't
> that fail in the staging branch now and make me look bad?

Yep, if a patch series contains a dockerfile change we *must* run the
container build no matter what, so tis patch doesn't fly.

This scenario a tricky problem, and I'm not seeing an easy answer to it
so far.

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 :|

Re: [PATCH 1/1] gitlab-ci: Only push docker images to registry from /master branch

[Gerd Hoffmann]

  Hi,

> This scenario a tricky problem, and I'm not seeing an easy answer to it
> so far.

Only exclude stable branches?

take care,
  Gerd

Re: [PATCH 1/1] gitlab-ci: Only push docker images to registry from /master branch

[Daniel P. Berrangé]

On Wed, Oct 27, 2021 at 01:35:25PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > This scenario a tricky problem, and I'm not seeing an easy answer to it
> > so far.
> 
> Only exclude stable branches?

The stable branches issue is just one of the bugs - the bigger bug is
that we're publishing containers in the qemu-project registry from
the 'staging' branch pipelines, even if the pipeline ultimately
fails.

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 :|

Re: [PATCH 1/1] gitlab-ci: Only push docker images to registry from /master branch

[Philippe Mathieu-Daudé]

Hi Thomas,

On 10/27/21 08:01, Thomas Huth wrote:
> On 26/10/2021 16.55, Philippe Mathieu-Daudé wrote:
>> Users expect images pulled from registry.gitlab.com/qemu-project/qemu/
>> to be stable. QEMU repository workflow pushes merge candidates to
>> the /staging branch, and on success the same commit is pushed as
>> /master. If /staging fails, we do not want to push the built images
>> to the registry. Therefore limit the 'docker push' command to the
>> /master branch on the mainstream CI. The fork behavior is unchanged.
> 
> Hmmm, what if I have a patch series that updates one of the containers
> and then also contains a new test that depends on the updated container?
> Won't that fail in the staging branch now and make me look bad?

Good point. My understanding is:

- All tests based on Docker containers pull from DOCKER_DEFAULT_REGISTRY
  (see tests/docker/Makefile.include). These tests can be run on CI
  but are principally run locally, using the ':latest' tag, which is
  the reason we don't want to push invalid / oudated images.

- CI tests also use the Docker containers. We might want to test freshly
  built images. Here we should explicit the use of local tag (without
  using the registry prefix) in this case. Otherwise default to the
  latest from registry (stable).

So the problem you mentioned is the second case, and should be reworked
in the YAML. I will revisit the overall CI YAML for your case, but the
first case seems equally important.

Regards,

Phil.

Re: [PATCH 0/1] gitlab-ci: Only push docker images to mainstream registry from /master

[Daniel P. Berrangé]

On Tue, Oct 26, 2021 at 04:55:08PM +0200, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> I guess I got very unlucky because I happened to pull the docker
> images from the mainstream registry in a very short time frame
> where they jumped back in time... Then I kept using them to run
> my tests during 1 week trying to understand why I was having odd
> build failures. Then I realize the Ubuntu docker images were out
> of sync. I pulled again and it was working, so I searched for the
> mainstream job producing the outdated images and found a pipeline
> pushing the 'stable-6.0-staging' branch. This branch doesn't
> contain the recent gitlab-ci and Dockerfile changes...
> 
> Similarly, this branch doesn't contain commit eafadbbbac0
> ("gitlab: only let pages be published from default branch") so
> outdated documentation got pushed for a short time.
> 
> This patch won't fix branches pushed from the past, but at least
> it should avoid to reproduce this problem in the future.
> 
> Any idea how to improve the GitLab infrastructure to avoid these
> kind of problems in the future? Is it possible to enforce
> restrictions from the project configuration, rather than the
> repository YAML file?

The problem here is the general way we are handling docker images
in the CI pipeline.

In the first stage of the pipeline we build and publish images
with a tag   "$reponame/$imagename:latest".  We need to publish
them because the next stage of the pipeline has to be able to
pull the just built images - stages can't directly inherit the
docker images from each stages without going via the registry.

The use of ":latest" here is the root cause of our problems.
It works only under the assumptions:

  - you have a single pipeline running for any repository at
    any point in time
  - you only need a single maintainance stage (master)

The first assumption was never really true. In the main qemu-project
namespace we sometimes end up ith pipelines runnning on 'master' for
just pushed series, and for 'staging' for series being tested for
merge. Most of the time we get away with this but we've seen a few
rare CI failures from it. Similarly in user forks most of the time
we get away with it, but sometimes users might quickly push to
different branches. In all cases the race is only a problem when
a patch series contains dockerfile changes, which is why it is such
a rare problem

So far we've only been relying on the CI pipeline for pushes to master,
since stable branch maint has been awol for a while. We're soon going
to starting violating this assumption though.

IOW, we can't keep relying on ":latest", as its going to cause increasing
trouble.

The reason we picked ":latest" though is because gitlab CI config is a
bit inflexible:

  - It has to be either a fixed string, or the contents of a standard
    environment variable present in gitlab:

    https://docs.gitlab.com/ee/ci/variables/predefined_variables.html

    We can't populate our own env variables dynamically, nor
    programatically define a tag


  - The tag contents has to satisfy the Docker restrictions on valid
    characters in tags

      "A tag name must be valid ASCII and may contain lowercase and 
       uppercase letters, digits, underscores, periods and dashes. 
       A tag name may not start with a period or a dash and may 
       contain a maximum of 128 characters."

Essentially we need two tags

 - One that is only for use during execution of the current pipeline
 - One that is published only on success of the pipeline on master,
   to serve as cache for future pipelines, or to let developer pull


Notably the latter is more restrictive that git branch names. We could
assume users always have "sensible" branch names that are less than
128 chars and only alpha-num characters plus dash/underscore. This
would be fine for my personal branch naming, but I wonder if anyone
uses wierd branch names that would violate docker tag name rules ?
Perhaps we just accept that risk and have the CI job that builds the
container fail + print a clear message to user to rename their branch
to something sensible.

Ideally we would take a sha256 sum of the dockerfile (and parent layers
it inherits from) and use that as the image tag. That fails with the
constraint number 1 though - we can't programmatically set tags. This
would maximise our cache hit rate though, compared to the single
"latest" tag we publish.

The other alternative option here is to use the current git commit
hash to as the image tag, since this is valid docker tag name and is
unique for the dockerfile contents even if multiple pipelines run for
the same commit.

An even more unique option is to use the unique pipeline ID as the
tag.

Using either commit hash or pipeline ID will lead to an explosion in
the number of tags present in the repository's container registry.
GitLab has ability to periodically cleanup old tags but on all my
repos this appears disabled by default.

We could manually delete the tag, at the end of the pipeline, but that
causes trouble if multiple pipelines for the same commit hash are runing
concurrently, as the tag might still be needed by something later. Also
it means the developer can't easily reproduce problems using the *exact*
image the pipeline used.


Maybe we put something at the start of the pipeline that manualyl deletes
any obsolete tags (eg > 7 days old) from previous pipelines. This re-invents
gitlab tag cleanup, in a way that doesn't require every develoiper to
toggle a setting in their repo.


So my overall inclination is

 - Modify ".container_job_template"

    - If image tag matching the sha256 sum of the dockerfile(s) exists:
    
       - Publish new tag based on $CI_PIPELINE_IID, copying the sha256 tag
       
    - Else

       - Build new image from scratch, no caching
       - Publish new tag based on sha256 sum of the dockerfile(s)
       - Publish new tag based on $CI_PIPELINE_IID

 - Modify ".native_build_job_template"

    - Use $CI_REGISTRY_IMAGE/qemu/$IMAGE:$CI_PIPELINE_IID

 - Add a final job

    - Publish a new tag ":latest" based on the image built earlier,
      *if* this branch is "master" and all jobs succeeded

      Possibly, we could add ":master" and ":stable-XXX" too as
      convenience tags ?

 - Add an early job

    - Delete all $CI_PIPELINE_IID based tags older than 7 days
    - Delete all sha256 sum tags older than 7 days *provided*
      the tag does not match the current sha256 content


In the common case I believe this will make our pipelines faster,
because publishing the $CI_PIPELINE_IID based oan copy of the sha256
tag can be done without even pulling down the image. It is a pure
metadata operation from docker registry POV.  Our current caching by
contrast needs to pull down existing images to see if their content
is usable, which takes time.

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 :|

Re: [PATCH 0/1] gitlab-ci: Only push docker images to mainstream registry from /master

[Gerd Hoffmann]

  Hi,

> Notably the latter is more restrictive that git branch names. We could
> assume users always have "sensible" branch names that are less than
> 128 chars and only alpha-num characters plus dash/underscore. This
> would be fine for my personal branch naming, but I wonder if anyone
> uses wierd branch names that would violate docker tag name rules ?

/me uses slashes in branch names, i.e.

queue/$topic for patch queues
$hostname/$subject for my development branches.

take care,
  Gerd

Re: [PATCH 0/1] gitlab-ci: Only push docker images to mainstream registry from /master

[Daniel P. Berrangé]

On Wed, Oct 27, 2021 at 02:26:58PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > Notably the latter is more restrictive that git branch names. We could
> > assume users always have "sensible" branch names that are less than
> > 128 chars and only alpha-num characters plus dash/underscore. This
> > would be fine for my personal branch naming, but I wonder if anyone
> > uses wierd branch names that would violate docker tag name rules ?
> 
> /me uses slashes in branch names, i.e.
> 
> queue/$topic for patch queues
> $hostname/$subject for my development branches.

Ok, good to know. So that example clearly rules out use of git branch
names as docker tags.

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 :|

Re: [PATCH 0/1] gitlab-ci: Only push docker images to mainstream registry from /master

[Philippe Mathieu-Daudé]

On 10/27/21 14:34, Daniel P. Berrangé wrote:
> On Wed, Oct 27, 2021 at 02:26:58PM +0200, Gerd Hoffmann wrote:
>>> Notably the latter is more restrictive that git branch names. We could
>>> assume users always have "sensible" branch names that are less than
>>> 128 chars and only alpha-num characters plus dash/underscore. This
>>> would be fine for my personal branch naming, but I wonder if anyone
>>> uses wierd branch names that would violate docker tag name rules ?
>>
>> /me uses slashes in branch names, i.e.
>>
>> queue/$topic for patch queues
>> $hostname/$subject for my development branches.
> 
> Ok, good to know. So that example clearly rules out use of git branch
> names as docker tags.

CI_COMMIT_REF_SLUG should work:

- CI_COMMIT_REF_NAME The branch or tag name for which project is built.

- CI_COMMIT_REF_NAME in lowercase, shortened to 63 bytes, and with
  everything except 0-9 and a-z replaced with -.
  No leading / trailing -. Use in URLs, host names and domain names.