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

Merge dashboard-ansible code into ceph-ansible #3426

Merged
merged 20 commits into from May 16, 2019
Merged

Merge dashboard-ansible code into ceph-ansible #3426

merged 20 commits into from May 16, 2019

Conversation

b-ranto
Copy link
Contributor

@b-ranto b-ranto commented Dec 7, 2018

This PR merges the dashboard instllation code into ceph-ansible. The code only supports containerized deployment and the PR creates four new roles: ceph-grafana, ceph-dashboard, ceph-prometheus and ceph-node-exporter.

Edit (@zmc):
To-do:

  • purge-cluster.yml
  • site-container.yml.sample
  • purge-docker-cluster.yml
  • Use packaged dashboards
  • Squash and/or clean up commits
  • Add podman support

@b-ranto b-ranto requested review from zmc, leseb and guits December 7, 2018 15:28
@leseb leseb added the DNM Do NOT merge label Dec 7, 2018
leseb
leseb previously requested changes Dec 7, 2018
- name: include fetch_image.yml
include_tasks: fetch_image.yml
tags:
- fetch_container_image
when: containerized_deployment
Copy link
Member

Choose a reason for hiding this comment

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

You're already in the ceph-container-common role which means you're doing containers already. You don't need this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is trickier. We only support containerized deployment in dashboard-ansible. I need to include this role to get docker installed even if ceph itself is not containerized. This allows me to do that.

@@ -31,10 +38,13 @@
changed_when: false
check_mode: no
register: ceph_version
when: containerized_deployment
Copy link
Member

Choose a reason for hiding this comment

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

ditto


- name: set_fact ceph_version ceph_version.stdout.split
set_fact:
ceph_version: "{{ ceph_version.stdout.split(' ')[2] }}"
when: containerized_deployment
Copy link
Member

Choose a reason for hiding this comment

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

ditto

roles/ceph-container-common/tasks/main.yml Show resolved Hide resolved
roles/ceph-dashboard/tasks/configure_dashboard.yml Outdated Show resolved Hide resolved
roles/ceph-grafana/tasks/main.yml Outdated Show resolved Hide resolved
roles/ceph-node-exporter/handlers/main.yml Outdated Show resolved Hide resolved
roles/ceph-node-exporter/tasks/main.yml Outdated Show resolved Hide resolved
roles/ceph-node-exporter/tasks/main.yml Outdated Show resolved Hide resolved
site.yml.sample Outdated Show resolved Hide resolved
@zmc
Copy link
Member

zmc commented Dec 14, 2018

While @b-ranto is out, I'm taking over this work. I've addressed most, if not all, of the feedback items, as well as fixed several issues I noticed while testing it out. I do currently have a successful deployment, but there are some outstanding issues. I'll edit the initial comment on the PR with a task list, and push my updates.

A copy of the original branch is at zmc/wip-dashboard-branto, just to make sure I didn't destroy the original copy.

@@ -51,7 +51,7 @@
"rgba(237, 129, 40, 0.9)",
"rgb(255, 0, 0)"
],
"datasource": "$datasource",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: This was intentional, with $datasource here, the datasource was actually selectable in grafana. We should probably drop the dashboards from the PR altogether and use those that are provided in ceph's ceph-grafana-dashboards package. Having them as part of the deployment was sort of a hack for luminous where these dashboards did not exist upstream, yet. Alternatively, we can have them as back-up in case the package does not exist (that was my original plan, the code should be ready for this, it just does not install the package, yet).

Copy link
Member

Choose a reason for hiding this comment

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

Oh, okay - it's just that none of the dashboards were working by default. I'll update the PR to use the packaged dashboards. It does look like they use a templated datasource name as well, so I'll see about fixing the default behavior.

I'd tentatively suggest that we shouldn't store them in two places, for maintenance reasons. It shouldn't be hard to backport the dashboards to luminous if we need to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a downstream hack so we probably don't need to care about it upstream and we can just drop the dashboards and install the package instead. The official dashboard v2 support is dedicated for nautilus+.

It seems weird that they did not work, what error did you get? What grafana version were you using? Did you by any chance hit this bug? grafana/grafana#14239

Copy link
Member

Choose a reason for hiding this comment

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

It was/is v5.4.2 - I actually can't remember if it produced an error - the main symptom was blank graphs. Once I get the purge playbook working I'll tear down my current setup and dig a little deeper.

Copy link
Member

Choose a reason for hiding this comment

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

Update: I switched to using the packaged dashboards. It looks like the failure in datasource name templating only happens when visiting grafana directly without being signed in; once I logged in as admin, they worked properly. When viewing from the dashboard, they worked as well (even before signing in on the other grafana tab).

@zmc zmc force-pushed the wip-dashboard branch 2 times, most recently from 252cd42 to ecb4650 Compare December 17, 2018 21:47
@zmc
Copy link
Member

zmc commented Dec 18, 2018

Looking at site-container.yml.sample and purge-docker-cluster.yml, I can't help but wish the various playbooks could share some code, and have access to var defaults. I wonder if it would ever make sense to move purge logic into relevant roles, and have their operation controlled by vars.

@zmc
Copy link
Member

zmc commented Dec 18, 2018

roles/ceph-infra/tasks/configure_firewall.yml Outdated Show resolved Hide resolved
roles/ceph-infra/tasks/configure_firewall.yml Outdated Show resolved Hide resolved
roles/ceph-grafana/tasks/configure_grafana.yml Outdated Show resolved Hide resolved
group_vars/all.yml.sample Outdated Show resolved Hide resolved
@zmc zmc force-pushed the wip-dashboard branch 4 times, most recently from e88d5f7 to 537e58d Compare January 29, 2019 23:58
@zmc
Copy link
Member

zmc commented Jan 30, 2019

Okay, I think this is ready to be merged. To fully tie everything together, it will require some changes to ceph's dashboard module, which is in-progress. When there's a PR for that, I'll link it here for posterity. It should be in the next day or so.

What it's going to do, is cause the dashboard to push the dashboard JSON files to Grafana when:

  1. The dashboard module starts up
  2. ceph dashboard update-grafana-dashboards is run
  3. A POST request is sent to /api/grafana/update_dashboards

@zmc zmc changed the title [DNM] Merge dashboard-ansible code into ceph-ansible Merge dashboard-ansible code into ceph-ansible Jan 30, 2019
@zmc zmc force-pushed the wip-dashboard branch 2 times, most recently from bb7c663 to 447cb39 Compare February 12, 2019 20:53
@b-ranto b-ranto force-pushed the wip-dashboard branch 3 times, most recently from 70581a2 to eddd413 Compare February 14, 2019 19:17
@b-ranto b-ranto dismissed leseb’s stale review February 14, 2019 19:18

These should already be addressed.

@guits
Copy link
Collaborator

guits commented May 16, 2019

jenkins test dev-centos-container-dashboard

guits added 12 commits May 16, 2019 15:07
there is no need to add more complexity for this, let's use
`containerized_deployment` in order to detect if we are running a
containerized deployment.
The idea is to use `container_exec_cmd` the same way we do in the rest of
the playbook to run the different ceph commands needed to deploy the
ceph-dashboard role.

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
use site.yml to deploy ceph-container-common in order to install docker
even in non-containerized deployments since there's no RPM available to
deploy the differents applications needed for ceph-dashboard.

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
use a block in grafana-server section to avoid duplicate condition.

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
since stable-4.0 isn't to deploy ceph releases prior to nautilus,
there's no need to add this complexity here.

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
use `0440` instead of `0644` is enough

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
this file seems to be no longer used, let's remove it.

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
generate all group_vars sample files corresponding to new roles added
for ceph-dashboard implementation.

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
make `dashboard_rgw_api_no_ssl_verify` a bool variable since it seems to
be used as it.

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
This commit aligns the way the different containers are managed with how
it's currently done with the other ceph daemon.

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
This commit add a new scenario to test the dashboard deployment via
ceph-ansible.

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
6f0643c introduced a typo, the role that should be run is
ceph-container-common, not ceph-common

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
This commit renames the `docker_exec_cmd` variable to
`container_exec_cmd` so it's more generic.

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
@guits guits changed the title Merge dashboard-ansible code into ceph-ansible [skip ci] Merge dashboard-ansible code into ceph-ansible May 16, 2019
@guits guits changed the title [skip ci] Merge dashboard-ansible code into ceph-ansible Merge dashboard-ansible code into ceph-ansible May 16, 2019
@guits
Copy link
Collaborator

guits commented May 16, 2019

jenkins test dev-centos-non_container-dashboard

@guits
Copy link
Collaborator

guits commented May 16, 2019

jenkins test dev-centos-container-dashboard

@guits guits changed the title Merge dashboard-ansible code into ceph-ansible [skip ci] Merge dashboard-ansible code into ceph-ansible May 16, 2019
There is no need to have default values for these variables in each roles
since there is no corresponding host groups

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
@guits guits changed the title [skip ci] Merge dashboard-ansible code into ceph-ansible Merge dashboard-ansible code into ceph-ansible May 16, 2019
@guits
Copy link
Collaborator

guits commented May 16, 2019

jenkins test dev-centos-container-dashboard

@guits
Copy link
Collaborator

guits commented May 16, 2019

jenkins test dev-centos-non_container-dashboard

@guits guits removed the DNM Do NOT merge label May 16, 2019
@guits guits merged commit 9f0d4d6 into master May 16, 2019
@guits guits deleted the wip-dashboard branch May 16, 2019 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants