feat(ansible): make ansible verbosity configurable #1852

djzager · 2019-08-21T19:26:41Z

The primary objective of this change is to make the verbosity of
individual ansible-runner command invocations configurable (not just
"-vv"). In order to accomplish this:

A new flag was added to the AnsibleOperatorFlags (ansible-verbosity)
with a default value of 2.
The entire flagset is given to the watches pkg Load() to provide one
place for handling command line arguments, watches config values, and
environment variables. This makes an individual watch more useful when
creating a runner and adding a GVK to the controller. Also, this puts
new fields on the Watch struct (MaxWorkers and AnsibleVerbosity).
New functions were added to the watches pkg -- New(gvk) instantiates a
Watch with sensible defaults. Validate() is used to verify a given
watch meets certain criteria (legit path to playbook/role and proper
finalizer). This makes it possible for someone who only knows their
GVK and role/playbook to easily create a watch that can be used with
the runner pkg.
When creating a runner from a watch, we will now validate it using
watches.Validate(). Essentially the watches pkg should know what is
and isn't a valid watch.
New helper functions in the runner pkg -- ansibleVerbosityString to
convert an integer AnsibleVerbosity to a string (ie. 2 becomes "-vv"),
playbookCmdFunc, and roleCmdFunc consolidate the logic for creating
the cmdFund.

pkg/ansible/runner/runner_test.go

djzager · 2019-08-22T13:35:05Z

/test e2e-aws-ansible

djzager · 2019-08-22T19:55:12Z

/test e2e-aws-ansible

jmrodri · 2019-08-28T13:22:12Z

I thought we said that the maxworkers were an operational item and shouldn't be in the watches file. That was the case when I added maxworkers. Unless things have changed since then. The idea was that the author of the operator would set it as a flag during deployment, then the operator/admin would override it via the environment variable based on their clusters needs/capabilities.

djzager · 2019-08-29T12:28:42Z

/retest

jmrodri

/lgtm

jmrodri · 2019-09-23T16:13:36Z

/lgtm

camilamacedo86 · 2019-09-23T16:14:26Z

HI @djzager,

Really great work here 👍 . However, shows that it is missing update the docs regards these changes.. could you please add this part in the PR?

camilamacedo86

Hi @djzager,

Really thank you for the contribution. 🥇

Could you please add a CHANGELOG entry for this additional? Also, it shows outdated could you rebase it with the master? I think it shows missing to update the doc as well. Could you please check it?

The primary objective of this change is to make the verbosity of individual `ansible-runner` command invocations configurable (not just "-vv"). In order to accomplish this: * A new flag was added to the AnsibleOperatorFlags (ansible-verbosity) with a default value of 2. * The entire flagset is given to the watches pkg Load() to provide one place for handling command line arguments, watches config values, and environment variables. This makes an individual watch more useful when creating a runner and adding a GVK to the controller. Also, this puts new fields on the Watch struct (MaxWorkers and AnsibleVerbosity). * New functions were added to the watches pkg -- New(gvk) instantiates a Watch with sensible defaults. Validate() is used to verify a given watch meets certain criteria (legit path to playbook/role and proper finalizer). This makes it possible for someone who only knows their GVK and role/playbook to easily create a watch that can be used with the runner pkg. * When creating a runner from a watch, we will now validate it using watches.Validate(). Essentially the watches pkg should know what is and isn't a valid watch. * New helper functions in the runner pkg -- ansibleVerbosityString to convert an integer AnsibleVerbosity to a string (ie. 2 becomes "-vv"), playbookCmdFunc, and roleCmdFunc consolidate the logic for creating the cmdFund.

Prevent configuration of ansible verbosity or max worker from watches.yaml

Since there are no tests added to the file, remove the file so it's obvious there are no tests for the Ansible operator's run function.

openshift-ci-robot · 2019-10-10T18:31:54Z

New changes are detected. LGTM label has been removed.

djzager · 2019-10-10T18:32:47Z

@camilamacedo86 I'm looking through to see if there is any more documenting that I need to do.

camilamacedo86 · 2019-10-13T11:07:18Z

CHANGELOG.md

@@ -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))


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))

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

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.

camilamacedo86 · 2019-10-13T11:09:07Z

pkg/ansible/run.go

