add a DEB integration test #4301

leehinman · 2024-02-21T04:25:26Z

What does this PR do?

Add an integration test that installs elastic-agent via the DEB package and makes sure that logs are sent to Elasticsearch.

Why is it important?

Need to test that DEB packages work.

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

Author's Checklist

[ ]

How to test this PR locally

mage integration:single TestDebLogIngestFleetManaged

Related issues

Relates The integration test framework should test the DEB and RPM installations of the agent #4086

Use cases

Screenshots

Logs

Questions to ask yourself

How are we going to support this in production?
How are we going to measure its adoption?
How are we going to debug this?
What are the metrics I should take care of?
...

mergify · 2024-02-21T04:26:04Z

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

backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

elasticmachine · 2024-02-21T04:51:35Z

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

cmacknz

LGTM, couple questions before approving.

Looking at the test time using a sample size of 1:

This PR's integration test step took 2h11 from https://buildkite.com/elastic/elastic-agent/builds/7435#018dd27c-0e2a-47c9-8e70-14b8a46d9677
The most recent success on main took 1h52m from https://buildkite.com/elastic/elastic-agent/builds/7437#018dd291-acc2-46e4-a2ed-ec33f9c51afb

That's an additional 19 minutes which is a sizeable increase. I am wondering if we can shave some of that off by not building the packages we don't need yet, namely rpm and docker. At the same time, we need them eventually.

I have a strong suspicion the docker packages cost the most time, because it builds several container variants for each architecture, and even if we were testing it we'd only need one of them.

cmacknz · 2024-02-23T01:35:10Z

.buildkite/scripts/steps/integration_tests.sh

@@ -19,7 +19,7 @@ else
 OVERRIDE_TEST_AGENT_VERSION=""
 fi
 # PACKAGE
-AGENT_PACKAGE_VERSION="${OVERRIDE_AGENT_PACKAGE_VERSION}" DEV=true EXTERNAL=true SNAPSHOT=true PLATFORMS=linux/amd64,linux/arm64,windows/amd64 PACKAGES=tar.gz,zip mage package
+AGENT_PACKAGE_VERSION="${OVERRIDE_AGENT_PACKAGE_VERSION}" DEV=true EXTERNAL=true SNAPSHOT=true PLATFORMS=linux/amd64,linux/arm64,windows/amd64 mage package


By removing PACKAGES, I think this also builds all the Docker container variants we don't need. Not wrong, but slower than it could be.

Does adding deb,rpm to the packages list or does it cause problems with windows?

Is it faster if we just build Windows with a separate invocation of mage package? I'm not sure if the underlying cross-compilation is actually parallelized.

We could also make the windows target smart enough to ignore the deb and rpm packages if that was in fact the problem.

Sorry, forgot to add the comment on this. I tried with PACKAGES="tar.gz,zip,deb,rpm" and it failed to build in CI, "compiled mage file failed to run" if I remember correctly. So I think we have a bug in the mage file, I'm still looking into it. If I don't find it today or tomorrow I'll open up an issue.

Thanks, not opposed to optimizing this separate from merging this PR.

So I finally found the timing issue in the magefile for building with the multiple package types defined. We tried build all the package types at the same time. So if you have "zip" and the timing is right you can have a compiled magefile that isn't for the GOOS you want. The "fix" will do the GOOS-ARCH in parallel, so all linux-amd64, then all linux-arm, then windows-amd64.

Interestingly the timing vs main is still exactly the same as what it was before (caveat of sample size of 1 still applies):

1h52m for a recent build on main https://buildkite.com/elastic/elastic-agent/builds/7645

2h11m for the most recent build here https://buildkite.com/elastic/elastic-agent/builds/7624

Blake's suggestion for the future in #4301 (comment) might be a way to reclaim some of this time but not uploading an unnecessary .tar.gz package. Probably worth putting that in an issue so it isn't lost in this PR.

cmacknz · 2024-02-23T01:39:31Z

pkg/testing/fixture_install.go

+	}
+
+	// apt install doesn't enroll, so need to do that
+	enrollArgs := []string{"elastic-agent", "enroll"}


Was this affected by #4084? Or are we just logging errors and not actually changing the exit code of the command?

I am interested to know why this doesn't doesn't catch that problem.

Not sure I'm following. If you run dpkg -i elastic-agent-8.13.0-SNAPSHOT-arm64.deb, to "install" the deb package, there is no place to add the enroll token. So that is what is happening here.

Right you need to run elastic-agent enroll. The "bug" in #4084 is that if you do this right after dpkg -i it fails with an error because it can't restart the agent, because the service isn't running yet.

I now see you don't hit this problem because you systemctl start elastic-agent first. This is not what our default instructions for installing the deb suggest however, although it is a totally reasonable thing to do.

I think we should add systemctl start elastic-agent to the DEB postinst script. Any reason not to do that?

I think it would prevent having elastic-agent enroll --delay-enroll work reliably now that I think about it a bit.

The use case would be to install the deb in a VM image and set it to enrol when it next boots up.

I think this requires it's own issue to discuss I added #4381 to do that. I think we can just leave this as-is for this PR, and adjust when we get #4381 figured out.

pkg/testing/fixture_install.go

cmacknz · 2024-02-23T01:44:40Z

testing/integration/logs_ingestion_test.go

@@ -143,6 +211,7 @@ func testMonitoringLogsAreShipped(
 			"add_cloud_metadata: received error failed requesting openstack metadata: Get \\\"https://169.254.169.254/2009-04-04/meta-data/instance-type\\\": dial tcp 169.254.169.254:443: connect: connection refused",               // okay for the cloud metadata to not work
 			"add_cloud_metadata: received error failed with http status code 404", // okay for the cloud metadata to not work
 			"add_cloud_metadata: received error failed fetching EC2 Identity Document: operation error ec2imds: GetInstanceIdentityDocument, http response error StatusCode: 404, request to EC2 IMDS failed", // okay for the cloud metadata to not work
+			"failed to invoke rollback watcher: failed to start Upgrade Watcher: fork/exec /var/lib/elastic-agent/elastic-agent: no such file or directory",                                                   //on debian this happens probably need to fix.


I assume this only happens for the DEB? If we are the DEB we probably shouldn't even be launching the watcher, or I suppose we could fix the path.

Either way this is worth a follow up issue.

AndersonQ · 2024-02-23T06:20:47Z

.buildkite/scripts/steps/integration_tests.sh

@@ -19,7 +19,7 @@ else
 OVERRIDE_TEST_AGENT_VERSION=""
 fi
 # PACKAGE
-AGENT_PACKAGE_VERSION="${OVERRIDE_AGENT_PACKAGE_VERSION}" DEV=true EXTERNAL=true SNAPSHOT=true PLATFORMS=linux/amd64,linux/arm64,windows/amd64 PACKAGES=tar.gz,zip mage package
+AGENT_PACKAGE_VERSION="${OVERRIDE_AGENT_PACKAGE_VERSION}" DEV=true EXTERNAL=true SNAPSHOT=true PLATFORMS=linux/amd64,linux/arm64,windows/amd64 mage package


AndersonQ · 2024-02-23T06:33:17Z

testing/integration/logs_ingestion_test.go

+	// name. This policy does not contain any integration.
+	t.Log("Enrolling agent in Fleet with a test policy")
+	createPolicyReq := kibana.AgentPolicy{
+		Name:        fmt.Sprintf("test-policy-enroll-%d", time.Now().Unix()),


[NIT]
IIRC, most tests do so by adding a uuid to the policy name.

pchila · 2024-02-23T12:12:02Z

pkg/testing/fetch_test.go

+	gtesting "testing"
+)
+
+func TestGetPackageSuffix(t *gtesting.T) {


pchila

Code looks good, I have just a small doubt about defining a new Deb group (this entails provisioning an extra runner) as I think we could put the test into the default group and skip it on non-debian OSes

pchila · 2024-02-23T12:31:42Z

testing/integration/groups_test.go

@@ -30,4 +30,7 @@ const (

 	// Upgrade group of tests. Used for testing upgrades.
 	Upgrade = "upgrade"
+
+	// Deb group of tests. Used for testing .deb packages install & upgrades
+	Deb = "deb"


Why do we define a separate group here? we could run the deb tests on any linux machine so I would be inclined to add them to default/other existing group (we save spawning one extra runner too)

Why do we define a separate group here? we could run the deb tests on any linux machine so I would be inclined to add them to default/other existing group (we save spawning one extra runner too)

Abundance of caution and me not knowing all the internals of the testing suite :-). I'll drop this. FYI we will have to add Rocky or Alma linux as target distros for the RPM tests. We really need to test DEB & RPM in their native environments.

I mean we don't necessarily need to drop it, putting it into the default group at the moment would save us the overhead. When we add the other runners we may want to re-assess the grouping and see what we can do.
I think ubuntu for the moment should do as a distro, maybe later we want to have proper debian server or other distros that we want to focus our testing on)

Please don't drop this. I think the isolation tests between deb/rpm's and install command installations are good to have.

@pchila & @blakerouse are you OK if we keep the "Deb" group for now, so we can get some deb testing started. And we come to a consensus when I work on the RPM test cases?

Good for me 👍

pchila · 2024-02-23T12:37:32Z

testing/integration/logs_ingestion_test.go

@@ -143,6 +211,7 @@ func testMonitoringLogsAreShipped(
 			"add_cloud_metadata: received error failed requesting openstack metadata: Get \\\"https://169.254.169.254/2009-04-04/meta-data/instance-type\\\": dial tcp 169.254.169.254:443: connect: connection refused",               // okay for the cloud metadata to not work
 			"add_cloud_metadata: received error failed with http status code 404", // okay for the cloud metadata to not work
 			"add_cloud_metadata: received error failed fetching EC2 Identity Document: operation error ec2imds: GetInstanceIdentityDocument, http response error StatusCode: 404, request to EC2 IMDS failed", // okay for the cloud metadata to not work
+			"failed to invoke rollback watcher: failed to start Upgrade Watcher: fork/exec /var/lib/elastic-agent/elastic-agent: no such file or directory",                                                   //on debian this happens probably need to fix.


blakerouse · 2024-03-07T15:00:20Z

testing/integration/groups_test.go

@@ -30,4 +30,7 @@ const (

 	// Upgrade group of tests. Used for testing upgrades.
 	Upgrade = "upgrade"
+
+	// Deb group of tests. Used for testing .deb packages install & upgrades
+	Deb = "deb"


Please don't drop this. I think the isolation tests between deb/rpm's and install command installations are good to have.

blakerouse · 2024-03-07T15:04:17Z

testing/integration/logs_ingestion_test.go

+		},
+		Local: false,
+		Sudo:  true,
+	})


I am not saying do it in this PR, but another way of handling the different between deb/RPM vs install subcommand would be to define it in the test requirements. An example could be to change to:

info := define.Require(t, define.Requirements{ Group: Deb, Stack: &define.Stack{}, Packaging: define.Deb, Local: false, Sudo: true, })

The testing framework could then look at the Packaging: define.Deb and determine automatically that this needs to be on an OS that supports deb's. Each OS can say what it supports in its definition. Then when running the framework with MATRIX="true" it could run on Ubuntu, Debian, etc. By default it would just pick Ubuntu. Doing it this way would also allow only the deb to be copied to the host. Currently with this change, it is going to copy both the targz and deb to the host, when both are not needed.

elastic-sonarqube · 2024-03-07T17:43:11Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% 0.0% Duplication on New Code

See analysis details on SonarQube

leehinman · 2024-03-07T20:14:47Z

@blakerouse @pchila @cmacknz are we "OK" to merge or anything else we have to address for this PR?

We absolutely have changes that need to happen before adding RPM.

cmacknz · 2024-03-08T19:31:31Z

I am fine to defer future improvements to other PRs and issues.

The only thing I noticed after approving is that #4334 was merged before this. We could add a check that the files were actually deleting after running apt-get purge as part of this test to ensure we don't regress on cleanup of the DEB package.

This reverts commit 4fd31f1.

This reverts commit 4fd31f1. The Serverless Beats Tests step in our daily builds started failing with: > Error: error running test: missing required Elastic Agent package builds for integration runner to execute: /tmp/beats-build/beats/x-pack/metricbeat/build/distributions/elastic-agent-8.14.0-SNAPSHOT-linux-x86_64.tar.gz, /tmp/beats-build/beats/x-pack/metricbeat/build/distributions/elastic-agent-8.14.0-SNAPSHOT-linux-x86_64.tar.gz.sha512, /tmp/beats-build/beats/x-pack/metricbeat/build/distributions/elastic-agent-8.14.0-SNAPSHOT-x86_64.rpm, /tmp/beats-build/beats/x-pack/metricbeat/build/distributions/elastic-agent-8.14.0-SNAPSHOT-x86_64.rpm.sha512, /tmp/beats-build/beats/x-pack/metricbeat/build/distributions/elastic-agent-8.14.0-SNAPSHOT-amd64.deb, /tmp/beats-build/beats/x-pack/metricbeat/build/distributions/elastic-agent-8.14.0-SNAPSHOT-amd64.deb.sha512 ~/builds/bk-agent-prod-gcp-1710406561496364062/elastic/elastic-agent The last successful build is https://buildkite.com/elastic/elastic-agent/builds/7707 The first failure is https://buildkite.com/elastic/elastic-agent/builds/7735 There were only 2 changes made between these, this change is one of them. Let's try to revert this first because it has the least impact.

mergify bot assigned leehinman Feb 21, 2024

mergify bot added the backport-skip label Feb 21, 2024

leehinman force-pushed the 4086_integration_deb_rpm branch from c9465c0 to c3bb8a1 Compare February 21, 2024 04:29

leehinman added the skip-changelog label Feb 21, 2024

leehinman force-pushed the 4086_integration_deb_rpm branch 3 times, most recently from 44b4692 to fe02cc3 Compare February 21, 2024 04:43

leehinman marked this pull request as ready for review February 21, 2024 04:51

leehinman requested a review from a team as a code owner February 21, 2024 04:51

leehinman requested review from AndersonQ and michalpristas February 21, 2024 04:51

leehinman added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Feb 21, 2024

leehinman force-pushed the 4086_integration_deb_rpm branch 2 times, most recently from 483028d to 25002c5 Compare February 21, 2024 20:47

cmacknz requested a review from pchila February 21, 2024 20:55

leehinman force-pushed the 4086_integration_deb_rpm branch 5 times, most recently from 5c4a06c to 93ea3e1 Compare February 22, 2024 20:21

cmacknz reviewed Feb 23, 2024

View reviewed changes

AndersonQ approved these changes Feb 23, 2024

View reviewed changes

pchila reviewed Feb 23, 2024

View reviewed changes

pkg/testing/fetch_test.go

gtesting "testing"

)

func TestGetPackageSuffix(t *gtesting.T) {

Copy link

Member

pchila Feb 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

pchila approved these changes Feb 23, 2024

View reviewed changes

leehinman mentioned this pull request Feb 23, 2024

[debian package] failed to invoke rollback watcher #4332

Open

leehinman added 2 commits March 6, 2024 19:06

add a DEB integration test

972a42a

fix package building of rpm and deb packages

c8a865f

leehinman force-pushed the 4086_integration_deb_rpm branch from 93ea3e1 to c8a865f Compare March 7, 2024 01:07

pierrehilbert linked an issue Mar 7, 2024 that may be closed by this pull request

The integration test framework should test the DEB and RPM installations of the agent #4086

Closed

blakerouse reviewed Mar 7, 2024

View reviewed changes

fix rebase conflicts

bce8d27

leehinman force-pushed the 4086_integration_deb_rpm branch from a2af0fd to bce8d27 Compare March 7, 2024 17:13

cmacknz approved these changes Mar 8, 2024

View reviewed changes

leehinman merged commit 4fd31f1 into elastic:main Mar 12, 2024
9 checks passed

jlind23 mentioned this pull request Mar 14, 2024

The integration test framework should test the DEB and RPM installations of Beats elastic/beats#38334

Open

rdner added a commit that referenced this pull request Mar 15, 2024

Revert "add a DEB integration test (#4301)"

ef7abb1

This reverts commit 4fd31f1.

rdner mentioned this pull request Mar 15, 2024

Revert "add a DEB integration test" #4419

Merged

This was referenced Mar 18, 2024

integration deb rpm #4424

Merged

add integration test for install via RPM on SLES #4461

Merged

add a DEB integration test #4301

add a DEB integration test #4301

Conversation

leehinman commented Feb 21, 2024

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

Questions to ask yourself

mergify bot commented Feb 21, 2024

elasticmachine commented Feb 21, 2024

cmacknz 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

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

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

pchila left a comment • edited Loading

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

Choose a reason for hiding this comment

elastic-sonarqube bot commented Mar 7, 2024

Quality Gate passed

leehinman commented Mar 7, 2024

cmacknz commented Mar 8, 2024

pchila left a comment •

edited

Loading