From 88c35ea306ab03b50d64939ff1d5e0ccc379a9e8 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Wed, 7 Aug 2024 10:18:57 +0200 Subject: [PATCH] osbuild: make `inputs` `map()` function use fd for reply as well We recently hit the issue that `osbuild` crashed with: ``` Unable to decode response body "Traceback (most recent call last): File \"/usr/bin/osbuild\", line 33, in sys.exit(load_entry_point('osbuild==124', 'console_scripts', 'osbuild')()) File \"/usr/lib/python3.9/site-packages/osbuild/main_cli.py\", line 181, in osbuild_cli r = manifest.build( File \"/usr/lib/python3.9/site-packages/osbuild/pipeline.py\", line 477, in build res = pl.run(store, monitor, libdir, debug_break, stage_timeout) File \"/usr/lib/python3.9/site-packages/osbuild/pipeline.py\", line 376, in run results = self.build_stages(store, File \"/usr/lib/python3.9/site-packages/osbuild/pipeline.py\", line 348, in build_stages r = stage.run(tree, File \"/usr/lib/python3.9/site-packages/osbuild/pipeline.py\", line 213, in run data = ipmgr.map(ip, store) File \"/usr/lib/python3.9/site-packages/osbuild/inputs.py\", line 94, in map reply, _ = client.call_with_fds(\"map\", {}, fds) File \"/usr/lib/python3.9/site-packages/osbuild/host.py\", line 373, in call_with_fds kind, data = self.protocol.decode_message(ret) File \"/usr/lib/python3.9/site-packages/osbuild/host.py\", line 83, in decode_message raise ProtocolError(\"message empty\") osbuild.host.ProtocolError: message empty cannot run osbuild: exit status 1" into osbuild result: invalid character 'T' looking for beginning of value ... input/packages (org.osbuild.files): Traceback (most recent call last): input/packages (org.osbuild.files): File "/usr/lib/osbuild/inputs/org.osbuild.files", line 226, in input/packages (org.osbuild.files): main() input/packages (org.osbuild.files): File "/usr/lib/osbuild/inputs/org.osbuild.files", line 222, in main input/packages (org.osbuild.files): service.main() input/packages (org.osbuild.files): File "/usr/lib/python3.11/site-packages/osbuild/host.py", line 250, in main input/packages (org.osbuild.files): self.serve() input/packages (org.osbuild.files): File "/usr/lib/python3.11/site-packages/osbuild/host.py", line 284, in serve input/packages (org.osbuild.files): self.sock.send(reply, fds=reply_fds) input/packages (org.osbuild.files): File "/usr/lib/python3.11/site-packages/osbuild/util/jsoncomm.py", line 407, in send input/packages (org.osbuild.files): n = self._socket.sendmsg([serialized], cmsg, 0) input/packages (org.osbuild.files): ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ input/packages (org.osbuild.files): OSError: [Errno 90] Message too long ``` The underlying issue is that the reply of the `map()` call is too big for the buffer that `jsoncomm` uses. This problem existed before for the args of map and was fixed by introducing a temporary file in https://github.com/osbuild/osbuild/pull/1331 (and similarly before in https://github.com/osbuild/osbuild/pull/824). This commit writes the return values also into a file. This should fix the crash above and make the function more symetrical as well. Alternative/complementary version of https://github.com/osbuild/osbuild/pull/1833 Closes: HMS-4537 --- osbuild/inputs.py | 30 +++++++++++++++---------- osbuild/util/jsoncomm.py | 2 +- test/mod/test_inputs.py | 47 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 12 deletions(-) create mode 100644 test/mod/test_inputs.py diff --git a/osbuild/inputs.py b/osbuild/inputs.py index 229c1264..24c580e6 100644 --- a/osbuild/inputs.py +++ b/osbuild/inputs.py @@ -88,10 +88,12 @@ class InputManager: } } - with make_args_file(store.tmp, args) as fd: - fds = [fd] + with make_args_and_reply_files(store.tmp, args) as (fd_args, fd_reply): + fds = [fd_args, fd_reply] client = self.service_manager.start(f"input/{ip.name}", ip.info.path) - reply, _ = client.call_with_fds("map", {}, fds) + _, _ = client.call_with_fds("map", {}, fds) + with os.fdopen(os.dup(fd_reply)) as f: + reply = json.loads(f.read()) path = reply["path"] @@ -106,11 +108,12 @@ class InputManager: @contextlib.contextmanager -def make_args_file(tmp, args): - with tempfile.TemporaryFile("w+", dir=tmp, encoding="utf-8") as f: - json.dump(args, f) - f.seek(0) - yield f.fileno() +def make_args_and_reply_files(tmp, args): + with tempfile.TemporaryFile("w+", dir=tmp, encoding="utf-8") as f_args, \ + tempfile.TemporaryFile("w+", dir=tmp, encoding="utf-8") as f_reply: + json.dump(args, f_args) + f_args.seek(0) + yield f_args.fileno(), f_reply.fileno() class InputService(host.Service): @@ -126,9 +129,11 @@ class InputService(host.Service): def stop(self): self.unmap() - def dispatch(self, method: str, _, _fds): + def dispatch(self, method: str, _, fds): if method == "map": - with os.fdopen(_fds.steal(0)) as f: + # map() sends fd[0] to read the arguments from and fd[1] to + # write the reply back. This avoids running into EMSGSIZE + with os.fdopen(fds.steal(0)) as f: args = json.load(f) store = StoreClient(connect_to=args["api"]["store"]) r = self.map(store, @@ -136,6 +141,9 @@ class InputService(host.Service): args["refs"], args["target"], args["options"]) - return r, None + with os.fdopen(fds.steal(1), "w") as f: + f.write(json.dumps(r)) + f.seek(0) + return "{}", None raise host.ProtocolError("Unknown method") diff --git a/osbuild/util/jsoncomm.py b/osbuild/util/jsoncomm.py index 04821257..8a0cbb7c 100644 --- a/osbuild/util/jsoncomm.py +++ b/osbuild/util/jsoncomm.py @@ -35,7 +35,7 @@ class FdSet: def __init__(self, *, rawfds): for i in rawfds: if not isinstance(i, int) or i < 0: - raise ValueError() + raise ValueError(f"unexpected fd {i}") self._fds = rawfds diff --git a/test/mod/test_inputs.py b/test/mod/test_inputs.py new file mode 100644 index 00000000..915d1313 --- /dev/null +++ b/test/mod/test_inputs.py @@ -0,0 +1,47 @@ +import json +import os +from unittest.mock import patch + +from osbuild import inputs +from osbuild.util.jsoncomm import FdSet + + +class FakeInputService(inputs.InputService): + def __init__(self, args): # pylint: disable=super-init-not-called + # do not call "super().__init__()" here to make it testable + self.map_calls = [] + + def map(self, _store, origin, refs, target, options): + self.map_calls.append([origin, refs, target, options]) + return "complex", 2, "reply" + + +def test_inputs_dispatches_map(tmp_path): + store_api_path = tmp_path / "api-store" + store_api_path.write_text("") + + args_path = tmp_path / "args" + reply_path = tmp_path / "reply" + args = { + "api": { + "store": os.fspath(store_api_path), + }, + "origin": "some-origin", + "refs": "some-refs", + "target": "some-target", + "options": "some-options", + } + args_path.write_text(json.dumps(args)) + reply_path.write_text("") + + with args_path.open() as f_args, reply_path.open("w") as f_reply: + fd_args, fd_reply = os.dup(f_args.fileno()), os.dup(f_reply.fileno()) + fds = FdSet.from_list([fd_args, fd_reply]) + fake_service = FakeInputService(args="some") + with patch.object(inputs, "StoreClient"): + r = fake_service.dispatch("map", None, fds) + assert r == ('{}', None) + assert fake_service.map_calls == [ + ["some-origin", "some-refs", "some-target", "some-options"], + ] + assert reply_path.read_text() == '["complex", 2, "reply"]'