@@ -107,7 +105,7 @@ func Run(flags *aoflags.AnsibleOperatorFlags) error {
 			GVK:             w.GroupVersionKind,
 			Runner:          runner,
 			ManageStatus:    w.ManageStatus,
-			MaxWorkers:      getMaxWorkers(w.GroupVersionKind, flags.MaxWorkers),


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

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.

camilamacedo86 · 2019-10-13T11:11:31Z

pkg/ansible/runner/runner.go

@@ -52,56 +52,65 @@ type Runner interface {
 	GetFinalizer() (string, bool)
 }

+func ansibleVerbosityString(verbosity int) string {
+	if verbosity > 0 {


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?

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.

camilamacedo86 · 2019-10-13T11:15:15Z

pkg/ansible/runner/runner_test.go

 	testCases := []struct {
-		name  string
-		watch watches.Watch


Why it was removed? Are the watches still working well after this change? I mean, is possible to define the resources which should be watched after this PR?

I'm not certain I follow. The structure of the testCase struct was modified so that the watch could be constructed as it is here

camilamacedo86 · 2019-10-13T11:17:35Z

pkg/ansible/runner/runner_test.go

+		gvk       schema.GroupVersionKind
+		playbook  string
+		role      string
+		finalizer *watches.Finalizer


Why is required change the watches and add the finalizer to add support to users defined the verbose level of the ansible container?

Since I am creating the watch below, I need all of the things that could be meaningfully define on a watch to be a part of each test case.

camilamacedo86 · 2019-10-13T11:19:22Z

pkg/ansible/runner/runner_test.go

+	for _, tc := range testCases {
+		gotString := ansibleVerbosityString(tc.verbosity)
+		if tc.expectedString != gotString {
+			t.Fatalf("Unexpected string %v expected %v", gotString, tc.expectedString)


Should not possible to add a test to ensure that if the -vv, for example, be used then the logs are with this verbose? Could we not check that the output which jut will occur in this scenario was not printed?

I think it would be possible to add a functional test to do this, yes. I do not believe it would be easy to construct a test in runner_test.go that does as you describe.

Wdyt about using the e2e tests for it?

camilamacedo86

Hi @djzager,

I looked better this PR and is not clear for me why was required to do some changes? Could we not just add the support which shows the goal of this PR and leave the rest of the impl as it is? To add the support would not just be required get the env var with the verbosity level and use it? Also, it is missing add the info over it in the doc

So, it shows for me that you are trying to address here more changes than what you described in the changelog. In this way, could you split your goals in PRs?

WDYT @jmrodri and @fabianvf ?

djzager · 2019-10-14T12:50:11Z

I looked better this PR and is not clear for me why was required to do some changes?

Part of the "additional" changes are from the fact that this is a continuation of my previous PRs to refactor the Ansible Operator:

That is an important point because, you are correct, more than just making it possible to control Ansible's verbosity is happening here. However, I would argue that as it currently stands where values are coming from is not straightforward...some are being handled in the ansible package while some others are being handled in the runner package or are hardcoded.

What this pull request aims to do, in addition to the verbosity stuff, was to centralize:

Where the logic for how we determine maxWorkers/ansibleVerbosity is located. It's now in the watches package.
On what these values are stored, the watch struct.

Also, it is missing add the info over it in the doc

This I need to update. Should I leave the discussion of using ANSIBLE_VERBOSITY since I see no reason why that wouldn't work...or should I only discuss ansible-verbosity flag + GVK specific environment variables?

camilamacedo86 · 2019-10-14T14:28:22Z

HI @djzager,

IMHO this PR has more than one purpose which makes hard to track these changes in the changelog, review and etc ... since it is not only about to add the flag as described there. Also, it has many changes in the tests as well which make harder we ensure that all still working properly. So, IMHO you should have a PR for each purpose instead of merging this one in order to help us rewview, ensure the quality, test and etc ... However, I will leave the decision to the team.

Hi @fabianvf,
Since I think you are who has better knowledge over it, could you help us by reviewing this PR?

camilamacedo86 · 2019-10-14T14:29:04Z

/test sanity

openshift-ci-robot · 2019-10-14T14:32:56Z

@djzager: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/sanity	`532ebb7`	link	`/test sanity`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

.

djzager · 2019-10-14T16:31:19Z

IMHO this PR has more than one purpose which makes hard to track these changes in the changelog, review and etc ... since it is not only about to add the flag as described there. Also, it has many changes in the tests as well which make harder we ensure that all still working properly. So, IMHO you should have a PR for each purpose instead of merging this one in order to help us rewview, ensure the quality, test and etc ... However, I will leave the decision to the team.

I do not disagree that there is more going on here than just "make ansible verbosity configurable". I do not particularly want to break this PR up into the 3-5 pieces but I will if that's what you need from me to review this PR.

I will wait on feedback from @camilamacedo86, @fabianvf, @jmrodri before doing any more work on this PR (specifically fixing the broken sanity test and addressing some of the remaining comments).

camilamacedo86 · 2019-10-14T17:27:17Z

Hi @djzager,

Really thank you for comprehension and I think is a good approach wait for the others review. Maybe they have more knowledge on this part and are ok with move in this way.

djzager · 2019-10-15T18:21:19Z

Without a good reason not to break up this work I have decided to go ahead and break this PR up into it's proper pieces.

camilamacedo86 · 2019-10-15T18:52:20Z

Really thank you @djzager for your contribution and understanding in this matter.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 21, 2019

openshift-ci-robot requested review from fabianvf and jmrodri August 21, 2019 19:26

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 21, 2019

djzager commented Aug 21, 2019

View reviewed changes

pkg/ansible/runner/runner_test.go Show resolved Hide resolved

djzager commented Aug 21, 2019

View reviewed changes

pkg/ansible/runner/runner_test.go Show resolved Hide resolved

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 28, 2019

djzager force-pushed the ao-verbose branch from 23999fe to 3764e8c Compare August 28, 2019 20:21

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 28, 2019

openshift-ci-robot assigned jmrodri Aug 30, 2019

jmrodri approved these changes Aug 30, 2019

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2019

djzager force-pushed the ao-verbose branch from 3764e8c to d6f8a95 Compare September 23, 2019 15:58

djzager changed the title ~~WIP: feat(ansible): make ansible verbosity configurable~~ feat(ansible): make ansible verbosity configurable Sep 23, 2019

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Sep 23, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2019

camilamacedo86 reviewed Oct 8, 2019

View reviewed changes

djzager added 4 commits October 10, 2019 12:12

Add specific test for ansibleVerbosityString

0d38e6b

Add timeout for metrics check

bc3a919

No worker/verbosity config on watches yaml

fb2c63f

Prevent configuration of ansible verbosity or max worker from watches.yaml

djzager added 3 commits October 10, 2019 12:17

Remove ansible run_test

18d1e6b

Since there are no tests added to the file, remove the file so it's obvious there are no tests for the Ansible operator's run function.

Don't allow ansible verbosity greater than 7

1ab748e

Add documentation

532ebb7

djzager force-pushed the ao-verbose branch from fce0735 to 532ebb7 Compare October 10, 2019 18:31

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 10, 2019

camilamacedo86 reviewed Oct 13, 2019

View reviewed changes

camilamacedo86 previously requested changes Oct 13, 2019

View reviewed changes

camilamacedo86 self-requested a review October 14, 2019 15:06

djzager closed this Oct 15, 2019

This was referenced Oct 16, 2019

test(ansible): Refactor ansible's functional tests #2065

Merged

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

Merged

feat(ansible): make ansible verbosity configurable #2087

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ansible): make ansible verbosity configurable #1852

feat(ansible): make ansible verbosity configurable #1852

djzager commented Aug 21, 2019

djzager commented Aug 22, 2019

djzager commented Aug 22, 2019

jmrodri commented Aug 28, 2019

djzager commented Aug 29, 2019

jmrodri left a comment

jmrodri commented Sep 23, 2019

camilamacedo86 commented Sep 23, 2019

camilamacedo86 left a comment •

edited

Loading

openshift-ci-robot commented Oct 10, 2019

djzager commented Oct 10, 2019

camilamacedo86 Oct 13, 2019

camilamacedo86 Oct 14, 2019

djzager Oct 14, 2019

camilamacedo86 Oct 13, 2019 •

edited

Loading

djzager Oct 14, 2019

camilamacedo86 Oct 13, 2019

djzager Oct 14, 2019

camilamacedo86 Oct 13, 2019 •

edited

Loading

djzager Oct 14, 2019

camilamacedo86 Oct 13, 2019

djzager Oct 14, 2019

camilamacedo86 Oct 13, 2019

djzager Oct 14, 2019

camilamacedo86 Oct 14, 2019

camilamacedo86 left a comment •

edited

Loading

djzager commented Oct 14, 2019

camilamacedo86 commented Oct 14, 2019 •

edited

Loading

camilamacedo86 commented Oct 14, 2019

openshift-ci-robot commented Oct 14, 2019

djzager commented Oct 14, 2019

camilamacedo86 commented Oct 14, 2019

djzager commented Oct 15, 2019

camilamacedo86 commented Oct 15, 2019

	- 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))

feat(ansible): make ansible verbosity configurable #1852

feat(ansible): make ansible verbosity configurable #1852

Conversation

djzager commented Aug 21, 2019

djzager commented Aug 22, 2019

djzager commented Aug 22, 2019

jmrodri commented Aug 28, 2019

djzager commented Aug 29, 2019

jmrodri left a comment

Choose a reason for hiding this comment

jmrodri commented Sep 23, 2019

camilamacedo86 commented Sep 23, 2019

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

openshift-ci-robot commented Oct 10, 2019

djzager commented Oct 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Oct 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Oct 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

djzager commented Oct 14, 2019

camilamacedo86 commented Oct 14, 2019 • edited Loading

camilamacedo86 commented Oct 14, 2019

openshift-ci-robot commented Oct 14, 2019

djzager commented Oct 14, 2019

camilamacedo86 commented Oct 14, 2019

djzager commented Oct 15, 2019

camilamacedo86 commented Oct 15, 2019

camilamacedo86 left a comment •

edited

Loading

camilamacedo86 Oct 13, 2019 •

edited

Loading

camilamacedo86 Oct 13, 2019 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

camilamacedo86 commented Oct 14, 2019 •

edited

Loading