mgr/orch: Make orchestrator interface synchronous. by sebastian-philipp · Pull Request #39352 · ceph/ceph

sebastian-philipp · 2021-02-08T09:48:32Z

Only Rook's create_osd function used the old interface. Everything else was redundant by now.

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

liewegas · 2021-02-08T14:45:26Z

src/pybind/mgr/cephadm/tests/fixtures.py

-            time.sleep(0.1)
-    assert False, "timeout" + str(c._state)
+    # type: (CephadmOrchestrator, OrchResult) -> Any
+    return raise_if_exception(c)


Does this method still need to exist? (Or should it be renamed?)

Right. it's still needed due to the sub-interpreter magic:

ceph/src/pybind/mgr/orchestrator/_interface.py

Lines 215 to 226 in 3b42756

def raise_if_exception(c: OrchResult[T]) -> T:

"""

Due to different sub-interpreters, this MUST not be in the `OrchResult` class.

"""

if c.serialized_exception is not None:

try:

e = pickle.loads(c.serialized_exception)

except (KeyError, AttributeError):

raise Exception(c.exception_str)

raise e

assert c.result is not None, 'OrchResult should either have an exception or a result'

return c.result

sebastian-philipp · 2021-02-08T15:16:58Z

2021-02-08 11:08:23,391.391 INFO:__main__:----------------------------------------------------------------------
2021-02-08 11:08:23,391.391 INFO:__main__:Traceback (most recent call last):
2021-02-08 11:08:23,392.392 INFO:__main__:  File "/home/jenkins-build/build/workspace/ceph-api/qa/tasks/mgr/test_module_selftest.py", line 87, in test_orchestrator
2021-02-08 11:08:23,392.392 INFO:__main__:    self._selftest_plugin("orchestrator")
2021-02-08 11:08:23,392.392 INFO:__main__:  File "/home/jenkins-build/build/workspace/ceph-api/qa/tasks/mgr/test_module_selftest.py", line 38, in _selftest_plugin
2021-02-08 11:08:23,392.392 INFO:__main__:    "mgr", "self-test", "module", module_name)
2021-02-08 11:08:23,392.392 INFO:__main__:  File "../qa/tasks/vstart_runner.py", line 1005, in raw_cluster_cmd
2021-02-08 11:08:23,392.392 INFO:__main__:    return self.run_cluster_cmd(**kwargs).stdout.getvalue()
2021-02-08 11:08:23,392.392 INFO:__main__:  File "../qa/tasks/vstart_runner.py", line 995, in run_cluster_cmd
2021-02-08 11:08:23,392.392 INFO:__main__:    return self.controller.run(**kwargs)
2021-02-08 11:08:23,393.393 INFO:__main__:  File "../qa/tasks/vstart_runner.py", line 399, in run
2021-02-08 11:08:23,393.393 INFO:__main__:    return self._do_run(**kwargs)
2021-02-08 11:08:23,393.393 INFO:__main__:  File "../qa/tasks/vstart_runner.py", line 467, in _do_run
2021-02-08 11:08:23,393.393 INFO:__main__:    proc.wait()
2021-02-08 11:08:23,393.393 INFO:__main__:  File "../qa/tasks/vstart_runner.py", line 213, in wait
2021-02-08 11:08:23,393.393 INFO:__main__:    raise CommandFailedError(self.args, self.exitstatus)
2021-02-08 11:08:23,393.393 INFO:__main__:teuthology.exceptions.CommandFailedError: Command failed with status 1: ['./bin/ceph', 'mgr', 'self-test', 'module', 'orchestrator']
2021-02-08 11:08:23,394.394 INFO:__main__:
Cannot find device "ceph-brx"

sebastian-philipp · 2021-02-08T16:42:55Z

I love it rebasing ❤️

sebastian-philipp · 2021-02-10T11:05:02Z

