Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Split install and enroll on test framework #4482

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Split install and enroll on test framework #4482

wants to merge 3 commits into from

Conversation

AndersonQ
Copy link
Member

@AndersonQ AndersonQ commented Mar 26, 2024 •
edited

What does this PR do?

It splits the install and enroll command on the test framework Fixture.Install
It also refactors the clean up function

Why is it important?

If any error happens during the agent, it'll automatically uninstall itself, which whereas good for production, prevent us from investigating what might have caused the error. It's specially important when the error happens during enroll.

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

  • Run the integration tests installing the agent, they should all succeed
  • Run a test installing and enrolling the agent
  • make the a test enrolling the agent fail at enrollment (use a bad fleet-server URL or enrollment token)
  • the agent should not uninstall itself
  • the framework should collect a diagnostics or the agent directory.

Related issues

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?
  • ...

@AndersonQ AndersonQ added enhancement New feature or request Team:Elastic-Agent Label for the Agent team backport-skip skip-changelog labels Mar 26, 2024
@AndersonQ AndersonQ self-assigned this Mar 26, 2024
@AndersonQ AndersonQ requested a review from a team as a code owner March 26, 2024 07:08
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

@elastic-sonarqube Elastic SonarQube
Copy link

Quality Gate passed Quality Gate passed

Kudos, no new issues were introduced!

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

See analysis details on SonarQube

@pierrehilbert pierrehilbert requested review from rdner and removed request for michalpristas March 26, 2024 08:48
rdner
rdner requested changes Mar 26, 2024
View reviewed changes
Copy link
Member

@rdner rdner left a comment

Choose a reason for hiding this comment

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

Multiple linter issues and some requests from my side.

pkg/testing/fixture_install.go
// check for running agents before installing, there should be no agent
// process running. However, even if there is one, let's keep the test going
// and add a failure so we can investigate what happened.
// assert.Empty(f.t, getElasticAgentProcesses(f.t), "there should be no running agent at beginning of Install()")
Copy link
Member

Choose a reason for hiding this comment

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

why is this commented now?

Copy link
Member Author

Choose a reason for hiding this comment

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

as I commented in PR, it's still in draft, sorry I forgot to set it as draft when opening the PR

pkg/testing/fixture_install.go
Comment on lines +255 to +259
if err != nil &&
(errors.Is(err, ErrNotInstalled) ||
strings.Contains(
err.Error(),
"elastic-agent: no such file or directory")) {
Copy link
Member

Choose a reason for hiding this comment

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

Since we're already refactoring, please assign each condition to a variable, this is very hard to read.

pkg/testing/fixture_install.go
if err != nil {
f.t.Logf("error serializing processes: %s", err)
}
if f.cleanUpKeepInstalled() {
Copy link
Member

Choose a reason for hiding this comment

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

This returns a boolean which is not documented, I cannot understand from the function name what this boolean is.

pkg/testing/fixture_install.go

f.t.Cleanup(func() {
if f.cleanUpUninstall() {
Copy link
Member

Choose a reason for hiding this comment

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

Same about boolean here.

pkg/testing/runner/runner.go
@@ -447,10 +447,20 @@ func (r *Runner) validate() error {

// getBuilds returns the build for the batch.
func (r *Runner) getBuilds(b OSBatch) []Build {
builds := []Build{}
var builds []Build
formats := []string{"targz", "zip", "rpm", "deb"}
Copy link
Member

Choose a reason for hiding this comment

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

Since we started using slice.Contains on this, why not define a global map[string]struct{} and just look strings up instead? It's way more efficient than iterating through a slice on each loop iteration.

@rdner
Copy link
Member

rdner commented Mar 26, 2024

@AndersonQ Seems like enroll does not work in integration tests now. See the CI build.

@AndersonQ AndersonQ marked this pull request as draft March 26, 2024 14:00
@AndersonQ
Copy link
Member Author

@AndersonQ Seems like enroll does not work in integration tests now. See the CI build.

my bad, it isn't ready fore review yet. I set it as draft again

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 15, 2024

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 4377-split-install-enroll upstream/4377-split-install-enroll
git merge upstream/main
git push upstream 4377-split-install-enroll

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rdner rdner rdner requested changes

@pchila pchila Awaiting requested review from pchila pchila is a code owner automatically assigned from elastic/elastic-agent-control-plane

Requested changes must be addressed to merge this pull request.

Assignees

@AndersonQ AndersonQ

Labels
backport-skip enhancement New feature or request skip-changelog Team:Elastic-Agent Label for the Agent team
Projects
None yet
Milestone
No milestone
3 participants
@AndersonQ @elasticmachine @rdner