From 820a606b8e4f361b4205b3e71fdaf327194231e5 Mon Sep 17 00:00:00 2001 From: Dasha Komsa Date: Tue, 13 Feb 2024 23:38:48 +0000 Subject: [PATCH 1/5] parse job events to extract failure messages Signed-off-by: Dasha Komsa --- go.mod | 2 +- internal/ansible/ansible.go | 119 ++++++++++++++++-- internal/ansible/ansible_test.go | 14 +-- internal/ansible/jobEvent.go | 27 ++++ internal/controller/ansibleRun/ansibleRun.go | 21 +--- .../controller/ansibleRun/ansibleRun_test.go | 89 +++++++------ 6 files changed, 196 insertions(+), 76 deletions(-) create mode 100644 internal/ansible/jobEvent.go diff --git a/go.mod b/go.mod index 7c59ab0..915e3fa 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/crossplane/crossplane-runtime v0.19.2 github.com/crossplane/crossplane-tools v0.0.0-20220310165030-1f43fc12793e github.com/google/go-cmp v0.5.9 + github.com/google/uuid v1.3.0 github.com/spf13/afero v1.9.5 gopkg.in/alecthomas/kingpin.v2 v2.2.6 gopkg.in/yaml.v2 v2.4.0 @@ -40,7 +41,6 @@ require ( github.com/golang/protobuf v1.5.3 // indirect github.com/google/gnostic v0.5.7-v3refs // indirect github.com/google/gofuzz v1.2.0 // indirect - github.com/google/uuid v1.3.0 // indirect github.com/imdario/mergo v0.3.12 // indirect github.com/inconshreveable/mousetrap v1.0.1 // indirect github.com/josharian/intern v1.0.0 // indirect diff --git a/internal/ansible/ansible.go b/internal/ansible/ansible.go index c9b7c49..38382c7 100644 --- a/internal/ansible/ansible.go +++ b/internal/ansible/ansible.go @@ -28,6 +28,7 @@ import ( "os/user" "path/filepath" "strconv" + "strings" "time" "github.com/apenella/go-ansible/pkg/stdoutcallback/results" @@ -36,7 +37,9 @@ import ( "github.com/crossplane-contrib/provider-ansible/pkg/runnerutil" "github.com/crossplane/crossplane-runtime/pkg/meta" "github.com/crossplane/crossplane-runtime/pkg/resource" + "github.com/google/uuid" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/log" ) const ( @@ -133,10 +136,10 @@ func withBehaviorVars(behaviorVars map[string]string) runnerOption { } } -// withAnsibleEnvDir set the runner env/extravars dir. -func withAnsibleEnvDir(dir string) runnerOption { +// withWorkDir set the runner working dir. +func withWorkDir(dir string) runnerOption { return func(r *Runner) { - r.AnsibleEnvDir = dir + r.workDir = dir } } @@ -312,14 +315,16 @@ func (p Parameters) Init(ctx context.Context, cr *v1alpha1.AnsibleRun, behaviorV return nil, err } - return new(withPath(path), + r := new(withPath(path), withCmdFunc(cmdFunc), withBehaviorVars(behaviorVars), withAnsibleRunPolicy(rPolicy), // TODO should be moved to connect() func - withAnsibleEnvDir(ansibleEnvDir), + withWorkDir(p.WorkingDirPath), withArtifactsHistoryLimit(p.ArtifactsHistoryLimit), - ), nil + ) + + return r, nil } // Runner struct holds the configuration to run the cmdFunc @@ -327,6 +332,7 @@ type Runner struct { Path string // absolute path on disk to a playbook or role depending on what cmdFunc expects behaviorVars map[string]string cmdFunc cmdFuncType // returns a Cmd that runs ansible-runner + workDir string AnsibleEnvDir string checkMode bool AnsibleRunPolicy *RunPolicy @@ -350,8 +356,12 @@ func (r *Runner) GetAnsibleRunPolicy() *RunPolicy { return r.AnsibleRunPolicy } +func (r *Runner) ansibleEnvDir() string { + return filepath.Clean(filepath.Join(r.workDir, "env")) +} + // Run execute the appropriate cmdFunc -func (r *Runner) Run() (*exec.Cmd, io.Reader, error) { +func (r *Runner) Run(ctx context.Context) (io.Reader, error) { var ( stdoutBuf bytes.Buffer stdoutWriter, stderrWriter io.Writer @@ -359,6 +369,10 @@ func (r *Runner) Run() (*exec.Cmd, io.Reader, error) { dc := r.cmdFunc(r.behaviorVars, r.checkMode) dc.Args = append(dc.Args, "--rotate-artifacts", strconv.Itoa(r.artifactsHistoryLimit)) + + id := uuid.New().String() + dc.Args = append(dc.Args, "--ident", id) + if !r.checkMode { // for disabled checkMode dc.Stdout and dc.Stderr are respectfully // written to os.Stdout and os.Stdout for debugging purpose @@ -383,10 +397,95 @@ func (r *Runner) Run() (*exec.Cmd, io.Reader, error) { err := dc.Start() if err != nil { - return nil, nil, err + return nil, err + } + + if err := dc.Wait(); err != nil { + jobEventsDir := filepath.Clean(filepath.Join(r.workDir, "artifacts", id, "job_events")) + failureReason, reasonErr := extractFailureReason(jobEventsDir) + if reasonErr != nil { + log.FromContext(ctx).Error(err, "extracting ansible failure message") + } + + return nil, fmt.Errorf("%w: %s", err, failureReason) + } + + return &stdoutBuf, nil +} + +func extractFailureReason(eventsDir string) (string, error) { + evts, err := parseEvents(eventsDir) + if err != nil { + return "", fmt.Errorf("parsing job events: %w", err) + } + + var msgs []string + for _, evt := range evts { + switch evt.Event { + case eventTypeRunnerFailed: + m, err := runnerEventMessage(evt, "Failed") + if err != nil { + return "", err + } + msgs = append(msgs, m) + case eventTypeRunnerUnreachable: + m, err := runnerEventMessage(evt, "Unreachable") + if err != nil { + return "", err + } + msgs = append(msgs, m) + default: + } + } + + return strings.Join(msgs, "; "), nil +} + +func parseEvents(dir string) ([]jobEvent, error) { + files, err := os.ReadDir(dir) + if err != nil { + return nil, fmt.Errorf("reading job events directory: %w", err) + } + + var evts []jobEvent + for _, file := range files { + evtBytes, err := os.ReadFile(filepath.Join(dir, file.Name())) + if err != nil { + return nil, fmt.Errorf("reading job event file %q: %w", file.Name(), err) + } + + var evt jobEvent + if err := json.Unmarshal(evtBytes, &evt); err != nil { + return nil, fmt.Errorf("unmarshaling job event from file %q: %w", file.Name(), err) + } + evts = append(evts, evt) } - return dc, &stdoutBuf, nil + return evts, nil +} + +func reunmarshal(data map[string]any, result any) error { + b, err := json.Marshal(data) + if err != nil { + return fmt.Errorf("marshaling: %w", err) + } + + return json.Unmarshal(b, result) +} + +func runnerEventMessage(evt jobEvent, reason string) (string, error) { + var evtData runnerEventData + if err := reunmarshal(evt.EventData, &evtData); err != nil { + return "", fmt.Errorf("unmarshaling job event %s as runner event: %w", evt.UUID, err) + } + + return fmt.Sprintf("%s on play %q, task %q, host %q: %s", + reason, + evtData.Play, + evtData.Task, + evtData.Host, + evtData.Result.Msg), nil + } // selectRolePath will determines the role path @@ -435,7 +534,7 @@ func addFile(path string, content []byte) error { // WriteExtraVar write extra var to env/extravars under working directory // it creates a non-existent env/extravars file func (r *Runner) WriteExtraVar(extraVar map[string]interface{}) error { - extraVarsPath := filepath.Join(r.AnsibleEnvDir, "extravars") + extraVarsPath := filepath.Join(r.ansibleEnvDir(), "extravars") contentVars := make(map[string]interface{}) data, err := os.ReadFile(filepath.Clean(extraVarsPath)) if err != nil { diff --git a/internal/ansible/ansible_test.go b/internal/ansible/ansible_test.go index e68d32a..5634029 100644 --- a/internal/ansible/ansible_test.go +++ b/internal/ansible/ansible_test.go @@ -189,7 +189,8 @@ func TestRun(t *testing.T) { runner := &Runner{ Path: dir, cmdFunc: func(_ map[string]string, _ bool) *exec.Cmd { - // echo works well for testing cause it will just print all the args and flags it doesn't recognize and return success + // echo works well for testing cause it will just print all the args and flags it doesn't recognize and return success, + // therefore checking its output also checks the args passed to it are correct return exec.CommandContext(context.Background(), "echo") }, AnsibleEnvDir: filepath.Join(dir, "env"), @@ -198,7 +199,6 @@ func TestRun(t *testing.T) { } expectedArgs := []string{"--rotate-artifacts", "3"} - expectedCmd := exec.Command(runner.cmdFunc(nil, false).Path, expectedArgs...) testCases := map[string]struct { checkMode bool @@ -216,19 +216,11 @@ func TestRun(t *testing.T) { for name, tc := range testCases { t.Run(name, func(t *testing.T) { runner.checkMode = tc.checkMode - cmd, outBuf, err := runner.Run() + outBuf, err := runner.Run(context.Background()) if err != nil { t.Fatalf("Unexpected Run() error: %v", err) } - if cmd.String() != expectedCmd.String() { - t.Errorf("Unexpected command %q expected %q", expectedCmd.String(), cmd.String()) - } - - if err := cmd.Wait(); err != nil { - t.Fatalf("Unexpected cmd.Wait() error: %v", err) - } - out, err := io.ReadAll(outBuf) if err != nil { t.Fatalf("Unexpected error reading command buffer: %v", err) diff --git a/internal/ansible/jobEvent.go b/internal/ansible/jobEvent.go new file mode 100644 index 0000000..3dfa655 --- /dev/null +++ b/internal/ansible/jobEvent.go @@ -0,0 +1,27 @@ +package ansible + +const ( + // https://github.com/ansible/awx/blob/devel/docs/job_events.md#job-event-relationships + // outlines various event types and the relationships between them + eventTypeRunnerFailed = "runner_on_failed" + eventTypeRunnerUnreachable = "runner_on_unreachable" +) + +// jobEvent represents [ansible-runner's job events](https://ansible.readthedocs.io/projects/runner/en/stable/intro/#artifactevents) +type jobEvent struct { + UUID string `json:"uuid"` + Stdout string `json:"stdout"` + Event string `json:"event"` + EventData map[string]any `json:"event_data"` +} + +type runnerEventData struct { + Play string `json:"play"` + Task string `json:"task"` + Host string `json:"host"` + Result runnerResult `json:"res"` +} + +type runnerResult struct { + Msg string `json:"msg"` +} diff --git a/internal/controller/ansibleRun/ansibleRun.go b/internal/controller/ansibleRun/ansibleRun.go index d5e4ad3..129a63d 100644 --- a/internal/controller/ansibleRun/ansibleRun.go +++ b/internal/controller/ansibleRun/ansibleRun.go @@ -24,7 +24,6 @@ import ( "fmt" "io" "os" - "os/exec" "path/filepath" "strings" "time" @@ -87,7 +86,7 @@ type ansibleRunner interface { GetAnsibleRunPolicy() *ansible.RunPolicy WriteExtraVar(extraVar map[string]interface{}) error EnableCheckMode(checkMode bool) - Run() (*exec.Cmd, io.Reader, error) + Run(ctx context.Context) (io.Reader, error) } // SetupOptions constains settings specific to the ansible run controller. @@ -362,13 +361,10 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex return managed.ExternalObservation{}, err } c.runner.EnableCheckMode(true) - dc, stdoutBuf, err := c.runner.Run() + stdoutBuf, err := c.runner.Run(ctx) if err != nil { return managed.ExternalObservation{}, err } - if err = dc.Wait(); err != nil { - return managed.ExternalObservation{}, err - } res, err := results.ParseJSONResultsStream(stdoutBuf) if err != nil { return managed.ExternalObservation{}, err @@ -412,7 +408,7 @@ func (c *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext return managed.ExternalUpdate{ConnectionDetails: nil}, nil } -func (c *external) Delete(_ context.Context, mg resource.Managed) error { +func (c *external) Delete(ctx context.Context, mg resource.Managed) error { cr, ok := mg.(*v1alpha1.AnsibleRun) if !ok { return errors.New(errNotAnsibleRun) @@ -427,13 +423,10 @@ func (c *external) Delete(_ context.Context, mg resource.Managed) error { if err := c.runner.WriteExtraVar(nestedMap); err != nil { return err } - dc, _, err := c.runner.Run() + _, err := c.runner.Run(ctx) if err != nil { return err } - if err = dc.Wait(); err != nil { - return err - } return nil } @@ -497,12 +490,8 @@ func (c *external) handleLastApplied(ctx context.Context, lastParameters *v1alph } func (c *external) runAnsible(ctx context.Context, cr *v1alpha1.AnsibleRun) error { - dc, _, err := c.runner.Run() + _, err := c.runner.Run(ctx) if err != nil { - return err - } - - if err = dc.Wait(); err != nil { cond := xpv1.Unavailable() cond.Message = err.Error() cr.SetConditions(cond) diff --git a/internal/controller/ansibleRun/ansibleRun_test.go b/internal/controller/ansibleRun/ansibleRun_test.go index 702f8a6..f487513 100644 --- a/internal/controller/ansibleRun/ansibleRun_test.go +++ b/internal/controller/ansibleRun/ansibleRun_test.go @@ -96,14 +96,15 @@ func (ps MockPs) AddFile(path string, content []byte) error { } type MockRunner struct { - MockRun func() (*exec.Cmd, io.Reader, error) + MockRun func(ctx context.Context) (io.Reader, error) MockWriteExtraVar func(extraVar map[string]interface{}) error MockAnsibleRunPolicy func() *ansible.RunPolicy MockEnableCheckMode func(checkMode bool) + MockFailureReason func() (string, error) } -func (r MockRunner) Run() (*exec.Cmd, io.Reader, error) { - return r.MockRun() +func (r MockRunner) Run(ctx context.Context) (io.Reader, error) { + return r.MockRun(ctx) } func (r MockRunner) WriteExtraVar(extraVar map[string]interface{}) error { @@ -118,6 +119,10 @@ func (r MockRunner) EnableCheckMode(checkMode bool) { r.MockEnableCheckMode(checkMode) } +func (r MockRunner) FailureReason() (string, error) { + return r.MockFailureReason() +} + func TestConnect(t *testing.T) { errBoom := errors.New("boom") pbCreds := "credentials" @@ -624,8 +629,8 @@ func TestObserve(t *testing.T) { MockWriteExtraVar: func(extraVar map[string]interface{}) error { return nil }, - MockRun: func() (*exec.Cmd, io.Reader, error) { - return nil, nil, fmt.Errorf("run should not have been called") + MockRun: func(ctx context.Context) (io.Reader, error) { + return nil, fmt.Errorf("run should not have been called") }, }, }, @@ -653,9 +658,10 @@ func TestObserve(t *testing.T) { MockWriteExtraVar: func(extraVar map[string]interface{}) error { return nil }, - MockRun: func() (*exec.Cmd, io.Reader, error) { + MockRun: func(ctx context.Context) (io.Reader, error) { cmd := exec.Command("ls") - return cmd, nil, cmd.Start() + cmd.Start() + return nil, cmd.Wait() }, }, }, @@ -678,8 +684,8 @@ func TestObserve(t *testing.T) { MockWriteExtraVar: func(extraVar map[string]interface{}) error { return nil }, - MockRun: func() (*exec.Cmd, io.Reader, error) { - return nil, nil, errBoom + MockRun: func(context.Context) (io.Reader, error) { + return nil, errBoom }, MockEnableCheckMode: func(checkMode bool) { @@ -711,6 +717,8 @@ func TestObserve(t *testing.T) { func TestCreateOrUpdate(t *testing.T) { errBoom := errors.New("boom") + unavaliableCond := xpv1.Unavailable() + unavaliableCond.Message = errBoom.Error() type fields struct { kube client.Client @@ -756,19 +764,21 @@ func TestCreateOrUpdate(t *testing.T) { } }, MockEnableCheckMode: func(checkMode bool) {}, - MockRun: func() (*exec.Cmd, io.Reader, error) { - return nil, nil, errBoom + MockRun: func(context.Context) (io.Reader, error) { + return nil, errBoom }, }, }, want: want{ - err: fmt.Errorf("running ansible: %w", errBoom), + err: fmt.Errorf("running ansible: %w", errBoom), + conditions: []xpv1.Condition{unavaliableCond}, }, }, "SuccessObserveAndDelete": { reason: "We should not return an error when we successfully delete the AnsibleRun resource", args: args{ - mg: &v1alpha1.AnsibleRun{}, + ctx: context.Background(), + mg: &v1alpha1.AnsibleRun{}, }, fields: fields{ kube: &test.MockClient{ @@ -781,11 +791,10 @@ func TestCreateOrUpdate(t *testing.T) { } }, MockEnableCheckMode: func(checkMode bool) {}, - MockRun: func() (*exec.Cmd, io.Reader, error) { - ctx := context.Background() + MockRun: func(ctx context.Context) (io.Reader, error) { cmd := exec.CommandContext(ctx, "ls") cmd.Start() - return cmd, nil, nil + return nil, cmd.Wait() }, }, }, @@ -796,7 +805,8 @@ func TestCreateOrUpdate(t *testing.T) { "RunErrorWithCheckWhenObservePolicy": { reason: "We should return any error we encounter when running the runner", args: args{ - mg: &v1alpha1.AnsibleRun{}, + ctx: context.Background(), + mg: &v1alpha1.AnsibleRun{}, }, fields: fields{ runner: &MockRunner{ @@ -806,19 +816,21 @@ func TestCreateOrUpdate(t *testing.T) { } }, MockEnableCheckMode: func(checkMode bool) {}, - MockRun: func() (*exec.Cmd, io.Reader, error) { - return nil, nil, errBoom + MockRun: func(context.Context) (io.Reader, error) { + return nil, errBoom }, }, }, want: want{ - err: fmt.Errorf("running ansible: %w", errBoom), + err: fmt.Errorf("running ansible: %w", errBoom), + conditions: []xpv1.Condition{unavaliableCond}, }, }, "SuccessCheckWhenObserve": { reason: "We should not return an error when we successfully delete the AnsibleRun resource", args: args{ - mg: &v1alpha1.AnsibleRun{}, + ctx: context.Background(), + mg: &v1alpha1.AnsibleRun{}, }, fields: fields{ kube: &test.MockClient{ @@ -831,11 +843,10 @@ func TestCreateOrUpdate(t *testing.T) { } }, MockEnableCheckMode: func(checkMode bool) {}, - MockRun: func() (*exec.Cmd, io.Reader, error) { - ctx := context.Background() + MockRun: func(ctx context.Context) (io.Reader, error) { cmd := exec.CommandContext(ctx, "ls") cmd.Start() - return cmd, nil, nil + return nil, cmd.Wait() }, }, }, @@ -919,7 +930,8 @@ func TestDelete(t *testing.T) { "RunErrorWithObserveAndDeletePolicy": { reason: "We should return any error we encounter when running the runner", args: args{ - mg: &v1alpha1.AnsibleRun{}, + ctx: context.Background(), + mg: &v1alpha1.AnsibleRun{}, }, fields: fields{ runner: &MockRunner{ @@ -931,8 +943,8 @@ func TestDelete(t *testing.T) { Name: "ObserveAndDelete", } }, - MockRun: func() (*exec.Cmd, io.Reader, error) { - return nil, nil, errBoom + MockRun: func(context.Context) (io.Reader, error) { + return nil, errBoom }, }, }, @@ -941,7 +953,8 @@ func TestDelete(t *testing.T) { "SuccessObserveAndDelete": { reason: "We should not return an error when we successfully delete the AnsibleRun resource", args: args{ - mg: &v1alpha1.AnsibleRun{}, + ctx: context.Background(), + mg: &v1alpha1.AnsibleRun{}, }, fields: fields{ runner: &MockRunner{ @@ -953,11 +966,10 @@ func TestDelete(t *testing.T) { Name: "ObserveAndDelete", } }, - MockRun: func() (*exec.Cmd, io.Reader, error) { - ctx := context.Background() + MockRun: func(ctx context.Context) (io.Reader, error) { cmd := exec.CommandContext(ctx, "ls") cmd.Start() - return cmd, nil, nil + return nil, cmd.Wait() }, }, }, @@ -966,7 +978,8 @@ func TestDelete(t *testing.T) { "RunErrorWithCheckWhenObservePolicy": { reason: "We should return any error we encounter when running the runner", args: args{ - mg: &v1alpha1.AnsibleRun{}, + ctx: context.Background(), + mg: &v1alpha1.AnsibleRun{}, }, fields: fields{ runner: &MockRunner{ @@ -978,8 +991,8 @@ func TestDelete(t *testing.T) { Name: "CheckWhenObserve", } }, - MockRun: func() (*exec.Cmd, io.Reader, error) { - return nil, nil, errBoom + MockRun: func(context.Context) (io.Reader, error) { + return nil, errBoom }, }, }, @@ -988,7 +1001,8 @@ func TestDelete(t *testing.T) { "SuccessCheckWhenObserve": { reason: "We should not return an error when we successfully delete the AnsibleRun resource", args: args{ - mg: &v1alpha1.AnsibleRun{}, + ctx: context.Background(), + mg: &v1alpha1.AnsibleRun{}, }, fields: fields{ runner: &MockRunner{ @@ -1000,11 +1014,10 @@ func TestDelete(t *testing.T) { Name: "CheckWhenObserve", } }, - MockRun: func() (*exec.Cmd, io.Reader, error) { - ctx := context.Background() + MockRun: func(ctx context.Context) (io.Reader, error) { cmd := exec.CommandContext(ctx, "ls") cmd.Start() - return cmd, nil, nil + return nil, cmd.Wait() }, }, }, From 111b324de6004c736f8e0142cc9b2ddc174afe24 Mon Sep 17 00:00:00 2001 From: Dasha Komsa Date: Thu, 15 Feb 2024 21:46:45 +0000 Subject: [PATCH 2/5] unit test for extractFailureReason Signed-off-by: Dasha Komsa --- internal/ansible/ansible_test.go | 77 ++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/internal/ansible/ansible_test.go b/internal/ansible/ansible_test.go index 5634029..dda4fd4 100644 --- a/internal/ansible/ansible_test.go +++ b/internal/ansible/ansible_test.go @@ -18,6 +18,7 @@ package ansible import ( "context" + "fmt" "io" "os" "os/exec" @@ -232,3 +233,79 @@ func TestRun(t *testing.T) { }) } } + +func TestExtractFailureReason(t *testing.T) { + playbookStartEvt := ` + { + "uuid": "63a52ed5-a403-4512-a430-c95f62fa3424", + "event": "playbook_on_start", + "event_data": { + "playbook": "playbook.yml" + } + } + ` + + runnerFailedEvt := ` + { + "uuid": "7097758b-1109-4fd9-af59-f545633794dd", + "event": "runner_on_failed", + "event_data": { + "play": "test", + "task": "file", + "host": "testhost", + "res": {"msg": "fake error"} + } + } + ` + + runnerUnreachableEvt := ` + { + "uuid": "ded6289b-e557-48c1-88e1-88eb630aec21", + "event": "runner_on_unreachable", + "event_data": { + "play": "test", + "task": "Gathering Facts", + "host": "testhost", + "res": {"msg": "Failed to connect to the host via ssh"} + } + } + ` + + cases := map[string]struct { + events []string + expectedReason string + }{ + "NoEvents": {}, + "NoFailedEvents": { + events: []string{playbookStartEvt}, + }, + "FailedEvent": { + events: []string{playbookStartEvt, runnerFailedEvt}, + expectedReason: `Failed on play "test", task "file", host "testhost": fake error`, + }, + "UnreachableEvent": { + events: []string{playbookStartEvt, runnerUnreachableEvt}, + expectedReason: `Unreachable on play "test", task "Gathering Facts", host "testhost": Failed to connect to the host via ssh`, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + dir := t.TempDir() + for i, evt := range tc.events { + if err := os.WriteFile(filepath.Join(dir, fmt.Sprintf("%d.json", i)), []byte(evt), 0600); err != nil { + t.Fatalf("Writing test event to file: %v", err) + } + } + + reason, err := extractFailureReason(dir) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if reason != tc.expectedReason { + t.Errorf("Unexpected reason %v, expected %v", reason, tc.expectedReason) + } + }) + } +} From e2f88c9c162ec0210f68aff8352ab4313a12f5c6 Mon Sep 17 00:00:00 2001 From: Dasha Komsa Date: Thu, 15 Feb 2024 22:12:44 +0000 Subject: [PATCH 3/5] linting fixes Signed-off-by: Dasha Komsa --- internal/ansible/ansible.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/ansible/ansible.go b/internal/ansible/ansible.go index 38382c7..653190f 100644 --- a/internal/ansible/ansible.go +++ b/internal/ansible/ansible.go @@ -447,9 +447,9 @@ func parseEvents(dir string) ([]jobEvent, error) { return nil, fmt.Errorf("reading job events directory: %w", err) } - var evts []jobEvent - for _, file := range files { - evtBytes, err := os.ReadFile(filepath.Join(dir, file.Name())) + evts := make([]jobEvent, len(files)) + for i, file := range files { + evtBytes, err := os.ReadFile(filepath.Clean(filepath.Join(dir, file.Name()))) if err != nil { return nil, fmt.Errorf("reading job event file %q: %w", file.Name(), err) } @@ -458,7 +458,7 @@ func parseEvents(dir string) ([]jobEvent, error) { if err := json.Unmarshal(evtBytes, &evt); err != nil { return nil, fmt.Errorf("unmarshaling job event from file %q: %w", file.Name(), err) } - evts = append(evts, evt) + evts[i] = evt } return evts, nil From 46540ecc565448a2c6779f8e03965aa06cfd33cd Mon Sep 17 00:00:00 2001 From: Dasha Komsa Date: Sat, 9 Mar 2024 00:21:12 +0000 Subject: [PATCH 4/5] feedback fixes Signed-off-by: Dasha Komsa --- internal/ansible/ansible.go | 31 ++++++++++++++++++------------- internal/ansible/ansible_test.go | 12 +++++++++--- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/internal/ansible/ansible.go b/internal/ansible/ansible.go index 653190f..87dd04a 100644 --- a/internal/ansible/ansible.go +++ b/internal/ansible/ansible.go @@ -56,6 +56,9 @@ const ( errMkdir = "cannot make directory" ) +// using a variable for uuid generator allows for stubbing in tests +var generateUUID = uuid.New + const ( // AnnotationKeyPolicyRun is the name of an annotation which instructs // the provider how to run the corresponding Ansible contents @@ -333,7 +336,6 @@ type Runner struct { behaviorVars map[string]string cmdFunc cmdFuncType // returns a Cmd that runs ansible-runner workDir string - AnsibleEnvDir string checkMode bool AnsibleRunPolicy *RunPolicy artifactsHistoryLimit int @@ -370,7 +372,7 @@ func (r *Runner) Run(ctx context.Context) (io.Reader, error) { dc := r.cmdFunc(r.behaviorVars, r.checkMode) dc.Args = append(dc.Args, "--rotate-artifacts", strconv.Itoa(r.artifactsHistoryLimit)) - id := uuid.New().String() + id := generateUUID().String() dc.Args = append(dc.Args, "--ident", id) if !r.checkMode { @@ -402,9 +404,10 @@ func (r *Runner) Run(ctx context.Context) (io.Reader, error) { if err := dc.Wait(); err != nil { jobEventsDir := filepath.Clean(filepath.Join(r.workDir, "artifacts", id, "job_events")) - failureReason, reasonErr := extractFailureReason(jobEventsDir) + failureReason, reasonErr := extractFailureReason(ctx, jobEventsDir) if reasonErr != nil { - log.FromContext(ctx).Error(err, "extracting ansible failure message") + log.FromContext(ctx).V(1).Info("extracting ansible failure message", "err", reasonErr) + return nil, err } return nil, fmt.Errorf("%w: %s", err, failureReason) @@ -413,8 +416,8 @@ func (r *Runner) Run(ctx context.Context) (io.Reader, error) { return &stdoutBuf, nil } -func extractFailureReason(eventsDir string) (string, error) { - evts, err := parseEvents(eventsDir) +func extractFailureReason(ctx context.Context, eventsDir string) (string, error) { + evts, err := parseEvents(ctx, eventsDir) if err != nil { return "", fmt.Errorf("parsing job events: %w", err) } @@ -441,24 +444,26 @@ func extractFailureReason(eventsDir string) (string, error) { return strings.Join(msgs, "; "), nil } -func parseEvents(dir string) ([]jobEvent, error) { +func parseEvents(ctx context.Context, dir string) ([]jobEvent, error) { files, err := os.ReadDir(dir) if err != nil { - return nil, fmt.Errorf("reading job events directory: %w", err) + return nil, fmt.Errorf("reading job events directory %q: %w", dir, err) } - evts := make([]jobEvent, len(files)) - for i, file := range files { + var evts []jobEvent + for _, file := range files { evtBytes, err := os.ReadFile(filepath.Clean(filepath.Join(dir, file.Name()))) if err != nil { - return nil, fmt.Errorf("reading job event file %q: %w", file.Name(), err) + log.FromContext(ctx).V(1).Info("reading job event file", "filename", file.Name(), "err", err) + continue } var evt jobEvent if err := json.Unmarshal(evtBytes, &evt); err != nil { - return nil, fmt.Errorf("unmarshaling job event from file %q: %w", file.Name(), err) + log.FromContext(ctx).V(1).Info("unmarshaling job event from file", "filename", file.Name(), "err", err) + continue } - evts[i] = evt + evts = append(evts, evt) } return evts, nil diff --git a/internal/ansible/ansible_test.go b/internal/ansible/ansible_test.go index dda4fd4..5b9ec91 100644 --- a/internal/ansible/ansible_test.go +++ b/internal/ansible/ansible_test.go @@ -29,6 +29,7 @@ import ( "github.com/crossplane-contrib/provider-ansible/apis/v1alpha1" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/google/uuid" "gotest.tools/v3/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -156,7 +157,7 @@ func TestInit(t *testing.T) { expectedRunner := &Runner{ Path: dir, cmdFunc: params.playbookCmdFunc(context.Background(), "playbook.yml", dir), - AnsibleEnvDir: filepath.Join(dir, "env"), + workDir: dir, AnsibleRunPolicy: &RunPolicy{"ObserveAndDelete"}, artifactsHistoryLimit: 3, } @@ -176,6 +177,9 @@ func TestInit(t *testing.T) { if runner.checkMode != expectedRunner.checkMode { t.Errorf("Unexpected Runner.checkMode %v expected %v", runner.checkMode, expectedRunner.checkMode) } + if runner.workDir != expectedRunner.workDir { + t.Errorf("Unexpected Runner.workDir %v expected %v", runner.workDir, expectedRunner.workDir) + } expectedCmd := expectedRunner.cmdFunc(nil, false) cmd := runner.cmdFunc(nil, false) @@ -194,12 +198,14 @@ func TestRun(t *testing.T) { // therefore checking its output also checks the args passed to it are correct return exec.CommandContext(context.Background(), "echo") }, - AnsibleEnvDir: filepath.Join(dir, "env"), AnsibleRunPolicy: &RunPolicy{"ObserveAndDelete"}, artifactsHistoryLimit: 3, } - expectedArgs := []string{"--rotate-artifacts", "3"} + expectedID := "217b3830-68fa-461b-90d1-1fb87c685010" + expectedArgs := []string{"--rotate-artifacts", "3", "--ident", expectedID} + + generateUUID = func() uuid.UUID { return uuid.MustParse(expectedID) } testCases := map[string]struct { checkMode bool From 56cc685b400a9a43acd310384ac031bca7fb277d Mon Sep 17 00:00:00 2001 From: Dasha Komsa Date: Sat, 9 Mar 2024 00:36:03 +0000 Subject: [PATCH 5/5] test and linting fixes after rebase Signed-off-by: Dasha Komsa --- internal/ansible/ansible.go | 2 +- internal/ansible/ansible_test.go | 2 +- internal/controller/ansibleRun/ansibleRun_test.go | 6 ++++++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/internal/ansible/ansible.go b/internal/ansible/ansible.go index 87dd04a..26aebec 100644 --- a/internal/ansible/ansible.go +++ b/internal/ansible/ansible.go @@ -450,7 +450,7 @@ func parseEvents(ctx context.Context, dir string) ([]jobEvent, error) { return nil, fmt.Errorf("reading job events directory %q: %w", dir, err) } - var evts []jobEvent + evts := make([]jobEvent, 0) for _, file := range files { evtBytes, err := os.ReadFile(filepath.Clean(filepath.Join(dir, file.Name()))) if err != nil { diff --git a/internal/ansible/ansible_test.go b/internal/ansible/ansible_test.go index 5b9ec91..61d7f17 100644 --- a/internal/ansible/ansible_test.go +++ b/internal/ansible/ansible_test.go @@ -304,7 +304,7 @@ func TestExtractFailureReason(t *testing.T) { } } - reason, err := extractFailureReason(dir) + reason, err := extractFailureReason(context.Background(), dir) if err != nil { t.Fatalf("Unexpected error: %v", err) } diff --git a/internal/controller/ansibleRun/ansibleRun_test.go b/internal/controller/ansibleRun/ansibleRun_test.go index f487513..7d0858b 100644 --- a/internal/controller/ansibleRun/ansibleRun_test.go +++ b/internal/controller/ansibleRun/ansibleRun_test.go @@ -757,6 +757,9 @@ func TestCreateOrUpdate(t *testing.T) { mg: &v1alpha1.AnsibleRun{}, }, fields: fields{ + kube: &test.MockClient{ + MockStatusUpdate: test.NewMockSubResourceUpdateFn(nil), + }, runner: &MockRunner{ MockAnsibleRunPolicy: func() *ansible.RunPolicy { return &ansible.RunPolicy{ @@ -809,6 +812,9 @@ func TestCreateOrUpdate(t *testing.T) { mg: &v1alpha1.AnsibleRun{}, }, fields: fields{ + kube: &test.MockClient{ + MockStatusUpdate: test.NewMockSubResourceUpdateFn(nil), + }, runner: &MockRunner{ MockAnsibleRunPolicy: func() *ansible.RunPolicy { return &ansible.RunPolicy{