Failing to detect SSDs in copyDir should not be a fatal error. (#3653)

* Failing to detect SSDs should not be a fatal error.

* ghw.Block errors should not be fatal during install.

* Log block HW errors when they occur.

* Improve error messages.

* Centralize uses of ghw.Block to prevent misuse.

Document that errors are not fatal.

Author: cmacknz

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"
 
@@ -386,13 +384,14 @@ func copyDir(l *logger.Logger, from, to string, ignoreErrs bool) error {
 		}
 	}
 
-	block, err := ghw.Block()
-	if err != nil {
-		return fmt.Errorf("ghw.Block() returned error: %w", err)
-	}
-
+	// Try to detect if we are running with SSDs. If we are increase the copy concurrency,
+	// otherwise fall back to the default.
 	copyConcurrency := 1
-	if install.HasAllSSDs(*block) {
+	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
 	}
 

internal/pkg/agent/cmd/install.go

@@ -177,7 +177,7 @@ func installCmd(streams *cli.IOStreams, cmd *cobra.Command) error {
 
 	cfgFile := paths.ConfigFile()
 	if status != install.PackageInstall {
-		err = install.Install(cfgFile, topPath, progBar)
+		err = install.Install(cfgFile, topPath, progBar, streams)
 		if err != nil {
 			return fmt.Errorf("error installing package: %w", err)
 		}

internal/pkg/agent/install/install.go

@@ -11,22 +11,22 @@ import (
 	"runtime"
 	"strings"
 
-	"github.com/kardianos/service"
-
 	"github.com/jaypipes/ghw"
+	"github.com/kardianos/service"
 	"github.com/otiai10/copy"
 	"github.com/schollz/progressbar/v3"
 
 	"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 (
 	darwin = "darwin"
 )
 
 // Install installs Elastic Agent persistently on the system including creating and starting its service.
-func Install(cfgFile, topPath string, pt *progressbar.ProgressBar) error {
+func Install(cfgFile, topPath string, pt *progressbar.ProgressBar, streams *cli.IOStreams) error {
 	dir, err := findDirectory()
 	if err != nil {
 		return errors.New(err, "failed to discover the source directory for installation", errors.TypeFilesystem)
@@ -62,15 +62,18 @@ func Install(cfgFile, topPath string, pt *progressbar.ProgressBar) error {
 	}
 
 	// copy source into install path
-	block, err := ghw.Block()
-	if err != nil {
-		return fmt.Errorf("ghw.Block() returned error: %w", err)
-	}
-
+	//
+	// Try to detect if we are running with SSDs. If we are increase the copy concurrency,
+	// otherwise fall back to the default.
 	copyConcurrency := 1
-	if HasAllSSDs(*block) {
+	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")
 	err = copy.Copy(dir, topPath, copy.Options{
 		OnSymlink: func(_ string) copy.SymlinkAction {
@@ -84,7 +87,8 @@ func Install(cfgFile, topPath string, pt *progressbar.ProgressBar) error {
 		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),
+		)
 	}
 	pt.Describe("Successfully copied files")
 
@@ -263,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:

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)
 		})
 	}