Merge pull request #696 from NAICNO/larstha-678-sonalyze-client-errors

Fix #678 - Channel remote errors properly

code/sonalyze/application/local.go

@@ -3,6 +3,7 @@
 package application
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"math"
@@ -19,17 +20,17 @@ import (
 	"sonalyze/sonarlog"
 )
 
-func LocalOperation(cmd SampleAnalysisCommand, _ io.Reader, stdout, stderr io.Writer) error {
-	args := cmd.SharedFlags()
+func LocalOperation(command SampleAnalysisCommand, _ io.Reader, stdout, stderr io.Writer) error {
+	args := command.SharedFlags()
 
-	cfg, err := db.MaybeGetConfig(cmd.ConfigFile())
+	cfg, err := db.MaybeGetConfig(command.ConfigFile())
 	if err != nil {
 		return err
 	}
 
-	hostGlobber, recordFilter, err := buildRecordFilters(cmd, cfg, args.Verbose)
+	hostGlobber, recordFilter, err := buildRecordFilters(command, cfg, args.Verbose)
 	if err != nil {
-		return fmt.Errorf("Failed to create record filter\n%w", err)
+		return fmt.Errorf("Failed to create record filter: %v", err)
 	}
 
 	var theLog db.SampleCluster
@@ -39,7 +40,7 @@ func LocalOperation(cmd SampleAnalysisCommand, _ io.Reader, stdout, stderr io.Wr
 		theLog, err = db.OpenPersistentCluster(args.DataDir, cfg)
 	}
 	if err != nil {
-		return fmt.Errorf("Failed to open log store\n%w", err)
+		return fmt.Errorf("Failed to open log store: %v", err)
 	}
 
 	streams, bounds, read, dropped, err :=
@@ -49,39 +50,33 @@ func LocalOperation(cmd SampleAnalysisCommand, _ io.Reader, stdout, stderr io.Wr
 			args.ToDate,
 			hostGlobber,
 			recordFilter,
-			cmd.NeedsBounds(),
+			command.NeedsBounds(),
 			args.Verbose,
 		)
 	if err != nil {
-		return fmt.Errorf("Failed to read log records\n%w", err)
+		return fmt.Errorf("Failed to read log records: %v", err)
 	}
 	if args.Verbose {
 		Log.Infof("%d records read + %d dropped\n", read, dropped)
 		UstrStats(stderr, false)
 	}
 
 	sonarlog.ComputePerSampleFields(streams)
-	err = cmd.Perform(stdout, cfg, theLog, streams, bounds, hostGlobber, recordFilter)
-
-	if err != nil {
-		return fmt.Errorf("Failed to perform operation\n%w", err)
-	}
-
-	return nil
+	return command.Perform(stdout, cfg, theLog, streams, bounds, hostGlobber, recordFilter)
 }
 
 func buildRecordFilters(
-	cmd SampleAnalysisCommand,
+	command SampleAnalysisCommand,
 	cfg *config.ClusterConfig,
 	verbose bool,
 ) (*hostglob.HostGlobber, *db.SampleFilter, error) {
-	args := cmd.SharedFlags()
+	args := command.SharedFlags()
 
 	// Temporary limitation.
 
-	if _, ok := cmd.(*profile.ProfileCommand); ok {
+	if _, ok := command.(*profile.ProfileCommand); ok {
 		if len(args.RecordFilterArgs.Job) != 1 || len(args.RecordFilterArgs.ExcludeJob) != 0 {
-			return nil, nil, fmt.Errorf("Exactly one specific job number is required by `profile`")
+			return nil, nil, errors.New("Exactly one specific job number is required by `profile`")
 		}
 	}
 
@@ -108,7 +103,7 @@ func buildRecordFilters(
 
 	// Command-specific defaults for the record filters.
 
-	allUsers, skipSystemUsers, excludeSystemCommands, excludeHeartbeat := cmd.DefaultRecordFilters()
+	allUsers, skipSystemUsers, excludeSystemCommands, excludeHeartbeat := command.DefaultRecordFilters()
 
 	// Included users, empty means "all"
 
@@ -137,7 +132,7 @@ func buildRecordFilters(
 		} else if name := os.Getenv("USER"); name != "" {
 			includeUsers[StringToUstr(name)] = true
 		} else {
-			return nil, nil, fmt.Errorf("Not able to determine user, none given and $LOGNAME and $USER are empty")
+			return nil, nil, errors.New("Not able to determine user, none given and $LOGNAME and $USER are empty")
 		}
 	}
 

code/sonalyze/application/remote.go

@@ -135,25 +135,33 @@ func RemoteOperation(rCmd RemotableCommand, verb string, stdin io.Reader, stdout
 	}
 
 	err = command.Run()
+
+	// If there is a processing error on the remote end then the server will respond with a 400 code
+	// and the text that would otherwise go to stderr, see runSonalyze() in daemon/perform.go.  That
+	// translates as a non-nil error with code 22 here, and the error message is on our local
+	// stdout.
+	//
+	// However if there is a problem with contacting the host or other curl failure, then the exit
+	// code is king and stderr may have some text.
+	//
+	// Curl has an elaborate set of exit codes, we could be more precise here but for most remote
+	// cases the user would just look them up.
+
 	if err != nil {
-		if rCmd.VerboseFlag() {
-			outs := newStdout.String()
-			if outs != "" {
-				fmt.Fprintf(stdout, "Output from failed (%s) subprocess: %s", err, outs)
-			}
-			errs := newStderr.String()
-			if errs != "" {
-				fmt.Fprintf(stdout, "Errors from failed (%s) subprocess: %s", err, errs)
+		if xe, ok := err.(*exec.ExitError); ok {
+			switch xe.ExitCode() {
+			case 22:
+				return fmt.Errorf("Remote: %s", newStdout.String())
+			case 5, 6, 7:
+				return fmt.Errorf("Failed to resolve remote host (or proxy).  Exit code %v, stderr=%s",
+					xe.ExitCode(), string(xe.Stderr))
+			default:
+				return fmt.Errorf("Curl problem: exit code %v, stderr=%s", xe.ExitCode(), string(xe.Stderr))
 			}
 		}
-		// Print this unredacted on the assumption that the remote sonalyzed/sonalyze don't
-		// reveal anything they shouldn't.
-		return err
-	}
-	errs := newStderr.String()
-	if errs != "" {
-		return errors.New(errs)
+		return fmt.Errorf("Could not start curl: %v", err)
 	}
+
 	// print, not println, or we end up adding a blank line that confuses consumers
 	fmt.Fprint(stdout, newStdout.String())
 	return nil

code/sonalyze/cmd/args.go

@@ -148,7 +148,7 @@ func (ra *RemotingArgs) Add(fs *flag.FlagSet) {
 func (ra *RemotingArgs) Validate() error {
 	if ra.Remote != "" || ra.Cluster != "" {
 		if ra.Remote == "" || ra.Cluster == "" {
-			return fmt.Errorf("-remote and -cluster must be used together")
+			return errors.New("-remote and -cluster must be used together")
 		}
 		ra.Remoting = true
 	}
@@ -243,10 +243,10 @@ func (s *SourceArgs) Validate() error {
 		// calling Validate(), it would confuse the matter - just disallow explicit values.  (This
 		// is a small abstraction leak.)
 		if s.DataDir != "" {
-			return fmt.Errorf("-data-dir may not be used with -remote or -cluster")
+			return errors.New("-data-dir may not be used with -remote or -cluster")
 		}
 		if len(s.LogFiles) > 0 {
-			return fmt.Errorf("-- logfile ... may not be used with -remote or -cluster")
+			return errors.New("-- logfile ... may not be used with -remote or -cluster")
 		}
 	} else {
 		// Compute and clean the dataDir and clean any logfiles.  If we have neither logfiles nor
@@ -260,7 +260,7 @@ func (s *SourceArgs) Validate() error {
 				s.LogFiles[i] = path.Clean(s.LogFiles[i])
 			}
 		} else if s.DataDir == "" {
-			return fmt.Errorf("Required -data-dir or -- logfile ...")
+			return errors.New("Required -data-dir or -- logfile ...")
 		}
 	}
 
@@ -523,7 +523,7 @@ func ValidateFormatArgs(
 	var others map[string]bool
 	fa.PrintFields, others, err = ParseFormatSpec(defaultFields, fa.Fmt, formatters, aliases)
 	if err == nil && len(fa.PrintFields) == 0 {
-		err = errors.New("No output fields were selected in format string")
+		err = errors.New("No valid output fields were selected in format string")
 	}
 	fa.PrintOpts = StandardFormatOptions(others, def)
 	return err
@@ -584,7 +584,7 @@ func (rs *RepeatableCommaSeparated[T]) Set(s string) error {
 	ws := make([]T, 0, len(ys))
 	for _, y := range ys {
 		if y == "" {
-			return errors.New("Empty string")
+			return errors.New("Empty string is an invalid argument")
 		}
 		n, err := rs.fromString(y)
 		if err != nil {

code/sonalyze/cmd/load/load.go

@@ -141,7 +141,7 @@ func (lc *LoadCommand) Validate() error {
 
 	var e4 error
 	if lc.All && lc.Last {
-		e4 = errors.New("Incoherent printing options")
+		e4 = errors.New("Incoherent printing options: both -all and -last")
 	}
 	if !lc.All && !lc.Last {
 		lc.All = true

code/sonalyze/cmd/nodes/nodes.go

@@ -113,7 +113,7 @@ func (nc *NodeCommand) Perform(_ io.Reader, stdout, stderr io.Writer) error {
 		theLog, err = db.OpenPersistentCluster(nc.DataDir, cfg)
 	}
 	if err != nil {
-		return fmt.Errorf("Failed to open log store\n%w", err)
+		return fmt.Errorf("Failed to open log store: %v", err)
 	}
 
 	// Read and filter the raw sysinfo data.
@@ -125,7 +125,7 @@ func (nc *NodeCommand) Perform(_ io.Reader, stdout, stderr io.Writer) error {
 		nc.Verbose,
 	)
 	if err != nil {
-		return fmt.Errorf("Failed to read log records\n%w", err)
+		return fmt.Errorf("Failed to read log records: %v", err)
 	}
 	records := uslices.Catenate(recordBlobs)
 	if nc.Verbose {
@@ -135,7 +135,7 @@ func (nc *NodeCommand) Perform(_ io.Reader, stdout, stderr io.Writer) error {
 
 	hostGlobber, recordFilter, err := nc.buildRecordFilter(nc.Verbose)
 	if err != nil {
-		return fmt.Errorf("Failed to create record filter\n%w", err)
+		return fmt.Errorf("Failed to create record filter: %v", err)
 	}
 
 	records = slices.DeleteFunc(records, func(s *config.NodeConfigRecord) bool {

code/sonalyze/cmd/profile/perform.go

@@ -3,6 +3,7 @@ package profile
 import (
 	"cmp"
 	"errors"
+	"fmt"
 	"io"
 	"math"
 	"slices"
@@ -33,7 +34,7 @@ func (pc *ProfileCommand) Perform(
 	jobId := pc.Job[0]
 
 	if len(streams) == 0 {
-		return errors.New("No processes")
+		return fmt.Errorf("No processes matching job ID(s): %v", pc.Job)
 	}
 
 	// Simplify: Assert that the input has only a single host.

code/sonalyze/cmd/profile/print.go

@@ -326,12 +326,12 @@ func lookupSingleFormatter(
 			formatter = formatGpuMem
 			n++
 		default:
-			err = fmt.Errorf("Not a known field: %s", f.Name)
+			err = fmt.Errorf("Not a printable field for this output format: %s", f.Name)
 			return
 		}
 	}
 	if n != 1 {
-		err = errors.New("formatted output needs exactly one valid field")
+		err = errors.New("Formatted output needs exactly one valid field")
 	}
 	return
 }

code/sonalyze/cmd/profile/profile.go

@@ -70,7 +70,7 @@ func (pc *ProfileCommand) Validate() error {
 		profileAliases,
 	)
 	if e4 == nil && len(pc.PrintFields) == 0 && !others["json"] {
-		e4 = errors.New("No output fields were selected in format string")
+		e4 = errors.New("No valid output fields were selected in format string")
 	}
 
 	// Options for profile are restrictive, and a little wonky because html is handled on the side,
@@ -86,7 +86,7 @@ func (pc *ProfileCommand) Validate() error {
 
 	var e5 error
 	if pc.htmlOutput && !pc.PrintOpts.IsDefaultFormat() {
-		e5 = errors.New("Multiple formats requested")
+		e5 = errors.New("Multiple output formats requested")
 	}
 
 	// The printing code uses custom logic for everything but Fixed layout, and the custom logic

code/sonalyze/cmd/report/report.go

@@ -75,7 +75,7 @@ func (rc *ReportCommand) Validate() error {
 	// combination of fs.ValidPath + filepath.Localize is just as good and more permissive, it would
 	// allow for files in subdirectories of the report directory too.
 	if !filenameRe.MatchString(rc.ReportName) || !filepath.IsLocal(rc.ReportName) {
-		return errors.New("Illegal report file name")
+		return errors.New("Illegal file name for -report-name")
 	}
 
 	if rc.ReportDir == "" {

code/sonalyze/cmd/sacct/perform.go

@@ -35,7 +35,7 @@ func (sc *SacctCommand) Perform(_ io.Reader, stdout, stderr io.Writer) error {
 		theLog, err = db.OpenPersistentCluster(sc.DataDir, cfg)
 	}
 	if err != nil {
-		return fmt.Errorf("Failed to open log store\n%w", err)
+		return fmt.Errorf("Failed to open log store: %v", err)
 	}
 
 	// Read the raw sacct data.
@@ -46,7 +46,7 @@ func (sc *SacctCommand) Perform(_ io.Reader, stdout, stderr io.Writer) error {
 		sc.Verbose,
 	)
 	if err != nil {
-		return fmt.Errorf("Failed to read log records\n%w", err)
+		return fmt.Errorf("Failed to read log records: %v", err)
 	}
 	// TODO: The catenation is expedient, we should be looping over the nested set (or in the
 	// future, using an iterator).

code/sonalyze/cmd/top/top.go

@@ -112,7 +112,7 @@ func (tc *TopCommand) Perform(stdin io.Reader, stdout, stderr io.Writer) error {
 		theLog, err = db.OpenPersistentCluster(tc.DataDir, cfg)
 	}
 	if err != nil {
-		return fmt.Errorf("Failed to open log store\n%w", err)
+		return fmt.Errorf("Failed to open log store: %v", err)
 	}
 
 	streams, _, read, dropped, err :=
@@ -124,7 +124,7 @@ func (tc *TopCommand) Perform(stdin io.Reader, stdout, stderr io.Writer) error {
 			tc.Verbose,
 		)
 	if err != nil {
-		return fmt.Errorf("Failed to read log records\n%w", err)
+		return fmt.Errorf("Failed to read log records: %v", err)
 	}
 	if tc.Verbose {
 		Log.Infof("%d records read + %d dropped\n", read, dropped)

code/sonalyze/cmd/uptime/uptime.go

@@ -37,7 +37,7 @@ func (uc *UptimeCommand) Add(fs *flag.FlagSet) {
 	uc.FormatArgs.Add(fs)
 
 	fs.UintVar(&uc.Interval, "interval", 0,
-		"The maximum sampling `interval` in minutes (before any randomization) seen in the data")
+		"The maximum sampling `interval` in minutes (before any randomization) seen in the data (required)")
 	fs.BoolVar(&uc.OnlyUp, "only-up", false, "Show only times when systems are up")
 	fs.BoolVar(&uc.OnlyDown, "only-down", false, "Show only times when systems are down")
 }

code/sonalyze/daemon/daemon.go

@@ -155,13 +155,13 @@ func (dc *DaemonCommand) Validate() error {
 	if dc.getAuthFile != "" {
 		dc.getAuthenticator, e4 = auth.ReadPasswords(dc.getAuthFile)
 		if e4 != nil {
-			e4 = fmt.Errorf("Failed to read analysis authentication file %w", e4)
+			e4 = fmt.Errorf("Failed to read analysis authentication file: %v", e4)
 		}
 	}
 	if dc.postAuthFile != "" {
 		dc.postAuthenticator, e5 = auth.ReadPasswords(dc.postAuthFile)
 		if e5 != nil {
-			return fmt.Errorf("Failed to read upload authentication file %w", e5)
+			return fmt.Errorf("Failed to read upload authentication file: %v", e5)
 		}
 	}
 	_, dc.aliasResolver, e6 = db.ReadClusterData(dc.jobanalyzerDir)

code/sonalyze/daemon/perform.go

@@ -40,7 +40,7 @@ import (
 func (dc *DaemonCommand) RunDaemon(_ io.Reader, _, stderr io.Writer) error {
 	logger, err := syslog.Dial("", "", syslog.LOG_INFO|syslog.LOG_USER, logTag)
 	if err != nil {
-		return fmt.Errorf("FATAL ERROR: Failing to open logger: %w", err)
+		return fmt.Errorf("FATAL ERROR: Failing to open logger: %v", err)
 	}
 	Log.SetUnderlying(logger)
 

code/sonalyze/db/clusterstore.go

@@ -480,7 +480,7 @@ func readRecordsFromFiles[T any](
 
 	if bad != "" {
 		recordBlobs = nil
-		err = fmt.Errorf("Failed to process one or more files:\n%s", bad)
+		err = fmt.Errorf("Failed to process one or more files: %s", bad)
 		return
 	}
 

code/sonalyze/db/logfile.go

@@ -254,7 +254,7 @@ func (lf *LogFile) flushSyncLocked() (err error) {
 		//
 		// Could also be too many open files, in which case we really want to close all open
 		// files and retry.
-		err = fmt.Errorf("Failed to open/create file (%v)", err)
+		err = fmt.Errorf("Failed to open/create file: %v", err)
 		return
 	}
 	defer f.Close()