-
Notifications
You must be signed in to change notification settings - Fork 148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add beat serverless tests & related support #3258
Conversation
Pinging @elastic/elastic-agent (Team:Elastic-Agent) |
buildkite failures appear to be unrelated: |
🌐 Coverage report
|
@pazone didn't you fix the fetchBinaryFromArtifactsApi issue? |
Best option I can think of that wouldn't require changing what Beats does (like injecting a tag) is to have the test code to look at the |
@pierrehilbert It's a bit another issue. Fixing it |
Alright, took @joshdover 's advice, and the dashboard tests will now check the updated time. |
pkg/testing/runner/debian.go
Outdated
@@ -106,6 +106,7 @@ func (DebianRunner) Copy(ctx context.Context, sshClient *ssh.Client, logger Logg | |||
"failed to remove agent directory before unziping new one: %w. stdout: %q, stderr: %q", | |||
err, stdout, stderr) | |||
} | |||
_, _, _ = sshRunCommand(ctx, sshClient, "sudo", []string{"rm", "-rf", "beats"}, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this removing a random beats directory? We set the remote path to $HOME/agent
I don't see why the same cannot be used for beats?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the working directory is the same here, since tests are still being run out of the agent repo, the only difference is that now we have additional beat artifacts that we upload in addition to the agent working directory.
We could refactor this so we don't have /agent
hard-coded, but we'd still need to find a way to delete the working directory and any non-working-directory artifacts. I wasn't too sure what the runner was doing, so I didn't want to blow away the entire home directory or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the best solution here is to probably just to make some of the copy code more generic, and just use some kind of heuristic to delete anything that looks like a working directory or artifact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, moved things around to make the code a little more generic, at least.
testbeatName string | ||
} | ||
|
||
func TestMetricbeatSeverless(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this being added in the Elastic Agent repository? I get that it might be faster, but probably only a little bit. All the modules you are using for this to work are in pkg/
which I did on purpose so other repositories can use that code. Why not just add this directly to the beats repository?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is being done here because it is much faster, serverless has a rapidly approaching deadline and we need tests to check whether Beats will work.
Beats hasn't been ported to Buildkite yet, it is the most complex build we have and it would have taken too long to do that before writing the tests.
This should be temporary, I'm hoping the work to port Beats to buildkite will start soon. We may be able to leave these tests on a branch depending on the timing to avoid this living for too long.
...never seen that error before
|
@fearful-symmetry we got this error in multiple other places, it has been introduced in the journey to remove our own folder on Windows. That's something we need to fix. We have an open PR for that: #3411 |
@joshdover Just to follow up on this, the core team decided in elastic/kibana#162418 that for now we will stick with our most recent plan of keeping the saved objects import/export APIs internal while we work out our long term vision for these APIs. This means Beats would indeed need to set Kibana won't care if you set this header unnecessarily, so if it's easier for Beats to just globally always use the header when talking to Kibana, that should be fine too. Sorry for the inconvenience! |
@lukeelmers does kibana care what the value of that header is? Does it have to be |
It is convenient to use the application name for tracking/monitoring purposes, however it isn't strictly required -- Kibana doesn't care what value you use. |
So, the actual serverless PR is here: elastic/beats#36649 |
The tests here aren't running in CI per the test report. Otherwise the tests would be failing because the Beats don't actually support serverless yet. While the tests are in the wrong repository, I think we can just move them to a daily schedule so that we get the test coverage without blocking agent PRs (since no agent code is involved in the standalone Beats tests) while we work on allowing these tests to be triggered from the Beats repository. I will create issues for both of these issues. We should add a test that ensures that Elastic Agent can be installed and enrolled with a Severless Observability project. Probably we can parameterize the monitoring logs integration test to run against both stateful and serverless. This test shouldn't require the DSL support since Fleet takes care of that for us. |
As part of this PR? @cmacknz |
Alright, need to pause merging/reviewing this for now, based on this conversation: elastic/beats#36649 (comment) Turns out under serverless, auth via username:pass and api_key results in different access permissions, which is causing some setup tasks to fail under serverless. Need to change how the tests do auth so they can connect to ES via |
SonarQube Quality Gate 0 Bugs No Coverage information |
@cmacknz I know we talked about it a bit yesterday, but what's the next steps for this? We want to break out all the backing library changes into a different PR? What was the goal with that? |
The Beat tests here can't be merged without #3470 or elastic/beats#36675. The support for provisioning serverless projects can be merged however, and we should have an integration test in the agent repository to test that Elastic Agent can ship data to Elasticsearch as mentioned in #3258 (comment). I'm comfortable with merging that to main and having it gate PRs. Such an agent test will also test that Beats can ship data to ES, but won't test the setup commands. My goal is to ensure we have appropriate serverless test coverage in the right places, triggered by the correct things. I want to avoid having tests against Beat snapshot images gate agent PRs, because that doesn't make any sense. So let's strip this PR down to just the Beat specific functionality and create a separate PR testing serverless support for agent. We can make a dedicated issue for this if it helps. |
This pull request is now in conflicts. Could you fix it? 🙏
|
Closed, now at #3658 |
What does this PR do?
There's a lot going on here. This PR:
Some important details and caveats also accompany this:
Why is it important?
We want to get rid of the old python tests, and also we need tests for serverless support.
Checklist
How to test this PR locally
mage -v integration:TestBeatServerless metricbeat
STACK_PROVISIONER
toess