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

Alter agent install to detect installation from package #30289

Merged

Conversation

michel-laterman
Copy link
Contributor

@michel-laterman michel-laterman commented Feb 8, 2022

What does this PR do?

Alter the elastic-agent install command to not run the installation or
uninstall steps (on failure) when the agent has been installed via the
package manager of a Linux os. Instead the enroll command will be
executed.

Why is it important?

The current documentation/Kibana gives install commands, having these fail can be frustrating.

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 PR
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

Install the rpm/deb package and run the install command that Kibana provides:

vagrant@ubuntu-focal:~$ sudo dpkg -i elastic-agent-8.2.0-SNAPSHOT-amd64.deb
Selecting previously unselected package elastic-agent.
(Reading database ... 69907 files and directories currently installed.)
Preparing to unpack elastic-agent-8.2.0-SNAPSHOT-amd64.deb ...
Unpacking elastic-agent (8.2.0) ...
Setting up elastic-agent (8.2.0) ...
Processing triggers for systemd (245.4-4ubuntu3.15) ...
vagrant@ubuntu-focal:~$ sudo elastic-agent install --url=https://579a0b58293d477da089b3e048311a63.fleet.us-central1.gcp.qa.cld.elstc.co:443 --enrollment-token=NTFqWF8zNEIyWVJ5Qm1EbkY1dzg6RUZVd1lIVEVTQk9oeXVtY3FHZ2pnZw==
Installed as a system package, installation will not be altered.
{"log.level":"info","@timestamp":"2022-02-16T00:10:23.061Z","log.origin":{"file.name":"cmd/enroll_cmd.go","file.line":455},"message":"Starting enrollment to URL: https://579a0b58293d477da089b3e048311a63.fleet.us-central1.gcp.qa.cld.elstc.co:443/","ecs.version":"1.6.0"}
{"log.level":"info","@timestamp":"2022-02-16T00:10:24.414Z","log.origin":{"file.name":"cmd/enroll_cmd.go","file.line":253},"message":"Elastic Agent might not be running; unable to trigger restart","ecs.version":"1.6.0"}
Successfully enrolled the Elastic Agent.
Elastic Agent has been successfully installed.
vagrant@ubuntu-focal:~$ ls /opt/
vagrant@ubuntu-focal:~$ systemctl status elastic-agent.service
● elastic-agent.service - Agent manages other beats based on configuration provided.
     Loaded: loaded (/lib/systemd/system/elastic-agent.service; disabled; vendor preset: enabled)
     Active: inactive (dead)
       Docs: https://www.elastic.co/beats/elastic-agent

Related issues

Alter the elastic-agent install command to not run the installation or
uninstall steps (on failure) when the agent has been installed via the
package manager of a Linux os. Instead the enroll command will be
executed.
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Feb 8, 2022
@michel-laterman michel-laterman added enhancement Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team labels Feb 8, 2022
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Feb 8, 2022
@mergify
Copy link
Contributor

mergify bot commented Feb 8, 2022

This pull request does not have a backport label. Could you fix it @michel-laterman? 🙏
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 7./d branch. /d is the digit

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

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Feb 8, 2022
Comment on lines +56 to +57
{:name => "opensuse153", :box => "bento/opensuse-leap-15.3", :platform => "opensuse"},
{:name => "sles12", :box => "elastic/sles-12-x86_64", :platform => "sles"},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

opensuse/sles12 added to help test suse

