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.

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

Improve elastic-agent install performance #3212

Merged
merged 23 commits into from
Oct 5, 2023

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented Aug 8, 2023

What does this PR do?

This PR makes elastic-agent install run slightly faster by concurrently copying files when the Agent is being installed on a host with SSDs. The same is done with Elastic Agent upgrades.

Summary of installation times

For all tests, the first installation result was discarded and the worse of the next two installation results (the result with the higher installation time) was used.

OS Disk type Before this PR (s) After this PR (s) Time savings (s) Time savings (%)
MacOS SSD 15.3s 13.9s 1.4s 9.2%
MacOS HDD
Linux SSD 12.2s 9.2s 3s 24.6%
Linux HDD 45.7s 46s -0.3s -0.7%
Windows SSD 9.6s 6.3s 3.3s 34.4%
Windows HDD

MacOS - SSD

Tested on 2023 MacOS (Ventura 13.5) running SSDs.

Before this PR

$ time sudo ./elastic-agent install -n
Installing in non-interactive mode.Uninstalling current Elastic Agent... DONE
Copying files............................................................. DONE
Installing service... DONE
Starting service... DONE
Elastic Agent has been successfully installed.
sudo ./elastic-agent install -n  0.51s user 2.90s system 22% cpu 15.265 total
$ find . | wc -l
    3464
$ sudo find /Library/Elastic/Agent | wc -l
    3490

After this PR

$ time sudo ./elastic-agent install -n
Installing in non-interactive mode.
Uninstalling current Elastic Agent... DONE
Copying files................................................. DONE
Installing service... DONE
Starting service... DONE
Elastic Agent has been successfully installed.
sudo ./elastic-agent install -n  0.99s user 6.28s system 52% cpu 13.888 total

$ find . | wc -l
    3464
$ sudo find /Library/Elastic/Agent | wc -l
    3490

MacOS - HDD

TODO

Before this PR

After this PR

Linux - SSD

Tested on Google Cloud VM (Ubuntu 22) running SSDs (balanced persistent disk).

Before this PR

$ time sudo ./elastic-agent install -n
Installing in non-interactive mode.Uninstalling current Elastic Agent... DONE
Copying files................................................ DONE
Installing service..... DONE
Starting service... DONE
Elastic Agent has been successfully installed.

real	0m12.150s
user	0m0.003s
sys	0m0.008s

$ find . | wc -l
3448
$ sudo find /opt/Elastic/Agent/ | wc -l
3478

After this PR

$ time sudo ./elastic-agent install -n
Installing in non-interactive mode.
Uninstalling current Elastic Agent... DONE
Copying files...................................... DONE
Installing service.... DONE
Starting service.... DONE
Elastic Agent has been successfully installed.

real	0m9.148s
user	0m0.010s
sys	0m0.000s

$ find . | wc -l
3448
$ sudo find /opt/Elastic/Agent/ | wc -l
3478

Linux - HDD

Tested on Azure Cloud VM (Ubuntu 22) running HDDs (Standard HDD LRS).

Before this PR

$ time sudo ./elastic-agent install -n
Installing in non-interactive mode.Uninstalling current Elastic Agent... DONE
Copying files......................................................................................................................................................................... DONE
Installing service.... DONE
Starting service... DONE
Elastic Agent has been successfully installed.

real	0m45.694s
user	0m0.015s
sys	0m0.005s

$ find . | wc -l
3448
$ sudo find /opt/Elastic/Agent/ | wc -l
3478

After this PR

$ time sudo ./elastic-agent install -n
Installing in non-interactive mode.
Uninstalling current Elastic Agent... DONE
Copying files......................................................................................................................................................................... DONE
Installing service..... DONE
Starting service... DONE
Elastic Agent has been successfully installed.

real	0m45.957s
user	0m0.001s
sys	0m0.019s

$ find . | wc -l
3448
$ sudo find /opt/Elastic/Agent/ | wc -l
3478

Windows - SSD

Tested on 2022 Dell laptop (Windows 11 Pro) running SSDs.

Before this PR

PS>  Measure-Command { .\elastic-agent.exe install -n }


Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 9
Milliseconds      : 616
Ticks             : 96167912
TotalDays         : 0.000111305453703704
TotalHours        : 0.00267133088888889
TotalMinutes      : 0.160279853333333
TotalSeconds      : 9.6167912
TotalMilliseconds : 9616.7912

PS> (Get-ChildItem -Recurse | Measure-Object).Count
3454
PS> cd 'C:\Program Files\Elastic\Agent\'
PS> (Get-ChildItem -Recurse | Measure-Object).Count
3482

After this PR

PS> Measure-Command { .\elastic-agent.exe install -n }


Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 6
Milliseconds      : 316
Ticks             : 63167305
TotalDays         : 7.3110306712963E-05
TotalHours        : 0.00175464736111111
TotalMinutes      : 0.105278841666667
TotalSeconds      : 6.3167305
TotalMilliseconds : 6316.7305

