mgr/cephadm: config service_url in Dashboard for Prometheus and Alert… by bk201 · Pull Request #35194 · ceph/ceph

bk201 · 2020-05-22T06:49:54Z

Calling Dashboard's CLI to set service URLs after deploying Prometheus
and AlertManager Daemons.

Fixes: https://tracker.ceph.com/issues/45625
Signed-off-by: Kiefer Chang kiefer.chang@suse.com

Dashboard page Cluster -> Monitoring is visible after setting service URLs.
`

Checklist

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

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 dashboard backend
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox

epuertat · 2020-05-22T15:56:29Z

src/pybind/mgr/cephadm/module.py

-            elif dd.daemon_type == 'iscsi':
-                iscsi_daemons.append(dd)
+            if dd.daemon_type in ['grafana', 'iscsi', 'prometheus', 'alertmanager']:
+                cephadm_service = self.cephadm_services.get(dd.daemon_type)


Why using the get() when Python has the simpler []?

Suggested change

cephadm_service = self.cephadm_services.get(dd.daemon_type)

cephadm_service = self.cephadm_services[dd.daemon_type]

what about making self.cephadm_services a method? That would prevent others from running into the same issue?

Maybe this also solves your

post_actions = {} # type: Dict[str, Any]

problem.

CephadmServices = TypeVar('CephadmService', bound=CephadmService) def get_service(self, what: str) -> CephadmServices: ...

Yeah, thanks! I definitely prefer that approach: encapsulating service functionality within the service class scope, rather than having the consumers of the dummy classes implementing the functionality.

Regarding the use of combinations of nested dicts + lists for storing structured data, it feels to me like bringing JSON back to Python, when even Javascript is moving towards Types & Classes (ES6, Typescript, ...). It's great to use mypy, but classes provide the only runtime type checking.

A method is created to get services. But I think all service usage in module.py should also be refactored. It will become a bit lengthy.

e.g.

return self._add_daemon('mon', spec, self.mon_service.create)

to

# maybe change 'mon' to a constant. return self._add_daemon('mon', spec, self._get_cephadm_service('mon').create)

src/pybind/mgr/cephadm/module.py

epuertat · 2020-05-22T16:54:12Z

src/pybind/mgr/cephadm/module.py

-                grafanas.insert(0, dd)
-            elif dd.daemon_type == 'iscsi':
-                iscsi_daemons.append(dd)
+            if dd.daemon_type in ['grafana', 'iscsi', 'prometheus', 'alertmanager']:


dd, v['daemons'].insert(0, dd)... That's closer to assembler than to Python.

What about daemon or daemon_desc and post_action['daemons'].insert(0, daemon_desc)?

Type hints are great but they cannot replace human-friendly naming. We are following that (good) practice in the dashboard front-end, so why not keeping that when coding elsewhere?

DaemonDescription is one of the fundamental data types here in mgr/cephadm. Having short names for them is imo a good thing.

At least, I'm getting a nice mouse-over:

(I'm just not sure, if v is a good name)

I think that code readability is not about core developers understanding the code they have written and read dozens of times, but everyone else doing so. And yeah, we all love (and use) fully-featured Graphical IDEs with ML-based auto-completion and cloud-based indexing, but there are plenty situations out there where that doesn't work. For example:

Github code reviews like this one,

Tracebacks or errors: (a user facing AttributeError: 'dd' object has no attribute 'dt' vs. AttributeError: 'daemon_desc' object has no attribute 'daemon_type' can at least open a meaningful issue: "mgr/cephadm: missing attribute 'daemon_type' in daemon_desc" vs. "mgr/cephadm: missing attribute 'dt' in dd").

Debugging a field issue on a remote system where you can barely have vi (not even the fancy vim with your super-crafted vimrc).

(I'm just not sure, if v is a good name)

It's removed.

src/pybind/mgr/cephadm/module.py

src/pybind/mgr/cephadm/services/cephadmservice.py

src/pybind/mgr/cephadm/services/monitoring.py

epuertat

Looks good, thanks for addressing my comments @bk201 !

epuertat · 2020-05-27T10:29:48Z

src/pybind/mgr/cephadm/services/monitoring.py


+    def get_active_daemon(self, daemon_descrs: List[DaemonDescription]) -> DaemonDescription:


?

Suggested change

def get_active_daemon(self, daemon_descrs: List[DaemonDescription]) -> DaemonDescription:

@staticmethod

def get_active_daemon(daemon_descrs: List[DaemonDescription]) -> DaemonDescription:

-1 for @staticmethod. I need to get the active daemon for MDS and MGR to properly schedule daemon removals. And I need self to access the mgr. Adding @staticmethod force me to remove @staticmethod again later on.

epuertat · 2020-05-27T10:30:52Z

src/pybind/mgr/cephadm/services/monitoring.py


+    def get_active_daemon(self, daemon_descrs: List[DaemonDescription]) -> DaemonDescription:


Suggested change

def get_active_daemon(self, daemon_descrs: List[DaemonDescription]) -> DaemonDescription:

@staticmethod

def get_active_daemon(daemon_descrs: List[DaemonDescription]) -> DaemonDescription:

…Manager Calling Dashboard's CLI to set service URLs after deploying Prometheus and AlertManager Daemons. Fixes: https://tracker.ceph.com/issues/45625 Signed-off-by: Kiefer Chang <kiefer.chang@suse.com>

Signed-off-by: Kiefer Chang <kiefer.chang@suse.com>

bk201 · 2020-05-28T02:32:01Z

Rebased.

tchaikov · 2020-05-28T07:11:39Z

failed tests are tracked by

bk201 added bug-fix dashboard cephadm labels May 22, 2020

bk201 requested a review from a team as a code owner May 22, 2020 06:49

epuertat reviewed May 22, 2020

View reviewed changes

sebastian-philipp reviewed May 25, 2020

View reviewed changes

bk201 force-pushed the wip-45625 branch from c5027c9 to 6e4d437 Compare May 27, 2020 06:46

epuertat approved these changes May 27, 2020

View reviewed changes

sebastian-philipp approved these changes May 27, 2020

View reviewed changes

sebastian-philipp added the needs-qa label May 27, 2020

tchaikov added the wip-kefu-testing label May 27, 2020

bk201 added 2 commits May 28, 2020 09:27

mgr/cephadm: config service_url in Dashboard for Prometheus and Alert…

a613542

…Manager Calling Dashboard's CLI to set service URLs after deploying Prometheus and AlertManager Daemons. Fixes: https://tracker.ceph.com/issues/45625 Signed-off-by: Kiefer Chang <kiefer.chang@suse.com>

mgr: include retval in MonCommandFailed exception

c336d60

Signed-off-by: Kiefer Chang <kiefer.chang@suse.com>

bk201 force-pushed the wip-45625 branch from 6e4d437 to c336d60 Compare May 28, 2020 02:31

tchaikov merged commit 698fe28 into ceph:master May 28, 2020

sebastian-philipp mentioned this pull request Jun 2, 2020

octopus: cephadm: batch backport June (1) #35347

Merged

bk201 deleted the wip-45625 branch July 10, 2020 04:07

	cephadm_service = self.cephadm_services.get(dd.daemon_type)
	cephadm_service = self.cephadm_services[dd.daemon_type]


		def get_active_daemon(self, daemon_descrs: List[DaemonDescription]) -> DaemonDescription:

-    def get_active_daemon(self, daemon_descrs: List[DaemonDescription]) -> DaemonDescription:
+    @staticmethod
+    def get_active_daemon(daemon_descrs: List[DaemonDescription]) -> DaemonDescription:

Conversation

bk201 commented May 22, 2020

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastian-philipp May 25, 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!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

epuertat left a comment

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!

bk201 commented May 28, 2020

Uh oh!

tchaikov commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sebastian-philipp May 25, 2020 •

edited

Loading

tchaikov commented May 28, 2020 •

edited

Loading