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.

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

Grafana dashboard w/ OBS packaging #160

Merged
merged 20 commits into from
Jun 15, 2020

Conversation

stefanotorresi
Copy link
Member

@stefanotorresi stefanotorresi commented Jun 12, 2020

This PR bundles the Grafana dashboards in this repository and introduce the OBS packaging to install those on top of Grafana.

SR to OBS devel upstream here: https://build.opensuse.org/request/show/815701

Copy link

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

@stefanotorresi I'm a bit lost with the latest discussions and actually I didn't know that the plan was to add the dashboards here. Anyway, if this was the chosen thing It's totally fine for me.

Some comments:

  • I miss some documentation in the main README saying that this projects stores grafana dashboards for this exporter. Mayne just add link from the main README to the dashboards README, and we can continue adding more data in the future. But it would be nice to know this when you enter to the project main documentation page.
  • I see that requiring grafana in the spec file is logical, but this might create issues during the installation as grafana is provided by SUMA and not SLES4SAP. Just to have this in mind. I guess that this was already discussed and I lost the thread XD
  • How is the versioning for the dashboards and the exporter? Do they use the same version? I cannot clearly understand this in the new code hehe If they use the same, we would have strange things like packages upgrading the version without any change, what most probably won't be liked by the packaging/maintenance teams

Source: %{name}-%{version}.tar.gz
BuildArch: noarch
Requires: grafana
BuildRequires: grafana

Choose a reason for hiding this comment

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

Is this required? I mean, to put some files in some folders during the build maybe grafana is not needed. If we can avoid it better hehe You know how the dependencies during the build time can be quite troubling

Copy link
Member Author

Choose a reason for hiding this comment

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

I initially tried to do without it, but we require the whole directory structure put in place by the Grafana package.
I guess I could duplicate some of the spec statements, but then what defines what package owns what path? Wouldn't we have conflicts?

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 is ok to require grafana, BuildRequires maybe we can spare it

Copy link
Member Author

Choose a reason for hiding this comment

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

the build will fail without that BuildRequire with the following error:

[   28s] grafana-ha-cluster-dashboards-1.0.0+git.1591970022.e33fbe9-1.1.noarch.rpm: directories not owned by a package:
[   28s]  - /etc/grafana
[   28s]  - /etc/grafana/provisioning
[   28s]  - /etc/grafana/provisioning/dashboards
[   28s]  - /var/lib/grafana
[   28s]  - /var/lib/grafana/dashboards

@@ -0,0 +1,3 @@
# Grafana dashboards

This directory contains the Grafana dashboards that leverage this exporter, along with a provider configuration file.

Choose a reason for hiding this comment

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

It would be nice to explain how to install the dashboards. A simple Installation chapter. We can add it in the future though

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this document is very much provisional, I even still have to decide if this should go in the project docs directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

@stefanotorresi I would reference the dashboard in our top-level readme.

To install a grafana dashboard is pretty straightforward imho, we just say import the dashboard. and we can reference upstream.

But we need to make it visible on the top-level readme so an user know there is a dashboard

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, I'm working on the docs right now!

Copy link
Contributor

@MalloZup MalloZup left a comment

Choose a reason for hiding this comment

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

do we need an additional package? maybe we can just include this 3 files on the exporter package without having another package to maintain.
What are the benifits of splitting it so small package?

@stefanotorresi
Copy link
Member Author

stefanotorresi commented Jun 12, 2020

do we need an additional package? maybe we can just include this 3 files on the exporter package without having another package to maintain.
What are the benifits of splitting it so small package?

we don't install the exporter and the dashboards in the same system! 🤪

@stefanotorresi
Copy link
Member Author

stefanotorresi commented Jun 12, 2020

@arbulu89

  1. Yeah, the documentation surely needs improvements. I wanted to first experiment with the rpm, I will see to improve it at a later stage, along with the CI.
  2. Yes, we require Grafana, which is provided by SUMA. This package wouldn't have any sense without Grafana installed. As long as Grafana will be delivered by SUMA, this package will only be available to customers buying bundled licenses of both SUMA and SLEHA.
  3. For the time being the versioning scheme is handled manually by changing the _service file. The only automated part is the +git.%timestamp.%hash suffix. Versioning will always be separate, as each release and commit regarding the exporter doesn't necessarily mean an update of the dashboards. This will be handled by reworking the CI a bit, in another PR.

Copy link
Contributor

@MalloZup MalloZup left a comment

Choose a reason for hiding this comment

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

do we need the screenshot.png? it might be depracted dashboards/screenshot.png

osc checkout $(OBS_PROJECT) $(OBS_PACKAGE) -o build/obs
rm -f build/obs/*.tar.gz
cp -rv packaging/obs/* build/obs/
exporter-obs-workdir:
Copy link
Contributor

Choose a reason for hiding this comment

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

what is used for the obs-workdir?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just forget the term

@stefanotorresi
Copy link
Member Author

do we need the screenshot.png? it might be depracted dashboards/screenshot.png

yes, the plan is to show what the dashboard looks like in the markdown docs.

Copy link

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

@stefanotorresi Thanks for adding the new info for the docs.
After this, at least from my side everything is really nice

Copy link
Contributor

@MalloZup MalloZup left a comment

Choose a reason for hiding this comment

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

lgtm to me

@MalloZup MalloZup merged commit 944fb45 into ClusterLabs:master Jun 15, 2020
@stefanotorresi stefanotorresi deleted the feature/grafana-dashboard branch June 15, 2020 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants