pkg/ansible: introduce managedStatus flag #744

djzager · 2018-11-14T20:36:21Z

Adds new bool flag to ansible operator managedStatus. When True
(expected default) the ansible operator manages the status. When false
the ansible operator does not touch the status of the CR.
Adds new field to watches.yaml, for each GVK, you can now set
managedStatus: bool see https://golang.org/pkg/strconv/#ParseBool
Update reconciler to only update the status of the CR when
managedStatus

Also move constants out of status/utils.go into status/types.go to
prevent confusion.

openshift-ci-robot · 2018-11-14T20:36:40Z

Hi @djzager. Thanks for your PR.

I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

djzager · 2018-11-14T20:36:56Z

Before I go through and update documentation, I'd like to get feedback on the general approach.

shawn-hurley

Can we move the constants back as well? Otherwise just some nits.

pkg/ansible/controller/status/types.go

pkg/ansible/runner/runner.go

dymurray

LGTM

dymurray · 2018-11-15T20:25:23Z

commands/operator-sdk/cmd/up/local.go

@@ -148,7 +148,7 @@ func upLocalAnsible() {
 	}

 	// start the operator
-	go ansibleOperator.Run(done, mgr, "./"+ansibleScaffold.WatchesYamlFile, time.Minute)
+	go ansibleOperator.Run(done, mgr, "./"+ansibleScaffold.WatchesYamlFile, time.Minute, true)


Just commenting that we agreed in person this would be configurable in the CLI flags in a separate PR

What's the use case for changing this at runtime vs. making it a more permanent behavior of a given operator? It seems risky to let an end user change this at runtime, vs. reserving this decision for the operator developer.

dymurray · 2018-11-15T20:25:38Z

pkg/ansible/controller/reconcile.go

+	// If the condition is currently running, making sure that the values are correct.
+	// If they are the same a no-op, if they are different then it is a good thing we
+	// are updating it.
+	if (errCond == nil && succCond == nil) || (succCond != nil && succCond.Reason != ansiblestatus.SuccessfulReason) {


shawn-hurley

I would also like to see some reconcile tests added.

shawn-hurley · 2018-11-19T19:11:55Z

doc/ansible/user-guide.md

@@ -55,13 +55,70 @@ layout][layout_doc] doc.

 #### Operator scope

