operator: Allow disabling Grafana deployment #1241

bison · 2021-06-23T11:15:53Z

This exposes a config option in the ConfigMap that allows disabling the Grafana deployment.

I added CHANGELOG entry for this change.
No user facing changes, so no entry in CHANGELOG was needed.

Summary of the changes:

Added Enabled field to GrafanaConfig struct.
The Grafana task is split into create() and destroy() methods.
The htpasswd related bits (volume and container argument) are removed from jsonnet.
The operator injects these at runtime if Grafana is enabled.
The shared config task doesn't inject URLs if they are nil.
Add some basic unit and e2e tests.

fpetkovski · 2021-06-29T12:05:36Z

pkg/manifests/config_test.go

@@ -186,3 +186,27 @@ func TestEtcdDefaultsToDisabled(t *testing.T) {
 		t.Error("an empty etcd configuration should have etcd disabled")
 	}
 }
+
+func TestGrafanaDefaultsToEnabled(t *testing.T) {


It would be nice to convert this into a table driven test to make sure each test case is run independently of each other. Here is one such example: https://github.com/openshift/cluster-monitoring-operator/blob/master/pkg/manifests/manifests_test.go#L60

ACK. I also prefer that. Honestly, I just copied the extremely similar test above this and changed it to test Grafana. I can try to find time to make it table driven.

bison · 2021-06-29T14:21:51Z

Quick note: There was some offline discussion about the fact that we generally try to keep the jsonnet / assets as close as possible to the default deployment. In this case that would require either needlessly enabling the static password auth when Grafana is disabled, or inverting the logic in manifests.go to remove things when Grafana is disabled instead of adding them when it is enabled. That is tricky in this case because these things are often buried in nested arrays. IMHO with the comments in the jsonnet, the original approach of adding the things at runtime is better.

simonpasquier

Just 2 minor comments but nothing blocking. Very nice job!

simonpasquier · 2021-06-29T16:07:45Z

pkg/manifests/manifests.go

+		})
+
+		// Add the htpasswd arg and volume mount to the proxy container.
+		for i := range p.Spec.Containers {


it might be clearer to move it to the range loop above where we alreayd tweak the prometheus-proxy container?

Happy to do that. I don't have a strong opinion. I guess I could actually move all of this there to keep it all together. Unless it's too weird to have the volume bit there as well.

simonpasquier · 2021-06-29T16:08:24Z

pkg/manifests/manifests.go

+		})
+
+		// Add the htpasswd arg and volume mount to the proxy container.
+		for i := range d.Spec.Template.Spec.Containers {


same remark here

Done here as well.

fpetkovski · 2021-06-30T07:09:00Z

test/e2e/config_test.go

+	assertOperatorCondition(t, configv1.OperatorAvailable, configv1.ConditionTrue)
+
+	// Push a default configuration that re-enables Grafana.
+	validCM.Data["config.yaml"] = "enableUserWorkload: true"


How does enabling user workload monitoring also enable grafana?

It doesn't. That was just setting the config back to what it was originally, i.e. removing the previously added Grafana bit. It was definitely confusing though. The new test is different.

fpetkovski · 2021-06-30T07:09:42Z

test/e2e/config_test.go

@@ -92,6 +94,50 @@ func TestClusterMonitoringOperatorConfiguration(t *testing.T) {
 	t.Log("asserting that CMO goes back healthy after the configuration is fixed")
 	assertOperatorCondition(t, configv1.OperatorDegraded, configv1.ConditionFalse)
 	assertOperatorCondition(t, configv1.OperatorAvailable, configv1.ConditionTrue)
+
+	// Push a configuration that disables Grafana.


I would extract this addition into a new independent test

paulfantom · 2021-06-30T11:16:25Z

pkg/manifests/manifests.go

+	data := map[string]string{}
+
+	// Configmap keys need to include "public" to indicate that they are public values.
+	// See https://bugzilla.redhat.com/show_bug.cgi?id=1807100.
+	if promHost != nil {
+		data["prometheusPublicURL"] = promHost.String()
+	}
+
+	if amHost != nil {
+		data["alertmanagerPublicURL"] = amHost.String()
+	}
+
+	if grafanaHost != nil {
+		data["grafanaPublicURL"] = grafanaHost.String()
+	}
+
+	if thanosHost != nil {
+		data["thanosPublicURL"] = thanosHost.String()
+	}
+


Note: This will be removed in #1223

fpetkovski · 2021-06-30T12:51:50Z

/lgtm but would be nice to get another review

/hold

simonpasquier

/lgtm
I've checked offline with @sichvoge who's fine with the field as exposed in the configmap.

fpetkovski · 2021-07-02T09:25:01Z

/unhold

This adds an option to the CMO ConfigMap that allows disabling the Grafana deployment. The actual removal of deployed Grafana resources is not yet implemented.

This implements the destroy method for the Grafana task. It deletes all the resources created by the create method in the reverse order.

bison · 2021-07-05T10:10:53Z

@simonpasquier @fpetkovski: I had to rebase this. Can you have another look when you have a chance?

simonpasquier

/lgtm

openshift-ci · 2021-07-05T10:34:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bison, fpetkovski, simonpasquier

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [bison,fpetkovski,simonpasquier]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2021-07-05T13:02:03Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-07-05T13:14:01Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-07-05T14:15:13Z

@bison: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-single-node	`91de7fb`	link	`/test e2e-aws-single-node`

Full PR test history. Your PR dashboard.

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.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 23, 2021

openshift-ci bot requested review from fpetkovski and prashbnair June 23, 2021 11:16

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2021

bison force-pushed the disable-grafana branch 2 times, most recently from 4ec99aa to b9ede49 Compare June 23, 2021 12:03

bison changed the title ~~WIP: operator: Allow disabling Grafana deployment~~ operator: Allow disabling Grafana deployment Jun 23, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 23, 2021

bison force-pushed the disable-grafana branch from b9ede49 to 3f7db03 Compare June 23, 2021 12:39

bison changed the title ~~operator: Allow disabling Grafana deployment~~ WIP: operator: Allow disabling Grafana deployment Jun 23, 2021

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 23, 2021

bison force-pushed the disable-grafana branch 3 times, most recently from 612e227 to f747f70 Compare June 29, 2021 11:51

bison changed the title ~~WIP: operator: Allow disabling Grafana deployment~~ operator: Allow disabling Grafana deployment Jun 29, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 29, 2021

fpetkovski reviewed Jun 29, 2021

View reviewed changes

bison force-pushed the disable-grafana branch from f747f70 to 03942c1 Compare June 29, 2021 14:02

bison force-pushed the disable-grafana branch 2 times, most recently from 469bd81 to 605592e Compare June 29, 2021 15:17

simonpasquier reviewed Jun 29, 2021

View reviewed changes

fpetkovski reviewed Jun 30, 2021

View reviewed changes

bison force-pushed the disable-grafana branch from 605592e to 6581f71 Compare June 30, 2021 11:14

paulfantom reviewed Jun 30, 2021

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 30, 2021

simonpasquier reviewed Jul 2, 2021

View reviewed changes

openshift-ci bot assigned simonpasquier Jul 2, 2021

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 2, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 2, 2021

bison added 4 commits July 5, 2021 12:06

operator: Allow disabling Grafana deployment

e97ecb8

This adds an option to the CMO ConfigMap that allows disabling the Grafana deployment. The actual removal of deployed Grafana resources is not yet implemented.

tasks: Implement destroy method for Grafana

ec2d198

This implements the destroy method for the Grafana task. It deletes all the resources created by the create method in the reverse order.

test/e2e: Add test for config option to disable Grafana

2fee744

Update CHANGELOG.md

91de7fb

bison force-pushed the disable-grafana branch from 6581f71 to 91de7fb Compare July 5, 2021 10:08

openshift-ci bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 5, 2021

simonpasquier reviewed Jul 5, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2021

openshift-merge-robot merged commit 24a43fb into openshift:master Jul 5, 2021

bison deleted the disable-grafana branch July 5, 2021 17:38

bison mentioned this pull request Aug 2, 2021

MON-1801: Document disabling Grafana in monitoring stack openshift/openshift-docs#35072

Merged

simonpasquier mentioned this pull request Sep 8, 2021

OCP 4.9 Release Notes Tracker openshift/openshift-docs#33497

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

operator: Allow disabling Grafana deployment #1241

operator: Allow disabling Grafana deployment #1241

bison commented Jun 23, 2021 •

edited

Loading

fpetkovski Jun 29, 2021

bison Jun 29, 2021

bison Jun 29, 2021

bison commented Jun 29, 2021

simonpasquier left a comment

simonpasquier Jun 29, 2021

bison Jun 29, 2021

bison Jun 30, 2021

simonpasquier Jun 29, 2021

bison Jun 30, 2021

fpetkovski Jun 30, 2021

bison Jun 30, 2021

fpetkovski Jun 30, 2021

bison Jun 30, 2021

paulfantom Jun 30, 2021

fpetkovski commented Jun 30, 2021

simonpasquier left a comment

fpetkovski commented Jul 2, 2021

bison commented Jul 5, 2021

simonpasquier left a comment

openshift-ci bot commented Jul 5, 2021

openshift-bot commented Jul 5, 2021

openshift-bot commented Jul 5, 2021

openshift-ci bot commented Jul 5, 2021 •

edited

Loading

operator: Allow disabling Grafana deployment #1241

operator: Allow disabling Grafana deployment #1241

Conversation

bison commented Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bison commented Jun 29, 2021

simonpasquier left a 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

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

fpetkovski commented Jun 30, 2021

simonpasquier left a comment

Choose a reason for hiding this comment

fpetkovski commented Jul 2, 2021

bison commented Jul 5, 2021

simonpasquier left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 5, 2021

openshift-bot commented Jul 5, 2021

openshift-bot commented Jul 5, 2021

openshift-ci bot commented Jul 5, 2021 • edited Loading

bison commented Jun 23, 2021 •

edited

Loading

openshift-ci bot commented Jul 5, 2021 •

edited

Loading