Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change grafana configuration to match hosted che configuration #14684

Merged
merged 3 commits into from
Oct 4, 2019
Merged

Change grafana configuration to match hosted che configuration #14684

merged 3 commits into from
Oct 4, 2019

Conversation

tomgeorge
Copy link
Contributor

@tomgeorge tomgeorge commented Sep 26, 2019 •
edited
Loading

What does this PR do?

  • Removes the dashboard provider yaml from grafana-dashboards.yaml
  • Changes the name of grafana-dashboards ConfigMap to che-grafana-dashboards
  • Create a grafana-dashboard-provider ConfigMap, that has a JSON spec (not yaml) defining the grafana dashboard provider, which looks like how hosted Che does it
  • Change che-monitoring.yaml to mount the grafana-dashboard-provider ConfigMap at /etc/grafana/provisioning/dashboards, and to mount the che-grafana-dashboards ConfigMap at /grafana-dashboard-definitions/0

What issues does this PR fix or reference?

#14652

Release Notes

To deploy:

oc apply -f grafana-dashboard-provider.yaml -f grafana-dashboards.yaml -f grafana-datasources.yaml -f prometheus-config.yaml
oc process -f che-monitoring.yaml | oc apply -f -

Docs PR

eclipse-che/che-docs#835

EDIT: Added doc PR link

@che-bot
Copy link
Contributor

che-bot commented Sep 26, 2019

Can one of the admins verify this patch?

1 similar comment
@che-bot
Copy link
Contributor

che-bot commented Sep 26, 2019

Can one of the admins verify this patch?

@tomgeorge
Copy link
Contributor Author

cc @ibuziuk

@che-bot che-bot added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/enhancement A feature request - must adhere to the feature request template. labels Sep 26, 2019
skabashnyuk
skabashnyuk reviewed Sep 27, 2019
View reviewed changes
Copy link
Contributor

@skabashnyuk skabashnyuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configs look good. Can you also add ${OC_BINARY} apply -f ${BASE_DIR}/templates/monitoring/grafana-dashboard-provider.yaml to deploy_che.sh method deployMetrics(){

@tomgeorge
move kind and metadata to the top of the ConfigMap definition, add a …
3620a05 
…step to deploy the grafana dashboard provider

Signed-off-by: Tom George <[email protected]>
ibuziuk
ibuziuk reviewed Sep 27, 2019
View reviewed changes
deploy/openshift/templates/monitoring/che-monitoring.yaml Outdated
@@ -83,8 +83,10 @@ objects:
volumeMounts:
- mountPath: /etc/grafana/provisioning/datasources
name: volume-datasources
- mountPath: /etc/grafana/provisioning/dashboards
- mountPath: /grafana-dashboard-definitions/0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is /grafana-dashboard-definitions/0 expected mountPath ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose that mount path because that is what downstream uses, grafana docs default is in /etc/grafana/provisioning/dashboards, but that is where we mount the grafana-dashboard-provider

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kind of strange. Can you explain why we need that in upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention of Hosted Che is to place the dashboard models in /grafana-dashboard-definitions. /etc/grafana/provisioning/dashboards is taken as a mount point by the dashboard provider configuration. I put this change in upstream to make upstream Che look more like Hosted Che.

Alternatively, I could change the mount path, or place everything in /etc/grafana/provisioning/dashboards using subPath, and tell the provider configuration to look in the subPath for the dashboard models.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in the case of che-monitoring.yaml we should be close to what vanilla Grafana documentation recommends. @ibuziuk wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ibuziuk is on PTO for the next two weeks. We could put the dashboard in e.g. /etc/grafana/provisioning/dashboards/che, but that would be against the goal of being able to run the same configmaps upstream and downstream without changing them.

The mount path of the dashboards IMO is an implementation detail that users shouldn't really have to care about. They should just know that when they deploy the monitoring stack, they will see the dashboards that they are supposed to see.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what we discussed with @ibuziuk the goal was to make grafana-dashboards.yaml as close to downstream as possible. Other configuration should be close to Grafana upstream in my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I made a change to che-monitoring.yaml to reflect this.

@tomgeorge
Copy link
Contributor Author

I think I need to make a docs PR to account for https://www.eclipse.org/che/docs/che-7/monitoring-che/#configuring-and-deploying-grafana

@skabashnyuk
Copy link
Contributor

Change che-monitoring.yaml to mount the grafana-dashboard-provider ConfigMap at /etc/grafana/provisioning/dashboards, and to mount the che-grafana-dashboards ConfigMap at /grafana-dashboard-definitions/0

@tomgeorge can you give more details about the reasoning/motivation of that change?

@tomgeorge
Copy link
Contributor Author

@skabashnyuk I thought that putting the dashboards in /etc/grafana/provisioning/dashboards/che would be in keeping with the upstream grafana convention of placing provisioning configuration in /etc/grafana/dashboards/provisioning

@skabashnyuk
Copy link
Contributor

@tomgeorge do you want to add anything else or we can merge this pr?

@tomgeorge
Copy link
Contributor Author

@skabashnyuk I think this is OK. I added eclipse-che/che-docs#835

@skabashnyuk
Copy link
Contributor

waiting for eclipse-che/che-docs#835 approval

@skabashnyuk
Copy link
Contributor

ci-build

@skabashnyuk skabashnyuk merged commit 6889118 into eclipse-che:master Oct 4, 2019
@che-bot che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Oct 4, 2019
@che-bot che-bot added this to the 7.3.0 milestone Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibuziuk ibuziuk ibuziuk left review comments

@skabashnyuk skabashnyuk skabashnyuk approved these changes

@l0rd l0rd Awaiting requested review from l0rd

@metlos metlos Awaiting requested review from metlos

@nickboldt nickboldt Awaiting requested review from nickboldt

@rhopp rhopp Awaiting requested review from rhopp

@sleshchenko sleshchenko Awaiting requested review from sleshchenko

Assignees
No one assigned
Labels
kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Milestone
7.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@tomgeorge @che-bot @skabashnyuk @ibuziuk