mgr/cephadm: allow custom dashboard grafana url by adk3798 · Pull Request #35747 · ceph/ceph

adk3798 · 2020-06-24T12:57:54Z

Instead of resetting grafana url to a default with
each run of the service loop, only set it when
initally deployed or when it's placement changes

Fixes: https://tracker.ceph.com/issues/44877
Signed-off-by: Adam King adking@redhat.com

sebastian-philipp · 2020-06-24T14:31:55Z

src/pybind/mgr/cephadm/module.py

            )
            daemons.append(sd)
+            if daemon_type in ['grafana', 'iscsi', 'prometheus', 'alertmanager', 'nfs']:
+                self._get_cephadm_service(daemon_type).daemon_check_post([sd])


instead of having this in _apply_service, what do you think of moving this into _create_daemon ?

I think the reason I chose to put it in _apply_service over _create_daemon originally was because the redeploy command also goes through _create_daemon and I didn't want to alter the url in that case. If you think changing the url is also okay in that case then I could move it there.

seems like we should update the dashboard urls during redeploy?

NFS pool/namespace could change during redeploy--

ceph/src/pybind/mgr/cephadm/services/nfs.py

Lines 74 to 95 in 07484c6

def config_dashboard(self, daemon_descrs: List[DaemonDescription]):

def get_set_cmd_dicts(out: str) -> List[dict]:

locations: List[str] = []

for dd in daemon_descrs:

spec = cast(NFSServiceSpec,

self.mgr.spec_store.specs.get(dd.service_name(), None))

if not spec or not spec.service_id:

logger.warning('No ServiceSpec or service_id found for %s', dd)

continue

locations.append('{}:{}/{}'.format(spec.service_id, spec.pool, spec.namespace))

new_value = ','.join(locations)

if new_value and new_value != out:

return [{'prefix': 'dashboard set-ganesha-clusters-rados-pool-namespace',

'value': new_value}]

return []

self._check_and_set_dashboard(

service_name='Ganesha',

get_cmd='dashboard get-ganesha-clusters-rados-pool-namespace',

get_set_cmd_dicts=get_set_cmd_dicts

)

Similar for iscsi too --

ceph/src/pybind/mgr/cephadm/services/iscsi.py

Lines 64 to 90 in 07484c6

def config_dashboard(self, daemon_descrs: List[DaemonDescription]):

def get_set_cmd_dicts(out: str) -> List[dict]:

gateways = json.loads(out)['gateways']

cmd_dicts = []

for dd in daemon_descrs:

spec = cast(IscsiServiceSpec,

self.mgr.spec_store.specs.get(dd.service_name(), None))

if not spec:

logger.warning('No ServiceSpec found for %s', dd)

continue

if not all([spec.api_user, spec.api_password]):

reason = 'api_user or api_password is not specified in ServiceSpec'

logger.warning(

'Unable to add iSCSI gateway to the Dashboard for %s: %s', dd, reason)

continue

host = self._inventory_get_addr(dd.hostname)

service_url = 'http://{}:{}@{}:{}'.format(

spec.api_user, spec.api_password, host, spec.api_port or '5000')

gw = gateways.get(host)

if not gw or gw['service_url'] != service_url:

logger.info('Adding iSCSI gateway %s to Dashboard', service_url)

cmd_dicts.append({

'prefix': 'dashboard iscsi-gateway-add',

'service_url': service_url,

'name': host

})

return cmd_dicts

Okay, in that case I will move it to _create_daemon where it should update during redeploy as well as when the placement changes

Completely against about this change. ( overwrite when redeploy).
If we are redeploying the daemon in the same host... (it does not matter what daemon) probably this is because we want to upgrade the container image or restart the daemon.
Overwriting config values with default values is something that probably is not going to make very happy to the user that has change the settings.

I think that it is ok to use default or calculated values when you are deploying for first time a container in one host... but if it has been deployed .. there is a true possibility that the user had changed settings to customize the daemon behaviour. Overwriting config settings is always dangerous.... so my suggestion is not to do this kind of things. (in any daemon)

Overwriting config settings is always dangerous.... so my suggestion is not to do this kind of things. (in any daemon)

Hence my suggestion to never overwrite any dashboard settings, but instead only provide default values for the dashboard.

Under the assumption that we go withthe solution to only overwrite the dashboard settings sometimes (like when deploying services, failovers etc), we have provide a way for users to "fix" the dashboard config, thus 👍 for also doing this on redeploy.

Just an update on this. I've been trying to do the check post call within _create_daemon but have run into a problem where the mon commands it sends in the process (it sends a "dashboard get-grafana-api-url" and "dashboard set-grafana-apit url " both within the _check_and_set_dashboard function of services/cephadmservice.py) never returns. Furthermore, if I interrupt the redeploy to regain control and then try to manually get or set the grafana url afterwards those commands also don't return. I haven't been able to figure out why this is happening. For reference, I only intended to add the two highlighted lines here to the _create_daemon function. I'm still looking into it but I haven't had much luck.

also, this seems to only be an issue during redeploy. _create_daemon runs fine with the daemon_check_post call during initial deployment and when changing placement.

I don't think a direct call within _create_daemon is possible due to this https://docs.ceph.com/docs/master/dev/cephadm/#note-regarding-network-calls-from-cli-handlers
so I've tried this new method of setting a sort of flag within _create_daemon so the post actions can be handled after within the service loop to hopefully get the same outcome as doing the post actions in _create_daemon without the mon commands hanging

src/pybind/mgr/cephadm/module.py

Instead of resetting grafana url to a default with each run of the service loop, only set it when initally deployed or when it's placement changes Fixes: https://tracker.ceph.com/issues/44877 Signed-off-by: Adam King <adking@redhat.com>

sebastian-philipp · 2020-07-01T13:15:48Z

green: https://pulpito.ceph.com/swagner-2020-07-01_10:16:24-rados:cephadm-wip-swagner3-testing-2020-07-01-1013-distro-basic-smithi/

still lots of failures:

https://tracker.ceph.com/issues/46300
503 from the fedora RPM mirrior
cephadm: Retry pull on transient error #35846

tchaikov · 2020-07-05T08:56:39Z

adk3798 · 2020-07-07T12:39:40Z

retest this please

adk3798 added the cephadm label Jun 24, 2020

adk3798 requested a review from a team as a code owner June 24, 2020 12:57

sebastian-philipp reviewed Jun 24, 2020

View reviewed changes

mgfritch reviewed Jun 24, 2020

View reviewed changes

src/pybind/mgr/cephadm/module.py Outdated Show resolved Hide resolved

adk3798 force-pushed the cephadm-44877 branch from 3167441 to c6895d0 Compare June 24, 2020 17:10

sebastian-philipp added the needs-rebase label Jun 26, 2020

adk3798 force-pushed the cephadm-44877 branch 2 times, most recently from 4a5f158 to 0329a5f Compare June 26, 2020 13:32

adk3798 removed the needs-rebase label Jun 26, 2020

sebastian-philipp added the wip-swagner-testing My Teuthology tests label Jun 29, 2020

adk3798 force-pushed the cephadm-44877 branch from 0329a5f to b708ded Compare June 29, 2020 15:01

sebastian-philipp removed the wip-swagner-testing My Teuthology tests label Jun 29, 2020

jmolmo approved these changes Jun 30, 2020

View reviewed changes

sebastian-philipp added the needs-rebase label Jun 30, 2020

mgr/cephadm: allow custom dashboard grafana url

fc14c11

Instead of resetting grafana url to a default with each run of the service loop, only set it when initally deployed or when it's placement changes Fixes: https://tracker.ceph.com/issues/44877 Signed-off-by: Adam King <adking@redhat.com>

adk3798 force-pushed the cephadm-44877 branch from b708ded to fc14c11 Compare June 30, 2020 13:13

adk3798 removed the needs-rebase label Jun 30, 2020

sebastian-philipp added the wip-swagner3-testing label Jul 1, 2020