2021-02-10T04:30:07.501 DEBUG:teuthology.orchestra.run.smithi089:> sudo adjust-ulimits ceph-coverage /home/ubuntu/cephtest/archive/coverage timeout 120 ceph --cluster ceph orch set backend test_orchestrator
2021-02-10T04:30:07.817 DEBUG:teuthology.orchestra.run.smithi089:> sudo adjust-ulimits ceph-coverage /home/ubuntu/cephtest/archive/coverage timeout 120 ceph --cluster ceph orch host rm hostname
2021-02-10T04:30:08.107 INFO:tasks.ceph.mgr.x.smithi089.stderr:2021-02-10T04:30:08.106+0000 7f74d420f700 -1 mgr handle_command module 'orchestrator' command handler threw exception: OrchResult should either have an exception or a result
2021-02-10T04:30:08.107 INFO:tasks.ceph.mgr.x.smithi089.stderr:2021-02-10T04:30:08.107+0000 7f74d420f700 -1 mgr.server reply reply (22) Invalid argument Traceback (most recent call last):
2021-02-10T04:30:08.107 INFO:tasks.ceph.mgr.x.smithi089.stderr:  File "/usr/share/ceph/mgr/mgr_module.py", line 1319, in _handle_command
2021-02-10T04:30:08.107 INFO:tasks.ceph.mgr.x.smithi089.stderr:    return self.handle_command(inbuf, cmd)
2021-02-10T04:30:08.107 INFO:tasks.ceph.mgr.x.smithi089.stderr:  File "/usr/share/ceph/mgr/orchestrator/_interface.py", line 167, in handle_command
2021-02-10T04:30:08.107 INFO:tasks.ceph.mgr.x.smithi089.stderr:    return dispatch[cmd['prefix']].call(self, cmd, inbuf)
2021-02-10T04:30:08.107 INFO:tasks.ceph.mgr.x.smithi089.stderr:  File "/usr/share/ceph/mgr/mgr_module.py", line 389, in call
2021-02-10T04:30:08.107 INFO:tasks.ceph.mgr.x.smithi089.stderr:    return self.func(mgr, **kwargs)
2021-02-10T04:30:08.107 INFO:tasks.ceph.mgr.x.smithi089.stderr:  File "/usr/share/ceph/mgr/orchestrator/_interface.py", line 108, in <lambda>
2021-02-10T04:30:08.107 INFO:tasks.ceph.mgr.x.smithi089.stderr:    wrapper_copy = lambda *l_args, **l_kwargs: wrapper(*l_args, **l_kwargs)
2021-02-10T04:30:08.107 INFO:tasks.ceph.mgr.x.smithi089.stderr:  File "/usr/share/ceph/mgr/orchestrator/_interface.py", line 97, in wrapper
2021-02-10T04:30:08.108 INFO:tasks.ceph.mgr.x.smithi089.stderr:    return func(*args, **kwargs)
2021-02-10T04:30:08.108 INFO:tasks.ceph.mgr.x.smithi089.stderr:  File "/usr/share/ceph/mgr/orchestrator/module.py", line 335, in _remove_host
2021-02-10T04:30:08.108 INFO:tasks.ceph.mgr.x.smithi089.stderr:    raise_if_exception(completion)
2021-02-10T04:30:08.108 INFO:tasks.ceph.mgr.x.smithi089.stderr:  File "/usr/share/ceph/mgr/orchestrator/_interface.py", line 225, in raise_if_exception
2021-02-10T04:30:08.108 INFO:tasks.ceph.mgr.x.smithi089.stderr:    assert c.result is not None, 'OrchResult should either have an exception or a result'
2021-02-10T04:30:08.108 INFO:tasks.ceph.mgr.x.smithi089.stderr:AssertionError: OrchResult should either have an exception or a result

ERROR: /home/jenkins-build/build/workspace/ceph-pull-requests/src/pybind/mgr/dashboard/services/orchestrator.py Imports are incorrectly sorted and/or formatted.
Skipped 2 files
ERROR: InvocationError for command /home/jenkins-build/build/workspace/ceph-pull-requests/src/pybind/mgr/dashboard/.tox/lint/bin/isort . --check (exited with code 1)

2021-02-10T04:56:48.519 DEBUG:teuthology.orchestra.run.smithi002:> sudo adjust-ulimits ceph-coverage /home/ubuntu/cephtest/archive/coverage timeout 120 ceph --cluster ceph nfs cluster create cephfs test
2021-02-10T04:56:50.523 INFO:teuthology.orchestra.run.smithi002.stderr:Error EPERM: 'Module' object has no attribute '_orchestrator_wait'
2021-02-10T04:56:50.526 DEBUG:teuthology.orchestra.run:got remote process result: 1

