Skip to content

Commit

Permalink
Failing to detect SSDs in copyDir should not be a fatal error. (#3653)
Browse files Browse the repository at this point in the history
* 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.
  • Loading branch information
cmacknz authored Oct 30, 2023
1 parent 66e0f95 commit 3d8bdf4
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 22 deletions.
15 changes: 7 additions & 8 deletions internal/pkg/agent/application/upgrade/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ import (
"runtime"
"strings"

"github.com/jaypipes/ghw"

"github.com/otiai10/copy"
"go.elastic.co/apm"

Expand Down Expand Up @@ -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
}

Expand Down
2 changes: 1 addition & 1 deletion internal/pkg/agent/cmd/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
42 changes: 30 additions & 12 deletions internal/pkg/agent/install/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand All @@ -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")

Expand Down Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion internal/pkg/agent/install/install_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
Expand Down

0 comments on commit 3d8bdf4

Please sign in to comment.