🐛 Fix: pulling unused images in the middle of tests #2271

peppi-lotta · 2025-01-23T13:48:59Z

What this PR does / why we need it: Let's take metal3-periodic-ubuntu-e2e-integration-test-release-1-9 as an example. This test runs make in metal3-dev-env where container images (with correct tags) get pulled. Some time later the test moves into cluster-api-provider-metal3 folder and runs make e2e-test. At this time CAPM3, BMO and IPAM images will get pulled again but this time the tag is set to latest. This is unnecessary. This fix will pull the correct images if they are missing instead of always bulling images that are unused in many cases.

Makefile

tuminoid · 2025-01-24T06:13:44Z

/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

tuminoid

Question:

tuminoid · 2025-01-24T06:19:18Z

Makefile

@@ -159,7 +159,10 @@ ARTIFACTS ?= $(ROOT_DIR)/_artifacts
 E2E_CONF_FILE ?= $(ROOT_DIR)/test/e2e/config/e2e_conf.yaml
 E2E_OUT_DIR ?= $(ROOT_DIR)/test/e2e/_out
 E2E_CONF_FILE_ENVSUBST ?= $(E2E_OUT_DIR)/$(notdir $(E2E_CONF_FILE))
-E2E_CONTAINERS ?= quay.io/metal3-io/cluster-api-provider-metal3 quay.io/metal3-io/baremetal-operator quay.io/metal3-io/ip-address-manager
+CAPM3_IMAGE ?= ${CONTAINER_REGISTRY}/metal3-io/cluster-api-provider-metal3:${CAPM3RELEASE:-latest}


Who initially defines CONTAINER_REGISTRY here?

In CAPM3, it is only mentioned in e2e_conf.yaml, so it is probably empty here, unless it was already defined in dev-env first. With CAPM3 e2e, the test cycle starts on CAPM3 side, not from dev-env, so does this actually work?

In this Makefile, registry variables defined are REGISTRY, STAGING_REGISTRY and PROD_REGISTRY?

CONTAINER_REGISTRY can be defined in project-infra (and is defined there for e2e-integration-tests) or gets a default value from dev-env. I can change ${CONTAINER_REGISTRY} to ${CONTAINER_REGISTRY:quay.io} just to be sure.

I've been looking at e2e-integration-test on release-1.9 when making this change.

Here are some copied from the log I've linked above. Times are system time. As you can see from here, yes we starts from CAPM3 e2e but the tests very quickly moves to execute in dev-env. Prepulling of images happens there and we come back to CAPM3 later where the some images get pulled again with latest tag (which is not correct for release-1.9 tests).

01:31:43 Running the tests 01:31:43 + make test-e2e ... 01:31:45 + pushd /opt/metal3-dev-env/metal3-dev-env 01:31:45 /opt/metal3-dev-env/metal3-dev-env ~/tested_repo 01:31:45 + make 01:31:45 make[1]: Entering directory '/opt/metal3-dev-env/metal3-dev-env' ... 01:37:25 ++ sudo docker pull registry.nordix.org/quay-io-proxy/metal3-io/baremetal-operator:v0.9.0 01:37:27 v0.9.0: Pulling from quay-io-proxy/metal3-io/baremetal-operator ... 01:56:45 + make e2e-tests 01:56:45 make[1]: Entering directory '/home/metal3ci/tested_repo' ... 01:56:50 Using default tag: latest 01:56:52 latest: Pulling from metal3-io/baremetal-operator 01:56:52 dd5ad9c9c29f: Already exists 01:56:52 960043b8858c: Already exists 01:56:52 b4ca4c215f48: Already exists 01:56:52 eebb06941f3e: Already exists 01:56:52 02cd68c0cbf6: Already exists 01:56:52 d3c894b5b2b0: Already exists 01:56:52 b40161cd83fc: Already exists 01:56:52 46ba3f23f1d3: Already exists 01:56:52 4fa131a1b726: Already exists 01:56:52 fc8b0dbaf292: Pulling fs layer 01:56:53 fc8b0dbaf292: Verifying Checksum 01:56:53 fc8b0dbaf292: Download complete 01:56:54 fc8b0dbaf292: Pull complete

Note that this is Makefile and not bash, so the variable syntax is not {} but ().

Also you need to make fallbacks differently.

Suggested change