sebastian-philipp · 2021-02-16T11:39:55Z

jenkins test make check

github-actions · 2021-02-18T11:22:40Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

varshar16

Following changes are nice to have. Otherwise looks good.

src/pybind/mgr/orchestrator/_interface.py

varshar16 · 2021-02-22T08:42:28Z

src/pybind/mgr/orchestrator/module.py

        raise_if_exception(completion)
        report = completion.result


report = raise_if_exception(completion)

I'd like to keep the changes to the interface limited. Follow-up might be to rename raise_if_exception to get_orch_result() or something.

I suggested because you made similar changes in _list_services(). If you are planning for follow-up PR then it is fine.

src/pybind/mgr/orchestrator/tests/test_orchestrator.py

varshar16 · 2021-02-22T09:24:23Z

src/pybind/mgr/test_orchestrator/module.py

@@ -20,41 +20,7 @@
 from mgr_module import MgrModule

 import orchestrator


Remove this one

that would blow-up the diff with unrelated changes.

varshar16 · 2021-02-22T09:25:25Z

src/pybind/mgr/test_orchestrator/module.py

-
-        return wrapper
-    return inner
+from orchestrator import handle_orch_error, raise_if_exception


Instead update the import here

I'd like to avoid introducing unrelated changes

As you are importing from orchestrator module here again, I suggested the change. If you don't plan do it then at least add a note to clean up later.

varshar16 · 2021-02-22T09:40:38Z

src/pybind/mgr/selftest/module.py

+            c = orchestrator.OrchResult(result=None, exception=ZeroDivisionError('hello, world'))
            return c


here too

return orchestrator.OrchResult(result=None, exception=ZeroDivisionError('hello, world'))

doc/mgr/orchestrator_modules.rst

varshar16 · 2021-02-22T09:43:28Z

src/pybind/mgr/volumes/fs/nfs.py

    completion = mgr.describe_service(service_type='nfs')
-    mgr._orchestrator_wait([completion])
    orchestrator.raise_if_exception(completion)
    return [cluster.spec.service_id for cluster in completion.result


result = orchestrator.raise_if_exception(completion)

varshar16 · 2021-02-22T09:45:21Z

src/pybind/mgr/volumes/fs/nfs.py

        completion = self.mgr.list_daemons(daemon_type='nfs')
-        self.mgr._orchestrator_wait([completion])
        orchestrator.raise_if_exception(completion)


nfs_daemon_ls = orchestrator.raise_if_exception(completion)

varshar16 · 2021-02-22T09:45:41Z

src/pybind/mgr/volumes/fs/nfs.py

-        self.mgr._orchestrator_wait([completion])
        orchestrator.raise_if_exception(completion)
        host_ip = []
        # Here completion.result is a list DaemonDescription objects


Update here as well

tchaikov · 2021-02-25T08:09:39Z

failures tracked by

github-actions · 2021-02-25T15:25:08Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

Greatly simplify the orchestrator interface Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>

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

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

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

varshar16

Looks good but follow-up PR is required for some of the changes.

sebastian-philipp · 2021-03-02T14:24:23Z

https://pulpito.ceph.com/swagner-2021-03-02_10:27:34-rados:cephadm-wip-swagner-testing-2021-03-01-1657-distro-basic-smithi/

sebastian-philipp added the cleanup label Feb 8, 2021

sebastian-philipp requested a review from jmolmo February 8, 2021 09:48

sebastian-philipp requested review from a team as code owners February 8, 2021 09:48

sebastian-philipp requested review from aaSharma14, nizamial09 and s0nea and removed request for a team February 8, 2021 09:48

github-actions bot added cephadm cephfs Ceph File System dashboard orchestrator pybind labels Feb 8, 2021

sebastian-philipp added the wip-swagner3-testing label Feb 8, 2021

liewegas reviewed Feb 8, 2021

View reviewed changes

sebastian-philipp added the needs-rebase label Feb 8, 2021

sebastian-philipp force-pushed the orch-remove-promise branch from 3b42756 to 5c1e213 Compare February 9, 2021 10:55

github-actions bot added documentation mgr and removed needs-rebase labels Feb 9, 2021

sebastian-philipp mentioned this pull request Feb 10, 2021

mgr/cephadm: disentangle offline status and maintenance mode #39163

Closed

3 tasks

sebastian-philipp removed the wip-swagner3-testing label Feb 10, 2021

sebastian-philipp force-pushed the orch-remove-promise branch from 57f75eb to 049d273 Compare February 10, 2021 12:41

sebastian-philipp requested a review from varshar16 February 16, 2021 19:24

github-actions bot added the needs-rebase label Feb 18, 2021

sebastian-philipp added the needs-qa label Feb 19, 2021

varshar16 reviewed Feb 22, 2021

View reviewed changes

tchaikov added the wip-kefu-testing label Feb 23, 2021

tchaikov removed needs-qa wip-kefu-testing labels Feb 25, 2021

github-actions bot added the needs-rebase label Feb 25, 2021

sebastian-philipp added 2 commits March 1, 2021 13:38

mgr/orch: replace Completion with OrchResult(Generic[T])

fcf7513

Greatly simplify the orchestrator interface Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>

mgr/orch: adapt orchestrator CLI to new interface

e4d7d76

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

sebastian-philipp force-pushed the orch-remove-promise branch from cd5436c to be5bd09 Compare March 1, 2021 12:42

github-actions bot removed the needs-rebase label Mar 1, 2021

sebastian-philipp added 11 commits March 1, 2021 16:48

mgr/orch: Remove old tests

b6c770a

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

mgr/cephadm: Adapt cephadm to new orch interface

68030be

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

mgr/rook: Adapt to new orch interface

42d5c89

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

mgr/test_orchestrator: adapt to now orch interface

1ecb93a

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

mgr/mds_autoscaler: adapt to now orch interface

1d40632

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

mgr/mds_autoscaler: Add to tox.ini

bca36ab

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

mgr/dashboard: adapt to now orch interface

b92dc71

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

mgr/selftest: adapt to now orch interface

828992d

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

doc/mgr/orchestrator_modules: adapt to now orch interface

d529f1d

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

mgr/volumes: adapt to now orch interface

a455dff

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

mgr/test_orchestrator: Refactor create_osds

0f22f24

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

sebastian-philipp force-pushed the orch-remove-promise branch from be5bd09 to 0f22f24 Compare March 1, 2021 15:51

sebastian-philipp requested a review from varshar16 March 1, 2021 15:52

sebastian-philipp added the wip-swagner-testing My Teuthology tests label Mar 1, 2021

varshar16 approved these changes Mar 2, 2021

View reviewed changes

sebastian-philipp merged commit 0cbec89 into ceph:master Mar 2, 2021

sebastian-philipp mentioned this pull request Mar 3, 2021

pacific: cephadm: Batch backport March (1) #39807

Merged

	def raise_if_exception(c: OrchResult[T]) -> T:
	"""
	Due to different sub-interpreters, this MUST not be in the `OrchResult` class.
	"""
	if c.serialized_exception is not None:
	try:
	e = pickle.loads(c.serialized_exception)
	except (KeyError, AttributeError):
	raise Exception(c.exception_str)
	raise e
	assert c.result is not None, 'OrchResult should either have an exception or a result'
	return c.result

		@@ -20,41 +20,7 @@
		from mgr_module import MgrModule

		import orchestrator

		c = orchestrator.OrchResult(result=None, exception=ZeroDivisionError('hello, world'))
		return c

Conversation

sebastian-philipp commented Feb 8, 2021

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastian-philipp commented Feb 8, 2021

Uh oh!

sebastian-philipp commented Feb 8, 2021

Uh oh!

sebastian-philipp commented Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebastian-philipp commented Feb 16, 2021

Uh oh!

github-actions bot commented Feb 18, 2021

Uh oh!

varshar16 left a comment

Choose a reason for hiding this comment

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!

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!

varshar16 Mar 2, 2021 • 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!

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!

tchaikov commented Feb 25, 2021

Uh oh!

github-actions bot commented Feb 25, 2021

Uh oh!

varshar16 left a comment

Choose a reason for hiding this comment

Uh oh!

sebastian-philipp commented Mar 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sebastian-philipp commented Feb 10, 2021 •

edited

Loading

varshar16 Mar 2, 2021 •

edited

Loading