feat(fio): add option to disable runtime (#1601)

emosbaugh · web-flow · commit 1b1efa133e01 · 2024-08-22T16:47:08.000-07:00
diff --git a/config/crds/troubleshoot.sh_hostcollectors.yaml b/config/crds/troubleshoot.sh_hostcollectors.yaml
@@ -1337,6 +1337,12 @@ spec:
                             background IOPS feature if enabled, since those must be fixed at 4096.
                           format: int64
                           type: integer
+                        runTime:
+                          description: |-
+                            Limit runtime. The test will run until it completes the configured I/O workload or until it
+                            has run for this specified amount of time, whichever occurs first. When the unit is omitted,
+                            the value is interpreted in seconds. Defaults to 120 seconds. Set to "0" to disable.
+                          type: string
                         sync:
                           description: Whether to call sync on the file after each
                             write. Does not apply to background IOPS task.
diff --git a/config/crds/troubleshoot.sh_hostpreflights.yaml b/config/crds/troubleshoot.sh_hostpreflights.yaml
@@ -1337,6 +1337,12 @@ spec:
                             background IOPS feature if enabled, since those must be fixed at 4096.
                           format: int64
                           type: integer
+                        runTime:
+                          description: |-
+                            Limit runtime. The test will run until it completes the configured I/O workload or until it
+                            has run for this specified amount of time, whichever occurs first. When the unit is omitted,
+                            the value is interpreted in seconds. Defaults to 120 seconds. Set to "0" to disable.
+                          type: string
                         sync:
                           description: Whether to call sync on the file after each
                             write. Does not apply to background IOPS task.
@@ -1819,6 +1825,12 @@ spec:
                             background IOPS feature if enabled, since those must be fixed at 4096.
                           format: int64
                           type: integer
+                        runTime:
+                          description: |-
+                            Limit runtime. The test will run until it completes the configured I/O workload or until it
+                            has run for this specified amount of time, whichever occurs first. When the unit is omitted,
+                            the value is interpreted in seconds. Defaults to 120 seconds. Set to "0" to disable.
+                          type: string
                         sync:
                           description: Whether to call sync on the file after each
                             write. Does not apply to background IOPS task.
diff --git a/config/crds/troubleshoot.sh_preflights.yaml b/config/crds/troubleshoot.sh_preflights.yaml
@@ -18723,6 +18723,12 @@ spec:
                             background IOPS feature if enabled, since those must be fixed at 4096.
                           format: int64
                           type: integer
+                        runTime:
+                          description: |-
+                            Limit runtime. The test will run until it completes the configured I/O workload or until it
+                            has run for this specified amount of time, whichever occurs first. When the unit is omitted,
+                            the value is interpreted in seconds. Defaults to 120 seconds. Set to "0" to disable.
+                          type: string
                         sync:
                           description: Whether to call sync on the file after each
                             write. Does not apply to background IOPS task.
diff --git a/config/crds/troubleshoot.sh_remotecollectors.yaml b/config/crds/troubleshoot.sh_remotecollectors.yaml
@@ -171,6 +171,12 @@ spec:
                             background IOPS feature if enabled, since those must be fixed at 4096.
                           format: int64
                           type: integer
+                        runTime:
+                          description: |-
+                            Limit runtime. The test will run until it completes the configured I/O workload or until it
+                            has run for this specified amount of time, whichever occurs first. When the unit is omitted,
+                            the value is interpreted in seconds. Defaults to 120 seconds. Set to "0" to disable.
+                          type: string
                         sync:
                           description: Whether to call sync on the file after each
                             write. Does not apply to background IOPS task.
diff --git a/config/crds/troubleshoot.sh_supportbundles.yaml b/config/crds/troubleshoot.sh_supportbundles.yaml
@@ -19951,6 +19951,12 @@ spec:
                             background IOPS feature if enabled, since those must be fixed at 4096.
                           format: int64
                           type: integer
+                        runTime:
+                          description: |-
+                            Limit runtime. The test will run until it completes the configured I/O workload or until it
+                            has run for this specified amount of time, whichever occurs first. When the unit is omitted,
+                            the value is interpreted in seconds. Defaults to 120 seconds. Set to "0" to disable.
+                          type: string
                         sync:
                           description: Whether to call sync on the file after each
                             write. Does not apply to background IOPS task.
diff --git a/pkg/analyze/host_filesystem_performance.go b/pkg/analyze/host_filesystem_performance.go
@@ -74,11 +74,18 @@ func (a *AnalyzeHostFilesystemPerformance) Analyze(
 		return nil, errors.Wrapf(err, "failed to unmarshal fio results from %s", name)
 	}
 
-	if len(fioResult.Jobs) == 0 {
-		return nil, errors.Errorf("no jobs found in fio results from %s", name)
+	var job *collect.FioJobs
+	for _, j := range fioResult.Jobs {
+		if j.JobName == collect.FioJobName {
+			job = &j
+			break
+		}
+	}
+	if job == nil {
+		return nil, errors.Errorf("no job named 'fsperf' found in fio results from %s", name)
 	}
 
-	fioWriteLatency := fioResult.Jobs[0].Sync
+	fioWriteLatency := job.Sync
 
 	fsPerf := fioWriteLatency.FSPerfResults()
 	if err := json.Unmarshal(contents, &fsPerf); err != nil {
diff --git a/pkg/apis/troubleshoot/v1beta2/hostcollector_shared.go b/pkg/apis/troubleshoot/v1beta2/hostcollector_shared.go
@@ -145,6 +145,11 @@ type FilesystemPerformance struct {
 	// Whether to call datasync on the file after each write. Skipped if Sync is also true. Does not
 	// apply to background IOPS task.
 	Datasync bool `json:"datasync,omitempty"`
+	// Limit runtime. The test will run until it completes the configured I/O workload or until it
+	// has run for this specified amount of time, whichever occurs first. When the unit is omitted,
+	// the value is interpreted in seconds. Defaults to 120 seconds. Set to "0" to disable.
+	RunTime *string `json:"runTime,omitempty"`
+
 	// Total timeout, including background IOPS setup and warmup if enabled.
 	Timeout string `json:"timeout,omitempty"`
 
diff --git a/pkg/apis/troubleshoot/v1beta2/remote_collector_shared.go b/pkg/apis/troubleshoot/v1beta2/remote_collector_shared.go
@@ -116,6 +116,10 @@ type RemoteFilesystemPerformance struct {
 	Datasync bool `json:"datasync,omitempty"`
 	// Total timeout, including background IOPS setup and warmup if enabled.
 	Timeout string `json:"timeout,omitempty"`
+	// Limit runtime. The test will run until it completes the configured I/O workload or until it
+	// has run for this specified amount of time, whichever occurs first. When the unit is omitted,
+	// the value is interpreted in seconds. Defaults to 120 seconds. Set to "0" to disable.
+	RunTime *string `json:"runTime,omitempty"`
 
 	// Enable the background IOPS feature.
 	EnableBackgroundIOPS bool `json:"enableBackgroundIOPS"`
diff --git a/pkg/apis/troubleshoot/v1beta2/zz_generated.deepcopy.go b/pkg/apis/troubleshoot/v1beta2/zz_generated.deepcopy.go
diff --git a/pkg/collect/host_filesystem_performance.go b/pkg/collect/host_filesystem_performance.go
@@ -19,6 +19,11 @@ import (
 	"k8s.io/klog/v2"
 )
 
+const (
+	FioJobName        = "fsperf"
+	DefaultFioRunTime = "120"
+)
+
 type Durations []time.Duration
 
 func (d Durations) Len() int {
@@ -367,22 +372,47 @@ func parseCollectorOptions(hostCollector *troubleshootv1beta2.FilesystemPerforma
 		return nil, nil, errors.New("Directory is required to collect filesystem performance info")
 	}
 
+	runtime, err := getFioRuntime(hostCollector.RunTime)
+	if err != nil {
+		return nil, nil, err
+	}
+
 	latencyBenchmarkOptions := FioJobOptions{
 		RW:        "write",
 		IOEngine:  "sync",
 		FDataSync: "1",
 		Directory: hostCollector.Directory,
 		Size:      strconv.FormatUint(fileSize, 10),
 		BS:        strconv.FormatUint(operationSize, 10),
-		Name:      "fsperf",
-		RunTime:   "120",
+		Name:      FioJobName,
+		RunTime:   runtime,
 	}
 
 	command := buildFioCommand(latencyBenchmarkOptions)
 
 	return command, &latencyBenchmarkOptions, nil
 }
 
+// getFioRuntime returns the runTime value or the default if runTime is nil, empty or <= 0.
+// This attepmts to maintain backwards compatibility prior to adding runTime to the collector spec,
+// defaulting to 120 seconds.
+func getFioRuntime(runTime *string) (string, error) {
+	if runTime == nil {
+		return DefaultFioRunTime, nil
+	}
+	if *runTime == "" {
+		return "", nil // disable
+	}
+	i, err := strconv.Atoi(*runTime)
+	if err != nil {
+		return "", errors.Wrapf(err, "failed to parse runTime %q", *runTime)
+	}
+	if i <= 0 {
+		return "", nil // disable
+	}
+	return *runTime, nil
+}
+
 func buildFioCommand(opts FioJobOptions) []string {
 	command := []string{"fio"}
 	v := reflect.ValueOf(opts)
@@ -407,7 +437,7 @@ func collectFioResults(ctx context.Context, hostCollector *troubleshootv1beta2.F
 	}
 
 	klog.V(2).Infof("collecting fio results: %s", strings.Join(command, " "))
-	output, err := exec.CommandContext(ctx, command[0], command[1:]...).Output()
+	output, err := exec.CommandContext(ctx, command[0], command[1:]...).Output() // #nosec G204
 	if err != nil {
 		if exitErr, ok := err.(*exec.ExitError); ok {
 			if exitErr.ExitCode() == 1 {
diff --git a/pkg/collect/host_filesystem_performance_linux.go b/pkg/collect/host_filesystem_performance_linux.go
@@ -33,8 +33,13 @@ func collectHostFilesystemPerformance(hostCollector *troubleshootv1beta2.Filesys
 		}
 		timeout = d
 	}
-	ctx, cancel := context.WithTimeout(context.Background(), timeout)
-	defer cancel()
+	timeoutCtx, timeoutCancel := context.WithTimeout(context.Background(), timeout)
+	defer timeoutCancel()
+
+	// Start a new context for the fio collector and handle the timeout separately so we can
+	// distinguish between the timeout and a command failure in the analyzer.
+	collectCtx, collectCancel := context.WithCancel(context.Background())
+	defer collectCancel()
 
 	collectorName := hostCollector.CollectorName
 	if collectorName == "" {
@@ -53,7 +58,7 @@ func collectHostFilesystemPerformance(hostCollector *troubleshootv1beta2.Filesys
 		jobs := hostCollector.BackgroundReadIOPSJobs + hostCollector.BackgroundWriteIOPSJobs
 		done := make(chan bool, jobs)
 		defer func() {
-			cancel()
+			collectCancel()
 			for i := 0; i < jobs; i++ {
 				<-done
 			}
@@ -65,24 +70,34 @@ func collectHostFilesystemPerformance(hostCollector *troubleshootv1beta2.Filesys
 			jobs:      hostCollector.BackgroundReadIOPSJobs,
 			directory: hostCollector.Directory,
 		}
-		backgroundIOPS(ctx, opts, done)
+		backgroundIOPS(collectCtx, opts, done)
 
 		opts = backgroundIOPSOpts{
 			read:      false,
 			iopsLimit: hostCollector.BackgroundWriteIOPS,
 			jobs:      hostCollector.BackgroundWriteIOPSJobs,
 			directory: hostCollector.Directory,
 		}
-		backgroundIOPS(ctx, opts, done)
+		backgroundIOPS(collectCtx, opts, done)
 
 		time.Sleep(time.Second * time.Duration(hostCollector.BackgroundIOPSWarmupSeconds))
 	}
 
 	var fioResult *FioResult
-
-	fioResult, err := collectFioResults(ctx, hostCollector)
-	if err != nil {
-		return nil, errors.Wrap(err, "failed to collect fio results")
+	errCh := make(chan error, 1)
+	go func() {
+		var err error
+		fioResult, err = collectFioResults(collectCtx, hostCollector)
+		errCh <- err
+	}()
+
+	select {
+	case <-timeoutCtx.Done():
+		return nil, errors.New("timeout")
+	case err := <-errCh:
+		if err != nil {
+			return nil, errors.Wrap(err, "failed to collect fio results")
+		}
 	}
 
 	b, err := json.Marshal(fioResult)
diff --git a/pkg/collect/host_filesystem_performance_test.go b/pkg/collect/host_filesystem_performance_test.go
diff --git a/schemas/preflight-troubleshoot-v1beta2.json b/schemas/preflight-troubleshoot-v1beta2.json
diff --git a/schemas/supportbundle-troubleshoot-v1beta2.json b/schemas/supportbundle-troubleshoot-v1beta2.json
