chore(ansible): Create service monitor #2179

djzager · 2019-11-12T16:46:41Z

Ansible based operator's should also create the service monitor as
appropriate.

openshift-ci-robot · 2019-11-12T16:46:55Z

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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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 · 2019-11-12T16:48:29Z

@fabianvf @jmrodri I think one thing that would be helpful, in the future to prevent needing these kinds of PRs, would be to pull out the startup tasks that are common to all operators into an operator-sdk lib that is used in the scaffolding. I'm sure there are risks in that kind of effort but worth considering.

jmrodri · 2019-11-12T17:01:39Z

/ok-to-test

djzager · 2019-11-12T20:56:30Z

/retest

djzager · 2019-11-13T14:06:05Z

/retest

camilamacedo86

Hi @djzager,

Really thank you for your contribution 🥇
What is missing here for we go forward is a Changelog entry.
Also, would be great to have a test to ensure that the Service Monitor was created on it. Could you please add these small nits?

pkg/ansible/run.go

camilamacedo86

/lgtm

djzager · 2020-01-17T16:25:35Z

It appears that my suspicion was correct (that the ServiceMonitor resource doesn't exist in the CI environment):

Ensure that no errors appear in the log
{"level":"info","ts":1578688227.027858,"logger":"cmd","msg":"Could not create ServiceMonitor object","Namespace":"default","error":"no ServiceMonitor registered with the API"}
{"level":"info","ts":1578688227.0278876,"logger":"cmd","msg":"Install prometheus-operator in your cluster to create ServiceMonitor objects","Namespace":"default","error":"no ServiceMonitor registered with the API"}

It may be possible to create the ServiceMonitor CRD simply to allow for it to be created and verified. wdyt?

fabianvf · 2020-01-20T17:00:20Z

pkg/ansible/run.go

+	services := []*v1.Service{service}
+	_, err = metrics.CreateServiceMonitors(cfg, namespace, services)
+	if err != nil {
+		log.Info("Could not create ServiceMonitor object", "error", err.Error())


Looks like CI is unhappy because the error is showing up in the logs (https://travis-ci.org/operator-framework/operator-sdk/jobs/635443111#L1665-L1667), we might need to either change that log or change the way we search for errors in the test.

Maybe we could call the error field something like reason instead?

Also minor not, I don't think we need to log twice here when the service monitor is not present, maybe make these two logs into an if .. else like

if err == metrics.ServiceMonitorNotPresent { // log about installing the operator } else { // generic error log }

just to cut down on the noise in the logs

I changed the error to reason. However, I elected not to prevent logging 2x for two reasons:

Match the scaffolding provided for go based operators

This only happens once on startup which makes me believe this isn't an unreasonable contribution to noise.

If you would still like me to restructure the logging here @fabianvf , I can do that.

I agree with @djzager
I think we should keep the same impl done in go and then, since it was updated to it lastest version, I do not think that the errors will appear too. WDYT @fabianvf

@djzager we need test locally as well operator-sdk run --local. I will do fully test with and let you know.

hack/lib/common.sh

CHANGELOG.md

camilamacedo86 · 2020-02-06T19:20:56Z

pkg/ansible/run.go

+	// necessary to configure Prometheus to scrape metrics from this operator.
+	services := []*v1.Service{service}
+	_, err = metrics.CreateServiceMonitors(cfg, namespace, services)
+	if err == metrics.ErrServiceMonitorNotPresent {


See that the golang scaffolded was changed indeed to not show log errors regards it when should not. See #2190. So, could you please update the implementation in order to reflect the current one made in go?

I went ahead and made this ServiceMonitor creation block equivalent to what is found in internal/scaffold/cmd.go

I mean: Should we not?

Add in the main func.

// Add the Metrics Service
addMetrics(ctx, cfg, namespace)

At the end, add all addMetrics implementation

Add the customization requested by @fabianvf in the addMetrics

WDYT?

Now I see what you are saying. I missed the addMetrics piece from the scaffold.

I'm not sure about which customization request you are referring to from @fabianvf :

If reason vs error, this isn't needed anymore because we are testing with the servicemonitor CRD installed in the cluster

If referring to removing one of the log.Info's .. I just don't agree that it's a valuable deviation from the scaffold. It would be one thing to me if this were in the Reconcile loop and it was an additional line of log output on every reconcile. However, in the majority of cases it will be two lines of output at the very beginning of operator startup saying "couldn't create serviceMonitor" and "install prometheus-operator to create ServiceMonitor objects".

djzager · 2020-02-06T20:43:05Z

CI will fail until I make the necessary updates to the molecule based testing. I will wait to do that until #2425 is merged.

Ansible based operator's should also create the service monitor as appropriate.

- Add a function to common that applies the servicemonitor CRD - Update the e2e ansible and e2e ansible molecule tests to verify the service monitor is created

djzager · 2020-02-07T15:35:26Z

@fabianvf @camilamacedo86 I believe I have addressed all of the comments and CI is passing. Please, take another look.

fabianvf

/lgtm

camilamacedo86 · 2020-02-07T17:44:56Z

CHANGELOG.md

@@ -5,6 +5,7 @@
 - Add a new option to set the minimum log level that triggers stack trace generation in logs (`--zap-stacktrace-level`) ([#2319](https://github.com/operator-framework/operator-sdk/pull/2319))
 - Added `pkg/status` with several new types and interfaces that can be used in `Status` structs to simplify handling of [status conditions](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties). ([#1143](https://github.com/operator-framework/operator-sdk/pull/1143))
 - Added support for relative Ansible roles and playbooks paths in the Ansible operator's file. ([#2273](https://github.com/operator-framework/operator-sdk/pull/2273))
+- Ansible based operators now creates prometheus service monitor, if available. ([#2179](https://github.com/operator-framework/operator-sdk/pull/2179))


Suggested change

- Ansible based operators now creates prometheus service monitor, if available. ([#2179](https://github.com/operator-framework/operator-sdk/pull/2179))

- Add Prometheus metrics support to Ansible - based operators. ([#2179](https://github.com/operator-framework/operator-sdk/pull/2179))

camilamacedo86

Tested locally as well. 👍 Great contribution.

@djzager could just address the nit in the CHANGELOG for we merge this one?

Running locally and in the cluster whiout promethues with success:

camilamacedo86 · 2020-02-07T17:51:49Z

/lgtm

fabianvf · 2020-02-07T19:47:04Z

/lgtm

openshift-ci-robot requested review from fabianvf and jmrodri November 12, 2019 16:46

openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 12, 2019

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 12, 2019

djzager force-pushed the ao-monitor branch from 68f076b to df53eee Compare November 12, 2019 17:32

camilamacedo86 added language/ansible Issue is related to an Ansible operator project kind/feature Categorizes issue or PR as related to a new feature. labels Nov 16, 2019

camilamacedo86 suggested changes Nov 21, 2019

View reviewed changes

camilamacedo86 added needs-changes and removed needs-changes labels Nov 21, 2019

camilamacedo86 reviewed Dec 4, 2019

View reviewed changes

pkg/ansible/run.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Dec 4, 2019

View reviewed changes

pkg/ansible/run.go Show resolved Hide resolved

camilamacedo86 added kind/bug Categorizes issue or PR as related to a bug. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Dec 4, 2019

djzager force-pushed the ao-monitor branch from df53eee to de51f2a Compare January 10, 2020 20:20

camilamacedo86 approved these changes Jan 10, 2020

View reviewed changes

openshift-ci-robot assigned camilamacedo86 Jan 10, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2020

camilamacedo86 self-requested a review January 10, 2020 20:29

camilamacedo86 removed the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2020

fabianvf reviewed Jan 20, 2020

View reviewed changes

djzager force-pushed the ao-monitor branch from de51f2a to f4f3845 Compare January 23, 2020 16:39

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 6, 2020

camilamacedo86 reviewed Feb 6, 2020

View reviewed changes

hack/lib/common.sh Outdated Show resolved Hide resolved

djzager force-pushed the ao-monitor branch from bd749c1 to 8170256 Compare February 6, 2020 19:13

camilamacedo86 reviewed Feb 6, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

djzager force-pushed the ao-monitor branch 2 times, most recently from ec08132 to 9423960 Compare February 6, 2020 19:20

camilamacedo86 suggested changes Feb 6, 2020

View reviewed changes

djzager added 5 commits February 6, 2020 15:44

chore(ansible): Create service monitor

eec670a

Ansible based operator's should also create the service monitor as appropriate.

test creation of servicemonitor in ansible

72b4c35

- Add a function to common that applies the servicemonitor CRD - Update the e2e ansible and e2e ansible molecule tests to verify the service monitor is created

Update error handling for serviceMonitor creation

77774b3

Remove molecule updates for now

9f1f5f9

Add servicemonitor test to molecule testing

ddcacf4

djzager force-pushed the ao-monitor branch from 5a31f7e to ddcacf4 Compare February 6, 2020 20:53

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 6, 2020

Fix line length

f1e626d

fabianvf approved these changes Feb 7, 2020

View reviewed changes

openshift-ci-robot assigned fabianvf Feb 7, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2020

camilamacedo86 reviewed Feb 7, 2020

View reviewed changes

camilamacedo86 approved these changes Feb 7, 2020

View reviewed changes

Update changelog entry

53fa750

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2020

fabianvf merged commit 96ce467 into operator-framework:master Feb 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(ansible): Create service monitor #2179

chore(ansible): Create service monitor #2179

djzager commented Nov 12, 2019

openshift-ci-robot commented Nov 12, 2019

djzager commented Nov 12, 2019

jmrodri commented Nov 12, 2019

djzager commented Nov 12, 2019

djzager commented Nov 13, 2019

camilamacedo86 left a comment •

edited

Loading

camilamacedo86 left a comment

djzager commented Jan 17, 2020

fabianvf Jan 20, 2020

djzager Jan 23, 2020

camilamacedo86 Feb 6, 2020 •

edited

Loading

camilamacedo86 Feb 6, 2020

camilamacedo86 Feb 6, 2020

djzager Feb 6, 2020

camilamacedo86 Feb 6, 2020 •

edited

Loading

djzager Feb 6, 2020

djzager Feb 6, 2020

djzager commented Feb 6, 2020

djzager commented Feb 7, 2020

fabianvf left a comment

camilamacedo86 Feb 7, 2020

camilamacedo86 left a comment

camilamacedo86 commented Feb 7, 2020

fabianvf commented Feb 7, 2020

	- Ansible based operators now creates prometheus service monitor, if available. ([#2179](https://github.com/operator-framework/operator-sdk/pull/2179))
	- Add Prometheus metrics support to Ansible - based operators. ([#2179](https://github.com/operator-framework/operator-sdk/pull/2179))

chore(ansible): Create service monitor #2179

chore(ansible): Create service monitor #2179

Conversation

djzager commented Nov 12, 2019

openshift-ci-robot commented Nov 12, 2019

djzager commented Nov 12, 2019

jmrodri commented Nov 12, 2019

djzager commented Nov 12, 2019

djzager commented Nov 13, 2019

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

djzager commented Jan 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Feb 6, 2020 • 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 Feb 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djzager commented Feb 6, 2020

djzager commented Feb 7, 2020

fabianvf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

camilamacedo86 commented Feb 7, 2020

fabianvf commented Feb 7, 2020

camilamacedo86 left a comment •

edited

Loading

camilamacedo86 Feb 6, 2020 •

edited

Loading

camilamacedo86 Feb 6, 2020 •

edited

Loading