Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ansible): make ansible verbosity configurable #1852

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
- Ansible based operators now gather and serve metrics about each custom resource on port 8686 of the metrics service. ([#1723](https://github.com/operator-framework/operator-sdk/pull/1723))
- Added the Go version, OS, and architecture to the output of `operator-sdk version` ([#1863](https://github.com/operator-framework/operator-sdk/pull/1863))
- Added support for `ppc64le-linux` for the `operator-sdk` binary and the Helm operator base image. ([#1533](https://github.com/operator-framework/operator-sdk/pull/1533))
- Ansible based operators now support verbosity configuration via the `ansible-verbosity` flag at the command line. ([#1852](https://github.com/operator-framework/operator-sdk/pull/1852))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Ansible based operators now support verbosity configuration via the `ansible-verbosity` flag at the command line. ([#1852](https://github.com/operator-framework/operator-sdk/pull/1852))
- Added support of verbosity configuration for Ansible-based Operators via the `ansible-verbosity` flag. ([#1852](https://github.com/operator-framework/operator-sdk/pull/1852))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is not this change cable of to cause break changes in the current projects?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed changes should not break current projects or require any changes in those projects. If you have a scenario or code path in mind I should certainly fix it.


### Changed

Expand Down
2 changes: 1 addition & 1 deletion ci/tests/e2e-ansible.sh
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ test_operator() {
fi

# verify that metrics reflect cr creation
if ! bash -c -- "kubectl run --attach --rm --restart=Never test-metrics --image=$metrics_test_image -- curl http://memcached-operator-metrics:8686/metrics | grep example-memcached";
if ! timeout 1m bash -c -- "until kubectl run -it --rm --restart=Never test-metrics --image=$metrics_test_image -- curl http://memcached-operator-metrics:8686/metrics | grep example-memcached; do sleep 1; done";
then
echo "Failed to verify custom resource metrics"
kubectl describe pods
Expand Down
8 changes: 4 additions & 4 deletions hack/tests/e2e-ansible.sh
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,15 @@ test_operator() {
if ! timeout 1m bash -c -- "until kubectl run --attach --rm --restart=Never test-metrics --image=$metrics_test_image -- curl -sfo /dev/null http://memcached-operator-metrics:8383/metrics; do sleep 1; done";
then
echo "Failed to verify that metrics endpoint exists"
kubectl logs deployment/memcached-operator
kubectl logs deployment/memcached-operator -c operator
exit 1
fi

# verify that the operator metrics endpoint exists
if ! timeout 1m bash -c -- "until kubectl run --attach --rm --restart=Never test-metrics --image=$metrics_test_image -- curl -sfo /dev/null http://memcached-operator-metrics:8686/metrics; do sleep 1; done";
then
echo "Failed to verify that metrics endpoint exists"
kubectl logs deployment/memcached-operator
kubectl logs deployment/memcached-operator -c operator
exit 1
fi

Expand All @@ -80,10 +80,10 @@ test_operator() {
fi

# verify that metrics reflect cr creation
if ! bash -c -- "kubectl run --attach --rm --restart=Never test-metrics --image=$metrics_test_image -- curl http://memcached-operator-metrics:8686/metrics | grep example-memcached";
if ! timeout 1m bash -c -- "until kubectl run -it --rm --restart=Never test-metrics --image=$metrics_test_image -- curl http://memcached-operator-metrics:8686/metrics | grep example-memcached; do sleep 1; done";
then
echo "Failed to verify custom resource metrics"
kubectl logs deployment/memcached-operator
kubectl logs deployment/memcached-operator -c operator
exit 1
fi
memcached_deployment=$(kubectl get deployment -l app=memcached -o jsonpath="{..metadata.name}")
Expand Down
12 changes: 10 additions & 2 deletions pkg/ansible/flags/flag.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,9 @@ import (
// AnsibleOperatorFlags - Options to be used by an ansible operator
type AnsibleOperatorFlags struct {
watch.WatchFlags
InjectOwnerRef bool
MaxWorkers int
InjectOwnerRef bool
MaxWorkers int
AnsibleVerbosity int
}

// AddTo - Add the ansible operator flags to the the flagset
Expand All @@ -47,5 +48,12 @@ func AddTo(flagSet *pflag.FlagSet, helpTextPrefix ...string) *AnsibleOperatorFla
"Maximum number of workers to use. Overridden by environment variable."),
" "),
)
flagSet.IntVar(&aof.AnsibleVerbosity,
"ansible-verbosity",
2,
strings.Join(append(helpTextPrefix,
"Ansible verbosity. Overridden by environment variable."),
" "),
)
return aof
}
47 changes: 10 additions & 37 deletions pkg/ansible/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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)
if err != nil {
log.Error(err, "Failed to load watches.")
return err
Expand All @@ -107,7 +105,7 @@ func Run(flags *aoflags.AnsibleOperatorFlags) error {
GVK: w.GroupVersionKind,
Runner: runner,
ManageStatus: w.ManageStatus,
MaxWorkers: getMaxWorkers(w.GroupVersionKind, flags.MaxWorkers),
Copy link
Contributor

@camilamacedo86 camilamacedo86 Oct 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, currently, we can define the MaxWorkers by CRD. Am I right? Why should we change it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two places to set MaxWorkers are on the command line or via an environment variable. The only changes made here are to put the resolution of those values in the watches package + storing those values on the watches struct to be reused throughout execution.

MaxWorkers: w.MaxWorkers,
ReconcilePeriod: w.ReconcilePeriod,
})
if ctr == nil {
Expand Down Expand Up @@ -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
}
}
73 changes: 0 additions & 73 deletions pkg/ansible/run_test.go

This file was deleted.

62 changes: 35 additions & 27 deletions pkg/ansible/runner/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,56 +52,65 @@ type Runner interface {
GetFinalizer() (string, bool)
}

func ansibleVerbosityString(verbosity int) string {
if verbosity > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would not it can be as v, or vv, or vvv, or vvvv? See https://docs.ansible.com/ansible/latest/cli/ansible-playbook.html#cmdoption-ansible-playbook-v

So, wdyt about your solution allow the user to inform it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. To me it made more sense to make this the integer value of the verbosity level. I don't believe this would be too surprising to Ansible users as that is how debug works. I also wanted to avoid any possible confusion with the operator-sdk's own verbosity.

return fmt.Sprintf("-%v", strings.Repeat("v", verbosity))
}
return ""
}

type cmdFuncType func(ident, inputDirPath string, maxArtifacts int) *exec.Cmd

func playbookCmdFunc(verbosity, path string) cmdFuncType {
return func(ident, inputDirPath string, maxArtifacts int) *exec.Cmd {
return exec.Command("ansible-runner", verbosity, "--rotate-artifacts", fmt.Sprintf("%v", maxArtifacts), "-p", path, "-i", ident, "run", inputDirPath)
}
}

func roleCmdFunc(verbosity, path string) cmdFuncType {
rolePath, roleName := filepath.Split(path)
return func(ident, inputDirPath string, maxArtifacts int) *exec.Cmd {
return exec.Command("ansible-runner", verbosity, "--rotate-artifacts", fmt.Sprintf("%v", maxArtifacts), "--role", roleName, "--roles-path", rolePath, "--hosts", "localhost", "-i", ident, "run", inputDirPath)
}
}

// New - creates a Runner from a Watch struct
func New(watch watches.Watch) (Runner, error) {
// handle role or playbook
var path string
var cmdFunc func(ident, inputDirPath string, maxArtifacts int) *exec.Cmd
var cmdFunc, finalizerCmdFunc cmdFuncType

err := watch.Validate()
if err != nil {
log.Error(err, "Failed to validate watch")
return nil, err
}
verbosityString := ansibleVerbosityString(watch.AnsibleVerbosity)

switch {
case watch.Playbook != "":
path = watch.Playbook
cmdFunc = func(ident, inputDirPath string, maxArtifacts int) *exec.Cmd {
return exec.Command("ansible-runner", "-vv", "--rotate-artifacts", fmt.Sprintf("%v", maxArtifacts), "-p", path, "-i", ident, "run", inputDirPath)
}
cmdFunc = playbookCmdFunc(verbosityString, path)
case watch.Role != "":
path = watch.Role
cmdFunc = func(ident, inputDirPath string, maxArtifacts int) *exec.Cmd {
rolePath, roleName := filepath.Split(path)
return exec.Command("ansible-runner", "-vv", "--rotate-artifacts", fmt.Sprintf("%v", maxArtifacts), "--role", roleName, "--roles-path", rolePath, "--hosts", "localhost", "-i", ident, "run", inputDirPath)
}
default:
return nil, fmt.Errorf("must specify Role or Path")
cmdFunc = roleCmdFunc(verbosityString, path)
}

// handle finalizer
var finalizer *watches.Finalizer
var finalizerCmdFunc func(ident, inputDirPath string, maxArtifacts int) *exec.Cmd
switch {
case watch.Finalizer == nil:
finalizer = nil
finalizerCmdFunc = nil
case watch.Finalizer.Playbook != "":
finalizer = watch.Finalizer
finalizerCmdFunc = func(ident, inputDirPath string, maxArtifacts int) *exec.Cmd {
return exec.Command("ansible-runner", "-vv", "--rotate-artifacts", fmt.Sprintf("%v", maxArtifacts), "-p", finalizer.Playbook, "-i", ident, "run", inputDirPath)
}
finalizerCmdFunc = playbookCmdFunc(verbosityString, watch.Finalizer.Playbook)
case watch.Finalizer.Role != "":
finalizer = watch.Finalizer
finalizerCmdFunc = func(ident, inputDirPath string, maxArtifacts int) *exec.Cmd {
path := strings.TrimRight(finalizer.Role, "/")
rolePath, roleName := filepath.Split(path)
return exec.Command("ansible-runner", "-vv", "--rotate-artifacts", fmt.Sprintf("%v", maxArtifacts), "--role", roleName, "--roles-path", rolePath, "--hosts", "localhost", "-i", ident, "run", inputDirPath)
}
finalizerCmdFunc = roleCmdFunc(verbosityString, watch.Finalizer.Role)
case len(watch.Finalizer.Vars) != 0:
finalizer = watch.Finalizer
finalizerCmdFunc = cmdFunc
}

return &runner{
Path: path,
cmdFunc: cmdFunc,
Finalizer: finalizer,
Finalizer: watch.Finalizer,
finalizerCmdFunc: finalizerCmdFunc,
GVK: watch.GroupVersionKind,
maxRunnerArtifacts: watch.MaxRunnerArtifacts,
Expand All @@ -119,7 +128,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()

Expand Down
Loading