CAPM3_IMAGE ?= ${CONTAINER_REGISTRY}/metal3-io/cluster-api-provider-metal3:${CAPM3RELEASE:-latest}

CAPM3RELEASE ?= latest

CAPM3_IMAGE ?= $(CONTAINER_REGISTRY)/metal3-io/cluster-api-provider-metal3:$(CAPM3RELEASE)

tuminoid · 2025-01-24T06:40:51Z

/retest

tuminoid · 2025-01-24T07:12:56Z

There also seems to be some actual issue triggered by this:

[2025-01-24T07:05:44.054Z] for image in registry.nordix.org/quay-io-proxy/metal3-io/cluster-api-provider-metal3: registry.nordix.org/quay-io-proxy/metal3-io/ip-address-manager: "registry.nordix.org/quay-io-proxy/metal3-io/baremetal-operator:; do \
[2025-01-24T07:05:44.054Z] 	podman pull $image; \
[2025-01-24T07:05:44.054Z] done
[2025-01-24T07:05:44.054Z] bash: -c: line 1: unexpected EOF while looking for matching `"'
[2025-01-24T07:05:44.054Z] bash: -c: line 4: syntax error: unexpected end of file

https://jenkins.nordix.org/blue/organizations/jenkins/metal3-centos-e2e-integration-test-main/detail/metal3-centos-e2e-integration-test-main/884/pipeline

peppi-lotta · 2025-01-24T07:54:48Z

/retest

peppi-lotta · 2025-01-24T07:59:08Z

/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

tuminoid · 2025-01-24T08:23:40Z

Makefile

@@ -159,7 +159,10 @@ ARTIFACTS ?= $(ROOT_DIR)/_artifacts
 E2E_CONF_FILE ?= $(ROOT_DIR)/test/e2e/config/e2e_conf.yaml
 E2E_OUT_DIR ?= $(ROOT_DIR)/test/e2e/_out
 E2E_CONF_FILE_ENVSUBST ?= $(E2E_OUT_DIR)/$(notdir $(E2E_CONF_FILE))
-E2E_CONTAINERS ?= quay.io/metal3-io/cluster-api-provider-metal3 quay.io/metal3-io/baremetal-operator quay.io/metal3-io/ip-address-manager
+CAPM3_IMAGE ?= ${CONTAINER_REGISTRY}/metal3-io/cluster-api-provider-metal3:${CAPM3RELEASE:-latest}


Note that this is Makefile and not bash, so the variable syntax is not {} but ().

Also you need to make fallbacks differently.

Suggested change

CAPM3_IMAGE ?= ${CONTAINER_REGISTRY}/metal3-io/cluster-api-provider-metal3:${CAPM3RELEASE:-latest}

CAPM3RELEASE ?= latest

CAPM3_IMAGE ?= $(CONTAINER_REGISTRY)/metal3-io/cluster-api-provider-metal3:$(CAPM3RELEASE)

Makefile

tuminoid · 2025-01-24T08:38:30Z

/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

tuminoid · 2025-01-24T09:56:22Z

/test ?

metal3-io-bot · 2025-01-24T09:56:25Z

@tuminoid: The following commands are available to trigger required jobs:

/test build

/test generate

/test gomod

/test manifestlint

/test markdownlint

/test metal3-centos-e2e-integration-test-main

/test metal3-ubuntu-e2e-integration-test-main

/test shellcheck

/test test

/test unit

The following commands are available to trigger optional jobs:

/test metal3-centos-e2e-basic-test-main

/test metal3-centos-e2e-feature-test-main-features

/test metal3-centos-e2e-feature-test-main-pivoting

/test metal3-centos-e2e-feature-test-main-remediation

/test metal3-centos-e2e-feature-test-main-scalability

/test metal3-e2e-1-29-1-30-upgrade-test-main

/test metal3-e2e-clusterctl-upgrade-test-main

/test metal3-ubuntu-e2e-basic-test-main

/test metal3-ubuntu-e2e-feature-test-main-features

/test metal3-ubuntu-e2e-feature-test-main-pivoting

/test metal3-ubuntu-e2e-feature-test-main-remediation

Use /test all to run the following jobs that were automatically triggered:

build

generate

gomod

manifestlint

shellcheck

test

unit

In response to this:

/test ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Makefile

tuminoid

/approve

metal3-io-bot · 2025-01-24T10:43:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tuminoid

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [tuminoid]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tuminoid · 2025-01-24T10:43:33Z

/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

mquhuy · 2025-01-24T14:35:50Z

This won't work. For e.g. check https://jenkins.nordix.org/blue/rest/organizations/jenkins/pipelines/metal3-centos-e2e-integration-test-main/runs/887/nodes/21/steps/35/log/?start=0

export CAPM3_IMAGE=registry.nordix.org/quay-io-proxy/metal3-io/cluster-api-provider-metal3:main

=> This is devenv's default. It should have taken into use the value we define here in CAPM3 e2e (tag should be latest).

These values should have been exported inside ci-e2e.sh, also please check that there are some default values, like the CAPM3RELEASE, which is defined based on the CAPM3RELEASEBRANCH (same redefinition will happen inside devenv), so the redefinition of that var we do here won't likely take any effects.

mquhuy · 2025-01-24T14:36:55Z

/hold
until the issue above is sorted out.

peppi-lotta · 2025-01-27T10:14:23Z

This won't work. For e.g. check https://jenkins.nordix.org/blue/rest/organizations/jenkins/pipelines/metal3-centos-e2e-integration-test-main/runs/887/nodes/21/steps/35/log/?start=0
export CAPM3_IMAGE=registry.nordix.org/quay-io-proxy/metal3-io/cluster-api-provider-metal3:main
=> This is devenv's default. It should have taken into use the value we define here in CAPM3 e2e (tag should be latest).

These values should have been exported inside ci-e2e.sh, also please check that there are some default values, like the CAPM3RELEASE, which is defined based on the CAPM3RELEASEBRANCH (same redefinition will happen inside devenv), so the redefinition of that var we do here won't likely take any effects.

I changed the code a little bit. These values are set and used after dev-env make has ran. Here we want to use the values that have been defined in dev-env or default to latest.

I'm not trying the set these values for the beginning of the testing process I'm just trying to make sure the same images are used trough each part of a given test.

peppi-lotta · 2025-01-28T07:24:54Z

/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

peppi-lotta · 2025-01-28T07:25:09Z

/unhold

mquhuy · 2025-01-28T08:32:05Z

I changed the code a little bit. These values are set and used after dev-env make has ran. Here we want to use the values that have been defined in dev-env or default to latest.

I'm not trying the set these values for the beginning of the testing process I'm just trying to make sure the same images are used trough each part of a given test.

It also wouldn't work in that case. Devenv make is ran inside a subshell, so whatever values it has set won't be picked by either ci-e2e.sh or make. The only way you could make sure the two processes use the same images is by setting the images here towards devenv (via ci-e2e.sh), not vice verse.

Signed-off-by: peppi-lotta <[email protected]>

peppi-lotta · 2025-01-30T08:40:54Z

I changed the code a little bit. These values are set and used after dev-env make has ran. Here we want to use the values that have been defined in dev-env or default to latest.
I'm not trying the set these values for the beginning of the testing process I'm just trying to make sure the same images are used trough each part of a given test.

It also wouldn't work in that case. Devenv make is ran inside a subshell, so whatever values it has set won't be picked by either ci-e2e.sh or make. The only way you could make sure the two processes use the same images is by setting the images here towards devenv (via ci-e2e.sh), not vice verse.

Oh I see you pont! I've made the change now so that it is using values that should be defined and coming from project-infra.

metal3-io-bot requested review from kashifest and mboukhalfa January 23, 2025 13:49

metal3-io-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 23, 2025

tuminoid reviewed Jan 23, 2025

View reviewed changes

Makefile Outdated Show resolved Hide resolved

peppi-lotta force-pushed the peppi-lotta/pre-pulling-images branch 3 times, most recently from 2083803 to a5add50 Compare January 23, 2025 15:13

tuminoid reviewed Jan 24, 2025

View reviewed changes

peppi-lotta force-pushed the peppi-lotta/pre-pulling-images branch from a5add50 to 11da043 Compare January 24, 2025 07:54

tuminoid reviewed Jan 24, 2025

View reviewed changes

peppi-lotta force-pushed the peppi-lotta/pre-pulling-images branch from 11da043 to 29f6f85 Compare January 24, 2025 08:36

metal3-io-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 24, 2025

tuminoid reviewed Jan 24, 2025

View reviewed changes

Makefile Outdated Show resolved Hide resolved

peppi-lotta force-pushed the peppi-lotta/pre-pulling-images branch from 29f6f85 to 73e17a7 Compare January 24, 2025 10:39

tuminoid reviewed Jan 24, 2025

View reviewed changes

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2025

metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 24, 2025

peppi-lotta force-pushed the peppi-lotta/pre-pulling-images branch from 73e17a7 to 2b55839 Compare January 27, 2025 10:04

metal3-io-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 27, 2025

peppi-lotta force-pushed the peppi-lotta/pre-pulling-images branch from 2b55839 to b0de156 Compare January 27, 2025 11:42

metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 28, 2025

peppi-lotta marked this pull request as draft January 29, 2025 07:41

metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 29, 2025

Fix: pulling unused images in the middle of tests

222b8f6

Signed-off-by: peppi-lotta <[email protected]>

peppi-lotta force-pushed the peppi-lotta/pre-pulling-images branch from b0de156 to 222b8f6 Compare January 30, 2025 08:38

metal3-io-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Fix: pulling unused images in the middle of tests #2271

🐛 Fix: pulling unused images in the middle of tests #2271

peppi-lotta commented Jan 23, 2025 •

edited

Loading

tuminoid commented Jan 24, 2025

tuminoid left a comment

tuminoid Jan 24, 2025

peppi-lotta Jan 24, 2025

tuminoid Jan 24, 2025

tuminoid commented Jan 24, 2025

tuminoid commented Jan 24, 2025

peppi-lotta commented Jan 24, 2025

peppi-lotta commented Jan 24, 2025

tuminoid Jan 24, 2025

tuminoid commented Jan 24, 2025

tuminoid commented Jan 24, 2025

metal3-io-bot commented Jan 24, 2025

tuminoid left a comment

metal3-io-bot commented Jan 24, 2025

tuminoid commented Jan 24, 2025

mquhuy commented Jan 24, 2025 •

edited

Loading

mquhuy commented Jan 24, 2025

peppi-lotta commented Jan 27, 2025 •

edited

Loading

peppi-lotta commented Jan 28, 2025

peppi-lotta commented Jan 28, 2025

mquhuy commented Jan 28, 2025

peppi-lotta commented Jan 30, 2025

	CAPM3_IMAGE ?= ${CONTAINER_REGISTRY}/metal3-io/cluster-api-provider-metal3:${CAPM3RELEASE:-latest}
	CAPM3RELEASE ?= latest
	CAPM3_IMAGE ?= $(CONTAINER_REGISTRY)/metal3-io/cluster-api-provider-metal3:$(CAPM3RELEASE)

🐛 Fix: pulling unused images in the middle of tests #2271

Are you sure you want to change the base?

🐛 Fix: pulling unused images in the middle of tests #2271

Conversation

peppi-lotta commented Jan 23, 2025 • edited Loading

tuminoid commented Jan 24, 2025

tuminoid left a comment

Choose a reason for hiding this comment

tuminoid Jan 24, 2025

Choose a reason for hiding this comment

peppi-lotta Jan 24, 2025

Choose a reason for hiding this comment

tuminoid Jan 24, 2025

Choose a reason for hiding this comment

tuminoid commented Jan 24, 2025

tuminoid commented Jan 24, 2025

peppi-lotta commented Jan 24, 2025

peppi-lotta commented Jan 24, 2025

tuminoid Jan 24, 2025

Choose a reason for hiding this comment

tuminoid commented Jan 24, 2025

tuminoid commented Jan 24, 2025

metal3-io-bot commented Jan 24, 2025

tuminoid left a comment

Choose a reason for hiding this comment

metal3-io-bot commented Jan 24, 2025

tuminoid commented Jan 24, 2025

mquhuy commented Jan 24, 2025 • edited Loading

mquhuy commented Jan 24, 2025

peppi-lotta commented Jan 27, 2025 • edited Loading

peppi-lotta commented Jan 28, 2025

peppi-lotta commented Jan 28, 2025

mquhuy commented Jan 28, 2025

peppi-lotta commented Jan 30, 2025

peppi-lotta commented Jan 23, 2025 •

edited

Loading

mquhuy commented Jan 24, 2025 •

edited

Loading

peppi-lotta commented Jan 27, 2025 •

edited

Loading