add integration server tests for resources version checks #17312

endorama · 2025-06-19T16:17:20Z

Motivation/summary

Add additional integrationservertest for a recent bug affecting some upgrade paths in APM Server.

Checklist

Update CHANGELOG.asciidoc
Documentation has been updated

For functional changes, consider:

Is it observable through the addition of either logging or metrics?
Is its use being published in telemetry to enable product improvement?
Have system tests been added to avoid regression?

How to test these changes

Related issues

This is useful when you want to manually inspect the results even on success

This aid troubleshooting as you don't need to look into the upgrade-config.yaml file but the indications are printed with the test

github-actions · 2025-06-19T16:17:30Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

mergify · 2025-06-19T16:17:58Z

This pull request does not have a backport label. Could you fix it @endorama? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-7.17 is the label to automatically backport to the 7.17 branch.
backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit.
backport-9./d is the label to automatically backport to the 9./d branch. /d is the digit.
backport-active-all is the label that automatically backports to all active branches.
backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

mergify · 2025-06-20T05:42:54Z

This pull request is now in conflicts. Could you fix it @endorama? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b integrationserver-tests-for-bug upstream/integrationserver-tests-for-bug
git merge upstream/main
git push upstream integrationserver-tests-for-bug

Add support for specifying patch versions in upgrade-config.yaml. If a patch version is present will take precedence over it's own minor version, if both are specified. Patch version use is supported in both keys and values. This allows to specify, for example, specific patch versions not triggering a lazy rollover when upgrading to another patch version. Overhauled the comments in upgrade-config.yaml to reflect this new possibilities and clarify how to write the configuration with examples.

Other than usual bug checks we need one more to check for event.success_count

ericywl · 2025-06-23T07:54:45Z

integrationservertest/apm-version-bug_test.go

+		if len(versions) == 2 { // if 2 versions we expect the first to be bugged and the second to be not
+			return []testStep{
+				steps[0], // create
+				steps[1], // ingest
+				zeroEventIngestedDocs,
+				noEventIngestedInMappings,
+				steps[2], // upgrade
+				steps[3], // ingest
+				someEventIngestedDocs,
+				eventIngestedHasMappings,
+			}
+
+		} else if len(versions) == 3 { // if 3 versions we expect the first to be bugged on create, the second on upgrade, the third to be not
+			return []testStep{
+				steps[0], // create
+				steps[1], // ingest
+				steps[2], // upgrade
+				steps[3], // ingest
+				zeroEventIngestedDocs,
+				noEventIngestedInMappings,
+				steps[4], // upgrade
+				steps[5], // upgrade
+				someEventIngestedDocs,
+				eventIngestedHasMappings,
+			}
+		}


This is definitely quite flimsy, I think having a way to tell when 1 "cycle" (create / upgrade -> ingest -> others) is done would be best. Perhaps having the cycles manually defined e.g.

[]testCycle{ { createStep, ingestStep, }, { upgradeStep, ingestStep, }, ... }

Or some other way to detect it.

You are suggesting to not use the buildTestStep function but create them manually so it's easier to follow the various cycles?

My bad I wasn't clear enough. Creating them manually works, but we can also change how steps are defined in the runner. In the example above, testCycle is essentially just: type testCycle []testStep, and demarcates where one version ends and the other starts.

In your case, since you are inserting the steps in between each "cycle", it would be much less flimsy since you can inject the steps at the end of each cycle.

Just an idea, not sure if it would work well.

Ah I see, I refactored it in a way that I think removes some of the ifs, but did not address this issue directly in 9874d2d (#17312)

I think there is value in doing something you propose if we decide is something we want to introduce but we should take some time to think the feature through, here I'm only interested in a quick way to have additional steps (we could also extract createStep and upgradeStep "builder" functions from the current one, so they are easier to compose, but I considered it out of scope here)

ericywl · 2025-06-23T07:55:44Z

integrationservertest/apm-version-bug_test.go

+
+	// this wraps the usual steps build to add additional checks in between.
+	// Is a bit brittle, maybe we should consider allowing this as a first class citizen.
+	buildteststepsForBug := func(t *testing.T, versions ech.Versions, config upgradeTestConfig) []testStep {


Suggested change

buildteststepsForBug := func(t *testing.T, versions ech.Versions, config upgradeTestConfig) []testStep {

buildTestStepsForBug := func(t *testing.T, versions ech.Versions, config upgradeTestConfig) []testStep {

ericywl · 2025-06-23T07:57:33Z

integrationservertest/apm-version-bug_test.go

+		runner.Run(t)
+	})
+
+	t.Run("8.18.2 to 8.18.3", func(t *testing.T) {


If this includes start, shouldn't it be "8.16.2 to 8.17.x to 8.18.x"? Also vsCache.GetLatestSnapshot gets the latest patch for the minor if you specify x.y, so the description will be inaccurate when new patches come out.

I named this after our test cases for this bug. I don't think we want to merge this PR as it is, with all these test cases for all these versions. This is now useful to ensure our fixes are working but I would make it more general if we want to include it as our regular testing.

I think a necessary test case to add is an upgrade test: from the "previous previous" minor (ie in this case 8.16.2) to the previous minor and than to the current minor. Such a test could find issues that only appears with an upgrade, a scenario we don't yet cover.

ericywl · 2025-06-23T07:58:30Z

integrationservertest/buglib.go

+}
+
+type checkMappingStep struct {
+	datastreamname string


Suggested change

datastreamname string

dataStreamName string

ericywl · 2025-06-23T08:01:49Z

integrationservertest/setup.go

-			t.Log("test failed and cleanup-on-failure is false, skipping cleanup")
-		}
-	})
+	if *doCleanup {


I don't think this works. The doCleanup is defined in main_test.go and so is only accessible in test files.

It does if you run the tests 😅 It's a quick hack for my use case.

Do you think there is value in bringing this to main? If yes I'll do a proper implementation.

TIL, my IDE complains that doCleanup doesn't exist 🤔

It's correct I'd say, mine too. I'd expect it's because it doesn't exist if you're running/compiling the go module 😅 it exists if you run the tests because _tests.go files are included in the compilation.

Finding event.ingested in 8.17.7 already after upgrade from 8.16.2 is expected due to the changes to apm-data between the versions.

Refactor version selection to ease the move to BCs. Don't use vsCache.GetLatestSnapshot as that will change version as we release new ones. These tests are version specific at the moment, so use fixed SNAPSHOT versions

Replace SNAPSHOT versions with BC or released versions. Use SNAPSHOTS for 8.19.0 test as there is no BC version for it yet and we cannot mix releases/BCs and SNAPSHOT versions.

ericywl · 2025-06-24T03:28:26Z

integrationservertest/apm-version-bug_test.go

-	version8190 := ech.NewVersion(8, 19, 0, "SNAPSHOT") // latest 8.19
+	version8162 := vsCache.GetLatestVersionOrSkip(t, "8.16.2")
+	version8177 := vsCache.GetLatestVersionOrSkip(t, "8.17.7")
+	version8178 := vsCache.GetLatestBCOrSkip(t, "8.17.8") // latest 8.17 as of today


This will skip the whole test entirely if the BC does not exist. Since both GetLatestVersionOrSkip and GetLatestBCOrSkip are not used currently, you can update them to achieve what you need instead.

endorama added 7 commits June 19, 2025 13:33

add TypedClient()

91754fb

make sure terraform outputs are available

1eae9bb

allow skipping cleanup entirely

07b3644

This is useful when you want to manually inspect the results even on success

extract get deployment info

cf94108

add tests for bug

6252267

with the new version there are more rollovers

af20d51

print when lazy rollover are expected to happen

b09f302

This aid troubleshooting as you don't need to look into the upgrade-config.yaml file but the indications are printed with the test

endorama added 2 commits June 20, 2025 11:58

rework 8.18.3 to 8.19.0 test

0b64a86

Other than usual bug checks we need one more to check for event.success_count

ericywl reviewed Jun 23, 2025

View reviewed changes

endorama added 3 commits June 23, 2025 12:05

fix 8.18.2 -> 8.18.3 test, add other notes

53bd820

refactor to clarify test steps

9874d2d

fix 8.17.7 to.8.17.8

e068415

Finding event.ingested in 8.17.7 already after upgrade from 8.16.2 is expected due to the changes to apm-data between the versions.

endorama mentioned this pull request Jun 23, 2025

8.18.3 Test Plan #17310

Closed

4 tasks

endorama added 2 commits June 23, 2025 17:30

refactor version selection and lock versions

bf1abc5

Refactor version selection to ease the move to BCs. Don't use vsCache.GetLatestSnapshot as that will change version as we release new ones. These tests are version specific at the moment, so use fixed SNAPSHOT versions

use BC/releases

cf7f10f

Replace SNAPSHOT versions with BC or released versions. Use SNAPSHOTS for 8.19.0 test as there is no BC version for it yet and we cannot mix releases/BCs and SNAPSHOT versions.

ericywl reviewed Jun 24, 2025

View reviewed changes

	buildteststepsForBug := func(t *testing.T, versions ech.Versions, config upgradeTestConfig) []testStep {
	buildTestStepsForBug := func(t *testing.T, versions ech.Versions, config upgradeTestConfig) []testStep {

add integration server tests for resources version checks #17312

Are you sure you want to change the base?

add integration server tests for resources version checks #17312

Uh oh!

Conversation

endorama commented Jun 19, 2025

Motivation/summary

Checklist

How to test these changes

Related issues

Uh oh!

github-actions bot commented Jun 19, 2025

🤖 GitHub comments

Uh oh!

mergify bot commented Jun 19, 2025

Uh oh!

mergify bot commented Jun 20, 2025

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!

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!

Uh oh!