From 599e267d966678ea1d75eebad8332b063d447664 Mon Sep 17 00:00:00 2001
From: Craig MacKenzie <craig.mackenzie@elastic.co>
Date: Thu, 26 Oct 2023 12:59:28 -0400
Subject: [PATCH] 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)
 		})
 	}