mgr/cephadm: move ok_to_stop to CephadmService by sebastian-philipp · Pull Request #35503 · ceph/ceph

sebastian-philipp · 2020-06-09T13:55:19Z

This PR is on the edge of being unnecessary. MDS is the only service that will benefit from this. Still:

it thought me how to proceed with [wip] mgr/cephadm: Add host draining #34617
helps with mgr/mds_autoscaler: add autoscaling for mds mgmt #32731
Moves code into better places.
introduces structure for future PRs.

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

sebastian-philipp · 2020-06-09T20:15:04Z

todo for tomorrow:

    @mock.patch("cephadm.module.CephadmOrchestrator._run_cephadm", _run_cephadm('{}'))
    @mock.patch("cephadm.services.cephadmservice.CephadmService.ok_to_stop")
    def test_daemon_ok_to_stop(self, ok_to_stop, cephadm_module: CephadmOrchestrator):
        spec = ServiceSpec(
            'mds',
            service_id='fsname',
            placement=PlacementSpec(hosts=['host1', 'host2'])
        )
        with with_host(cephadm_module, 'host1'), with_host(cephadm_module, 'host2'):
            c = cephadm_module.apply_mds(spec)
            out = wait(cephadm_module, c)
            match_glob(out, "Scheduled mds.fsname update...")
            cephadm_module._apply_all_services()
    
            spec.placement.set_hosts(['host2'])
    
            c = cephadm_module.apply_mds(spec)
            out = wait(cephadm_module, c)
            match_glob(out, "Scheduled mds.fsname update...")
            cephadm_module._apply_all_services()
    
>           [daemon] = cephadm_module.cache.daemons['host1'].keys()
E           ValueError: not enough values to unpack (expected 1, got 0)

mgfritch

👍 generally moving these to a CephadmService is much cleaner ... I’m just a little worried about some of the edge cases where service_type doesn’t agree with TYPE...

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

src/pybind/mgr/cephadm/upgrade.py

sebastian-philipp · 2020-06-12T12:22:01Z

green: http://pulpito.ceph.com/swagner-2020-06-11_14:15:42-rados:cephadm-wip-swagner-testing-2020-06-11-1428-distro-basic-smithi/

kshtsk · 2020-06-16T11:43:15Z

src/pybind/mgr/cephadm/module.py

-                self._remove_daemon(d.name(), d.hostname)
-                r = True
+        to_remove = [d for d in daemons if d.hostname not in target_hosts]
+        while to_remove and not self.cephadm_services[daemon_type].ok_to_stop([d.daemon_id for d in to_remove]):


Is there any sense to use list construction instead of iterator?

kshtsk · 2020-06-16T11:47:45Z

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

+        new_mons = [m for m in mons if m != mon_id]
+        new_quorum = [m for m in j['quorum_names'] if m != mon_id]
+        if len(new_quorum) > len(new_mons) / 2:
+            logger.info('Safe to remove mon.%s: new quorum should be %s (from %s)' % (mon_id, new_quorum, new_mons))


Since this is a logger.info, then it is better to right?

logger.info('Safe to remove mon.%s: new quorum should be %s (from %s)', mon_id, new_quorum, new_mons)

Otherwise, why not keep the same style using f' ' strings?

kshtsk · 2020-06-16T11:47:58Z

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

+        self._check_safe_to_destroy(daemon_id)
+
+        # remove mon from quorum before we destroy the daemon
+        logger.info('Removing monitor %s from monmap...' % daemon_id)


kshtsk · 2020-06-16T11:48:16Z

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

+        if len(new_quorum) > len(new_mons) / 2:
+            logger.info('Safe to remove mon.%s: new quorum should be %s (from %s)' % (mon_id, new_quorum, new_mons))
+            return
+        raise OrchestratorError('Removing %s would break mon quorum (new quorum %s, new mons %s)' % (mon_id, new_quorum, new_mons))


kshtsk · 2020-06-16T11:50:36Z

src/pybind/mgr/cephadm/upgrade.py

