Closes #5565: Helm reconcile period from CR annotations or watches.yaml #5585

VenkatRamaraju · 2022-03-07T22:56:14Z

Description of the change:

Added the parsing of the helm.sdk.operatorframework.io/reconcile-period key in the custom resource's annotations.
Added the option to specify the reconcile-period through the watches.yaml file.

The value obtained from either of these two is set to the RequeueAfter field of the reconciler for Helm. If both are specified, then the CR annotations takes precedence.

Motivation for the change:
Close #5565

Checklist

Add a new changelog fragment in changelog/fragments
Add or update relevant sections of the docs website in website/content/en/docs

Note:
As mentioned in my comment on the issue: this functionality already exists via a command line flag. This PR is to provide the user with the option of doing the same in the CR annotations or watches.yaml, which there seems to be a demand for. This feature already exists for the Ansible-based operator, so I thought it would be a good idea to do the same for the Helm-based operator.

The current order of precedence is: flag < watches < CR annotations.

camilamacedo86 · 2022-03-08T15:47:03Z

website/content/en/docs/building-operators/helm/reference/advanced_features/reconcile-period.md

+metadata:
+  name: nginx-sample
+  annotations:
+    helm.operator-sdk/reconcile-period: "5s"


What happens if the reconciliation period not be informed?
What is the default behaviour?
Have we the same behaviour for Ansible and Helm in this case?
Also, is not it an issue for Ansible as well?

If no reconcile period is specified in the CR annotations, then the default value is 1m as indicated here.

This behavior exists for Ansible: https://sdk.operatorframework.io/docs/building-operators/ansible/development-tips/#annotations-for-custom-resource.

So, it shows that we also need to check/fix it for Ansible.
Historic we try to keep help/ansible working similar as much as possible since it helps keep both maintained, help us and the users switch easily between the languages as well.

Could we ensure the changes for ansible here as well?

c/c @asmacdo @fabianvf

Can be a follow up either, but should we not. ensure that same for Ansible?

@camilamacedo86: So we are now going to accept reconcile Period through 3 ways:

from CR annotations

command line flags

through watches.yaml
The order of precedence imo should be CR annotations > watches.yaml > cmd line flags. We want it to be in sync with ansible, but ansible does not accept it from flags and neither does the hybrid helm implementation. We would want to eventually remove accepting it through cmd line flags, but I think that should be a follow up. From discussion with @VenkatRamaraju, looks like CR annotations override values from watches.yaml in ansible. So I think we are good with that.

jmrodri

a couple of suggestions and a question

jmrodri · 2022-03-08T15:42:47Z

internal/helm/controller/reconcile.go

@@ -388,9 +389,33 @@ func (r HelmOperatorReconciler) Reconcile(ctx context.Context, request reconcile
 		Manifest: expectedRelease.Manifest,
 	}
 	err = r.updateResourceStatus(ctx, o, status)
+
+	// Parse the reconcile period from CR's annotations
+	annotationsReconcilePeriod := parseReconcilePeriod(o)


For the record, o is a horrible variable name for this but since it is an existing structure, I won't make you rewrite it here.

website/content/en/docs/building-operators/helm/reference/advanced_features/reconcile-period.md

jmrodri · 2022-03-08T15:45:09Z

website/content/en/docs/building-operators/helm/reference/advanced_features/reconcile-period.md

+
+The value that is present under this key must be in the h/m/s format. For example, 1h2m4s, 3m0s, 4s are all valid values, but 1x3m9s is invalid. 
+
+**NOTE**: This is just one way of specifying the reconcile period for helm operators. Another way is using the `--reconcile-period` command line flag while running the operator. However, if both


Suggested change

**NOTE**: This is just one way of specifying the reconcile period for helm operators. Another way is using the `--reconcile-period` command line flag while running the operator. However, if both

**NOTE**: This is just one way of specifying the reconcile period for Helm-based operators. Another way is using the `--reconcile-period` command line flag while running the operator. However, if both

website/content/en/docs/building-operators/helm/reference/advanced_features/reconcile-period.md

varshaprasad96 · 2022-03-08T16:56:25Z

@VenkatRamaraju if possible, can you also add this in https://github.com/operator-framework/helm-operator-plugins?

VenkatRamaraju · 2022-03-08T17:21:06Z

@varshaprasad96 yup, I can do that.

changelog/fragments/helm-annotations-reconcile-period.yaml

varshaprasad96 · 2022-03-08T22:09:45Z

internal/helm/controller/reconcile.go