PS> (Get-ChildItem -Recurse | Measure-Object).Count
3454
PS> cd 'C:\Program Files\Elastic\Agent\'
PS> (Get-ChildItem -Recurse | Measure-Object).Count
3482

Windows - HDD

TODO

Before this PR

After this PR

Why is it important?

Better user experience.

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

@mergify
Copy link
Contributor

mergify bot commented Aug 8, 2023

This pull request does not have a backport label. Could you fix it @ycombinator? 🙏
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.

@mergify mergify bot added the backport-skip label Aug 8, 2023
@elasticmachine
Copy link
Contributor

elasticmachine commented Aug 8, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-10-04T21:37:50.734+0000

  • Duration: 26 min 36 sec

Test stats 🧪

Test Results
Failed 0
Passed 6445
Skipped 59
Total 6504

💚 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!)

@ycombinator ycombinator changed the title Concurrent install Improve elastic-agent install performance Aug 9, 2023
@mergify
Copy link
Contributor

mergify bot commented Aug 11, 2023

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 concurrent-install upstream/concurrent-install
git merge upstream/main
git push upstream concurrent-install

@ycombinator ycombinator force-pushed the concurrent-install branch 4 times, most recently from 79342af to 6b04bf4 Compare September 1, 2023 18:21
@ycombinator ycombinator added the enhancement New feature or request label Sep 1, 2023
@ycombinator ycombinator marked this pull request as ready for review September 1, 2023 18:25
@ycombinator ycombinator requested a review from a team as a code owner September 1, 2023 18:25
@elasticmachine
Copy link
Contributor

elasticmachine commented Sep 1, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 98.78% (81/82) 👍
Files 66.0% (198/300) 👍 0.333
Classes 65.171% (363/557) 👍 0.18
Methods 52.333% (1144/2186) 👍 0.022
Lines 37.982% (13027/34298) 👎 -0.023
Conditionals 100.0% (0/0) 💚

@pierrehilbert pierrehilbert added the Team:Elastic-Agent Label for the Agent team label Sep 3, 2023
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@faec faec left a comment

Choose a reason for hiding this comment

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

Looks good, modulo the question about comments

func HasAllSSDs() bool {
block, err := ghw.Block()
if err != nil {
//fmt.Fprintf(os.Stdout, "HasAllSSDs: ghw.Block() returned error: %s\n", err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the commented Fprintf's left here intentionally or just leftover from testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leftovers, will remove. Thanks!

@mergify
Copy link
Contributor

mergify bot commented Sep 8, 2023

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 concurrent-install upstream/concurrent-install
git merge upstream/main
git push upstream concurrent-install

@ycombinator
Copy link
Contributor Author

Currently waiting on otiai10/copy#123 to be merged, then will update go.mod on this PR.

@mergify
Copy link
Contributor

mergify bot commented Sep 21, 2023

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 concurrent-install upstream/concurrent-install
git merge upstream/main
git push upstream concurrent-install

@ycombinator
Copy link
Contributor Author

ycombinator commented Sep 29, 2023

Hey @michalpristas, I was wondering if you have a Windows machine with an HDD (as opposed to SSD). @pierrehilbert suggested that you might.

If you do, would you mind testing the performance of elastic-agent install on that machine before and after this PR? You can follow the same steps that I did for Windows - SSD (scroll down a bit).

If you don't have such a machine, just let me know. Thanks either way!

@ycombinator
Copy link
Contributor Author

ycombinator commented Oct 2, 2023

Hey @michalpristas, I was wondering if you have a Windows machine with an HDD (as opposed to SSD). @pierrehilbert suggested that you might.

If you do, would you mind testing the performance of elastic-agent install on that machine before and after this PR? You can follow the same steps that I did for Windows - SSD (scroll down a bit).

If you don't have such a machine, just let me know. Thanks either way!

Chatted with @michalpristas off-PR. He doesn't have an HDD machine where elastic-agent can be installed.

The HDD test is mainly to make sure there's no regression in performance with this change on HDDs, which I tested for Linux so I think that's good enough.

@elastic-sonarqube
Copy link

@ycombinator
Copy link
Contributor Author

@cmacknz Worth backporting to 8.11?

@ycombinator ycombinator merged commit a4003e0 into elastic:main Oct 5, 2023
11 checks passed
@ycombinator ycombinator deleted the concurrent-install branch October 5, 2023 04:39
@cmacknz
Copy link
Member

cmacknz commented Oct 24, 2023

Missed the original ping here, not a bug fix so we shouldn't have backported.

@ycombinator
Copy link
Contributor Author

Missed the original ping here, not a bug fix so we shouldn't have backported.

👍 This PR was not backported so we're good. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip enhancement New feature or request Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants