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

Add SAP and HA dashboards to grafana formula #67

Merged
merged 8 commits into from
Oct 6, 2020

Conversation

MalloZup
Copy link
Contributor

@MalloZup MalloZup commented Aug 4, 2020

desc:

This pr add sap and ha cluster dashboards maintained by SAP/ha team

I have followed the same pattern you have on the grafana-formula
note that I didn't tested this.

Consideration/Request for comments:

Since we package already our dashboards, putting the json files here would be somehow not the cleanest solution since we would need to maintain json here AND in the source code of the original dashboards. ( so more then 2 places)

On the other side, I don't remember if the SUSE-Manager server has access to the SLES repository for installing the RPM packages but I guess yes and that this approach should be ok.

let me know

Copy link

@stefanotorresi stefanotorresi left a comment

Choose a reason for hiding this comment

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

looks good, but I'm not sure this works as it is without configuring packagehub, since the dashboards are not in any SLE channel yet, they're only in openSUSE:Factory.

grafana-formula/grafana/init.sls Outdated Show resolved Hide resolved
@MalloZup
Copy link
Contributor Author

MalloZup commented Aug 7, 2020

@renner @cavalheiro @witekest what is your pov on this?

ju SLE15 submissions are done for the PKGs. can suma-server install such server or how can we install the dash? tia

Copy link
Member

@renner renner left a comment

Choose a reason for hiding this comment

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

It looks good, AFAIK we ship and support grafana to be installed with this Formula for SLE12 and 15 with the respective SUSE Manager client tools channels (SLE12 and SLE15). I think for consistency we should either package all the dashboards as RPMs and ship those packages with the same client tools as grafana itself or include all the json files with this Formula, including yours.

grafana-formula/metadata/form.yml Outdated Show resolved Hide resolved
grafana-formula/metadata/form.yml Outdated Show resolved Hide resolved
@MalloZup MalloZup requested a review from renner August 7, 2020 13:27
@MalloZup
Copy link
Contributor Author

@juliogonzalez cc

@juliogonzalez
Copy link
Member

On the other side, I don't remember if the SUSE-Manager server has access to the SLES repository for installing the RPM packages but I guess yes and that this approach should be ok.

It all the depends, on what channel(s) is this package?

@MalloZup
Copy link
Contributor Author

@juliogonzalez it is on SLES codebase. Update sent to you on rocketchat

@juliogonzalez
Copy link
Member

Uh-oh, I am afraid we are going to have a problem here.

SUSE manager its own grafana-formula on the SUSE Manager codestream: https://build.suse.de/package/show/SUSE:SLE-15-SP2:Update:Products:Manager41/grafana-formula

We can of course take it from SLES codestream as we are lucky and the version there is higher. But then @witekest needs to be aware that the maintenance will happen there by branching the package. And before doing that, I'd like to have his OK in case there's something I don't have in mind right now.

Do we really need this for 4.1.1 or can it wait for 4.1.2? I'd really like to avoid yet another last-minute-change, as this was not even at https://github.com/SUSE/spacewalk/issues/11942 or https://jira.suse.com/browse/ECO-2237 on the list of packages part of the ECO.

@renner
Copy link
Member

renner commented Aug 11, 2020

Uh-oh, I am afraid we are going to have a problem here.

SUSE manager its own grafana-formula on the SUSE Manager codestream: https://build.suse.de/package/show/SUSE:SLE-15-SP2:Update:Products:Manager41/grafana-formula

We can of course take it from SLES codestream as we are lucky and the version there is higher. But then @witekest needs to be aware that the maintenance will happen there by branching the package. And before doing that, I'd like to have his OK in case there's something I don't have in mind right now.

As I understood, Dario was referring to the packages that would be installed with the grafana-formula after this patch (rather than grafana-formula itself):

  • grafana-ha-cluster-dashboards
  • grafana-sap-hana-dashboards
  • grafana-sap-netweaver-dashboards

He submitted those to the SLES 15 channels, so in the current setup this part of the formula works only for systems that have those channels assigned. As mentioned before, I think a more consistent approach would be to either ship those dashboards as json files like the other ones, or provide all dashboards as packages with the same channels as we are providing Grafana itself, i.e. the client tools channels for SLE15 and SLE12.

Do we really need this for 4.1.1 or can it wait for 4.1.2? I'd really like to avoid yet another last-minute-change, as this was not even at SUSE/spacewalk#11942 or https://jira.suse.com/browse/ECO-2237 on the list of packages part of the ECO.

I don't think it's required for 4.1.1, we should probably aim for releasing this with 4.1.2 and try to get the mentioned packaging / linking done in order to support all platforms where we are support the provisioning of Grafana with this formula?

@juliogonzalez
Copy link
Member

juliogonzalez commented Aug 11, 2020

I don't think it's required for 4.1.1, we should probably aim for releasing this with 4.1.2 and try to get the mentioned packaging / linking done in order to support all platforms where we are support the provisioning of Grafana with this formula?

I guess the most sensible solution is submitting this update of grafana-formula to the SLE15 codestreams, and having SUSE Manager taking it from there with 4.1.2 (via product definition for devel, or via channel definitions for the release) :-)

@renner
Copy link
Member

renner commented Aug 11, 2020

I don't think it's required for 4.1.1, we should probably aim for releasing this with 4.1.2 and try to get the mentioned packaging / linking done in order to support all platforms where we are support the provisioning of Grafana with this formula?

I guess the most sensible solution is submitting this update of grafana-formula to the SLE15 codestreams, and having SUSE Manager taking it from there with 4.1.2 (via product definition for devel, or via channel definitions for the release) :-)

Julio, grafana-formula is a package that we are shipping with the SUSE Manager Server channels to make it appear in the UI. This should not change, we would just update the package with this patch for 4.1.2. The formula contains states for provisioning Grafana itself and dashboards on managed systems (that can be either SLE12 or SLE15). Therefore our client tools contain the grafana package and I guess we should be shipping those new packaged dashboards as well with those client tools? So we would need to somehow bring those packages that Dario submitted to SLE15 to our SLE12 and 15 client tools.

@juliogonzalez
Copy link
Member

Julio, grafana-formula is a package that we are shipping with the SUSE Manager Server channels to make it appear in the UI. This should not change, we would just update the package with this patch for 4.1.2. The formula contains states for provisioning Grafana itself and dashboards on managed systems (that can be either SLE12 or SLE15). Therefore our client tools contain the grafana package and I guess we should be shipping those new packaged dashboards as well with those client tools? So we would need to somehow bring those packages that Dario submitted to SLE15 to our SLE12 and 15 client tools.

Let me clarify. We have grafana in our client tools project (devel) because we maintain grafana. But we submit it to the SLE12 and SLE15 codestreams.

Now the dashboards are a different story, as they it seems they are not maintained by us, IIUC. Therefore we should add them to the SLE15 client tools product definitions and SLE15 client tools channel definitions, but there's no need to to link them or copypac them to our devel projects.

Now the question is what happens about other OS: Who is going to maintain the packages for those OS? Us or dario/sap? And where are we going to maintain them? At the OS codestreams so other projects can use them, or at the client tools codestreams (only available as separate codestreams for RES8 and Ubuntu20?

@MalloZup
Copy link
Contributor Author

MalloZup commented Aug 11, 2020

@juliogonzalez so we are going to maintain the grafana-dashboards rpms packages.

However we never tested for RES8/Ubuntu, is this strictly mandatory or the grafana-formula can be more "elastic" in that sense?

As stated from @renner ,

I could submit the json files but this is a PITA for maintainers. Each time we change a dashboard which we develop we would need to submit a PR here. And imagine if Suse-manager would have like multiple dashboard from every team, this would make lot of PRs. ( and manual intervertion/error prone)

If it is strictly mandatory to have RHEL/DEBian packages, we can help you to have such packages.

As you can see, we have the RPM here which is automated by github-actions.
https://github.com/ClusterLabs/ha_cluster_exporter/blob/master/packaging/obs/grafana-ha-cluster-dashboards/grafana-ha-cluster-dashboards.spec#L16

The dashboard package itself is just a json file

@renner
Copy link
Member

renner commented Aug 12, 2020

Now the question is what happens about other OS: Who is going to maintain the packages for those OS? Us or dario/sap? And where are we going to maintain them? At the OS codestreams so other projects can use them, or at the client tools codestreams (only available as separate codestreams for RES8 and Ubuntu20?

We are not actually supporting grafana itself for other OSs (RES or Ubuntu), only for SLE12 and SLE15. Therefore - from my perspective - we should just ship Dario's dashboard packages alongside the grafana package with our SLE12 and SLE15 client tools.

Copy link
Member

@renner renner left a comment

Choose a reason for hiding this comment

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

It looks good to me! 👌 As discussed it could be released with 4.1.2, we only need to make sure that those dashboard packages are present in our SLE12 and 15 client tools (alongside grafana).

@juliogonzalez
Copy link
Member

We are not actually supporting grafana itself for other OSs (RES or Ubuntu), only for SLE12 and SLE15. Therefore - from my perspective - we should just ship Dario's dashboard packages alongside the grafana package with our SLE12 and SLE15 client tools.

Ok, then we need @MalloZup to prepare a submission to the SLE12 codestreams (SUSE:SLE-12:Update, and from there it should get into all SPs), as I understand from his comment that he will be the maintainer.

With that, we can ask maintenance to modify the channel definitions to take the packages from the SLE channels.

@MalloZup
Copy link
Contributor Author

@juliogonzalez Yes for SLE12 we have in our backlog a plan to submit ECO for dashboards

@juliogonzalez
Copy link
Member

@MalloZup what will be the package?

Ideally we should have that package available at SLE12 before the second week of September. If it's a new package that for now only SUSE Manager is going to use, you don't really need an ECO if you just want to submit to SUSE:SLE-12:Update, AFAIK

@juliogonzalez
Copy link
Member

Also we'll need this updated grafana-formula package submitted to the Devel:Galaxy:Manager:4.1, Devel:Galaxy:Manager:Head and systemsmanagement:Uyuni:Master. I will create a card for you @renner

Copy link
Contributor

@witekest witekest left a comment

Choose a reason for hiding this comment

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

When trying to apply highstate I'm getting

- "Rendering SLS 'base:grafana' failed: while parsing a block mapping\n  in \"<unicode string>\", line 6, column 1\ndid not find expected key\n  in \"<unicode\
    \ string>\", line 47, column 3"

grafana-formula/grafana-formula.spec Outdated Show resolved Hide resolved
grafana-formula/metadata/form.yml Outdated Show resolved Hide resolved
@MalloZup
Copy link
Contributor Author

MalloZup commented Aug 13, 2020

thx for all reviews.

resuming the follow-up todos:

  • fix review comment and test formula with RPM (@MalloZup ) and add grafana-sap-providers
  • submit the packages to SLE12 as discussed with @juliogonzalez

I guess after this we should be ok to merge the PR, or I'm missing something?

@witekest
Copy link
Contributor

I guess after this we should be ok to merge the PR, or I'm missing something?

If you want to split the work into smaller steps I'd suggest creating a feature branch and continuing the work there.
I'm against merging it into master in current shape as the syntax errors break the installation of existing dashboards.

@MalloZup
Copy link
Contributor Author

@witekest no worries, if you check my first point is about what you suggest. see #67 (comment) 🌻

@juliogonzalez
Copy link
Member

I guess after this we should be ok to merge the PR, or I'm missing something?

There's one extra task, which is for SUSE Manager to update grafana-formula on the SUSE Manager and Uyuni codestreams as the package is only there and NOT at the SLE/openSUSE codestreams, and I guess we want to keep it that way).

I created an internal card for it.

@MalloZup MalloZup changed the title Add SAP and HA dashboards to grafana formula [WIP]: Add SAP and HA dashboards to grafana formula Aug 13, 2020
@MalloZup MalloZup changed the title [WIP]: Add SAP and HA dashboards to grafana formula Add SAP and HA dashboards to grafana formula Sep 10, 2020
@MalloZup
Copy link
Contributor Author

@witekest thx for taking care of this. we need to wait to merge this until grafana dashboards lands to sle15 ..

@juliogonzalez
Copy link
Member

juliogonzalez commented Oct 2, 2020

@MalloZup the packages are on SLE12 and SLE15 already, and you have two approvals and no blockers. Is there anything else pending?

@MalloZup
Copy link
Contributor Author

MalloZup commented Oct 2, 2020

@juliogonzalez nope nothing pending for dashboards.

@MalloZup
Copy link
Contributor Author

MalloZup commented Oct 5, 2020

@witekest the grafana pkg are merged so we can merge this unless there are other issues

@juliogonzalez
Copy link
Member

@MalloZup you have conflicts on the changelog, you will need to fix them.

@MalloZup
Copy link
Contributor Author

MalloZup commented Oct 5, 2020

@juliogonzalez the conflicts where caused from 2 hours ago:
#81 (comment)
:(

@juliogonzalez
Copy link
Member

If this is ready to be merged, I suggest you go ahead with it so @witekest can prepare the SRs :-)

@witekest witekest merged commit 912b0be into SUSE:master Oct 6, 2020
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.

5 participants