-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Changes from 2 commits
d2924aa
1122db3
cd28a37
d381fcf
cd36f10
86cf7ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,6 +59,10 @@ func installCmd(streams *cli.IOStreams, cmd *cobra.Command, args []string) error | |
return fmt.Errorf("already installed at: %s", paths.InstallPath) | ||
} | ||
|
||
if status == install.PackageInstall { | ||
fmt.Fprintf(streams.Out, "Installed as a system package, installation will not be altered.\n") | ||
} | ||
|
||
// check the lock to ensure that elastic-agent is not already running in this directory | ||
locker := filelock.NewAppLocker(paths.Data(), paths.AgentLockFileName) | ||
if err := locker.TryLock(); err != nil { | ||
|
@@ -80,7 +84,7 @@ func installCmd(streams *cli.IOStreams, cmd *cobra.Command, args []string) error | |
return fmt.Errorf("installation was cancelled by the user") | ||
} | ||
} | ||
} else { | ||
} else if status != install.PackageInstall { | ||
if !force { | ||
confirm, err := cli.Confirm(fmt.Sprintf("Elastic Agent will be installed at %s and will run as a service. Do you want to continue?", paths.InstallPath), true) | ||
if err != nil { | ||
|
@@ -141,30 +145,33 @@ func installCmd(streams *cli.IOStreams, cmd *cobra.Command, args []string) error | |
} | ||
} | ||
} | ||
cfgFile := paths.ConfigFile() | ||
err = install.Install(cfgFile) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
defer func() { | ||
if err != nil { | ||
install.Uninstall(cfgFile) | ||
} | ||
}() | ||
|
||
if !delayEnroll { | ||
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? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure either, what is the behavior in the OS? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
if err != nil { | ||
fmt.Fprintf(streams.Out, "Installation failed to start Elastic Agent service.\n") | ||
return err | ||
} | ||
|
||
defer func() { | ||
if err != nil { | ||
install.StopService() | ||
install.Uninstall(cfgFile) | ||
} | ||
}() | ||
|
||
if !delayEnroll { | ||
err = install.StartService() | ||
if err != nil { | ||
fmt.Fprintf(streams.Out, "Installation failed to start Elastic Agent service.\n") | ||
return err | ||
} | ||
|
||
defer func() { | ||
if err != nil { | ||
install.StopService() | ||
} | ||
}() | ||
} | ||
} | ||
|
||
if enroll { | ||
|
@@ -180,10 +187,12 @@ func installCmd(streams *cli.IOStreams, cmd *cobra.Command, args []string) error | |
} | ||
err = enrollCmd.Wait() | ||
if err != nil { | ||
install.Uninstall(cfgFile) | ||
exitErr, ok := err.(*exec.ExitError) | ||
if ok { | ||
return fmt.Errorf("enroll command failed with exit code: %d", exitErr.ExitCode()) | ||
if status != install.PackageInstall { | ||
install.Uninstall(cfgFile) | ||
exitErr, ok := err.(*exec.ExitError) | ||
if ok { | ||
return fmt.Errorf("enroll command failed with exit code: %d", exitErr.ExitCode()) | ||
} | ||
} | ||
return fmt.Errorf("enroll command failed for unknown reason: %s", err) | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -7,8 +7,59 @@ | |||||
|
||||||
package install | ||||||
|
||||||
import ( | ||||||
"os" | ||||||
"os/exec" | ||||||
"runtime" | ||||||
"strings" | ||||||
|
||||||
"github.com/elastic/beats/v7/x-pack/elastic-agent/pkg/agent/application/paths" | ||||||
) | ||||||
|
||||||
// postInstall performs post installation for unix-based systems. | ||||||
func postInstall() error { | ||||||
// do nothing | ||||||
return nil | ||||||
} | ||||||
|
||||||
func checkPackageInstall() bool { | ||||||
if runtime.GOOS != "linux" { | ||||||
return false | ||||||
} | ||||||
|
||||||
// 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 | ||||||
Comment on lines
+29
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would like feedback for this suggestion There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sadly yes, this is expected. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
|
||||||
// check debian based systems (or systems that use dpkg) | ||||||
// 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 { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you thing about doing a |
||||||
out, err := exec.Command("dpkg-query", "-W", "-f", "${Status}", paths.BinaryName).Output() | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The output of
where the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @blakerouse, do you know where the package name is defined? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One question, here, I see we are interacting directly with the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 beats/dev-tools/mage/settings.go Lines 70 to 71 in 2379370
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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sound good |
||||||
if err != nil { | ||||||
return false | ||||||
} | ||||||
if strings.HasPrefix(string(out), "deinstall") { | ||||||
return false | ||||||
} | ||||||
return true | ||||||
} | ||||||
|
||||||
// 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 { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about |
||||||
out, err := exec.Command("rpm", "-q", paths.BinaryName, "--state").Output() | ||||||
if err != nil { | ||||||
return false | ||||||
} | ||||||
if strings.HasSuffix(string(out), "not installed") { | ||||||
return false | ||||||
} | ||||||
return true | ||||||
|
||||||
} | ||||||
|
||||||
return false | ||||||
} |
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.
opensuse/sles12 added to help test suse