Refactor and fix all upgrade integration tests #3477

blakerouse · 2023-09-26T17:41:18Z

What does this PR do?

This refactors and fixes all upgrade integration tests. This provides a unified path for the tests to follow for testing upgrade. Includes code to make the upgrades faster, and allows downgrades to work. Includes fixes for upgrade tests that where skipped because they where flaky and enables the retry upgrade test on Windows.

Why is it important?

Ensures that all upgrade tests are working properly so we can catch real errors instead of fighting the CI.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~[ ] I have made corresponding changes to the documentation~~
~~[ ] I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works
~~[ ] I have added an entry in ./changelog/fragments using the changelog tool~~
I have added an integration test or an E2E test

How to test this PR locally

mage integration:test

Related issues

blakerouse · 2023-09-26T17:45:33Z

This has a replace present until this PR is merged and new elastic-agent-libs is released - elastic/elastic-agent-libs#152

elasticmachine · 2023-09-26T17:48:09Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-10-02T13:46:00.500+0000
Duration: 29 min 56 sec

Test stats 🧪

Test	Results
Failed	0
Passed	6425
Skipped	59
Total	6484

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages.
run integration tests : Run the Elastic Agent Integration tests.
run end-to-end tests : Generate the packages and run the E2E Tests.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2023-09-26T18:26:10Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	98.78% (`81/82`)	❕
Files	65.886% (`197/299`)	❕
Classes	65.343% (`362/554`)	❕
Methods	52.697% (`1143/2169`)	❕
Lines	38.328% (`13018/33965`)	❕
Conditionals	100.0% (`0/0`)	💚

cmacknz · 2023-09-26T19:21:55Z

