Added s390x arch in Makefiles to build multi arch images and skipped some tests on s390x because of image unavailability by rajnish-jauhari · Pull Request #2130 · skupperproject/skupper

rajnish-jauhari · 2025-05-19T06:44:10Z

Added s390x arch in Makefiles to build multi arch images and
Skipping below tests on s390x because of image unavailability :

TestGateway/local-gateway-service
TestBookinfo
TestMongo
TestHipsterShop
TestPostgres

cc: @hash-d

hash-d

For the tests that cannot be run on Z because of missing images, the cluster's architecture should be checked, instead of the local test machine.

If you're running from an x86 VM pointing to a Z cluster, for example, those tests would not be skipped and the test would fail.

hash-d · 2025-09-18T20:19:10Z

test/integration/examples/bookinfo_test.go

 )

 func TestBookinfo(t *testing.T) {
+	if runtime.GOARCH == "s390x" {


Checking runtime.GOARCH is ok for testLocalGatewayService, because that's a local test. For TestBookInfo, however, you should be inspecting the cluster's architecture, not the local machine.

Check how that's done for ARM, you can adapt that for Z.

rajnish-jauhari · 2025-11-10T08:58:40Z

@hash-d made the changes as requested by you

hash-d

Please make the check pass, and we should be good to merge.

hash-d · 2025-11-14T14:37:04Z

test/integration/examples/custom/hipstershop/hipstershop_test.go

+	//Skipping test on s390x architecture as images unavailable for s390x
+	cluster := base.GetClusterContext()
+	if err := arch.SkipOnlyS390x(t, cluster); err != nil {
+	t.Fatal(err)


The check failed because of incorrect Go formatting. This is one such case (missing indent). Please fix these so the check passes.

hash-d · 2025-11-14T14:41:57Z

test/utils/arch/k8s.go

 }
+
+// CheckOnlyS390x skips ONLY if the architecture is s390x
+func CheckOnlyS390x(clusters ...*base.ClusterContext) (err error, skip bool) {


It would have been best to generalize Check(), as listed on its TODO, but this works fine

hash-d · 2026-02-19T19:30:57Z

@rajnish-jauhari, while I'm good with the PR, you'll need to address the build failures.

Here's what I found with help from @fgiorgetti:

When connecting to a failed build with SSH, running the same command results in the same error.
However, trying to user docker ps also results in error:

circleci@ip-x:~/project$ docker ps
Error response from daemon: client version 1.53 is too new. Maximum supported API version is 1.41

Looking into CircleCI documentation for the command install_docker [1], it states:

Version of Docker to install, defaults to the latest stable release.
And there has been some recent Docker releases [2]. In particular, 29.2.0 has been released in the end of January [3]

Our guess is that, while the Docker orb is tied to version 2.6.0, the docker/install-docker step is not (at least not in all its invocations, and in particular it is not in the invocation on build_and_save_test_images), and that the Docker version of the executor is not compatible with the latest Docker release.

So, check .circleci/config.yml, and you'll see other calls to install-docker; try setting the version on all calls to docker-install and see whether the build passes.

[1] https://circleci.com/developer/orbs/orb/circleci/docker#commands-install_docker
[2] https://docs.docker.com/engine/release-notes/
[3] https://docs.docker.com/engine/release-notes/29/#2920

hash-d · 2026-02-20T17:45:17Z

@rajnish-jauhari, it seems the Docker issue is now bypassed, but there are still some code issues. Please review the test failure logs and fix them.

rajnish-jauhari · 2026-02-25T08:35:39Z

@hash-d , after making necessary changes, all the checks are now passing, please provide a review regarding the same

hash-d

I think it is ok that formatting changes are introduced on files that are changing for the purposes of the PR, even if they pollute the diff (for the PR review and for future git log analysis).

However, the removals of comments and changes in message strings adds burden to the review and do not really add to the final product.

In fact, some of those changes may be detrimental to the tests. Some comments are there to explain the reason a piece of the code is as it is; logs may have been added to help debug recurring problems.

In specific, for example on test/integration/examples/mongodb/mongo.go, I can't see any changes that relate to the PR. Perhaps they are buried behind the other changes?

The changes on test/integration/examples/bookinfo_test.go are not only unnecessary, they actually change the way the test behaves for other architectures. Are those changes positive? I honestly do not know, but I don't think we want to introduce changes to v1 this late in the game.

Please remove the extraneous changes and submit again.

hash-d · 2026-02-26T12:16:38Z

test/integration/examples/mongodb/mongo.go


-	// Make sure you have a minimum of three members on the replicaset
-	// https://www.mongodb.com/docs/manual/tutorial/deploy-replica-set/
-	// https://www.mongodb.com/docs/manual/core/replica-set-architecture-three-members/


Why did you remove these comments?

hash-d · 2026-02-26T12:17:34Z

test/integration/examples/mongodb/mongo.go


-	// Let's wait until the election is settled, for a maximum of 5 min
-	log.Print("Waiting for the Mongo replicaset primary member election to be settled")
+	log.Print("Waiting for Mongo replicaset primary election")


Here, too, why make these changes? What's the relation of them with the PR?

hash-d · 2026-02-26T12:22:36Z

test/integration/examples/mongodb/mongo.go

-	job, err := k8s.WaitForJob(pubCluster1.Namespace, pubCluster1.VanClient.KubeClient, jobName, constants.ImagePullingAndResourceCreationTimeout)
-	jobLogs, _ := k8s.GetJobsLogs(pubCluster1.Namespace, pubCluster1.VanClient.KubeClient, jobName, true)
-	t.Logf("%s logs:", jobName)
-	t.Logf("%s", jobLogs)


Why removing those logs?

hash-d · 2026-02-26T12:38:19Z

test/integration/examples/bookinfo_test.go

+	if err != nil {
+		t.Fatal(err)
+	}
+


The changes above are completely changing how the test behaves, why are you doing that? A shared ClusterTestRunnerBase already exists and is created on test/integration/examples/main_test.go, and was used by this test, why create a new ClusterNeeds?

You probably want to just add arch.SkipOnlyS390x() to Setup() on test/integration/examples/bookinfo/bookinfo.go, instead.

hash-d · 2026-02-26T12:41:15Z

test/integration/performance/postgres_test.go

+	needs := base.ClusterNeeds{
+		NamespaceId:     "perf-archcheck",
+		PublicClusters:  1,
+		PrivateClusters: 0,


Same thing as for Mongo test, here: why a new ClusterNeeds?

hash-d · 2026-02-26T12:42:10Z

test/integration/performance/postgres_test.go

+	}
+
+	// Cleanup the temporary namespace used for arch detection
+	defer base.RemoveNamespacesForContexts(r, []int{1}, []int{})


Why a temporary namespace for arch detection? The creation and removal of namespaces add time to the test execution; use the actual namespaces, instead.

Also, probably not the case here as the focus is on v2, but if new namespaces are introduced in the test by future changes, the temporary ones may be left behind, unaltered. It's better to test the actual namespaces, in the place where they are created or accessed by the test, to ensure future changes take the verification you're introducing in mind.

hash-d · 2026-02-26T12:47:04Z

test/utils/arch/k8s.go

 // use build flags or some other technique.
-//
-// Usage: check first skip; only check err if skip is false.  If skip is true, error
-// will be non-nil, with information on why skipping


Why removing the Usage documentation?

hash-d · 2026-02-26T12:47:27Z

test/utils/arch/k8s.go

-// will be non-nil, with information on why skipping
-//
-// TODO: make it more granular, allow for hibrid clusters?
-// TODO: allow for list of accepted archs?


These TODOs have not been implemented; why removing them?

…some tests on s390x because of image unavailability

rajnish-jauhari · 2026-02-27T07:21:34Z

@hash-d all the requested changes have been made, please review it

hash-d self-requested a review September 18, 2025 20:06

hash-d requested changes Sep 18, 2025

View reviewed changes

rajnish-jauhari force-pushed the v1 branch 2 times, most recently from aaec1ba to b71e806 Compare November 10, 2025 08:22

hash-d requested changes Nov 14, 2025

View reviewed changes

rajnish-jauhari force-pushed the v1 branch 2 times, most recently from c6c8fbd to 5717795 Compare February 16, 2026 07:02

rajnish-jauhari force-pushed the v1 branch from 5d9d8cb to 01d8ccb Compare February 20, 2026 08:08

rajnish-jauhari force-pushed the v1 branch from f472420 to 79a7774 Compare February 25, 2026 05:27

hash-d requested changes Feb 26, 2026

View reviewed changes

rajnish-jauhari force-pushed the v1 branch from ecc2d39 to 1e3816d Compare February 26, 2026 16:37

Added s390x arch in Makefiles to build multi arch images and skipped …

e5211aa

…some tests on s390x because of image unavailability

rajnish-jauhari force-pushed the v1 branch from ea671e8 to e5211aa Compare February 27, 2026 05:41

Conversation

rajnish-jauhari commented May 19, 2025

Uh oh!

hash-d left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajnish-jauhari commented Nov 10, 2025

Uh oh!

hash-d left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hash-d commented Feb 19, 2026

Uh oh!

hash-d commented Feb 20, 2026

Uh oh!

rajnish-jauhari commented Feb 25, 2026

Uh oh!

hash-d left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajnish-jauhari commented Feb 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants