refactor(ansible): store relevant data on watch (#2067)

* refactor(ansible): store relevant data on watch

Bring all of the logic for how values are reconciled, and where
important values are stored, into the watches package. This allows us to
handle command line flags, environment variables, defaults, and values
from the watches file in one place with sane defaults.

- Add new functions to watches package `New()` and `Validate()`
- Move `getMaxWorkers()` to watches package (from runner)
- Handle command line arguments in watches package

* Don't pass flags to watches.Load()

Instead of giving all of the flags to `watches.Load()`, we should only
pass down the values that are needed.

Also update `New()` so that it's possible to create a valid watch.

* Log invalid value for maxWorkers and use default

Author: djzager

pkg/ansible/run.go

@@ -19,30 +19,28 @@ import (
 	"fmt"
 	"os"
 	"runtime"
-	"strconv"
-	"strings"
 
 	"github.com/operator-framework/operator-sdk/pkg/ansible/controller"
-	aoflags "github.com/operator-framework/operator-sdk/pkg/ansible/flags"
-	proxy "github.com/operator-framework/operator-sdk/pkg/ansible/proxy"
 	"github.com/operator-framework/operator-sdk/pkg/ansible/proxy/controllermap"
 	"github.com/operator-framework/operator-sdk/pkg/ansible/runner"
 	"github.com/operator-framework/operator-sdk/pkg/ansible/watches"
 	"github.com/operator-framework/operator-sdk/pkg/k8sutil"
-	kubemetrics "github.com/operator-framework/operator-sdk/pkg/kube-metrics"
 	"github.com/operator-framework/operator-sdk/pkg/leader"
 	"github.com/operator-framework/operator-sdk/pkg/metrics"
 	"github.com/operator-framework/operator-sdk/pkg/restmapper"
-	sdkVersion "github.com/operator-framework/operator-sdk/version"
-	v1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/intstr"
-
 	"sigs.k8s.io/controller-runtime/pkg/client/config"
-	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/manager/signals"
+
+	aoflags "github.com/operator-framework/operator-sdk/pkg/ansible/flags"
+	proxy "github.com/operator-framework/operator-sdk/pkg/ansible/proxy"
+	kubemetrics "github.com/operator-framework/operator-sdk/pkg/kube-metrics"
+	sdkVersion "github.com/operator-framework/operator-sdk/version"
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	logf "sigs.k8s.io/controller-runtime/pkg/log"
 )
 
 var (
@@ -91,7 +89,7 @@ func Run(flags *aoflags.AnsibleOperatorFlags) error {
 
 	var gvks []schema.GroupVersionKind
 	cMap := controllermap.NewControllerMap()
-	watches, err := watches.Load(flags.WatchesFile)
+	watches, err := watches.Load(flags.WatchesFile, flags.MaxWorkers)
 	if err != nil {
 		log.Error(err, "Failed to load watches.")
 		return err
@@ -107,7 +105,7 @@ func Run(flags *aoflags.AnsibleOperatorFlags) error {
 			GVK:             w.GroupVersionKind,
 			Runner:          runner,
 			ManageStatus:    w.ManageStatus,
-			MaxWorkers:      getMaxWorkers(w.GroupVersionKind, flags.MaxWorkers),
+			MaxWorkers:      w.MaxWorkers,
 			ReconcilePeriod: w.ReconcilePeriod,
 		})
 		if ctr == nil {
@@ -188,28 +186,3 @@ func Run(flags *aoflags.AnsibleOperatorFlags) error {
 	log.Info("Exiting.")
 	return nil
 }
-
-// if the WORKER_* environment variable is set, use that value.
-// Otherwise, use the value from the CLI. This is definitely
-// counter-intuitive but it allows the operator admin adjust the
-// number of workers based on their cluster resources. While the
-// author may use the CLI option to specify a suggested
-// configuration for the operator.
-func getMaxWorkers(gvk schema.GroupVersionKind, defValue int) int {
-	envVar := strings.ToUpper(strings.Replace(
-		fmt.Sprintf("WORKER_%s_%s", gvk.Kind, gvk.Group),
-		".",
-		"_",
-		-1,
-	))
-	switch maxWorkers, err := strconv.Atoi(os.Getenv(envVar)); {
-	case maxWorkers <= 1:
-		return defValue
-	case err != nil:
-		// we don't care why we couldn't parse it just use default
-		log.Info("Failed to parse %v from environment. Using default %v", envVar, defValue)
-		return defValue
-	default:
-		return maxWorkers
-	}
-}

pkg/ansible/run_test.go

@@ -58,6 +58,12 @@ func New(watch watches.Watch) (Runner, error) {
 	var path string
 	var cmdFunc func(ident, inputDirPath string, maxArtifacts int) *exec.Cmd
 
+	err := watch.Validate()
+	if err != nil {
+		log.Error(err, "Failed to validate watch")
+		return nil, err
+	}
+
 	switch {
 	case watch.Playbook != "":
 		path = watch.Playbook
@@ -119,7 +125,6 @@ type runner struct {
 }
 
 func (r *runner) Run(ident string, u *unstructured.Unstructured, kubeconfig string) (RunResult, error) {
-
 	timer := metrics.ReconcileTimer(r.GVK.String())
 	defer timer.ObserveDuration()