-A namespace-scoped operator (the default) watches and manages resources in a single namespace, whereas a cluster-scoped operator watches and manages resources cluster-wide. Namespace-scoped operators are preferred because of their flexibility. They enable decoupled upgrades, namespace isolation for failures and monitoring, and differing API definitions. However, there are use cases where a cluster-scoped operator may make sense. For example, the [cert-manager](https://github.com/jetstack/cert-manager) operator is often deployed with cluster-scoped permissions and watches so that it can manage issuing certificates for an entire cluster.
+A namespace-scoped operator (the default) watches and manages resources in a


Why are we making this format changes?

shawn-hurley · 2018-11-19T19:14:31Z

doc/ansible/user-guide.md


 If you'd like to create your memcached-operator project to be cluster-scoped use the following `operator-sdk new` command instead:
 ```
 $ operator-sdk new memcached-operator --cluster-scoped --api-version=cache.example.com/v1alpha1 --kind=Memcached --type=ansible
 ```

+### Watches file


Can you also update the doc/ansible/dev/developer-guide.yaml and either remove the watches file section there or update it?

I vote to remove so that this is only documented in one place to keep track of.

shawn-hurley

LGTM

Can we add 1 test to make sure that setting managed status to true is covered in the watches file?

mhrivnak · 2018-11-20T20:32:20Z

pkg/ansible/runner/runner.go

@@ -54,6 +55,7 @@ type watch struct {
 	Playbook        string     `yaml:"playbook"`
 	Role            string     `yaml:"role"`
 	ReconcilePeriod string     `yaml:"reconcilePeriod"`
+	ManagedStatus   *bool      `yaml:"manageStatus"`


Why translate here from "manage" to "managed"?

I like the verb form better FWIW. Is the status managed? Sure it is, but that doesn't tell us by who. "Manage status" is more imperative, telling the operator that it should manage the status.

Also, why is this a pointer to a bool instead of just a bool?

Also, why is this a pointer to a bool instead of just a bool?

Since the uninitialized value for bool is false. It would be difficult to tell if the user specified false in their watches file or not at all. This allows us to take the value specified in Run() when the user didn't specify.

I think it's better to set the default value before the unmarshal, and let the unmarshal overwrite it if the user specified a value. Then you can just trust that you have the correct value to pass around, and there's no risk of trying to dereference a nil pointer deeper in the stack.

w := watch{ManageStatus: true} err := yaml.Unmarshal(data, &w)

That seems like a reasonable suggestion and at first I was in a panic that I hadn't thought of it. Unfortunately, we don't actually have access to the "default" value inside this function.

We would have to update NewFromWatches() to take the managedStatus which would be the default value. If we are going to do that, then we should also include the reconcilePeriod. @shawn-hurley do you have any thoughts?

I am ok with the changes that he is proposing. But I think that reconcilePeriod and ManagedStatus are turning into different things. I think that @mhrivnak is suggesting that we do not have a CLI flag for this and instead use this to set it true by default.

I had thought that it made sense to allow someone to have this have a CLI flag, but I believe @mhrivnak is right, that why should a user of an AO based operator be able to turn this off, the developer of the operator should have some control of this one.

If in the future we want to change this, we can add a CLI, but I think that we should remove the options to pass to the operator/operator.go

@mhrivnak can you confirm that is what you were thinking are you are ok with the above?

Yep, that's right. I suggest we do not let the user set this at runtime, and only let it be set in watches.yaml. Then we can hard-code the default value in one place (in NewFromWatches) and also document that default.

mhrivnak · 2018-11-20T20:37:06Z

doc/ansible/user-guide.md

+  field is mutually exclusive with the "role" field.
+* **reconcilePeriod** (optional): The reconciliation interval, how often the
+  role/playbook is run, for a given CR.
+* **manageStatus** (optional): Specifies who is managing the status for the CR.


I suggest documenting this from the opposite perspective. There are potentially several use cases for setting this to false, and I don't think it's worth trying to list them unless you want to cite them as examples. Maybe something like this:

"When true, the operator will manage the status for the CR in a generic way. Set to false is status is managed somewhere else, such as in a role or playbook, or in a separate controller."

It's also helpful to explicitly document the default value since this is optional.

It's also helpful to explicitly document the default value since this is optional.

It is not clear to me how to do that since that is entirely dependent on what's specified in Run(). It's presumably true by default.

- Adds new bool flag to ansible operator `managedStatus`. When True (expected default) the ansible operator manages the status. When false the ansible operator does not touch the status of the CR. - Adds new field to `watches.yaml`, for each GVK, you can now set `managedStatus: bool` see https://golang.org/pkg/strconv/#ParseBool - Update reconciler to only update the status of the CR when `managedStatus` Also move constants out of `status/utils.go` into `status/types.go` to prevent confusion.

mhrivnak

LGTM after the nits. No need for re-review from me.

mhrivnak · 2018-11-26T14:29:44Z

doc/ansible/user-guide.md

+  role/playbook is run, for a given CR.
+* **manageStatus** (optional): When true (default), the operator will manage
+  the status of the CR generically. Set to false, the status of the CR is
+  managed elsewhere, the specified role/playbook or in a separate controller.


I think there might be a couple of words missing here.

mhrivnak · 2018-11-26T14:32:34Z

pkg/ansible/controller/reconcile.go

@@ -54,6 +54,7 @@ type AnsibleOperatorReconciler struct {
 	Client          client.Client
 	EventHandlers   []events.EventHandler
 	ReconcilePeriod time.Duration
+	ManageStatus   bool


Looks like the indentation drifted. I have a vim plugin that runs "go fmt" on every save, which is handy for preventing that sort of thing.

mhrivnak · 2018-11-28T17:02:24Z

pkg/ansible/controller/controller.go

@@ -42,6 +42,7 @@ type Options struct {
 	Runner          runner.Runner
 	GVK             schema.GroupVersionKind
 	ReconcilePeriod time.Duration
+	ManageStatus   bool


Looks like the indentation drifted. I have a vim plugin that runs "go fmt" on every save, which is handy for preventing that sort of thing.

mhrivnak · 2018-11-28T17:02:33Z

pkg/ansible/controller/controller.go

@@ -58,6 +59,7 @@ func Add(mgr manager.Manager, options Options) {
 		Runner:          options.Runner,
 		EventHandlers:   eventHandlers,
 		ReconcilePeriod: options.ReconcilePeriod,
+		ManageStatus:   options.ManageStatus,


Looks like the indentation drifted. I have a vim plugin that runs "go fmt" on every save, which is handy for preventing that sort of thing.

mhrivnak · 2018-11-28T17:10:54Z

pkg/ansible/controller/reconcile_test.go

@@ -46,6 +46,7 @@ func TestReconcile(t *testing.T) {
 		Name            string
 		GVK             schema.GroupVersionKind
 		ReconcilePeriod time.Duration
+		ManageStatus   bool
