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

mgr/dashboard: monitoring: replace Grafana JSON with Grafonnet based code #42194

Merged
merged 1 commit into from
Aug 11, 2021

Conversation

aaSharma14
Copy link
Contributor

@aaSharma14 aaSharma14 commented Jul 6, 2021

This PR intends to add grafonnet to generate grafana JSON files

Fixes: https://tracker.ceph.com/issues/45184
Signed-off-by: Aashish Sharma aasharma@redhat.com

Before:
Screenshot from 2021-07-15 15-38-13

After:
Screenshot from 2021-07-15 15-38-32

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@aaSharma14 aaSharma14 requested a review from a team as a code owner July 6, 2021 11:07
@aaSharma14 aaSharma14 added this to In progress in Dashboard via automation Jul 6, 2021
@alfonsomthd alfonsomthd moved this from In progress to Review in progress in Dashboard Jul 7, 2021
@aaSharma14 aaSharma14 force-pushed the add-grafonnet-grafana branch 2 times, most recently from 02def87 to 8929802 Compare July 8, 2021 06:06
@p-se
Copy link
Contributor

p-se commented Jul 8, 2021

It already looks like we'll be saving a lot of boilerplate code with this approach. Some of it is due to the data being written differently. In jsonnet there are some attributes on a single line, in JSON they aren't. But, assuming that radosgw-sync-overview.jsonnet in this PR is complete, it reduces the LOC from 440 in radosgw-sync-voerview.json to 66 in jsonnet. Which I think is significant. And we might even be able to reduce that a little bit more by outsourcing grafana.annotation.datasource.

@epuertat epuertat marked this pull request as draft July 9, 2021 08:38
@aaSharma14 aaSharma14 marked this pull request as ready for review July 13, 2021 11:47
@aaSharma14 aaSharma14 changed the title [WIP]mgr/dashboard: monitoring: replace Grafana JSON with Grafonnet based code mgr/dashboard: monitoring: replace Grafana JSON with Grafonnet based code Jul 14, 2021
@aaSharma14 aaSharma14 requested a review from p-se July 14, 2021 08:01
Copy link
Contributor

@p-se p-se left a comment

Choose a reason for hiding this comment

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

I've added a bunch of comments of things that I noticed and might have done differently. Those are not necessarily required to be changed, it's just my opinion.

But there a few things that we need to have a look at:

  • the jsondiff tools seems to be a different one on my system (Ubuntu 20.04) and is not compatible with these changes
  • I'm not sure if it is safe to remove "iteration", but it might be. I hope it is, as it doesn't seem to be possible to keep it, is that correct?
  • a refresh time interval has likely been forgotten to be included
  • the unit field and its value has been removed in two panels. It might be okay, but we'll need to check.

Please consider the rest not as requested changes but as benevolent suggestions.

monitoring/grafana/dashboards/test-jsonnet.sh Outdated Show resolved Hide resolved
monitoring/grafana/dashboards/test-jsonnet.sh Outdated Show resolved Hide resolved
monitoring/grafana/dashboards/test-jsonnet.sh Show resolved Hide resolved
monitoring/grafana/dashboards/test-jsonnet.sh Outdated Show resolved Hide resolved
monitoring/grafana/dashboards/test-jsonnet.sh Outdated Show resolved Hide resolved
monitoring/grafana/dashboards/test-jsonnet.sh Outdated Show resolved Hide resolved
monitoring/grafana/dashboards/test-jsonnet.sh Outdated Show resolved Hide resolved
@aaSharma14
Copy link
Contributor Author

I've added a bunch of comments of things that I noticed and might have done differently. Those are not necessarily required to be changed, it's just my opinion.

But there a few things that we need to have a look at:

  • the jsondiff tools seems to be a different one on my system (Ubuntu 20.04) and is not compatible with these changes
  • I'm not sure if it is safe to remove "iteration", but it might be. I hope it is, as it doesn't seem to be possible to keep it, is that correct?
  • a refresh time interval has likely been forgotten to be included
  • the unit field and its value has been removed in two panels. It might be okay, but we'll need to check.

Please consider the rest not as requested changes but as benevolent suggestions.

Thanks @p-se . I have addressed most of your comments. But as of now I don't see any side-effects of removing iteration and unit properties and also I couldn't yet find a way to add them as well. I have attached the screenshots for the RGW SYNC OVERVIEW grafana panel, before and after this PR. I can do more research on the iteration and unit properties and will update the PR accordingly.

@p-se
Copy link
Contributor

p-se commented Jul 16, 2021

I've added a bunch of comments of things that I noticed and might have done differently. Those are not necessarily required to be changed, it's just my opinion.
But there a few things that we need to have a look at:

  • the jsondiff tools seems to be a different one on my system (Ubuntu 20.04) and is not compatible with these changes
  • I'm not sure if it is safe to remove "iteration", but it might be. I hope it is, as it doesn't seem to be possible to keep it, is that correct?
  • a refresh time interval has likely been forgotten to be included
  • the unit field and its value has been removed in two panels. It might be okay, but we'll need to check.

Please consider the rest not as requested changes but as benevolent suggestions.

Thanks @p-se . I have addressed most of your comments. But as of now I don't see any side-effects of removing iteration and unit properties and also I couldn't yet find a way to add them as well. I have attached the screenshots for the RGW SYNC OVERVIEW grafana panel, before and after this PR. I can do more research on the iteration and unit properties and will update the PR accordingly.

Yeah, might be the case that we don't need to worry about those.

Have you found anything out about the jsondiff issue? Is that due to different versions of jsondiff or do I happen to have a different application installed? I'm asking because I'm not even how to resolve that issue for me, but I also wonder if we need to add another dependency to install-dep.sh or not.

@aaSharma14
Copy link
Contributor Author

aaSharma14 commented Jul 21, 2021

I've added a bunch of comments of things that I noticed and might have done differently. Those are not necessarily required to be changed, it's just my opinion.
But there a few things that we need to have a look at:

  • the jsondiff tools seems to be a different one on my system (Ubuntu 20.04) and is not compatible with these changes
  • I'm not sure if it is safe to remove "iteration", but it might be. I hope it is, as it doesn't seem to be possible to keep it, is that correct?
  • a refresh time interval has likely been forgotten to be included
  • the unit field and its value has been removed in two panels. It might be okay, but we'll need to check.

Please consider the rest not as requested changes but as benevolent suggestions.

Thanks @p-se . I have addressed most of your comments. But as of now I don't see any side-effects of removing iteration and unit properties and also I couldn't yet find a way to add them as well. I have attached the screenshots for the RGW SYNC OVERVIEW grafana panel, before and after this PR. I can do more research on the iteration and unit properties and will update the PR accordingly.

Yeah, might be the case that we don't need to worry about those.

Have you found anything out about the jsondiff issue? Is that due to different versions of jsondiff or do I happen to have a different application installed? I'm asking because I'm not even how to resolve that issue for me, but I also wonder if we need to add another dependency to install-dep.sh or not.

I have updated the jsondiff command and removed -s and -i flags which didn't seem to be compatible for some distros. Are you still facing the same issue with jsondiff?

I've added a bunch of comments of things that I noticed and might have done differently. Those are not necessarily required to be changed, it's just my opinion.
But there a few things that we need to have a look at:

  • the jsondiff tools seems to be a different one on my system (Ubuntu 20.04) and is not compatible with these changes
  • I'm not sure if it is safe to remove "iteration", but it might be. I hope it is, as it doesn't seem to be possible to keep it, is that correct?
  • a refresh time interval has likely been forgotten to be included
  • the unit field and its value has been removed in two panels. It might be okay, but we'll need to check.

Please consider the rest not as requested changes but as benevolent suggestions.

Thanks @p-se . I have addressed most of your comments. But as of now I don't see any side-effects of removing iteration and unit properties and also I couldn't yet find a way to add them as well. I have attached the screenshots for the RGW SYNC OVERVIEW grafana panel, before and after this PR. I can do more research on the iteration and unit properties and will update the PR accordingly.

Yeah, might be the case that we don't need to worry about those.

Have you found anything out about the jsondiff issue? Is that due to different versions of jsondiff or do I happen to have a different application installed? I'm asking because I'm not even how to resolve that issue for me, but I also wonder if we need to add another dependency to install-dep.sh or not.

@p-se I have added the jsondiff dependency in requirements.txt and install-deps.sh file. Please let me know if it works fine for you now.

ideepika pushed a commit to ideepika/ceph that referenced this pull request Aug 12, 2021
…et-grafana"

This reverts commit afadfed, reversing
changes made to 6f8bdfb.
ideepika pushed a commit to ideepika/ceph that referenced this pull request Aug 12, 2021
…et-grafana"

This reverts commit afadfed, reversing
changes made to 6f8bdfb.

Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
@ideepika
Copy link
Member

#42765

aaSharma14 pushed a commit to rhcs-dashboard/ceph that referenced this pull request Aug 12, 2021
Signed-off-by: Aashish Sharma <aasharma@redhat.com>
@epuertat
Copy link
Member

@epuertat @aaSharma14 this change broke the build

Processing files: ceph-grafana-dashboards-17.0.0-6884.g034de1f5.el8.noarch
error: Directory not found: /home/jenkins-build/build/workspace/ceph-dev-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/17.0.0-6884-g034de1f5/rpm/el8/BUILDROOT/ceph-17.0.0-6884.g034de1f5.el8.x86_64/etc/grafana/dashboards/ceph-dashboard
error: File not found: /home/jenkins-build/build/workspace/ceph-dev-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/17.0.0-6884-g034de1f5/rpm/el8/BUILDROOT/ceph-17.0.0-6884.g034de1f5.el8.x86_64/etc/grafana/dashboards/ceph-dashboard/*
Executing(%doc): /bin/sh -e /var/tmp/rpm-tmp.EAiLqt
+ umask 022
+ cd /home/jenkins-build/build/workspace/ceph-dev-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/17.0.0-6884-g034de1f5/rpm/el8/BUILD
+ cd ceph-17.0.0-6884-g034de1f5
+ DOCDIR=/home/jenkins-build/build/workspace/ceph-dev-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/17.0.0-6884-g034de1f5/rpm/el8/BUILDROOT/ceph-17.0.0-6884.g034de1f5.el8.x86_64/usr/share/doc/ceph-grafana-dashboards
+ export LC_ALL=C
+ LC_ALL=C
+ export DOCDIR
+ /usr/bin/mkdir -p /home/jenkins-build/build/workspace/ceph-dev-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/17.0.0-6884-g034de1f5/rpm/el8/BUILDROOT/ceph-17.0.0-6884.g034de1f5.el8.x86_64/usr/share/doc/ceph-grafana-dashboards
+ cp -pr monitoring/grafana/dashboards/README /home/jenkins-build/build/workspace/ceph-dev-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/17.0.0-6884-g034de1f5/rpm/el8/BUILDROOT/ceph-17.0.0-6884.g034de1f5.el8.x86_64/usr/share/doc/ceph-grafana-dashboards
+ cp -pr monitoring/grafana/README.md /home/jenkins-build/build/workspace/ceph-dev-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/17.0.0-6884-g034de1f5/rpm/el8/BUILDROOT/ceph-17.0.0-6884.g034de1f5.el8.x86_64/usr/share/doc/ceph-grafana-dashboards
+ exit 0


RPM build errors:
    Directory not found: /home/jenkins-build/build/workspace/ceph-dev-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/17.0.0-6884-g034de1f5/rpm/el8/BUILDROOT/ceph-17.0.0-6884.g034de1f5.el8.x86_64/etc/grafana/dashboards/ceph-dashboard
    File not found: /home/jenkins-build/build/workspace/ceph-dev-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/17.0.0-6884-g034de1f5/rpm/el8/BUILDROOT/ceph-17.0.0-6884.g034de1f5.el8.x86_64/etc/grafana/dashboards/ceph-dashboard/*
+ rm -fr /tmp/install-deps.891037
Build step 'Execute shell' marked build as failure

see jenkins.ceph.com/job/ceph-dev-build/ARCH=x86_64,AVAILABLE_ARCH=x86_64,AVAILABLE_DIST=centos8,DIST=centos8,MACHINE_SIZE=gigantic/48127//consoleFull

@tchaikov @ideepika as discussed in the chat, @aaSharma14 is reverting a small subset of changes (installing Grafana json files in the CMakeLists.txt) to see if that's the reason for the failure.

aaSharma14 pushed a commit to rhcs-dashboard/ceph that referenced this pull request Aug 12, 2021
Fixes:https://tracker.ceph.com/issues/52238
Signed-off-by: Aashish Sharma <aasharma@redhat.com>
aaSharma14 pushed a commit to rhcs-dashboard/ceph that referenced this pull request Aug 12, 2021
This PR tends to fix the issue caused by ceph#42194

Fixes:https://tracker.ceph.com/issues/52238
Signed-off-by: Aashish Sharma <aasharma@redhat.com>
aaSharma14 pushed a commit to rhcs-dashboard/ceph that referenced this pull request Aug 12, 2021
This PR tends to fix the issue caused by ceph#42194

Fixes:https://tracker.ceph.com/issues/52238
Signed-off-by: Aashish Sharma <aasharma@redhat.com>
aaSharma14 pushed a commit to aaSharma14/ceph that referenced this pull request Aug 17, 2021
This PR tends to fix the issue caused by ceph#42194

Fixes:https://tracker.ceph.com/issues/52238
Signed-off-by: Aashish Sharma <aasharma@redhat.com>
(cherry picked from commit 4907c78)

Conflicts:
	ceph.spec.in (added jsonnet package)
	monitoring/grafana/dashboards/CMakeLists.txt (added grafana jsons code)
aaSharma14 pushed a commit to aaSharma14/ceph that referenced this pull request Aug 17, 2021
This PR tends to fix the issue caused by ceph#42194

Fixes:https://tracker.ceph.com/issues/52238
Signed-off-by: Aashish Sharma <aasharma@redhat.com>
(cherry picked from commit 4907c78)

Conflicts:
	ceph.spec.in (added jsonnet package)
	monitoring/grafana/dashboards/CMakeLists.txt (added grafana jsons code)
(cherry picked from commit 570160e)
aaSharma14 pushed a commit to aaSharma14/ceph that referenced this pull request Aug 17, 2021
This PR tends to fix the issue caused by ceph#42194

Fixes:https://tracker.ceph.com/issues/52238
Signed-off-by: Aashish Sharma <aasharma@redhat.com>
(cherry picked from commit 4907c78)

Conflicts:
	ceph.spec.in (added jsonnet package)
	monitoring/grafana/dashboards/CMakeLists.txt (added grafana jsons code)
(cherry picked from commit 570160e)
@smithfarm
Copy link
Contributor

@epuertat @aaSharma14 This PR causes the build to reach out onto the Internet and attempt to download a git repo from github. In environments like the openSUSE Build Service where the build machine does not have a network connection, that completely breaks the build.

Since we already use the git submodule feature for including code from other git repos, I wanted to ask whether this external git project could be brought in that way, instead of trying (and failing) to download it at build time?

@smithfarm
Copy link
Contributor

smithfarm commented Aug 19, 2021

Also, whatever the fix is, it will need to be backported to pacific, because this PR has already been backported to (and merged into) pacific via #42812 :-/

@p-se
Copy link
Contributor

p-se commented Aug 19, 2021

Since the compiled Grafana dashboards are included in the repository anyway and the part of the build process that requires a working internet connection seems to be a test only (provided that is correct), would it be viable to disable WITH_GRAFANA by default? Changes to those files would still be tested by make check when added to the repository but wouldn't require a working internet connection to be build.

@smithfarm
Copy link
Contributor

would it be viable to disable WITH_GRAFANA by default

Hm, wouldn't that break the build of the ceph-grafana-dashboards subpackage, though? See 5debf36b378b

@smithfarm
Copy link
Contributor

smithfarm commented Aug 19, 2021

In general, I believe any cmake code that introduces ExternalProject_Add should be guarded appropriately to avoid breaking the standard DEB, RPM builds on machines that aren't connected to the Internet.

There are two ways to download code from the Internet for use in Ceph:

  1. git submodule
  2. make-dist

The latter (make-dist) is a script in the top-level directory of the Ceph git repo. The purpose of this script is to generate the tarball from which the DEB,RPM packages are built.

@p-se
Copy link
Contributor

p-se commented Aug 19, 2021

would it be viable to disable WITH_GRAFANA by default

Hm, wouldn't that break the build of the ceph-grafana-dashboards subpackage, though? See 5debf36

Indeed, it does look so: https://github.com/ceph/ceph/blob/master/CMakeLists.txt#L685-L688

It is quite interesting that there's a switch that seemingly (unless I do not fully understand it) does not work properly when turned off.

[ 3098s] Processing files: ceph-grafana-dashboards-16.2.5-371.g601a4407a67.noarch
[ 3098s] error: Directory not found: /home/abuild/rpmbuild/BUILDROOT/ceph-16.2.5-371.g601a4407a67.x86_64/etc/grafana
[ 3098s] error: Directory not found: /home/abuild/rpmbuild/BUILDROOT/ceph-16.2.5-371.g601a4407a67.x86_64/etc/grafana/dashboards
[ 3098s] error: Directory not found: /home/abuild/rpmbuild/BUILDROOT/ceph-16.2.5-371.g601a4407a67.x86_64/etc/grafana/dashboards/ceph-dashboard
[ 3098s] error: File not found: /home/abuild/rpmbuild/BUILDROOT/ceph-16.2.5-371.g601a4407a67.x86_64/etc/grafana/dashboards/ceph-dashboard/*

Next idea would be to change WITH_GRAFANA (only the occurrences of it in this PR) to WITH_GRAFANA_TESTS or similar and only use that for the make check but, by default, leave it out for building.

The other options, I think, are the onces you've already mentioned, @smithfarm.

@smithfarm
Copy link
Contributor

Next idea would be to change WITH_GRAFANA (only the occurrences of it in this PR) to
WITH_GRAFANA_TESTS or similar and only use that for the make check but, by default,
leave it out for building.

Yes. Or, just in general, guard any cmake ExternalProject_Add directives appropriately to avoid breaking the standard DEB, RPM builds on machines that aren't connected to the Internet.

aaSharma14 pushed a commit to rhcs-dashboard/ceph that referenced this pull request Aug 19, 2021
This PR intends to fix the error caused by ceph#42194

Signed-off-by: Aashish Sharma <aasharma@redhat.com>
aaSharma14 pushed a commit to rhcs-dashboard/ceph that referenced this pull request Aug 19, 2021
This PR intends to fix the error caused by ceph#42194

Signed-off-by: Aashish Sharma <aasharma@redhat.com>
aaSharma14 pushed a commit to rhcs-dashboard/ceph that referenced this pull request Aug 19, 2021
This PR intends to fix the error caused by ceph#42194

Signed-off-by: Aashish Sharma <aasharma@redhat.com>
aaSharma14 pushed a commit to rhcs-dashboard/ceph that referenced this pull request Aug 20, 2021
This PR intends to fix the error caused by ceph#42194

Fixes:https://tracker.ceph.com/issues/52338
Signed-off-by: Aashish Sharma <aasharma@redhat.com>
aaSharma14 pushed a commit to rhcs-dashboard/ceph that referenced this pull request Aug 20, 2021
This PR intends to fix the error caused by ceph#42194

Fixes:https://tracker.ceph.com/issues/52338
Signed-off-by: Aashish Sharma <aasharma@redhat.com>
aaSharma14 pushed a commit to rhcs-dashboard/ceph that referenced this pull request Aug 20, 2021
This PR intends to fix the error caused by ceph#42194

Fixes:https://tracker.ceph.com/issues/52338
Signed-off-by: Aashish Sharma <aasharma@redhat.com>
aaSharma14 pushed a commit to rhcs-dashboard/ceph that referenced this pull request Aug 25, 2021
This PR tends to fix the issue caused by ceph#42194

Fixes:https://tracker.ceph.com/issues/52238
Signed-off-by: Aashish Sharma <aasharma@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Dashboard
  
Done
10 participants