sebastian-philipp added needs-qa and removed wip-swagner3-testing labels Jul 1, 2020

tchaikov added the wip-kefu-testing label Jul 1, 2020

sebastian-philipp added wip-swagner-testing My Teuthology tests and removed wip-swagner-testing My Teuthology tests labels Jul 2, 2020

tchaikov removed the wip-kefu-testing label Jul 5, 2020

tchaikov merged commit 631fba8 into ceph:master Jul 11, 2020

sebastian-philipp mentioned this pull request Jul 15, 2020

octopus: cephadm batch backport July (2) #36109

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mgr/cephadm: allow custom dashboard grafana url#35747

mgr/cephadm: allow custom dashboard grafana url#35747
tchaikov merged 1 commit intoceph:masterfrom
adk3798:cephadm-44877

adk3798 commented Jun 24, 2020

Uh oh!

sebastian-philipp Jun 24, 2020

Uh oh!

adk3798 Jun 24, 2020

Uh oh!

mgfritch Jun 24, 2020 •

edited

Loading

Uh oh!

adk3798 Jun 24, 2020

Uh oh!

jmolmo Jun 25, 2020

Uh oh!

sebastian-philipp Jun 25, 2020

Uh oh!

adk3798 Jun 26, 2020

Uh oh!

adk3798 Jun 26, 2020

Uh oh!

adk3798 Jun 29, 2020

Uh oh!

Uh oh!

sebastian-philipp commented Jul 1, 2020

Uh oh!

tchaikov commented Jul 5, 2020

Uh oh!

adk3798 commented Jul 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	def config_dashboard(self, daemon_descrs: List[DaemonDescription]):

	def get_set_cmd_dicts(out: str) -> List[dict]:
	locations: List[str] = []
	for dd in daemon_descrs:
	spec = cast(NFSServiceSpec,
	self.mgr.spec_store.specs.get(dd.service_name(), None))
	if not spec or not spec.service_id:
	logger.warning('No ServiceSpec or service_id found for %s', dd)
	continue
	locations.append('{}:{}/{}'.format(spec.service_id, spec.pool, spec.namespace))
	new_value = ','.join(locations)
	if new_value and new_value != out:
	return [{'prefix': 'dashboard set-ganesha-clusters-rados-pool-namespace',
	'value': new_value}]
	return []

	self._check_and_set_dashboard(
	service_name='Ganesha',
	get_cmd='dashboard get-ganesha-clusters-rados-pool-namespace',
	get_set_cmd_dicts=get_set_cmd_dicts
	)

	def config_dashboard(self, daemon_descrs: List[DaemonDescription]):
	def get_set_cmd_dicts(out: str) -> List[dict]:
	gateways = json.loads(out)['gateways']
	cmd_dicts = []
	for dd in daemon_descrs:
	spec = cast(IscsiServiceSpec,
	self.mgr.spec_store.specs.get(dd.service_name(), None))
	if not spec:
	logger.warning('No ServiceSpec found for %s', dd)
	continue
	if not all([spec.api_user, spec.api_password]):
	reason = 'api_user or api_password is not specified in ServiceSpec'
	logger.warning(
	'Unable to add iSCSI gateway to the Dashboard for %s: %s', dd, reason)
	continue
	host = self._inventory_get_addr(dd.hostname)
	service_url = 'http://{}:{}@{}:{}'.format(
	spec.api_user, spec.api_password, host, spec.api_port or '5000')
	gw = gateways.get(host)
	if not gw or gw['service_url'] != service_url:
	logger.info('Adding iSCSI gateway %s to Dashboard', service_url)
	cmd_dicts.append({
	'prefix': 'dashboard iscsi-gateway-add',
	'service_url': service_url,
	'name': host
	})
	return cmd_dicts

Conversation

adk3798 commented Jun 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgfritch Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sebastian-philipp commented Jul 1, 2020

Uh oh!

tchaikov commented Jul 5, 2020

Uh oh!

adk3798 commented Jul 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mgfritch Jun 24, 2020 •

edited

Loading