fix: only deploy to DockerHub on tag-triggered builds

[dduportal]

Related to jenkinsci/docker-agent#605.

Closes jenkins-infra/helpdesk#2

This PR updates the Jenkinsfile (pipeline) and the docker-bake.hcl manifest with the following changes:

• fix(Jenkinsfile): only deploy to DockerHub (--push argument for docker buildx bake command) when a tag triggered a build
• fix(Jenkinsfile): stop polling for code change, to avoid triggering unexpected tag builds
• chore(Jenkins): use declarative syntax as much as posible (simplification)
• feat(docker-bake.hcl) automatic detection of the remoting version and build number from the tag

[dduportal]

Ping @timja the windows build seems flacky. Do you mind merging or shall we fix it ?

[slide]

Yes, there is a commit that broke tests, if the build stage is passing for now it is probably ok. I am trying to address the issues in another PR.

[dduportal]

Yes, there is a commit that broke tests, if the build stage is passing for now it is probably ok. I am trying to address the issues in another PR.

Since I was slower to update, I rebased now that your PR is merged (THANKS).
I had to change the parallel thing, let me kno what you think: I've used scripted inside declarative, to keep the Linux / Windows parallel as well. Reason is that windows agent allocation takes a lot of time so better to start it as soon as posible in the pipeline.

This PR is required to ensure that jenkinsci/docker-agent#604 and jenkinsci/docker-agent#603 can be fixed without regression

[slide]

The creation of the nmap container prior to the tests starting requires updating the tests as well. The nmap container is cleaned up in the tests, so that would need to be removed as well so it doesn't get cleaned up and then recreated anyway. Also, the builds may be on other nodes, so the nmap container needs to be part of the parallel part, or am I missing something?

[dduportal]

The creation of the nmap container prior to the tests starting requires updating the tests as well. The nmap container is cleaned up in the tests, so that would need to be removed as well so it doesn't get cleaned up and then recreated anyway.

Thanks for this info. I initially thought that building it would, at least, keep the image layers into the Docker Engine cache, so the docker build in the pester tests would benefit the cache but would still be kept so contributoir could run it autonomously. I'll have a look then.

Also, the builds may be on other nodes, so the nmap container needs to be part of the parallel part, or am I missing something?

Not in the actual version of the pipeline: The agent directive which spawns a Windows VM agent is defined at the parent stage level.
But your message makes me wonder if we should not run each step of the "matrix" on a separate node: worth comparing the efficiency of both solutions:

• On one side, these machines are huge, so building the images concurently might be efficient, and spawning them takes time and costs us money (billing per hour for Windows VM, instead of per-minute for Linux).

• On the other side, the concurent tests that spawn a lot of containers could benefit a lot from NOT being run concurenctly on the same machines (docker build is optimized, docker run is not)

[timja]

Not in the actual version of the pipeline: The agent directive which spawns a Windows VM agent is defined at the parent stage level.

but the previous version was doing that wasn't it? We changed to an agent per image because we kept getting disk full on the windows builds

[dduportal]

Not in the actual version of the pipeline: The agent directive which spawns a Windows VM agent is defined at the parent stage level.

but the previous version was doing that wasn't it? We changed to an agent per image because we kept getting disk full on the windows builds

True: I misread the agent block location in the matrix (I should not have used the diff view :D).
Thanks folks, let me change this in the PR

[dduportal]

Ready ro review folks:

• Windows builds are executed per-node as @slide did in the previous PR
• Build passing (I've pushed forced a cleaned up history)
• We should not see any more disk full as per fix(ci.jenkins.io) recycle Windows Azure VM once jobs finishes jenkins-infra/jenkins-infra#2401

[timja]

This is not handling the case where we have to use a different docker-agent build number than in inbound-agent.

sometimes we have to use e.g. 4.13.5-6 due to release issues or whatever in the docker-agent repo but here it's the first release version.

We can't just always sync them as if we have changes in this repository e.g. to change the dockerfile here, we need to release a new build but that would require a new docker-agent release in a different repository which I don't think is right.

(if they were in the same repo then I guess we could release all at once always)

[timja]

visualisation is broken in blue ocean unfortunately as it can't handle this case 😢

[dduportal]

As far as I remember, it is the main reason why nesting matrix/parallel are not supported in declarative: to avoid breaking the UI.

But in fact, it looks better once the build is finished (wether it's successful, failing or aborted):

• Check https://ci.jenkins.io/job/Packaging/job/docker-inbound-agent/job/PR-280/12/pipeline-graph/

• Compare in BlueOcean the following screenshot:

  • "Broken" graph while it's building (I use quotes because it is still usable)

• Fixed graph once built:

=> when using a clean agent (with no priori cache), the new build jumped from ~39min to ~32 min

[timja]

interesting that it works after complete

[dduportal]

interesting that it works after complete

Yep, I realized it while looking to this comment: I was sure it was working and just discovered that's it is broken "while" building.

[dduportal]

This is not handling the case where we have to use a different docker-agent build number than in inbound-agent.

sometimes we have to use e.g. 4.13.5-6 due to release issues or whatever in the docker-agent repo but here it's the first release version.

We can't just always sync them as if we have changes in this repository e.g. to change the dockerfile here, we need to release a new build but that would require a new docker-agent release in a different repository which I don't think is right.

(if they were in the same repo then I guess we could release all at once always)

That is the goal of the variable AGENT_IMAGE_BUILD_NUMBER in the bake file (I've added a comment because it's confusing until we are able to merge on a single repository): https://github.com/jenkinsci/docker-inbound-agent/pull/280/files#diff-870f6fe23fc034f008f5203ee1e628d7bfa65a49095d5a4688db498328449ed2R37-R40

With your example, if you want to build the image jenkins/inbound-agent:4.13.5-1 inheriting from jenkins/agent:4.13.5-6 then you would do the same as usual: IMAGE_TAG=4.13.5-1 AGENT_IMAGE_BUILD_NUMBER=6 docker buildx bake <...>.

The change made in the bake file removes at least one case from the path (as it forces the remoting version to be the same for parent and agent image)

[timja]

there's already a build number variable in this file

[dduportal]

this one is the inbound-agent build number (It took me 8hours in the plane to fully get it :D ).

Let me add a comment on this one.

[timja]

you sure?

[dduportal]

Oooh good catch: this 2nd occurence is the result of me messing up the rebase.

Should be fixed now

[dduportal]

@timja as soon as the JDK8/11 issues are fixed, I'm willing to open a PR to jenkinsci/agent repo to "merge" Dockerfiles, so any remoting updates would only be done once (and we could get rid of all this duplication).

Sounds good to you?

[timja]

@timja as soon as the JDK8/11 issues are fixed, I'm willing to open a PR to jenkinsci/agent repo to "merge" Dockerfiles, so any remoting updates would only be done once (and we could get rid of all this duplication).

Sounds good to you?

would be great yeah