testing/** needs to be added to the SonarQube exclusion list to silence the coverage failure.

https://github.com/elastic/elastic-agent/blob/main/sonar-project.properties

cmacknz

Looks good so far, want to see the tests passing before approving.

Tests are much easier to follow like this (at least to me).

pkg/testing/fetcher_artifact.go

pkg/testing/fixture.go

testing/upgradetest/upgrader.go

testing/upgradetest/watcher.go

pchila

There is at least 1 test that has changed meaning in the refactor: TestStandaloneDowngradeToPreviousSnapshotBuild is supposed to test the upgrade in form of elastic-agent upgrade x.y.z-SNAPSHOT+<buildid> and from what I see it doesn't do that anymore but it just performs a downgrade 2 minors back

There are a few other comments on other parts of the code but another element that pops is that we use version.GetPreviousMinor() more even if we know that it has some limitation as it tries to guess what the previous minor would be.
Maybe you want to take a look at #3458 (which I imagine is now replaced by this) for a possible alternative of selecting previous versions

testing/integration/upgrade_broken_package_test.go

testing/integration/upgrade_fleet_test.go

testing/integration/upgrade_downgrade_test.go

testing/upgradetest/upgrader.go

blakerouse · 2023-09-27T15:46:57Z

The modreplace has been removed but the lint is still failing, not clear as to why.

testing/upgradetest/upgrader.go

…s are different and the commits are the same.

blakerouse · 2023-09-28T20:33:37Z

@ycombinator Don't worry about it. I have fixed the conflict and updated the test to use the new patterns.

blakerouse · 2023-09-28T21:07:06Z

@pchila I believe this is ready for a final review. I have fixed all conflicts and issues you pointed out.

mergify · 2023-09-29T00:44:28Z

This pull request is now in conflicts. Could you fix it? 🙏
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 fix-all-upgrade-tests upstream/fix-all-upgrade-tests
git merge upstream/main
git push upstream fix-all-upgrade-tests

pchila

The TestStandaloneDowngradeToSpecificSnapshotBuild looks better but we need to select the penultimate build to have a result that is noticeably different from a normal upgrade to a snapshot version

pkg/testing/fetcher_artifact.go

testing/integration/upgrade_downgrade_test.go

…view.

blakerouse · 2023-09-29T13:27:39Z

@pchila I have fixed that test and fixed the conflicts.

@ycombinator I have fixed the two tests in conflict if you want to give them a look over. Both where new tests that you added.

pchila

LGTM

ycombinator · 2023-09-29T16:49:55Z

testing/integration/upgrade_standalone_inprogress.go

+	// before trying to perform another upgrade
+	err = upgradetest.WaitHealthyAndVersion(ctx, startFixture, endVersionInfo.Binary, 2*time.Minute, 10*time.Second, t)
+	if err != nil {
+		// agent never got healthy, but we need to ensure the watcher is stopped before continuing


It wasn't immediately obvious to me why the watcher was being killed here. I think it's so it doesn't interfere with other tests (as opposed to having anything to do with continuing on in this test, since this test should now fail due to the require.NoError(t, err) on line 79), right? Could you clarify that in the code comment here please?

I added more to the comment to make this clear. The reason you gave is the correct.

ycombinator · 2023-09-29T16:53:34Z

testing/integration/upgrade_standalone_inprogress.go

+	// killTimeout is greater than timeout as the watcher should have been
+	// stopped on its own, and we don't want this test to hide that fact


ycombinator · 2023-09-29T16:57:06Z

testing/integration/upgrade_rollback_test.go

+	// Start at the build version as we want to test the retry
+	// logic that is in the build.


I think this comment is not relevant to this test?

It is relevant.

ycombinator · 2023-09-29T16:58:31Z

testing/integration/upgrade_rollback_test.go

+		// agent never got healthy, but we need to ensure the watcher is stopped before continuing
+		// this kills the watcher instantly and waits for it to be gone before continuing


Same comment as https://github.com/elastic/elastic-agent/pull/3477/files#r1341596538 about the "continuing" being unclear.

ycombinator · 2023-09-29T16:58:48Z

testing/integration/upgrade_rollback_test.go

+	// killTimeout is greater than timeout as the watcher should have been
+	// stopped on its own, and we don't want this test to hide that fact


ycombinator · 2023-09-29T16:59:56Z

@ycombinator I have fixed the two tests in conflict if you want to give them a look over. Both where new tests that you added.

@blakerouse I reviewed the two tests. The logic in both looks good and even improved towards the end of the tests! I left some feedback about clarifying comments, that's all.

elastic-sonarqube · 2023-10-02T13:57:45Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

pchila · 2023-10-02T15:03:07Z

@blakerouse If we have a snapshot with a buildId we need to prepare the uri in the format of x.y.z-, for example
https://snapshots.elastic.co/8.11.0-d55157ca/downloads/beats/elastic-agent/elastic-agent-8.11.0-SNAPSHOT-linux-x86_64.tar.gz (we can see the build details from https://artifacts-api.elastic.co/v1/versions/8.11.0-SNAPSHOT/builds/8.11.0-d55157ca)

From the error in https://buildkite.com/elastic/elastic-agent/builds/3611 it seems the artifact fetcher is not resolving the uri correctly

blakerouse · 2023-10-02T15:46:55Z

@pchila You are correct it was not, my most recent commit fixed it. See here - 967969f

* Fix all upgrade tests. * Fix imports and headers. * Update notice. * Exclude testing/** from sonar. * Fix comments from code review. * Add extra error information in the artifact fetcher. * Fixes from code review. * Add upgrade uninstall kill watcher test. * Remove go replace. Regenerate notice. Fix lint. * Import WithSourceURI logic. Fix fleet test to not skip if the versions are different and the commits are the same. * More test fixes. * Fix imports. * Re-add TestStandaloneUpgradeFailsWhenUpgradeIsInProgress. Fix code review. (cherry picked from commit 6201e19) # Conflicts: # sonar-project.properties # testing/integration/upgrade_test.go

…#3495) * Refactor and fix all upgrade integration tests (#3477) * Fix all upgrade tests. * Fix imports and headers. * Update notice. * Exclude testing/** from sonar. * Fix comments from code review. * Add extra error information in the artifact fetcher. * Fixes from code review. * Add upgrade uninstall kill watcher test. * Remove go replace. Regenerate notice. Fix lint. * Import WithSourceURI logic. Fix fleet test to not skip if the versions are different and the commits are the same. * More test fixes. * Fix imports. * Re-add TestStandaloneUpgradeFailsWhenUpgradeIsInProgress. Fix code review. (cherry picked from commit 6201e19) # Conflicts: # sonar-project.properties # testing/integration/upgrade_test.go * Fix merge. --------- Co-authored-by: Blake Rouse <blake.rouse@elastic.co>

Fix all upgrade tests.

6087ed1

blakerouse added Team:Elastic-Agent Label for the Agent team backport-v8.10.0 Automated backport with mergify labels Sep 26, 2023

blakerouse requested a review from a team as a code owner September 26, 2023 17:41

blakerouse self-assigned this Sep 26, 2023

blakerouse requested review from AndersonQ and michalpristas September 26, 2023 17:41

blakerouse added the skip-changelog label Sep 26, 2023

Fix imports and headers.

791bb01

This was referenced Sep 26, 2023

Change the selection of downgrade version in TestUpgradeBrokenPackageVersion #3458

Closed

Move agent upgrade tests to official snapshots to a scheduled job #3475

Closed

Update notice.

68d76ee

Exclude testing/** from sonar.

b3e6492

cmacknz requested review from pchila and ycombinator September 26, 2023 20:31

cmacknz reviewed Sep 26, 2023

View reviewed changes

blakerouse added 2 commits September 26, 2023 23:04

Fix comments from code review.

829e681

Add extra error information in the artifact fetcher.

2c70114

pchila requested changes Sep 27, 2023

View reviewed changes

blakerouse added 3 commits September 27, 2023 09:34

Fixes from code review.

5f1e53d

Add upgrade uninstall kill watcher test.

57cdd8c

Remove go replace. Regenerate notice. Fix lint.

9920a8c

pchila reviewed Sep 27, 2023

View reviewed changes

testing/upgradetest/upgrader.go Outdated Show resolved Hide resolved

Import WithSourceURI logic. Fix fleet test to not skip if the version…

c19eeaa

…s are different and the commits are the same.

pchila mentioned this pull request Sep 28, 2023

[Standalone Agent] Disallow upgrade if upgrade is already in progress #3473

Merged

7 tasks

blakerouse force-pushed the fix-all-upgrade-tests branch from 800e598 to a264c09 Compare September 28, 2023 20:32

ycombinator mentioned this pull request Sep 28, 2023

Un-skip and fix flaky TestStandaloneUpgradeFailsStatus #3408

Closed

Fix imports.

a2b6c3d

jlind23 requested a review from pchila September 29, 2023 04:47

pchila requested changes Sep 29, 2023

View reviewed changes

pkg/testing/fetcher_artifact.go Outdated Show resolved Hide resolved

testing/integration/upgrade_downgrade_test.go Outdated Show resolved Hide resolved

testing/integration/upgrade_downgrade_test.go Outdated Show resolved Hide resolved

blakerouse added 2 commits September 29, 2023 08:40

Merge branch 'main' into fix-all-upgrade-tests

4bc1905

Re-add TestStandaloneUpgradeFailsWhenUpgradeIsInProgress. Fix code re…

e97f732

…view.

pchila approved these changes Sep 29, 2023

View reviewed changes

blakerouse enabled auto-merge (squash) September 29, 2023 13:46

ycombinator reviewed Sep 29, 2023

View reviewed changes

blakerouse added 2 commits September 30, 2023 09:52

Improve error message for fetcher.

111f598

More fixes from code reviews.

967969f

blakerouse mentioned this pull request Oct 2, 2023

Set force:true in Fleet upgrade request to bypass check on latest released agent version #3494

Closed

7 tasks

blakerouse merged commit 6201e19 into elastic:main Oct 2, 2023
11 checks passed

mergify bot mentioned this pull request Oct 2, 2023

[8.10](backport #3477) Refactor and fix all upgrade integration tests #3495

Merged

pchila mentioned this pull request Oct 3, 2023

Broken/Flaky integration tests on main #3502

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and fix all upgrade integration tests #3477

Refactor and fix all upgrade integration tests #3477

blakerouse commented Sep 26, 2023

blakerouse commented Sep 26, 2023

elasticmachine commented Sep 26, 2023 •

edited

Loading

Build stats

Test stats 🧪

elasticmachine commented Sep 26, 2023 •

edited

Loading

cmacknz commented Sep 26, 2023

cmacknz left a comment

pchila left a comment

blakerouse commented Sep 27, 2023

blakerouse commented Sep 28, 2023

blakerouse commented Sep 28, 2023

mergify bot commented Sep 29, 2023

pchila left a comment

blakerouse commented Sep 29, 2023

pchila left a comment

ycombinator Sep 29, 2023

blakerouse Oct 2, 2023

ycombinator Sep 29, 2023

ycombinator Sep 29, 2023

blakerouse Oct 2, 2023

ycombinator Sep 29, 2023

blakerouse Oct 2, 2023

ycombinator Sep 29, 2023

ycombinator commented Sep 29, 2023

elastic-sonarqube bot commented Oct 2, 2023

pchila commented Oct 2, 2023

blakerouse commented Oct 2, 2023

		// killTimeout is greater than timeout as the watcher should have been
		// stopped on its own, and we don't want this test to hide that fact

		// Start at the build version as we want to test the retry
		// logic that is in the build.

		// agent never got healthy, but we need to ensure the watcher is stopped before continuing
		// this kills the watcher instantly and waits for it to be gone before continuing

Refactor and fix all upgrade integration tests #3477

Refactor and fix all upgrade integration tests #3477

Conversation

blakerouse commented Sep 26, 2023

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Related issues

blakerouse commented Sep 26, 2023

elasticmachine commented Sep 26, 2023 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

elasticmachine commented Sep 26, 2023 • edited Loading

🌐 Coverage report

cmacknz commented Sep 26, 2023

cmacknz left a comment

Choose a reason for hiding this comment

pchila left a comment

Choose a reason for hiding this comment

blakerouse commented Sep 27, 2023

blakerouse commented Sep 28, 2023

blakerouse commented Sep 28, 2023

mergify bot commented Sep 29, 2023

pchila left a comment

Choose a reason for hiding this comment

blakerouse commented Sep 29, 2023

pchila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycombinator commented Sep 29, 2023

elastic-sonarqube bot commented Oct 2, 2023

pchila commented Oct 2, 2023

blakerouse commented Oct 2, 2023

elasticmachine commented Sep 26, 2023 •

edited

Loading

elasticmachine commented Sep 26, 2023 •

edited

Loading