+	// Parse the reconcile period from CR's annotations
+	annotationsReconcilePeriod := parseReconcilePeriod(o)
+
+	if annotationsReconcilePeriod != "" {


can we a have a small test that verifies the overriding hierarchy, just to make sure that when a flag is passed the annotation values (if present) override them?

yup, I just added a couple of tests to verify the order of precedence and parsing of the field in the annotations.

I had to modify the signature of the function to make it easier to test.

varshaprasad96 · 2022-03-10T00:25:29Z

internal/helm/controller/controller.go

@@ -49,7 +49,7 @@ type WatchOptions struct {
 	Namespace               string
 	GVK                     schema.GroupVersionKind
 	ManagerFactory          release.ManagerFactory
-	ReconcilePeriod         time.Duration
+	ReconcilePeriod         string


Can we revert this? Eventually watches struct would be exposed in hybrid implementation. To remain backwards compatible, let's avoid making changes to it.

varshaprasad96

just one nit and /lgtm!

varshaprasad96 · 2022-03-10T22:07:11Z

changelog/fragments/helm-annotations-reconcile-period.yaml

+      value from the custom resource annotations for helm operators. This value is set to 
+      then set to the 'reconcile-period' field of the reconciler to reconcile the cluster


Nit:

Suggested change

value from the custom resource annotations for helm operators. This value is set to

then set to the 'reconcile-period' field of the reconciler to reconcile the cluster

value from the custom resource annotations for helm operators. This value is

then set to the 'reconcile-period' field of the reconciler to reconcile the cluster

Signed-off-by: Venkat Ramaraju <[email protected]>

openshift-ci bot requested review from asmacdo and joelanford March 7, 2022 22:56

VenkatRamaraju force-pushed the helm-reconcile-period branch 3 times, most recently from ee557c0 to 2dc8eea Compare March 7, 2022 23:41

camilamacedo86 reviewed Mar 8, 2022

View reviewed changes

jmrodri requested changes Mar 8, 2022

View reviewed changes

VenkatRamaraju force-pushed the helm-reconcile-period branch from 2dc8eea to 64bf0e5 Compare March 8, 2022 16:19

VenkatRamaraju requested review from jmrodri and camilamacedo86 March 8, 2022 16:32

camilamacedo86 reviewed Mar 8, 2022

View reviewed changes

changelog/fragments/helm-annotations-reconcile-period.yaml Outdated Show resolved Hide resolved

VenkatRamaraju force-pushed the helm-reconcile-period branch from 64bf0e5 to 7193943 Compare March 8, 2022 20:44

varshaprasad96 reviewed Mar 8, 2022

View reviewed changes

VenkatRamaraju force-pushed the helm-reconcile-period branch from 7193943 to f34d901 Compare March 9, 2022 02:16

VenkatRamaraju requested a review from varshaprasad96 March 9, 2022 02:36

VenkatRamaraju force-pushed the helm-reconcile-period branch 3 times, most recently from fed8ef4 to 1cd6231 Compare March 9, 2022 16:53

VenkatRamaraju changed the title ~~Closes #5565: Helm reconcile period from CR annotations~~ Closes #5565: Helm reconcile period from CR annotations or watches.yaml Mar 9, 2022

VenkatRamaraju force-pushed the helm-reconcile-period branch from 23cf6e1 to 3bc5570 Compare March 9, 2022 19:35

varshaprasad96 requested changes Mar 10, 2022

View reviewed changes

VenkatRamaraju force-pushed the helm-reconcile-period branch 2 times, most recently from 6fcd943 to cad82fa Compare March 10, 2022 01:41

VenkatRamaraju requested review from camilamacedo86 and varshaprasad96 March 10, 2022 01:41

VenkatRamaraju force-pushed the helm-reconcile-period branch 3 times, most recently from 9f4ff38 to 2c13436 Compare March 10, 2022 18:55

jmrodri approved these changes Mar 10, 2022

View reviewed changes

varshaprasad96 approved these changes Mar 10, 2022

View reviewed changes

Close operator-framework#5565: Added parsing of annotations

e7fd7b6

Signed-off-by: Venkat Ramaraju <[email protected]>

VenkatRamaraju force-pushed the helm-reconcile-period branch from 2c13436 to e7fd7b6 Compare March 10, 2022 22:42

VenkatRamaraju mentioned this pull request Mar 10, 2022

Reconcile period CLI flag for Ansible operators not working #5594

Closed

jmrodri merged commit d632ae2 into operator-framework:master Mar 11, 2022

VenkatRamaraju mentioned this pull request Mar 14, 2022

Helm reconcile period: fixed docs + align with Ansible #5599

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #5565: Helm reconcile period from CR annotations or watches.yaml #5585

Closes #5565: Helm reconcile period from CR annotations or watches.yaml #5585

VenkatRamaraju commented Mar 7, 2022 •

edited

Loading

camilamacedo86 Mar 8, 2022 •

edited

Loading

VenkatRamaraju Mar 8, 2022 •

edited

Loading

camilamacedo86 Mar 8, 2022

camilamacedo86 Mar 8, 2022

varshaprasad96 Mar 10, 2022

jmrodri left a comment

jmrodri Mar 8, 2022

jmrodri Mar 8, 2022

varshaprasad96 commented Mar 8, 2022

VenkatRamaraju commented Mar 8, 2022

varshaprasad96 Mar 8, 2022

VenkatRamaraju Mar 9, 2022

varshaprasad96 Mar 10, 2022

varshaprasad96 left a comment

varshaprasad96 Mar 10, 2022


		The value that is present under this key must be in the h/m/s format. For example, 1h2m4s, 3m0s, 4s are all valid values, but 1x3m9s is invalid.

		NOTE: This is just one way of specifying the reconcile period for helm operators. Another way is using the `--reconcile-period` command line flag while running the operator. However, if both

		value from the custom resource annotations for helm operators. This value is set to
		then set to the 'reconcile-period' field of the reconciler to reconcile the cluster

Closes #5565: Helm reconcile period from CR annotations or watches.yaml #5585

Closes #5565: Helm reconcile period from CR annotations or watches.yaml #5585

Conversation

VenkatRamaraju commented Mar 7, 2022 • edited Loading

camilamacedo86 Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

VenkatRamaraju Mar 8, 2022 • 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

jmrodri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varshaprasad96 commented Mar 8, 2022

VenkatRamaraju commented Mar 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varshaprasad96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VenkatRamaraju commented Mar 7, 2022 •

edited

Loading

camilamacedo86 Mar 8, 2022 •

edited

Loading

VenkatRamaraju Mar 8, 2022 •

edited

Loading