From 25713b3625937e0b2bf60aa128be8bc766f25f66 Mon Sep 17 00:00:00 2001 From: Micah Lee Date: Fri, 20 Jan 2023 17:11:52 -0500 Subject: [PATCH] Add IOPs checks to preflight These are based on the `fio` tool, which must be installed separately as a prereq for these checks. Further, the IOPs checks require `libaio` to be installed separately, and is only available on linux. --- CHANGELOG.md | 4 + .../Dockerfile.rhel.all-dependencies | 43 ++++ .../Dockerfile.ubuntu.all-dependencies | 3 + dev/Dockerfile | 40 ++++ pkg/checks/disk/fio/command_wrapper.go | 42 ++++ pkg/checks/disk/fio/command_wrapper_test.go | 34 ++++ pkg/checks/disk/fio/job.go | 117 +++++++++++ pkg/checks/disk/fio/job_test.go | 134 +++++++++++++ pkg/checks/disk/fio/types.go | 45 +++++ pkg/checks/disk/iops.go | 172 ++++++++++++++++ pkg/checks/disk/iops_test.go | 182 +++++++++++++++++ pkg/checks/disk/latency.go | 151 ++++++++++++++ pkg/checks/disk/latency_test.go | 189 ++++++++++++++++++ pkg/checks/{disk.go => disk/space.go} | 9 +- .../{disk_test.go => disk/space_test.go} | 8 +- pkg/cmd/root.go | 2 +- pkg/report/default.go | 10 +- pkg/report/default_test.go | 18 ++ 18 files changed, 1191 insertions(+), 12 deletions(-) create mode 100644 pkg/checks/disk/fio/command_wrapper.go create mode 100644 pkg/checks/disk/fio/command_wrapper_test.go create mode 100644 pkg/checks/disk/fio/job.go create mode 100644 pkg/checks/disk/fio/job_test.go create mode 100644 pkg/checks/disk/fio/types.go create mode 100644 pkg/checks/disk/iops.go create mode 100644 pkg/checks/disk/iops_test.go create mode 100644 pkg/checks/disk/latency.go create mode 100644 pkg/checks/disk/latency_test.go rename pkg/checks/{disk.go => disk/space.go} (78%) rename pkg/checks/{disk_test.go => disk/space_test.go} (81%) create mode 100644 pkg/report/default_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 37259b6..de58810 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - A new CLI flag `--debug` causes `conjur-preflight` to log more verbose information about the execution of the application and its checks. [conjurinc/conjur-preflight#19](https://github.com/conjurinc/conjur-preflight/pull/19) +- `conjur-preflight` now includes disk related checks for read, write, and sync + latency, as well as read and write operations per second (IOPs). These require + `fio` and `libaio` to be present as a prerequesite for these checks. + [conjurinc/conjur-preflight#19](https://github.com/conjurinc/conjur-preflight/pull/19) ### Fixed - Previously, the application version was not properly embedded in the final diff --git a/ci/integration/Dockerfile.rhel.all-dependencies b/ci/integration/Dockerfile.rhel.all-dependencies index e62c874..d9f34d5 100644 --- a/ci/integration/Dockerfile.rhel.all-dependencies +++ b/ci/integration/Dockerfile.rhel.all-dependencies @@ -1,3 +1,46 @@ FROM redhat/ubi8 RUN yum install -y podman + +# Install FIO from source, since it's not available from the +# UBI package repositories unless also running on a registered +# RHEL host. +RUN yum install -y \ + wget \ + gcc \ + make && \ + # + # Build libaio for iops tests + # + wget \ + -O libaio.tar.gz \ + https://pagure.io/libaio/archive/libaio-0.3.111/libaio-libaio-0.3.111.tar.gz && \ + mkdir libaio && \ + tar \ + --strip-components=1 \ + --directory=libaio \ + -xvf libaio.tar.gz && \ + cd libaio && \ + make install && \ + ldconfig && \ + cd .. && \ + rm libaio.tar.gz && \ + # + # Build fio + # + wget \ + -O fio.tar.gz \ + https://github.com/axboe/fio/archive/refs/tags/fio-3.33.tar.gz && \ + mkdir fio && \ + tar \ + --strip-components=1 \ + --directory=fio \ + -xvf fio.tar.gz && \ + cd fio && \ + ./configure && \ + make && \ + make install && \ + cd .. && \ + rm -rf fio.tar.gz && \ + yum remove -y wget make gcc + diff --git a/ci/integration/Dockerfile.ubuntu.all-dependencies b/ci/integration/Dockerfile.ubuntu.all-dependencies index 64c354f..66c538f 100644 --- a/ci/integration/Dockerfile.ubuntu.all-dependencies +++ b/ci/integration/Dockerfile.ubuntu.all-dependencies @@ -22,3 +22,6 @@ RUN apt-get update && \ docker-ce \ docker-ce-cli \ containerd.io + +RUN apt-get update && \ + apt-get install -y fio diff --git a/dev/Dockerfile b/dev/Dockerfile index b4a55b9..75db3c1 100644 --- a/dev/Dockerfile +++ b/dev/Dockerfile @@ -4,3 +4,43 @@ RUN yum install -y \ git \ golang \ make + +# Install FIO from source, since it's not available from the +# UBI package repositories unless also running on a registered +# RHEL host. +RUN yum install -y \ + wget && \ + # + # Build libaio for iops tests + # + wget \ + -O libaio.tar.gz \ + https://pagure.io/libaio/archive/libaio-0.3.111/libaio-libaio-0.3.111.tar.gz && \ + mkdir libaio && \ + tar \ + --strip-components=1 \ + --directory=libaio \ + -xvf libaio.tar.gz && \ + cd libaio && \ + make install && \ + ldconfig && \ + cd .. && \ + rm libaio.tar.gz && \ + # + # Build fio + # + wget \ + -O fio.tar.gz \ + https://github.com/axboe/fio/archive/refs/tags/fio-3.33.tar.gz && \ + mkdir fio && \ + tar \ + --strip-components=1 \ + --directory=fio \ + -xvf fio.tar.gz && \ + cd fio && \ + ./configure && \ + make && \ + make install && \ + cd .. && \ + rm -rf fio.tar.gz && \ + yum remove -y wget diff --git a/pkg/checks/disk/fio/command_wrapper.go b/pkg/checks/disk/fio/command_wrapper.go new file mode 100644 index 0000000..f74d992 --- /dev/null +++ b/pkg/checks/disk/fio/command_wrapper.go @@ -0,0 +1,42 @@ +package fio + +import ( + "bytes" + "os/exec" +) + +type command interface { + Run() ([]byte, error) +} + +type commandWrapper struct { + command *exec.Cmd + stdout bytes.Buffer + stderr bytes.Buffer +} + +func newCommandWrapper(name string, args ...string) command { + wrapper := commandWrapper{} + + // Instantiate the command + command := exec.Command(name, args...) + + // Bind the stdout and stderr + command.Stdout = &wrapper.stdout + command.Stderr = &wrapper.stderr + + // Wrap the command + wrapper.command = command + + return &wrapper +} + +func (wrapper *commandWrapper) Run() ([]byte, error) { + err := wrapper.command.Run() + + if err != nil { + return nil, err + } + + return wrapper.stdout.Bytes(), nil +} diff --git a/pkg/checks/disk/fio/command_wrapper_test.go b/pkg/checks/disk/fio/command_wrapper_test.go new file mode 100644 index 0000000..d9c9579 --- /dev/null +++ b/pkg/checks/disk/fio/command_wrapper_test.go @@ -0,0 +1,34 @@ +package fio + +import ( + "bytes" + "testing" +) + +func TestNewCommandWrapper(t *testing.T) { + cmd := newCommandWrapper("echo", "hello world") + if cmd == nil { + t.Error("Expected CommandWrapper, got nil") + } +} + +func TestCommandWrapper_Run(t *testing.T) { + cmd := newCommandWrapper("echo", "hello world") + + output, err := cmd.Run() + if err != nil { + t.Errorf("Expected nil error, got %s", err) + } + if !bytes.Equal(output, []byte("hello world\n")) { + t.Errorf("Expected 'hello world\\n', got %s", string(output)) + } +} + +func TestCommandWrapper_Run_Error(t *testing.T) { + cmd := newCommandWrapper("invalid_command", "hello world") + + _, err := cmd.Run() + if err == nil { + t.Error("Expected error, got nil") + } +} diff --git a/pkg/checks/disk/fio/job.go b/pkg/checks/disk/fio/job.go new file mode 100644 index 0000000..126d921 --- /dev/null +++ b/pkg/checks/disk/fio/job.go @@ -0,0 +1,117 @@ +package fio + +import ( + "encoding/json" + "fmt" + "os" + "os/exec" + + "github.com/conjurinc/conjur-preflight/pkg/log" +) + +const fioExecutable = "fio" + +// Executable represents an operation that can produce an fio result and +// emit raw output data. +type Executable interface { + Exec() (*Result, error) + OnRawOutput(func([]byte)) +} + +// Job is a concrete Executable for executing fio jobs +type Job struct { + // Required fields: + // ---------------------- + + Name string + Args []string + + // Optional fields: + // ---------------------- + + // OnRawOutput may be configured to receive the full standard output + // of the fio command. For example, to write the full output to a file. + rawOutputCallback func([]byte) + + // Injected dependencies: + // ---------------------- + + // Lookup function to return the full path for a command name + execLookPath func(string) (string, error) + newCommandWrapper func(string, ...string) command + jsonUnmarshal func([]byte, any) error +} + +// NewJob constructs a Job with the default dependencies +func NewJob(name string, args []string) Executable { + return &Job{ + // Set required fields + Name: name, + Args: args, + + // Construct default dependencies + execLookPath: exec.LookPath, + newCommandWrapper: newCommandWrapper, + jsonUnmarshal: json.Unmarshal, + } +} + +// Exec runs the given fio job in a temporary directory +func (job *Job) Exec() (*Result, error) { + // Create the directory for running the fio test. We have this return the + // cleanup method as well to simplify deferring this task when the function + // finishes. + cleanup, err := usingTestDirectory(job.Name) + if err != nil { + return nil, fmt.Errorf("unable to create test directory: %s", err) + } + defer cleanup() + + // Lookup full path for 'fio' + fioPath, err := job.execLookPath(fioExecutable) + if err != nil { + return nil, fmt.Errorf("unable to find 'fio' path: %s", err) + } + + // Run 'fio' command + commandWrapper := job.newCommandWrapper(fioPath, job.Args...) + output, err := commandWrapper.Run() + + if err != nil { + return nil, fmt.Errorf("unable to execute 'fio' job: %s", err) + } + + // If there is a configured result listener, notify it of the result output + if job.rawOutputCallback != nil { + job.rawOutputCallback(output) + } + + // Parse the result JSON + jsonResult := Result{} + err = job.jsonUnmarshal(output, &jsonResult) + if err != nil { + return nil, fmt.Errorf("unable to parse 'fio' output: %s", err) + } + + return &jsonResult, nil +} + +// OnRawOutput sets the callback to receive standard output from the fio +// command. +func (job *Job) OnRawOutput(callback func([]byte)) { + job.rawOutputCallback = callback +} + +func usingTestDirectory(jobName string) (func(), error) { + err := os.MkdirAll(jobName, os.ModePerm) + if err != nil { + return nil, err + } + + return func() { + err := os.RemoveAll(jobName) + if err != nil { + log.Warn("Unable to clean up test directory for job: %s", jobName) + } + }, nil +} diff --git a/pkg/checks/disk/fio/job_test.go b/pkg/checks/disk/fio/job_test.go new file mode 100644 index 0000000..a3ff47e --- /dev/null +++ b/pkg/checks/disk/fio/job_test.go @@ -0,0 +1,134 @@ +package fio + +import ( + "encoding/json" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +var testJobArgs = []string{"test_arg_1", "test_arg_2"} + +type mockCommand struct { + OutputBytes []byte + Error error +} + +func (mockCommand *mockCommand) Run() ([]byte, error) { + return mockCommand.OutputBytes, mockCommand.Error +} + +func TestNewJob(t *testing.T) { + exec := NewJob("test_job", testJobArgs) + if exec == nil { + t.Error("Expected Job, got nil") + } +} + +func TestJob_Exec(t *testing.T) { + var outputDestination []byte + + expectedOutput := []byte(`{"test_key": "test_value"}`) + + // Create Job with mocked dependencies + exec := &Job{ + Name: "test_job", + Args: testJobArgs, + + rawOutputCallback: func(data []byte) { + outputDestination = data + }, + + execLookPath: func(string) (string, error) { + return "fio", nil + }, + + newCommandWrapper: func(string, ...string) command { + return &mockCommand{ + OutputBytes: expectedOutput, + } + }, + + jsonUnmarshal: func(data []byte, v interface{}) error { + return json.Unmarshal(data, v) + }, + } + + result, err := exec.Exec() + if err != nil { + t.Errorf("Expected nil error, got %s", err) + } + + if result == nil { + t.Error("Expected Result, got nil") + } + + assert.Equal(t, expectedOutput, outputDestination) +} + +func TestJob_Exec_LookupError(t *testing.T) { + exec := &Job{ + Name: "test_job", + Args: testJobArgs, + + execLookPath: func(string) (string, error) { + return "", fmt.Errorf("Test error") + }, + } + + _, err := exec.Exec() + assert.ErrorContains(t, err, "unable to find 'fio' path:") +} + +func TestJob_Exec_CommandError(t *testing.T) { + exec := &Job{ + Name: "test_job", + Args: testJobArgs, + + execLookPath: func(string) (string, error) { + return "", nil + }, + + newCommandWrapper: func(string, ...string) command { + return &mockCommand{ + Error: fmt.Errorf("test error"), + } + }, + } + + _, err := exec.Exec() + assert.ErrorContains(t, err, "unable to execute 'fio' job:") +} + +func TestJob_Exec_ParseError(t *testing.T) { + exec := &Job{ + Name: "test_job", + Args: testJobArgs, + + execLookPath: func(string) (string, error) { + return "", nil + }, + + newCommandWrapper: func(string, ...string) command { + return &mockCommand{ + OutputBytes: []byte(`{"test_key": "test_value"}`), + } + }, + + jsonUnmarshal: func(data []byte, v interface{}) error { + return fmt.Errorf("test error") + }, + } + + _, err := exec.Exec() + assert.ErrorContains(t, err, "unable to parse 'fio' output:") +} + +func TestUsingTestDirectory(t *testing.T) { + cleanup, err := usingTestDirectory("test_dir") + if err != nil { + t.Errorf("Expected nil error, got %s", err) + } + cleanup() +} diff --git a/pkg/checks/disk/fio/types.go b/pkg/checks/disk/fio/types.go new file mode 100644 index 0000000..2cb9a6d --- /dev/null +++ b/pkg/checks/disk/fio/types.go @@ -0,0 +1,45 @@ +package fio + +// Result is a data structure that represents the JSON +// output returned from running `fio`. +type Result struct { + Version string `json:"fio version"` + Jobs []JobResult `json:"jobs"` +} + +// JobResult represents the results from an individual fio job. A FioResult +// may include multiple job results. +type JobResult struct { + Sync JobModeResult `json:"sync"` + Read JobModeResult `json:"read"` + Write JobModeResult `json:"write"` +} + +// JobModeResult represents the measurements for a given test mode +// (e.g. read, write). Not all modes provide all values. The populated +// values depend on the fio job parameters. +type JobModeResult struct { + Iops float64 `json:"iops"` + IopsMin int64 `json:"iops_min"` + IopsMax int64 `json:"iops_max"` + IopsMean float64 `json:"iops_mean"` + IopsStddev float64 `json:"iops_stddev"` + + LatNs ResultStats `json:"lat_ns"` +} + +// ResultStats represents the statistical measurements provided by fio. +type ResultStats struct { + Min int64 `json:"min"` + Max int64 `json:"max"` + Mean float64 `json:"mean"` + StdDev float64 `json:"stddev"` + N int64 `json:"N"` + Percentile Percentile `json:"percentile"` +} + +// Percentile provides a simple interface to return particular statistical +// percentiles from the fio stats results. +type Percentile struct { + NinetyNinth int64 `json:"99.000000"` +} diff --git a/pkg/checks/disk/iops.go b/pkg/checks/disk/iops.go new file mode 100644 index 0000000..fc8a602 --- /dev/null +++ b/pkg/checks/disk/iops.go @@ -0,0 +1,172 @@ +package disk + +import ( + "fmt" + "os" + + "github.com/conjurinc/conjur-preflight/pkg/checks/disk/fio" + "github.com/conjurinc/conjur-preflight/pkg/framework" + "github.com/conjurinc/conjur-preflight/pkg/log" +) + +const iopsJobName = "conjur-fio-iops" + +// IopsCheck is a pre-flight check to report the read and write IOPs for the +// directory in which `conjur-preflight` is run. +type IopsCheck struct { + // When debug mode is enabled, the IOPs check will write the full fio + // results to a file. + debug bool + + // We inject the fio command execution as a dependency that we can swap for + // unit testing + fioNewJob func(string, []string) fio.Executable +} + +// NewIopsCheck instantiates an Iops check with the default dependencies +func NewIopsCheck(debug bool) *IopsCheck { + return &IopsCheck{ + debug: debug, + + // Default dependencies + fioNewJob: fio.NewJob, + } +} + +// Run executes the IopsCheck by running `fio` and processing its output +func (iopsCheck *IopsCheck) Run() <-chan []framework.CheckResult { + future := make(chan []framework.CheckResult) + + go func() { + + fioResult, err := iopsCheck.runFioIopsTest() + + if err != nil { + future <- []framework.CheckResult{ + { + Title: "FIO IOPs", + Status: framework.STATUS_ERROR, + Value: "N/A", + Message: err.Error(), + }, + } + + return + } + + // Make sure a job exists in the fio results + if len(fioResult.Jobs) < 1 { + future <- []framework.CheckResult{ + { + Title: "FIO IOPs", + Status: framework.STATUS_ERROR, + Value: "N/A", + Message: "No job results returned by 'fio'", + }, + } + + return + } + + future <- []framework.CheckResult{ + fioReadIopsResult(&fioResult.Jobs[0]), + fioWriteIopsResult(&fioResult.Jobs[0]), + } + }() // async + + return future +} + +func fioReadIopsResult(job *fio.JobResult) framework.CheckResult { + + // 50 iops min from https://etcd.io/docs/v3.3/op-guide/hardware/ + status := framework.STATUS_INFO + if job.Read.Iops < 50 { + status = framework.STATUS_WARN + } + + // Format title + path, _ := os.Getwd() + titleStr := fmt.Sprintf("FIO - Read IOPs (%s)", path) + + // Format value + valueStr := fmt.Sprintf( + "%0.2f (Min: %d, Max: %d, StdDev: %0.2f)", + job.Read.Iops, + job.Read.IopsMin, + job.Read.IopsMax, + job.Read.IopsStddev, + ) + + return framework.CheckResult{ + Title: titleStr, + Status: status, + Value: valueStr, + } +} + +func fioWriteIopsResult(job *fio.JobResult) framework.CheckResult { + + // 50 iops min from https://etcd.io/docs/v3.3/op-guide/hardware/ + status := framework.STATUS_INFO + if job.Write.Iops < 50 { + status = framework.STATUS_WARN + } + + // Format title + path, _ := os.Getwd() + titleStr := fmt.Sprintf("FIO - Write IOPs (%s)", path) + + // Format value + valueStr := fmt.Sprintf( + "%0.2f (Min: %d, Max: %d, StdDev: %0.2f)", + job.Write.Iops, + job.Write.IopsMin, + job.Write.IopsMax, + job.Write.IopsStddev, + ) + + return framework.CheckResult{ + Title: titleStr, + Status: status, + Value: valueStr, + } +} + +func (iopsCheck *IopsCheck) runFioIopsTest() (*fio.Result, error) { + job := iopsCheck.fioNewJob( + iopsJobName, + []string{ + "--filename=conjur-fio-iops/data", + "--size=100MB", + "--direct=1", + "--rw=randrw", + "--bs=4k", + "--ioengine=libaio", + "--iodepth=256", + "--runtime=10", + "--numjobs=4", + "--time_based", + "--group_reporting", + "--output-format=json", + "--name=conjur-fio-iops", + }, + ) + + // In debug mode, we'll write out the raw results from 'fio' + if iopsCheck.debug { + job.OnRawOutput(func(data []byte) { writeResultToFile(data, iopsJobName) }) + } + + return job.Exec() +} + +func writeResultToFile(buffer []byte, jobName string) { + outputFilename := fmt.Sprintf("%s.json", jobName) + + err := os.WriteFile(outputFilename, buffer, 0644) + + if err != nil { + log.Warn("Failed to write result file for %s: %s", jobName, err) + } +} diff --git a/pkg/checks/disk/iops_test.go b/pkg/checks/disk/iops_test.go new file mode 100644 index 0000000..1a9e8c8 --- /dev/null +++ b/pkg/checks/disk/iops_test.go @@ -0,0 +1,182 @@ +// This can't be in the disk_test package because this requires access to the +// internal fioExec field on LatencyCheck. +package disk + +import ( + "errors" + "regexp" + "testing" + + "github.com/conjurinc/conjur-preflight/pkg/checks/disk/fio" + "github.com/conjurinc/conjur-preflight/pkg/framework" + "github.com/stretchr/testify/assert" +) + +type mockFioJob struct { + Result fio.Result + Error error +} + +func (job *mockFioJob) Exec() (*fio.Result, error) { + return &job.Result, job.Error +} + +func (job *mockFioJob) OnRawOutput(func([]byte)) { + // no-op +} + +func TestIopsCheck(t *testing.T) { + testCheck := &IopsCheck{ + debug: true, + fioNewJob: newSuccessfulIopsFioJob, + } + resultChan := testCheck.Run() + results := <-resultChan + + assert.Equal( + t, + 2, + len(results), + "There are read and write IOPs results present", + ) + + assertReadIopsResult(t, results[0], framework.STATUS_INFO) + assertWriteIopsResult(t, results[1], framework.STATUS_INFO) +} + +func TestIopsCheckWithPoorPerformance(t *testing.T) { + testCheck := &IopsCheck{ + fioNewJob: newPoorIopsPerformanceFioJob, + } + resultChan := testCheck.Run() + results := <-resultChan + + assert.Equal( + t, + 2, + len(results), + "There are read and write IOPs results present", + ) + + assertReadIopsResult(t, results[0], framework.STATUS_WARN) + assertWriteIopsResult(t, results[1], framework.STATUS_WARN) +} + +func TestIopsWithError(t *testing.T) { + testCheck := &IopsCheck{ + fioNewJob: newErrorFioJob, + } + resultChan := testCheck.Run() + results := <-resultChan + + // Expect only the error result + assert.Equal(t, 1, len(results)) + + assert.Equal(t, "FIO IOPs", results[0].Title) + assert.Equal(t, framework.STATUS_ERROR, results[0].Status) + assert.Equal(t, "N/A", results[0].Value) + assert.Equal(t, "test error", results[0].Message) +} + +func TestIopsWithNoJobs(t *testing.T) { + testCheck := &IopsCheck{ + fioNewJob: newEmptyFioJob, + } + resultChan := testCheck.Run() + results := <-resultChan + + // Expect only the error result + assert.Equal(t, 1, len(results)) + + assert.Equal(t, "FIO IOPs", results[0].Title) + assert.Equal(t, framework.STATUS_ERROR, results[0].Status) + assert.Equal(t, "N/A", results[0].Value) + assert.Equal(t, "No job results returned by 'fio'", results[0].Message) +} + +func assertReadIopsResult( + t *testing.T, + result framework.CheckResult, + expectedStatus string, +) { + assert.Regexp( + t, + regexp.MustCompile(`FIO - read iops \(.+\)`), + result.Title, + ) + assert.Equal(t, expectedStatus, result.Status) + assert.Regexp( + t, + regexp.MustCompile(`.+ \(Min: .+, Max: .+, StdDev: .+\)`), + result.Value, + ) +} + +func assertWriteIopsResult( + t *testing.T, + result framework.CheckResult, + expectedStatus string, +) { + assert.Regexp( + t, + regexp.MustCompile(`FIO - write iops \(.+\)`), + result.Title, + ) + assert.Equal(t, expectedStatus, result.Status) + assert.Regexp( + t, + regexp.MustCompile(`.+ \(Min: .+, Max: .+, StdDev: .+\)`), + result.Value, + ) +} + +func newSuccessfulIopsFioJob(jobName string, args []string) fio.Executable { + return &mockFioJob{ + Result: fio.Result{ + Jobs: []fio.JobResult{ + { + Read: fio.JobModeResult{ + Iops: 50, + }, + Write: fio.JobModeResult{ + Iops: 50, + }, + }, + }, + }, + } +} + +func newPoorIopsPerformanceFioJob( + jobName string, + args []string, +) fio.Executable { + return &mockFioJob{ + Result: fio.Result{ + Jobs: []fio.JobResult{ + { + Read: fio.JobModeResult{ + Iops: 48, + }, + Write: fio.JobModeResult{ + Iops: 48, + }, + }, + }, + }, + } +} + +func newErrorFioJob(jobName string, args []string) fio.Executable { + return &mockFioJob{ + Error: errors.New("test error"), + } +} + +func newEmptyFioJob(jobName string, args []string) fio.Executable { + return &mockFioJob{ + Result: fio.Result{ + Jobs: []fio.JobResult{}, + }, + } +} diff --git a/pkg/checks/disk/latency.go b/pkg/checks/disk/latency.go new file mode 100644 index 0000000..d31d260 --- /dev/null +++ b/pkg/checks/disk/latency.go @@ -0,0 +1,151 @@ +package disk + +import ( + "fmt" + "os" + + "github.com/conjurinc/conjur-preflight/pkg/checks/disk/fio" + "github.com/conjurinc/conjur-preflight/pkg/framework" +) + +// LatencyCheck is a pre-flight check to report the read, write, and sync +// latency for the directory in which `conjur-preflight` is run. +type LatencyCheck struct { + // When debug mode is enabled, the latency check will write the full fio + // results to a file. + debug bool + + // We inject the fio command execution as a dependency that we can swap for + // unit testing + fioNewJob func(string, []string) fio.Executable +} + +// NewLatencyCheck instantiates a Latency check with the default dependencies +func NewLatencyCheck(debug bool) *LatencyCheck { + return &LatencyCheck{ + debug: debug, + + // Default dependencies + fioNewJob: fio.NewJob, + } +} + +// Run executes the LatencyCheck by running `fio` and processing its output +func (latencyCheck *LatencyCheck) Run() <-chan []framework.CheckResult { + future := make(chan []framework.CheckResult) + + go func() { + fioResult, err := latencyCheck.runFioLatencyTest() + + if err != nil { + future <- []framework.CheckResult{ + { + Title: "FIO Latency", + Status: framework.STATUS_ERROR, + Value: "N/A", + Message: err.Error(), + }, + } + + return + } + + // Make sure a job exists in the fio results + if len(fioResult.Jobs) < 1 { + future <- []framework.CheckResult{ + { + Title: "FIO Latency", + Status: framework.STATUS_ERROR, + Value: "N/A", + Message: "No job results returned by 'fio'", + }, + } + + return + } + + future <- []framework.CheckResult{ + fioReadLatencyResult(&fioResult.Jobs[0]), + fioWriteLatencyResult(&fioResult.Jobs[0]), + fioSyncLatencyResult(&fioResult.Jobs[0]), + } + }() // async + + return future +} + +func fioReadLatencyResult(jobResult *fio.JobResult) framework.CheckResult { + // Convert the nanosecond result to milliseconds for readability + latMs := float64(jobResult.Read.LatNs.Percentile.NinetyNinth) / 1e6 + + latMsStr := fmt.Sprintf("%0.2f ms", latMs) + + status := framework.STATUS_INFO + if latMs > 10.0 { + status = framework.STATUS_WARN + } + + path, _ := os.Getwd() + + return framework.CheckResult{ + Title: fmt.Sprintf("FIO - Read Latency (99%%, %s)", path), + Status: status, + Value: latMsStr, + } +} + +func fioWriteLatencyResult(jobResult *fio.JobResult) framework.CheckResult { + // Convert the nanosecond result to milliseconds for readability + latMs := float64(jobResult.Write.LatNs.Percentile.NinetyNinth) / 1e6 + + latMsStr := fmt.Sprintf("%0.2f ms", latMs) + + status := framework.STATUS_INFO + if latMs > 10.0 { + status = framework.STATUS_WARN + } + + path, _ := os.Getwd() + + return framework.CheckResult{ + Title: fmt.Sprintf("FIO - Write Latency (99%%, %s)", path), + Status: status, + Value: latMsStr, + } +} + +func fioSyncLatencyResult(jobResult *fio.JobResult) framework.CheckResult { + // Convert the nanosecond result to milliseconds for readability + latMs := float64(jobResult.Sync.LatNs.Percentile.NinetyNinth) / 1e6 + + latMsStr := fmt.Sprintf("%0.2f ms", latMs) + + status := framework.STATUS_INFO + if latMs > 10.0 { + status = framework.STATUS_WARN + } + + path, _ := os.Getwd() + + return framework.CheckResult{ + Title: fmt.Sprintf("FIO - Sync Latency (99%%, %s)", path), + Status: status, + Value: latMsStr, + } +} + +func (latencyCheck *LatencyCheck) runFioLatencyTest() (*fio.Result, error) { + return latencyCheck.fioNewJob( + "conjur-fio-latency", + []string{ + "--rw=readwrite", + "--ioengine=sync", + "--fdatasync=1", + "--directory=conjur-fio-latency", + "--size=22m", + "--bs=2300", + "--name=conjur-fio-latency", + "--output-format=json", + }, + ).Exec() +} diff --git a/pkg/checks/disk/latency_test.go b/pkg/checks/disk/latency_test.go new file mode 100644 index 0000000..12874f7 --- /dev/null +++ b/pkg/checks/disk/latency_test.go @@ -0,0 +1,189 @@ +// This can't be in the disk_test package because this requires access to the +// internal fioExec field on LatencyCheck. +package disk + +import ( + "regexp" + "testing" + + "github.com/conjurinc/conjur-preflight/pkg/checks/disk/fio" + "github.com/conjurinc/conjur-preflight/pkg/framework" + "github.com/stretchr/testify/assert" +) + +func TestLatencyCheck(t *testing.T) { + testCheck := &LatencyCheck{ + fioNewJob: newSuccessfulLatencyFioJob, + } + resultChan := testCheck.Run() + results := <-resultChan + + assert.Equal(t, 3, len(results), "There are disk latency results present") + + assertReadLatencyResult(t, results[0], framework.STATUS_INFO) + assertWriteLatencyResult(t, results[1], framework.STATUS_INFO) + assertSyncLatencyResult(t, results[2], framework.STATUS_INFO) +} + +func TestLatencyCheckWithPoorPerformance(t *testing.T) { + testCheck := &LatencyCheck{ + fioNewJob: newPoorLatencyPerformanceFioJob, + } + resultChan := testCheck.Run() + results := <-resultChan + + assert.Equal(t, 3, len(results), "There are disk latency results present") + + assertReadLatencyResult(t, results[0], framework.STATUS_WARN) + assertWriteLatencyResult(t, results[1], framework.STATUS_WARN) + assertSyncLatencyResult(t, results[2], framework.STATUS_WARN) +} + +func TestLatencyWithError(t *testing.T) { + testCheck := &LatencyCheck{ + fioNewJob: newErrorFioJob, + } + resultChan := testCheck.Run() + results := <-resultChan + + // Expect only the error result + assert.Equal(t, 1, len(results)) + + assert.Equal(t, "FIO Latency", results[0].Title) + assert.Equal(t, framework.STATUS_ERROR, results[0].Status) + assert.Equal(t, "N/A", results[0].Value) + assert.Equal(t, "test error", results[0].Message) +} + +func TestLatencyWithNoJobs(t *testing.T) { + testCheck := &LatencyCheck{ + fioNewJob: newEmptyFioJob, + } + resultChan := testCheck.Run() + results := <-resultChan + + // Expect only the error result + assert.Equal(t, 1, len(results)) + + assert.Equal(t, "FIO Latency", results[0].Title) + assert.Equal(t, framework.STATUS_ERROR, results[0].Status) + assert.Equal(t, "N/A", results[0].Value) + assert.Equal(t, "No job results returned by 'fio'", results[0].Message) +} + +func assertReadLatencyResult( + t *testing.T, + result framework.CheckResult, + expectedStatus string, +) { + assertLatencyResult( + t, + result, + `FIO - read latency \(99%, .+\)`, + expectedStatus, + ) +} + +func assertWriteLatencyResult( + t *testing.T, + result framework.CheckResult, + expectedStatus string, +) { + assertLatencyResult( + t, + result, + `FIO - write latency \(99%, .+\)`, + expectedStatus, + ) +} + +func assertSyncLatencyResult( + t *testing.T, + result framework.CheckResult, + expectedStatus string, +) { + assertLatencyResult( + t, + result, + `FIO - sync latency \(99%, .+\)`, + expectedStatus, + ) +} + +func assertLatencyResult( + t *testing.T, + result framework.CheckResult, + expectedTitleRegex string, + expectedStatus string, +) { + assert.Regexp(t, regexp.MustCompile(expectedTitleRegex), result.Title) + assert.Equal(t, expectedStatus, result.Status) + assert.Regexp(t, regexp.MustCompile(`.+ ms`), result.Value) +} + +func newSuccessfulLatencyFioJob(jobName string, args []string) fio.Executable { + return &mockFioJob{ + Result: fio.Result{ + Jobs: []fio.JobResult{ + { + Read: fio.JobModeResult{ + LatNs: fio.ResultStats{ + Percentile: fio.Percentile{ + NinetyNinth: 10 * 1e6, + }, + }, + }, + Write: fio.JobModeResult{ + LatNs: fio.ResultStats{ + Percentile: fio.Percentile{ + NinetyNinth: 10 * 1e6, + }, + }, + }, + Sync: fio.JobModeResult{ + LatNs: fio.ResultStats{ + Percentile: fio.Percentile{ + NinetyNinth: 10 * 1e6, + }, + }, + }, + }, + }, + }, + } +} + +func newPoorLatencyPerformanceFioJob( + jobName string, + args []string, +) fio.Executable { + return &mockFioJob{ + Result: fio.Result{ + Jobs: []fio.JobResult{ + { + Read: fio.JobModeResult{ + LatNs: fio.ResultStats{ + Percentile: fio.Percentile{ + NinetyNinth: 11 * 1e6, + }, + }, + }, + Write: fio.JobModeResult{ + LatNs: fio.ResultStats{ + Percentile: fio.Percentile{ + NinetyNinth: 11 * 1e6, + }, + }, + }, + Sync: fio.JobModeResult{ + LatNs: fio.ResultStats{ + Percentile: fio.Percentile{ + NinetyNinth: 11 * 1e6, + }, + }, + }, + }, + }, + }, + } +} diff --git a/pkg/checks/disk.go b/pkg/checks/disk/space.go similarity index 78% rename from pkg/checks/disk.go rename to pkg/checks/disk/space.go index bc3187d..4c5585d 100644 --- a/pkg/checks/disk.go +++ b/pkg/checks/disk/space.go @@ -1,4 +1,4 @@ -package checks +package disk import ( "fmt" @@ -8,10 +8,13 @@ import ( "github.com/shirou/gopsutil/v3/disk" ) -type DiskSpace struct { +// SpaceCheck reports on the available partitions and devices on the current +// machine, as well as their available disk space. +type SpaceCheck struct { } -func (diskSpace *DiskSpace) Run() <-chan []framework.CheckResult { +// Run executes the disk checks and returns their results +func (*SpaceCheck) Run() <-chan []framework.CheckResult { future := make(chan []framework.CheckResult) go func() { diff --git a/pkg/checks/disk_test.go b/pkg/checks/disk/space_test.go similarity index 81% rename from pkg/checks/disk_test.go rename to pkg/checks/disk/space_test.go index 1de69da..8ddc35e 100644 --- a/pkg/checks/disk_test.go +++ b/pkg/checks/disk/space_test.go @@ -1,16 +1,16 @@ -package checks_test +package disk_test import ( "regexp" "testing" - "github.com/conjurinc/conjur-preflight/pkg/checks" + "github.com/conjurinc/conjur-preflight/pkg/checks/disk" "github.com/conjurinc/conjur-preflight/pkg/framework" "github.com/stretchr/testify/assert" ) -func TestDiskRun(t *testing.T) { - testCheck := &checks.DiskSpace{} +func TestSpaceCheck(t *testing.T) { + testCheck := &disk.SpaceCheck{} resultChan := testCheck.Run() results := <-resultChan diff --git a/pkg/cmd/root.go b/pkg/cmd/root.go index 7fce9c1..e50f388 100644 --- a/pkg/cmd/root.go +++ b/pkg/cmd/root.go @@ -23,7 +23,7 @@ func newRootCommand() *cobra.Command { log.EnableDebugMode() } - report := report.NewDefaultReport() + report := report.NewDefaultReport(debug) log.Debug("Running report...") result := report.Run() diff --git a/pkg/report/default.go b/pkg/report/default.go index 7199225..60352a8 100644 --- a/pkg/report/default.go +++ b/pkg/report/default.go @@ -2,10 +2,12 @@ package report import ( "github.com/conjurinc/conjur-preflight/pkg/checks" + "github.com/conjurinc/conjur-preflight/pkg/checks/disk" "github.com/conjurinc/conjur-preflight/pkg/framework" ) -func NewDefaultReport() framework.Report { +// NewDefaultReport returns a report containing the standard pre-flight checks +func NewDefaultReport(debug bool) framework.Report { return framework.Report{ Sections: []framework.ReportSection{ // TODO: @@ -16,12 +18,12 @@ func NewDefaultReport() framework.Report { &checks.Cpu{}, }, }, - // TODO - // - IOPS { Title: "Disk", Checks: []framework.Check{ - &checks.DiskSpace{}, + &disk.SpaceCheck{}, + disk.NewIopsCheck(debug), + disk.NewLatencyCheck(debug), }, }, { diff --git a/pkg/report/default_test.go b/pkg/report/default_test.go new file mode 100644 index 0000000..9c9715f --- /dev/null +++ b/pkg/report/default_test.go @@ -0,0 +1,18 @@ +package report_test + +import ( + "testing" + + "github.com/conjurinc/conjur-preflight/pkg/report" + "github.com/stretchr/testify/assert" +) + +func TestNewDefaultReport(t *testing.T) { + // We don't want to re-define the default report structure here, so we just + // call the constructor to ensure we don't introduce any runtime errors and + // that there are some report sections. + + report := report.NewDefaultReport(false) + + assert.Greater(t, len(report.Sections), 0) +}