err = install.StartService()
cfgFile := paths.ConfigFile()
if status != install.PackageInstall {
err = install.Install(cfgFile) // can't run install for package installed agent, but must stop the running service and re-enroll, then start the service?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if we need to do the commented steps, currently the enroll command will be ran with the --from-install flag, but I don't know if that's correct

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure either, what is the behavior in the OS?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do not need to stop the running service, it should just enroll and then trigger a restart through the Elastic Agent control protocol. It should not need to be restarted through the service manager of the host.

Comment on lines +30 to +32
// NOTE searching for english words might not be a great idea as far as portability goes.
// list all installed packages then search for paths.BinaryName?
// dpkg is strange as the remove and purge processes leads to the package bing isted after a remove, but not after a purge
Copy link
Contributor Author

Choose a reason for hiding this comment

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

would like feedback for this suggestion

Copy link
Contributor

Choose a reason for hiding this comment

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

This is expected because remove does not remove modified configuration files, those will remain. Only remove with purge completely removes all data on the system related to a package.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sadly yes, this is expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'll keep the comments here as the extra documentation can help if anyone else needs to work on this

@elasticmachine
Copy link
Collaborator

elasticmachine commented Feb 9, 2022

💚 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: 2022-02-24T16:27:19.739+0000

  • Duration: 158 min 55 sec

Test stats 🧪

Test Results
Failed 0
Passed 38335
Skipped 3320
Total 41655

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

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

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@michel-laterman michel-laterman marked this pull request as ready for review February 16, 2022 00:27
@elasticmachine
Copy link
Collaborator

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

@michel-laterman
Copy link
Contributor Author

/test

// If the package has been installed, the status starts with "install"
// If the package has been removed (but not pruged) status starts with "deinstall"
// If purged or never installed, rc is 1
if _, err := os.Stat("/etc/dpkg"); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you thing about doing a which dpkg-query instead? Only thing would be is which always present on a system? I believe so.

Comment on lines +30 to +32
// NOTE searching for english words might not be a great idea as far as portability goes.
// list all installed packages then search for paths.BinaryName?
// dpkg is strange as the remove and purge processes leads to the package bing isted after a remove, but not after a purge
Copy link
Contributor

Choose a reason for hiding this comment

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

This is expected because remove does not remove modified configuration files, those will remain. Only remove with purge completely removes all data on the system related to a package.

// If the package has been removed (but not pruged) status starts with "deinstall"
// If purged or never installed, rc is 1
if _, err := os.Stat("/etc/dpkg"); err == nil {
out, err := exec.Command("dpkg-query", "-W", "-f", "${Status}", paths.BinaryName).Output()
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to use the package name explicitly instead of the binary name. Even if they are the same at the moment use a specific constant for that would be better, that way if it ever changes.

Could you also use -s which will only show the status?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The output of -s includes more information than what we need:

vagrant@ubuntu-focal:~$ dpkg-query -s elastic-agent
Package: elastic-agent
Status: install ok installed
Priority: extra
Section: default
Installed-Size: 427208
Maintainer: <@ace0c0ea54fb>
Architecture: amd64
Version: 8.2.0
Conffiles:
 /etc/elastic-agent/.elastic-agent.active.commit 75bbbe44ef52356bb5e85a458166b1b6
 /etc/elastic-agent/elastic-agent.reference.yml 23aa69e9e641a43d7daaa0975edf0ec6
 /etc/elastic-agent/elastic-agent.yml f169e731b8e4c8a571dca0107954534d
 /etc/init.d/elastic-agent 79b301b56cc62629b9d276d6ae24ca2f
Description: Agent manages other beats based on configuration provided.
License: Elastic-License
Vendor: Elastic
Homepage: https://www.elastic.co/beats/elastic-agent

where the -W -f '${Status}' flags will give just that Status line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@blakerouse, do you know where the package name is defined?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it is using the name of the beat for the package name, so the way you have it works. I am just suggesting we use a different constant, with the same value. Just to future proof it that maybe one day they could be different.

Its more of a nitpick and if you prefer they way you have it, that is okay as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

One question, here, I see we are interacting directly with the CLI, is there any C-Libary of even go package that we can use to abstract the package manager?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not finding a c or go library that can query dpkg/rpm for installed packages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After a bit of digging we can find that the package name is defined by our Mage files as the BeatServiceName, this defaults to the same as BeatName for almost all cases (exception being heartbeat).

BeatName = EnvOr("BEAT_NAME", filepath.Base(CWD()))
BeatServiceName = EnvOr("BEAT_SERVICE_NAME", BeatName)

The value is set to the directory name we run mage out of (i.e., x-pack/elastic-agent). So I don't think it's a good idea to introduce a new variable for package name within the current codebase as it won't be picked up by our tooling.

Copy link
Contributor

Choose a reason for hiding this comment

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

sound good

// check rhel and sles based systems (or systems that use rpm)
// if package has been installed query retuns with a list of associated files.
// otherwise if uninstalled, or has never been installled status ends with "not installed"
if _, err := os.Stat("/etc/rpm"); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about which rpm here as well?

@michel-laterman
Copy link
Contributor Author

/test

@ph ph self-requested a review February 21, 2022 13:46
Vagrantfile Outdated Show resolved Hide resolved
err = install.StartService()
cfgFile := paths.ConfigFile()
if status != install.PackageInstall {
err = install.Install(cfgFile) // can't run install for package installed agent, but must stop the running service and re-enroll, then start the service?
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure either, what is the behavior in the OS?

Comment on lines +30 to +32
// NOTE searching for english words might not be a great idea as far as portability goes.
// list all installed packages then search for paths.BinaryName?
// dpkg is strange as the remove and purge processes leads to the package bing isted after a remove, but not after a purge
Copy link
Contributor

Choose a reason for hiding this comment

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

Sadly yes, this is expected.

// If the package has been removed (but not pruged) status starts with "deinstall"
// If purged or never installed, rc is 1
if _, err := os.Stat("/etc/dpkg"); err == nil {
out, err := exec.Command("dpkg-query", "-W", "-f", "${Status}", paths.BinaryName).Output()
Copy link
Contributor

Choose a reason for hiding this comment

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

One question, here, I see we are interacting directly with the CLI, is there any C-Libary of even go package that we can use to abstract the package manager?

michel-laterman and others added 2 commits February 23, 2022 12:49
Co-authored-by: Pier-Hugues Pellerin <phpellerin@gmail.com>
Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

Looks ready to me!

Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

LGTM thanks @michel-laterman

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Skip notification from the automated backport with mergify enhancement Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team
Projects
None yet
4 participants