fix(buildPlugin) ensure the custom label is passed to node when adding custom platform #658

dduportal · 2023-04-26T16:48:31Z

As pointed out by @jglick and @jtnord (thanks folks 🍯 ) #522 introduced an issue to the buildPlugin() function when a custom platform is defined: https://github.com/jenkins-infra/pipeline-library/pull/522/files#r1177751286.

For instance, if you call buildPlugin(platforms['maven-windows']), it will spawn a node with the linux label (as fallback) while you should have a (Windows) node with maven-windows label.

This PR fixes the behavior by using the values passed to the platform collection instead of fallbacking to linux but keeps a warning message to end users.

It also adds unit test to catch this behavior (and improve existing unit tests around the default labels and usage of useContainerAgent in the hope that we would switch to container by default in the future)

Note about the effect of this as it was surfaced by unwanted behaviors when dealing with Docker.

Jenkins infrastructure provides:

Docker Engine with Windows containers only on Windows VMs, and Docker Engine with Linux containers
Only the Docker CLI on Linux containers
No Docker CLI at all on Windows container (until feat: build Windows Server container images packer-images#321 is merged and used for the Windows containers agent on ci.jenkins)

=> but some integration tests (using the jenkinsci/docker-fixture project) are trying to run linux containers if the docker CLI is detected. Running such tests on the Windows VMs of the infrastructure fails, because Windows Server only support Docker CE with Windows containers.

…g custom platform Signed-off-by: Damien Duportal <[email protected]>

jtnord · 2023-04-26T17:12:09Z

vars/buildPlugin.groovy

@@ -58,8 +58,8 @@ def call(Map params = [:]) {
          label = 'vm && linux'
          break
        default:
-          echo "WARNING: Unknown platform '${platform}'. Agent label set to fallback value 'linux'"
-          label = 'linux'
+          echo "WARNING: Unknown Virtual Machine platform '${platform}'. Set useContainerAgent to 'true' unless you want to be in uncharted territory."


this still seems at odds with the documentation (ie this is supposed to support labels - so using a label (even a label expression if I read this doc correct) should be ok here and not produce a warning?)

yep, updating the documentation is the next step 👍 thanks for pointing out, I would have forgotten to be honest.

We are trying to update the label models because the current one is not usable in 2023. Some examples:

Should you expect docker to be available when specifying maven-windows (no obvious answer of course :D). Which JDK should you have?

When you say linux, do you mean x86 or arm64? With or without Docker Engine?

=> our current strategy is to move to explicit tags, #522 was driven by this idea. Becasue most of the labels are specified by the pipeline library, only edge cases uses custom labels.

=> a takeaway, I strongly suggest to change

buildPlugin(platforms: ['linux','windows'])

by

buildPlugin( useContainerAgent: true, configurations: [ [platform: 'linux', jdk: 11], [platform: 'windows', jdk: 11], ])

When you say linux, do you mean x86 or arm64? With or without Docker Engine?

I mean I do not care :) (currently as it was restricted - linux is ambiguos and the useage of it in this library has not helped plugin authors)..

But in reality it may depend on if this is actually allowing a label, or a label expression. If it would be a label expression then a developer could say linux && docker && x64
Would that pick up highmen - as is very likely - but we should be able to make highmen (and other expensive options) only available if a specific label is actually given. This may require a plugin (not sure off hand) but would seem like a useful thing to have to the wider community.

Absolutely, our analysis matches yours! That would be the road to a (breaking) change in the provided labels on ci.jenkins.io.

The behavior in label matching you describe is the reason why, in 2021, for 2 months, we used ARM VM agents for a lot of builds without even knowing it, because linux was resolved by both ARM and x86 machines.
=> As soon as Jenkins find an agent template matching the labels, it uses it.

We are thinking about a policy with looooong tags such as <os>-<cpu>-<qualifier> but still note sure where to put the cursor with or without too much details as we have different dimensions but it might not be needed (or event wanted) to provide all of them:

OS (Linux Ubuntu, Windows Core Server)

OS version (22.04 for Ubuntu, 2019/2022 for Windows Core Server)

size (such as highmem)

Compute type: VM, container

Tools (docker, maven, jdk17)

Feature: jdk-next (for edge non stable JDK), experiemental, etc.

Cloud/Location (aws, azure, etc.)

Infrastructure (such as cluster name or region name: doks, etc.)

We though about the platform labeler plugin, but haven't look or tried closely. Any idea or tip is welcome.

jtnord

Looks correct - not sure about the warning given the documentation around this case.

jglick

I am still unclear on the takeaway for plugins—what should they specify to build on

A Linux VM with Docker available.
A Windows container (without Docker).

?

dduportal · 2023-04-26T17:47:13Z

I am still unclear on the takeaway for plugins—what should they specify to build on
* A Linux VM with Docker available.

* A Windows container (without Docker).
?

Ideally, container agents (linux or windows) should be the default unless Docker engine is required.
Docker CLI should be expected on all kinds to be found though to allow "client side" operation (wether with buildx or compose) without involving containers (for instance: parsing a bake.hcl or compose.yml file)

The function buildPlugin and the way to specify platform is confusing to me though, as there seems to be tons of options to bypass the default: might need thinking this a bit more thoroughly. But short term, fixing the documentation (by infra team) would help a lot

jglick · 2023-04-26T18:04:02Z

the default unless Docker engine is required

Right, my question is what buildPlugin format is recommended for a plugin which does require Docker (server supporting Linux containers, as well as CLI) for some tests, and also wishes to run non-Docker-based tests on Windows.

dduportal · 2023-04-26T18:27:55Z

the default unless Docker engine is required

Right, my question is what buildPlugin format is recommended for a plugin which does require Docker (server supporting Linux containers, as well as CLI) for some tests, and also wishes to run non-Docker-based tests on Windows.

I thought setting useContainerAgent to false would be it, but I realize that this attribute is scope to the whole set of platforms/configuration 🤦

Good point!

Short term (modulo the JDK version you might want to change):

buildPlugin(
  useContainerAgent: false,
  configurations: [
    [platform: 'linux', jdk: 11],
    [platform: 'windows', jdk: 11],
])

would ensure that VM are used:

For Linux, to ensure Docker is present
For Windows, not needed but at least should work for now

=> got plenty of feedback for improvement thanks!

jglick · 2023-04-26T18:38:16Z

But the above will build on a Windows VM, right? So if the Windows branch can run fine in a container, then you can still specify maven-windows as in jenkinsci/docker-workflow-plugin#222 (comment)?

dduportal · 2023-04-26T19:02:25Z

But the above will build on a Windows VM, right? So if the Windows branch can run fine in a container, then you can still specify maven-windows as in jenkinsci/docker-workflow-plugin#222 (comment)?

Yes, absolutely (haven't thought about it). WDYT about an "explicit" docker request, something like

buildPlugin(platforms: [
    'linux && docker',
    'maven-windows && !docker'
])

jtnord · 2023-04-28T10:18:46Z

Yes, absolutely (haven't thought about it). WDYT about an "explicit" docker request, something like

as for linux being a mostly bad label you have the similarly bad docker.
what does this mean? the docker cli is installed? the docker cli is installed and points to a working docker install (local / remote?) the docker cli is installed and supports specific platforms (linux/arm64 windows....)?

what if I do want to run the docker (linux containers) tests on windows (and such support was available)?

buildPlugin(platforms: [
    'linux && docker',
    'maven-windows && docker && docker_linux_container'
])

and for things that want windows containers

buildPlugin(platforms: [
    'linux && docker',
    'maven-windows && docker && docker_windows_container'
])

see how docker is redundant here? linux with docker should become linux && docker_linux_containers

and this does not discuss testcontainers.cloud which needs a docker cli but not necessarily a docker daemon (I have not played - may be incorrect)

* origin/master: Bump `hashicorp-tools` docker image to 0.5.163 (jenkins-infra#659) fix(buildPlugin) ensure the custom label is passed to node when adding custom platform (jenkins-infra#658) feat(buildPluginWithGradle) Add quality checks (jenkins-infra#652) hotfix(terraform) add spot toleration hotfix(terraform) add nodeselector Update updatecli version to v0.49.2 (jenkins-infra#657) fix(terraform): add toleration to use infracipool (jenkins-infra#655) Bump `hashicorp-tools` docker image to 0.5.162 (jenkins-infra#651)

…g custom platform (jenkins-infra#658) Signed-off-by: Damien Duportal <[email protected]>

fix(buildPlugin) ensure the custom label is passed to node when addin…

d0c301c

…g custom platform Signed-off-by: Damien Duportal <[email protected]>

dduportal mentioned this pull request Apr 26, 2023

Feat(agent label): use a specific label for windows if not a container #522

Merged

dduportal changed the title ~~fix(buildPlugin) ensure the custom label is passed to node when addin…~~ fix(buildPlugin) ensure the custom label is passed to node when adding custom platform Apr 26, 2023

dduportal marked this pull request as ready for review April 26, 2023 17:02

dduportal requested a review from a team April 26, 2023 17:02

jtnord reviewed Apr 26, 2023

View reviewed changes

jtnord approved these changes Apr 26, 2023

View reviewed changes

jglick approved these changes Apr 26, 2023

View reviewed changes

jglick mentioned this pull request Apr 26, 2023

MinioIntegrationTest should assume Docker Linux specifically jenkinsci/artifact-manager-s3-plugin#392

Merged

dduportal merged commit 5bdb6bd into jenkins-infra:master Apr 26, 2023

dduportal deleted the fix/buildPlugin/custom-label branch April 26, 2023 18:00

jglick mentioned this pull request Oct 2, 2023

Refresh jenkinsci/groovy-plugin#61

Merged

smerle33 pushed a commit to smerle33/pipeline-library that referenced this pull request Jan 16, 2024

fix(buildPlugin) ensure the custom label is passed to node when addin…

2dd0c33

…g custom platform (jenkins-infra#658) Signed-off-by: Damien Duportal <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(buildPlugin) ensure the custom label is passed to node when adding custom platform #658

fix(buildPlugin) ensure the custom label is passed to node when adding custom platform #658

dduportal commented Apr 26, 2023 •

edited

Loading

jtnord Apr 26, 2023

dduportal Apr 26, 2023

jtnord Apr 28, 2023

dduportal Apr 28, 2023

jtnord left a comment

jglick left a comment

dduportal commented Apr 26, 2023

jglick commented Apr 26, 2023

dduportal commented Apr 26, 2023

jglick commented Apr 26, 2023

dduportal commented Apr 26, 2023

jtnord commented Apr 28, 2023

fix(buildPlugin) ensure the custom label is passed to node when adding custom platform #658

fix(buildPlugin) ensure the custom label is passed to node when adding custom platform #658

Conversation

dduportal commented Apr 26, 2023 • edited Loading

jtnord Apr 26, 2023

Choose a reason for hiding this comment

dduportal Apr 26, 2023

Choose a reason for hiding this comment

jtnord Apr 28, 2023

Choose a reason for hiding this comment

dduportal Apr 28, 2023

Choose a reason for hiding this comment

jtnord left a comment

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

dduportal commented Apr 26, 2023

jglick commented Apr 26, 2023

dduportal commented Apr 26, 2023

jglick commented Apr 26, 2023

dduportal commented Apr 26, 2023

jtnord commented Apr 28, 2023

dduportal commented Apr 26, 2023 •

edited

Loading