From 476c8764094bf083bf8d661f13fa0f6a6a329a51 Mon Sep 17 00:00:00 2001 From: Craig MacKenzie Date: Tue, 24 Oct 2023 13:55:11 -0400 Subject: [PATCH 1/5] Failing to detect SSDs should not be a fatal error. --- internal/pkg/agent/application/upgrade/upgrade.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 36276f239b6..5bf94cbe07f 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -386,14 +386,16 @@ func copyDir(l *logger.Logger, from, to string, ignoreErrs bool) error { } } + // Try to detect if we are running with SSDs. If we are increase the copy concurrency, + // otherwise fall back to the default. + copyConcurrency := 1 block, err := ghw.Block() if err != nil { - return fmt.Errorf("ghw.Block() returned error: %w", err) - } - - copyConcurrency := 1 - if install.HasAllSSDs(*block) { - copyConcurrency = runtime.NumCPU() * 4 + l.Warnw("Error detecting block storage type", "error.message", err) + } else { + if install.HasAllSSDs(*block) { + copyConcurrency = runtime.NumCPU() * 4 + } } return copy.Copy(from, to, copy.Options{ From 43439b4b2badcb1acf9926655301a3a549a82a72 Mon Sep 17 00:00:00 2001 From: Craig MacKenzie Date: Thu, 26 Oct 2023 11:08:03 -0400 Subject: [PATCH 2/5] ghw.Block errors should not be fatal during install. --- internal/pkg/agent/install/install.go | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/internal/pkg/agent/install/install.go b/internal/pkg/agent/install/install.go index 08617db2fcc..932050481d6 100644 --- a/internal/pkg/agent/install/install.go +++ b/internal/pkg/agent/install/install.go @@ -61,15 +61,18 @@ func Install(cfgFile, topPath string, pt ProgressTrackerStep) error { } // copy source into install path - block, err := ghw.Block() - if err != nil { - return fmt.Errorf("ghw.Block() returned error: %w", err) - } - copyConcurrency := 1 - if HasAllSSDs(*block) { - copyConcurrency = runtime.NumCPU() * 4 + block, blockErr := ghw.Block() + + // Detecting the block HW type may fail (particularly on MacOS) but this is not a fatal error. + // The error will be logged only if the copy failed to avoid showing errors to users when they + // first use Elastic Agent unnecessarily. + if blockErr == nil { + if HasAllSSDs(*block) { + copyConcurrency = runtime.NumCPU() * 4 + } } + s := pt.StepStart("Copying files") err = copy.Copy(dir, topPath, copy.Options{ OnSymlink: func(_ string) copy.SymlinkAction { @@ -80,10 +83,13 @@ func Install(cfgFile, topPath string, pt ProgressTrackerStep) error { }) if err != nil { s.Failed() + // If the copy fails also return any error we encountered detecting the block device. return errors.New( err, fmt.Sprintf("failed to copy source directory (%s) to destination (%s)", dir, topPath), - errors.M("source", dir), errors.M("destination", topPath)) + errors.M("source", dir), errors.M("destination", topPath), + errors.M("concurrency", copyConcurrency), errors.M("blkerror", blockErr), + ) } s.Succeeded() From fc800be27075d9383e90554eab75430de961f7a1 Mon Sep 17 00:00:00 2001 From: Craig MacKenzie Date: Thu, 26 Oct 2023 11:54:45 -0400 Subject: [PATCH 3/5] Log block HW errors when they occur. --- internal/pkg/agent/cmd/install.go | 2 +- internal/pkg/agent/install/install.go | 18 ++++++++---------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/internal/pkg/agent/cmd/install.go b/internal/pkg/agent/cmd/install.go index 4fbd37f40da..f52fec6ef17 100644 --- a/internal/pkg/agent/cmd/install.go +++ b/internal/pkg/agent/cmd/install.go @@ -185,7 +185,7 @@ func installCmd(streams *cli.IOStreams, cmd *cobra.Command) error { cfgFile := paths.ConfigFile() if status != install.PackageInstall { - err = install.Install(cfgFile, topPath, s) + err = install.Install(cfgFile, topPath, s, streams) if err != nil { return err } diff --git a/internal/pkg/agent/install/install.go b/internal/pkg/agent/install/install.go index 932050481d6..360ec8fd04c 100644 --- a/internal/pkg/agent/install/install.go +++ b/internal/pkg/agent/install/install.go @@ -11,13 +11,13 @@ import ( "runtime" "strings" - "github.com/kardianos/service" - "github.com/jaypipes/ghw" + "github.com/kardianos/service" "github.com/otiai10/copy" "github.com/elastic/elastic-agent/internal/pkg/agent/application/paths" "github.com/elastic/elastic-agent/internal/pkg/agent/errors" + "github.com/elastic/elastic-agent/internal/pkg/cli" ) const ( @@ -25,7 +25,7 @@ const ( ) // Install installs Elastic Agent persistently on the system including creating and starting its service. -func Install(cfgFile, topPath string, pt ProgressTrackerStep) error { +func Install(cfgFile, topPath string, pt ProgressTrackerStep, streams *cli.IOStreams) error { dir, err := findDirectory() if err != nil { return errors.New(err, "failed to discover the source directory for installation", errors.TypeFilesystem) @@ -61,13 +61,12 @@ func Install(cfgFile, topPath string, pt ProgressTrackerStep) error { } // copy source into install path - copyConcurrency := 1 - block, blockErr := ghw.Block() - // Detecting the block HW type may fail (particularly on MacOS) but this is not a fatal error. - // The error will be logged only if the copy failed to avoid showing errors to users when they - // first use Elastic Agent unnecessarily. - if blockErr == nil { + copyConcurrency := 1 + block, err := ghw.Block() + if err != nil { + fmt.Fprintf(streams.Out, "Could not determine block hardware type, disabling concurrency: %s\n", err) + } else { if HasAllSSDs(*block) { copyConcurrency = runtime.NumCPU() * 4 } @@ -88,7 +87,6 @@ func Install(cfgFile, topPath string, pt ProgressTrackerStep) error { err, fmt.Sprintf("failed to copy source directory (%s) to destination (%s)", dir, topPath), errors.M("source", dir), errors.M("destination", topPath), - errors.M("concurrency", copyConcurrency), errors.M("blkerror", blockErr), ) } s.Succeeded() From 5c9d6a6cc0ce8dca38bf704624cea34fa2964222 Mon Sep 17 00:00:00 2001 From: Craig MacKenzie Date: Thu, 26 Oct 2023 11:56:55 -0400 Subject: [PATCH 4/5] Improve error messages. --- internal/pkg/agent/application/upgrade/upgrade.go | 2 +- internal/pkg/agent/install/install.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 5bf94cbe07f..625bbbd7238 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -391,7 +391,7 @@ func copyDir(l *logger.Logger, from, to string, ignoreErrs bool) error { copyConcurrency := 1 block, err := ghw.Block() if err != nil { - l.Warnw("Error detecting block storage type", "error.message", err) + l.Infow("Could not determine block storage type, disabling copy concurrency", "error.message", err) } else { if install.HasAllSSDs(*block) { copyConcurrency = runtime.NumCPU() * 4 diff --git a/internal/pkg/agent/install/install.go b/internal/pkg/agent/install/install.go index 360ec8fd04c..a97a8b39e44 100644 --- a/internal/pkg/agent/install/install.go +++ b/internal/pkg/agent/install/install.go @@ -65,7 +65,7 @@ func Install(cfgFile, topPath string, pt ProgressTrackerStep, streams *cli.IOStr copyConcurrency := 1 block, err := ghw.Block() if err != nil { - fmt.Fprintf(streams.Out, "Could not determine block hardware type, disabling concurrency: %s\n", err) + fmt.Fprintf(streams.Out, "Could not determine block hardware type, disabling copy concurrency: %s\n", err) } else { if HasAllSSDs(*block) { copyConcurrency = runtime.NumCPU() * 4 From 599e267d966678ea1d75eebad8332b063d447664 Mon Sep 17 00:00:00 2001 From: Craig MacKenzie Date: Thu, 26 Oct 2023 12:59:28 -0400 Subject: [PATCH 5/5] Centralize uses of ghw.Block to prevent misuse. Document that errors are not fatal. --- .../pkg/agent/application/upgrade/upgrade.go | 15 ++++---- internal/pkg/agent/install/install.go | 35 +++++++++++++------ internal/pkg/agent/install/install_test.go | 2 +- 3 files changed, 32 insertions(+), 20 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 625bbbd7238..da0ea3df6be 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -12,8 +12,6 @@ import ( "runtime" "strings" - "github.com/jaypipes/ghw" - "github.com/otiai10/copy" "go.elastic.co/apm" @@ -389,13 +387,12 @@ func copyDir(l *logger.Logger, from, to string, ignoreErrs bool) error { // Try to detect if we are running with SSDs. If we are increase the copy concurrency, // otherwise fall back to the default. copyConcurrency := 1 - block, err := ghw.Block() - if err != nil { - l.Infow("Could not determine block storage type, disabling copy concurrency", "error.message", err) - } else { - if install.HasAllSSDs(*block) { - copyConcurrency = runtime.NumCPU() * 4 - } + hasSSDs, detectHWErr := install.HasAllSSDs() + if detectHWErr != nil { + l.Infow("Could not determine block storage type, disabling copy concurrency", "error.message", detectHWErr) + } + if hasSSDs { + copyConcurrency = runtime.NumCPU() * 4 } return copy.Copy(from, to, copy.Options{ diff --git a/internal/pkg/agent/install/install.go b/internal/pkg/agent/install/install.go index fd2b183946c..7c3ee14b038 100644 --- a/internal/pkg/agent/install/install.go +++ b/internal/pkg/agent/install/install.go @@ -62,15 +62,16 @@ func Install(cfgFile, topPath string, pt *progressbar.ProgressBar, streams *cli. } // copy source into install path - // Detecting the block HW type may fail (particularly on MacOS) but this is not a fatal error. + // + // Try to detect if we are running with SSDs. If we are increase the copy concurrency, + // otherwise fall back to the default. copyConcurrency := 1 - block, err := ghw.Block() - if err != nil { - fmt.Fprintf(streams.Out, "Could not determine block hardware type, disabling copy concurrency: %s\n", err) - } else { - if HasAllSSDs(*block) { - copyConcurrency = runtime.NumCPU() * 4 - } + hasSSDs, detectHWErr := HasAllSSDs() + if detectHWErr != nil { + fmt.Fprintf(streams.Out, "Could not determine block hardware type, disabling copy concurrency: %s\n", detectHWErr) + } + if hasSSDs { + copyConcurrency = runtime.NumCPU() * 4 } pt.Describe("Copying install files") @@ -266,8 +267,22 @@ func verifyDirectory(dir string) error { } // HasAllSSDs returns true if the host we are on uses SSDs for -// all its persistent storage; false otherwise or on error -func HasAllSSDs(block ghw.BlockInfo) bool { +// all its persistent storage; false otherwise. Returns any error +// encountered detecting the hardware type for informational purposes. +// Errors from this function are not fatal. Note that errors may be +// returned on some Mac hardware configurations as the ghw package +// does not fully support MacOS. +func HasAllSSDs() (bool, error) { + block, err := ghw.Block() + if err != nil { + return false, err + } + + return hasAllSSDs(*block), nil +} + +// Internal version of HasAllSSDs for testing. +func hasAllSSDs(block ghw.BlockInfo) bool { for _, disk := range block.Disks { switch disk.DriveType { case ghw.DRIVE_TYPE_FDD, ghw.DRIVE_TYPE_ODD: diff --git a/internal/pkg/agent/install/install_test.go b/internal/pkg/agent/install/install_test.go index 2c0ae1b0b2d..dd73cac17a8 100644 --- a/internal/pkg/agent/install/install_test.go +++ b/internal/pkg/agent/install/install_test.go @@ -52,7 +52,7 @@ func TestHasAllSSDs(t *testing.T) { for name, test := range cases { t.Run(name, func(t *testing.T) { - actual := HasAllSSDs(test.block) + actual := hasAllSSDs(test.block) require.Equal(t, test.expected, actual) }) }