+            ok = self.mgr.cephadm_services[s.daemon_type].ok_to_stop([s.daemon_id])
+
+            if ok:
+                logger.info('Upgrade: It is presumed safe to stop %s.%s' %


See comment above regarding logger.info

kshtsk · 2020-06-16T11:50:44Z

src/pybind/mgr/cephadm/upgrade.py

+                logger.info('Upgrade: It is presumed safe to stop %s.%s' %
                              (s.daemon_type, s.daemon_id))
                return True
+            logger.info('Upgrade: It is NOT safe to stop %s.%s' %


See comment above regarding logger.info

kshtsk · 2020-06-16T11:51:25Z

src/python-common/ceph/deployment/service_spec.py

        # To backpopulate the .hosts attribute when using labels or count
        # in the orchestrator backend.
-        self.hosts = hosts
+        if all([isinstance(host, HostPlacementSpec) for host in hosts]):


there is no need to construct a list

kshtsk · 2020-06-16T11:58:56Z

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

+        names = [f'{self.TYPE}.{d_id}' for d_id in daemon_ids]
+
+        if self.TYPE not in ['mon', 'osd', 'mds']:
+            logger.info('Upgrade: It is presumed safe to stop %s' % names)


% names => , names?

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>

mgfritch

lgtm, this structure will make future PRs much cleaner!

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

src/pybind/mgr/cephadm/upgrade.py

sebastian-philipp · 2020-06-24T13:01:06Z

unfortunately, I think this is blocked, till the upgrade test is working again.

sebastian-philipp · 2020-06-25T14:52:06Z

green: http://pulpito.ceph.com/swagner-2020-06-25_13:58:46-rados:cephadm-wip-swagner-testing-2020-06-25-1435-distro-basic-smithi/

sebastian-philipp · 2020-06-25T14:59:51Z

jenkins test docs

sebastian-philipp added the cephadm label Jun 9, 2020

sebastian-philipp requested a review from a team as a code owner June 9, 2020 13:55

sebastian-philipp added the cleanup label Jun 9, 2020

sebastian-philipp force-pushed the rm-daemon-ok-to-stop branch from eb41156 to 94c8999 Compare June 9, 2020 13:59

mgfritch reviewed Jun 10, 2020

View reviewed changes

src/pybind/mgr/cephadm/services/cephadmservice.py Show resolved Hide resolved

src/pybind/mgr/cephadm/upgrade.py Show resolved Hide resolved

sebastian-philipp force-pushed the rm-daemon-ok-to-stop branch from 94c8999 to 8f69936 Compare June 11, 2020 11:58

sebastian-philipp added the needs-qa label Jun 11, 2020

sebastian-philipp force-pushed the rm-daemon-ok-to-stop branch from 8f69936 to a897858 Compare June 11, 2020 12:04

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

sebastian-philipp added needs-review and removed needs-qa wip-swagner-testing My Teuthology tests labels Jun 12, 2020

sebastian-philipp force-pushed the rm-daemon-ok-to-stop branch from a897858 to 1dae421 Compare June 16, 2020 11:10

kshtsk reviewed Jun 16, 2020

View reviewed changes

sebastian-philipp added 4 commits June 18, 2020 14:30

mgr/cephadm: move ok_to_stop to CephadmService

2b41d0a

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>

mgr/cephadm: move _check_safe_to_destroy_mon to MonService

cb92429

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>

mgr/cephadm: scheduler: make use of ok-to-stop

f421825

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>

mgr/cephadm: add test for ok-to-stop

5c91f1b

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>

sebastian-philipp force-pushed the rm-daemon-ok-to-stop branch from 1dae421 to 5c91f1b Compare June 18, 2020 12:30

mgfritch added the wip-mgfritch-testing label Jun 18, 2020

mgfritch approved these changes Jun 23, 2020

View reviewed changes

src/pybind/mgr/cephadm/services/cephadmservice.py Show resolved Hide resolved

src/pybind/mgr/cephadm/upgrade.py Show resolved Hide resolved

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

sebastian-philipp changed the title ~~mgr/cephadm: move ok_to_stop to CephadmService~~ [Blocked by #35740] mgr/cephadm: move ok_to_stop to CephadmService Jun 24, 2020

sebastian-philipp added DNM wip-swagner-testing My Teuthology tests and removed wip-mgfritch-testing wip-swagner-testing My Teuthology tests needs-review labels Jun 24, 2020

sebastian-philipp changed the title ~~[Blocked by #35740] mgr/cephadm: move ok_to_stop to CephadmService~~ mgr/cephadm: move ok_to_stop to CephadmService Jun 25, 2020

sebastian-philipp merged commit 21a9fb9 into ceph:master Jun 25, 2020

sebastian-philipp mentioned this pull request Jul 2, 2020

octopus: cephadm batch backport July (1) #35898

Merged

mchangir mentioned this pull request Jul 6, 2020

mgr/mds_autoscaler: add autoscaling for mds mgmt #32731

Merged

3 tasks

Conversation

sebastian-philipp commented Jun 9, 2020

Checklist

Uh oh!

sebastian-philipp commented Jun 9, 2020

Uh oh!

mgfritch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sebastian-philipp commented Jun 12, 2020

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!

Choose a reason for hiding this comment

Uh oh!

mgfritch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sebastian-philipp commented Jun 24, 2020

Uh oh!

sebastian-philipp commented Jun 25, 2020

Uh oh!

sebastian-philipp commented Jun 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants