Merge pull request #314 from d-honeybadger/clean-artifacts-history

limit the number of artifacts directories retained on disk

cmd/provider/main.go

@@ -22,6 +22,7 @@ import (
 
 	"github.com/crossplane-contrib/provider-ansible/apis"
 	ansible "github.com/crossplane-contrib/provider-ansible/internal/controller"
+	ansiblerun "github.com/crossplane-contrib/provider-ansible/internal/controller/ansibleRun"
 	"github.com/crossplane/crossplane-runtime/pkg/controller"
 	"github.com/crossplane/crossplane-runtime/pkg/feature"
 	"github.com/crossplane/crossplane-runtime/pkg/logging"
@@ -42,6 +43,7 @@ func main() {
 		timeout                = app.Flag("timeout", "Controls how long Ansible processes may run before they are killed.").Default("20m").Duration()
 		leaderElection         = app.Flag("leader-election", "Use leader election for the controller manager.").Short('l').Default("false").OverrideDefaultFromEnvar("LEADER_ELECTION").Bool()
 		maxReconcileRate       = app.Flag("max-reconcile-rate", "The maximum number of concurrent reconciliation operations.").Default("1").Int()
+		artifactsHistoryLimit  = app.Flag("artifacts-history-limit", "Each attempt to run the playbook/role generates a set of artifacts on disk. This settings limits how many of these to keep.").Default("10").Int()
 	)
 	kingpin.MustParse(app.Parse(os.Args[1:]))
 
@@ -76,6 +78,12 @@ func main() {
 		Features:                &feature.Flags{},
 	}
 
-	kingpin.FatalIfError(ansible.Setup(mgr, o, *ansibleCollectionsPath, *ansibleRolesPath, *timeout), "Cannot setup Ansible controllers")
+	ansibleOpts := ansiblerun.SetupOptions{
+		AnsibleCollectionsPath: *ansibleCollectionsPath,
+		AnsibleRolesPath:       *ansibleRolesPath,
+		Timeout:                *timeout,
+		ArtifactsHistoryLimit:  *artifactsHistoryLimit,
+	}
+	kingpin.FatalIfError(ansible.Setup(mgr, o, ansibleOpts), "Cannot setup Ansible controllers")
 	kingpin.FatalIfError(mgr.Start(ctrl.SetupSignalHandler()), "Cannot start controller manager")
 }

internal/ansible/ansible.go

@@ -27,6 +27,7 @@ import (
 	"os/exec"
 	"os/user"
 	"path/filepath"
+	"strconv"
 	"time"
 
 	"github.com/apenella/go-ansible/pkg/stdoutcallback/results"
@@ -69,6 +70,8 @@ type Parameters struct {
 	CollectionsPath string
 	// The source of this filed is either controller flag `--ansible-roles-path` or the env vars : `ANSIBLE_ROLES_PATH` , DEFAULT_ROLES_PATH`
 	RolesPath string
+	// the limit on the number of artifact directories to keep for each run
+	ArtifactsHistoryLimit int
 }
 
 // RunPolicy represents the run policies of Ansible.
@@ -144,6 +147,14 @@ func withAnsibleRunPolicy(p *RunPolicy) runnerOption {
 	}
 }
 
+// withArtifactsHistoryLimit sets the limit on the number of artifacts
+// directories to keep; each invocation of ansible-runner produces an artifacts directory.
+func withArtifactsHistoryLimit(limit int) runnerOption {
+	return func(r *Runner) {
+		r.artifactsHistoryLimit = limit
+	}
+}
+
 type cmdFuncType func(behaviorVars map[string]string, checkMode bool) *exec.Cmd
 
 // playbookCmdFunc mimics https://github.com/operator-framework/operator-sdk/blob/707240f006ecfc0bc86e5c21f6874d302992d598/internal/ansible/runner/runner.go#L75-L90
@@ -307,17 +318,19 @@ func (p Parameters) Init(ctx context.Context, cr *v1alpha1.AnsibleRun, behaviorV
 		withAnsibleRunPolicy(rPolicy),
 		// TODO should be moved to connect() func
 		withAnsibleEnvDir(ansibleEnvDir),
+		withArtifactsHistoryLimit(p.ArtifactsHistoryLimit),
 	), nil
 }
 
 // Runner struct holds the configuration to run the cmdFunc
 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
-	AnsibleEnvDir    string
-	checkMode        bool
-	AnsibleRunPolicy *RunPolicy
+	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
+	AnsibleEnvDir         string
+	checkMode             bool
+	AnsibleRunPolicy      *RunPolicy
+	artifactsHistoryLimit int
 }
 
 // new returns a runner that will be used as ansible-runner client
@@ -345,6 +358,7 @@ 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))
 	if !r.checkMode {
 		// for disabled checkMode dc.Stdout and dc.Stderr are respectfully
 		// written to os.Stdout and os.Stdout for debugging purpose

internal/ansible/ansible_test.go

@@ -18,11 +18,16 @@ package ansible
 
 import (
 	"context"
+	"io"
 	"os"
+	"os/exec"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/crossplane-contrib/provider-ansible/apis/v1alpha1"
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"gotest.tools/v3/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -123,3 +128,115 @@ func TestAnsibleRunPolicyInit(t *testing.T) {
 		})
 	}
 }
+
+func TestInit(t *testing.T) {
+	dir := t.TempDir()
+
+	fakePlaybook := "fake playbook"
+	run := &v1alpha1.AnsibleRun{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: map[string]string{
+				AnnotationKeyPolicyRun: "ObserveAndDelete",
+			},
+		},
+		Spec: v1alpha1.AnsibleRunSpec{
+			ForProvider: v1alpha1.AnsibleRunParameters{
+				PlaybookInline: &fakePlaybook,
+			},
+		},
+	}
+
+	params := Parameters{
+		RunnerBinary:          "fake-runner",
+		WorkingDirPath:        dir,
+		ArtifactsHistoryLimit: 3,
+	}
+
+	expectedRunner := &Runner{
+		Path:                  dir,
+		cmdFunc:               params.playbookCmdFunc(context.Background(), "playbook.yml", dir),
+		AnsibleEnvDir:         filepath.Join(dir, "env"),
+		AnsibleRunPolicy:      &RunPolicy{"ObserveAndDelete"},
+		artifactsHistoryLimit: 3,
+	}
+
+	runner, err := params.Init(context.Background(), run, nil)
+	if err != nil {
+		t.Fatalf("Unexpected Init() error: %v", err)
+	}
+
+	if diff := cmp.Diff(expectedRunner, runner, cmpopts.IgnoreUnexported(Runner{})); diff != "" {
+		t.Errorf("Unexpected Runner -want, +got:\n%s\n", diff)
+	}
+	// check fields that couldn't be compared with cmp.Diff
+	if runner.artifactsHistoryLimit != expectedRunner.artifactsHistoryLimit {
+		t.Errorf("Unexpected Runner.artifactsHistoryLimit %v expected %v", runner.artifactsHistoryLimit, expectedRunner.artifactsHistoryLimit)
+	}
+	if runner.checkMode != expectedRunner.checkMode {
+		t.Errorf("Unexpected Runner.checkMode %v expected %v", runner.checkMode, expectedRunner.checkMode)
+	}
+
+	expectedCmd := expectedRunner.cmdFunc(nil, false)
+	cmd := runner.cmdFunc(nil, false)
+	if cmd.String() != expectedCmd.String() {
+		t.Errorf("Unexpected Runner.cmdFunc output %q expected %q", expectedCmd.String(), cmd.String())
+	}
+}
+
+func TestRun(t *testing.T) {
+	dir := t.TempDir()
+
+	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
+			return exec.CommandContext(context.Background(), "echo")
+		},
+		AnsibleEnvDir:         filepath.Join(dir, "env"),
+		AnsibleRunPolicy:      &RunPolicy{"ObserveAndDelete"},
+		artifactsHistoryLimit: 3,
+	}
+
+	expectedArgs := []string{"--rotate-artifacts", "3"}
+	expectedCmd := exec.Command(runner.cmdFunc(nil, false).Path, expectedArgs...)
+
+	testCases := map[string]struct {
+		checkMode      bool
+		expectedOutput string
+	}{
+		"WithoutCheckMode": {
+			expectedOutput: "",
+		},
+		"WithCheckMode": {
+			checkMode:      true,
+			expectedOutput: strings.Join(expectedArgs, " ") + "\n",
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			runner.checkMode = tc.checkMode
+			cmd, outBuf, err := runner.Run()
+			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)
+			}
+
+			if string(out) != tc.expectedOutput {
+				t.Errorf("Unexpected output in the command buffer %q, want %q", string(out), tc.expectedOutput)
+			}
+		})
+	}
+}

internal/controller/ansible.go

@@ -17,8 +17,6 @@ limitations under the License.
 package controller
 
 import (
-	"time"
-
 	ansiblerun "github.com/crossplane-contrib/provider-ansible/internal/controller/ansibleRun"
 	"github.com/crossplane/crossplane-runtime/pkg/controller"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -28,14 +26,14 @@ import (
 
 // Setup creates all Template controllers with the supplied logger and adds them to
 // the supplied manager.
-func Setup(mgr ctrl.Manager, o controller.Options, ansibleCollectionsPath, ansibleRolesPath string, timeout time.Duration) error {
-	for _, setup := range []func(ctrl.Manager, controller.Options, string, string, time.Duration) error{
-		config.Setup,
-		ansiblerun.Setup,
-	} {
-		if err := setup(mgr, o, ansibleCollectionsPath, ansibleRolesPath, timeout); err != nil {
-			return err
-		}
+func Setup(mgr ctrl.Manager, o controller.Options, s ansiblerun.SetupOptions) error {
+	if err := config.Setup(mgr, o); err != nil {
+		return err
+	}
+
+	if err := ansiblerun.Setup(mgr, o, s); err != nil {
+		return err
 	}
+
 	return nil
 }

internal/controller/ansibleRun/ansibleRun.go

@@ -90,8 +90,16 @@ type ansibleRunner interface {
 	Run() (*exec.Cmd, io.Reader, error)
 }
 
+// SetupOptions constains settings specific to the ansible run controller.
+type SetupOptions struct {
+	AnsibleCollectionsPath string
+	AnsibleRolesPath       string
+	Timeout                time.Duration
+	ArtifactsHistoryLimit  int
+}
+
 // Setup adds a controller that reconciles AnsibleRun managed resources.
-func Setup(mgr ctrl.Manager, o controller.Options, ansibleCollectionsPath, ansibleRolesPath string, timeout time.Duration) error {
+func Setup(mgr ctrl.Manager, o controller.Options, s SetupOptions) error {
 	name := managed.ControllerName(v1alpha1.AnsibleRunGroupKind)
 
 	fs := afero.Afero{Fs: afero.NewOsFs()}
@@ -111,11 +119,12 @@ func Setup(mgr ctrl.Manager, o controller.Options, ansibleCollectionsPath, ansib
 		fs:    fs,
 		ansible: func(dir string) params {
 			return ansible.Parameters{
-				WorkingDirPath:  dir,
-				GalaxyBinary:    galaxyBinary,
-				RunnerBinary:    runnerBinary,
-				CollectionsPath: ansibleCollectionsPath,
-				RolesPath:       ansibleRolesPath,
+				WorkingDirPath:        dir,
+				GalaxyBinary:          galaxyBinary,
+				RunnerBinary:          runnerBinary,
+				CollectionsPath:       s.AnsibleCollectionsPath,
+				RolesPath:             s.AnsibleRolesPath,
+				ArtifactsHistoryLimit: s.ArtifactsHistoryLimit,
 			}
 		},
 	}
@@ -124,7 +133,7 @@ func Setup(mgr ctrl.Manager, o controller.Options, ansibleCollectionsPath, ansib
 		resource.ManagedKind(v1alpha1.AnsibleRunGroupVersionKind),
 		managed.WithExternalConnecter(c),
 		managed.WithLogger(o.Logger.WithValues("controller", name)),
-		managed.WithTimeout(timeout),
+		managed.WithTimeout(s.Timeout),
 		managed.WithRecorder(event.NewAPIRecorder(mgr.GetEventRecorderFor(name))))
 
 	return ctrl.NewControllerManagedBy(mgr).

internal/controller/config/config.go

@@ -17,8 +17,6 @@ limitations under the License.
 package config
 
 import (
-	"time"
-
 	"github.com/crossplane/crossplane-runtime/pkg/controller"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/source"
@@ -33,7 +31,7 @@ import (
 
 // Setup adds a controller that reconciles ProviderConfigs by accounting for
 // their current usage.
-func Setup(mgr ctrl.Manager, o controller.Options, _, _ string, _ time.Duration) error {
+func Setup(mgr ctrl.Manager, o controller.Options) error {
 	name := providerconfig.ControllerName(v1alpha1.ProviderConfigGroupKind)
 
 	of := resource.ProviderConfigKinds{