From 42ba935ac37fa185ec6f1441ed98a256f08b1385 Mon Sep 17 00:00:00 2001 From: Albert Esteve Date: Fri, 22 Jul 2022 11:07:52 +0200 Subject: [PATCH 1/3] images_test: import units Import and use units constants in the test to improve readability. Signed-off-by: Albert Esteve --- test/handlers/images_test.py | 55 ++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/test/handlers/images_test.py b/test/handlers/images_test.py index f00875a5..c6ceb03d 100644 --- a/test/handlers/images_test.py +++ b/test/handlers/images_test.py @@ -15,6 +15,7 @@ from ovirt_imageio._internal import config from ovirt_imageio._internal import server +from ovirt_imageio._internal.units import KiB, MiB from .. import testutil from .. import http @@ -236,7 +237,7 @@ def test_upload_invalid_range(tmpdir, srv, client, content_range): def test_upload_close_connection(tmpdir, srv, client): - image_size = 4096 + image_size = 4 * KiB image = testutil.create_tempfile(tmpdir, "image", b"a" * image_size) ticket = testutil.create_ticket( url="file://" + str(image), size=image_size) @@ -296,24 +297,24 @@ def test_download(tmpdir, srv, client, rng, start, end): def test_download_image_size_gt_ticket_size(tmpdir, srv, client): image = testutil.create_tempfile(tmpdir, "image", size=8192) - ticket = testutil.create_ticket(url="file://" + str(image), size=4096) + ticket = testutil.create_ticket(url="file://" + str(image), size=4 * KiB) srv.auth.add(ticket) res = client.get("/images/" + ticket["uuid"]) assert res.status == 200 - assert len(res.read()) == 4096 + assert len(res.read()) == 4 * KiB def test_download_ticket_size_gt_image_size(tmpdir, srv, client): - image = testutil.create_tempfile(tmpdir, "image", size=4096) + image = testutil.create_tempfile(tmpdir, "image", size=4 * KiB) ticket = testutil.create_ticket(url="file://" + str(image), size=8192) srv.auth.add(ticket) res = client.get("/images/" + ticket["uuid"]) assert res.status == 200 - assert len(res.read()) == 4096 + assert len(res.read()) == 4 * KiB def test_download_range_forbidden(tmpdir, srv, client): - image = testutil.create_tempfile(tmpdir, "image", size=4096) + image = testutil.create_tempfile(tmpdir, "image", size=4 * KiB) ticket = testutil.create_ticket(url="file://" + str(image), size=8192) srv.auth.add(ticket) res = client.get("/images/" + ticket["uuid"], @@ -324,7 +325,7 @@ def test_download_range_forbidden(tmpdir, srv, client): def test_download_range_unavailable(tmpdir, srv, client): image = testutil.create_tempfile(tmpdir, "image", size=8192) - ticket = testutil.create_ticket(url="file://" + str(image), size=4096) + ticket = testutil.create_ticket(url="file://" + str(image), size=4 * KiB) srv.auth.add(ticket) res = client.get("/images/" + ticket["uuid"], headers={"Range": "bytes=0-4096"}) @@ -333,7 +334,7 @@ def test_download_range_unavailable(tmpdir, srv, client): def test_download_no_range(tmpdir, srv, client): - size = 1024 + size = KiB image = testutil.create_tempfile(tmpdir, "image", size=size) ticket = testutil.create_ticket(url="file://" + str(image), size=size) srv.auth.add(ticket) @@ -344,7 +345,7 @@ def test_download_no_range(tmpdir, srv, client): def test_download_extends_ticket(tmpdir, srv, client, fake_time): - size = 1024 + size = KiB image = testutil.create_tempfile(tmpdir, "image", size=size) ticket = testutil.create_ticket(url="file://" + str(image), size=size) srv.auth.add(ticket) @@ -381,7 +382,7 @@ def test_download_partial_not_satistieble(tmpdir, srv, client): # actual image size reported by vdsm - one byte difference is enough to # cause a failure. # See https://bugzilla.redhat.com/1512315. - size = 1024 + size = KiB image = testutil.create_tempfile(tmpdir, "image", size=size) ticket = testutil.create_ticket(url="file://" + str(image), size=size + 1) srv.auth.add(ticket) @@ -397,7 +398,7 @@ def test_download_partial_no_range(tmpdir, srv, client): # is only an upper limit. Or maybe we should treat the ticket size as the # expected size? # This is another variant of https://bugzilla.redhat.com/1512315. - size = 1024 + size = KiB image = testutil.create_tempfile(tmpdir, "image", size=size) ticket = testutil.create_ticket(url="file://" + str(image), size=size + 1) srv.auth.add(ticket) @@ -405,7 +406,7 @@ def test_download_partial_no_range(tmpdir, srv, client): assert res.status == http_client.OK # Should return the available image data, not the ticket size. Reading # this response will fail with IncompleteRead. - assert res.length == 1024 + assert res.length == KiB def test_download_partial_no_range_empty(tmpdir, srv, client): @@ -413,7 +414,7 @@ def test_download_partial_no_range_empty(tmpdir, srv, client): # http response that fail on the client side with BadStatusLine: ''. # See https://bugzilla.redhat.com/1512312 image = testutil.create_tempfile(tmpdir, "image") # Empty image - ticket = testutil.create_ticket(url="file://" + str(image), size=1024) + ticket = testutil.create_ticket(url="file://" + str(image), size=KiB) srv.auth.add(ticket) res = client.get("/images/" + ticket["uuid"]) assert res.status == http_client.OK @@ -421,7 +422,7 @@ def test_download_partial_no_range_empty(tmpdir, srv, client): def test_download_no_range_end(tmpdir, srv, client): - size = 1024 + size = KiB image = testutil.create_tempfile(tmpdir, "image", size=size) ticket = testutil.create_ticket(url="file://" + str(image), size=size) srv.auth.add(ticket) @@ -433,7 +434,7 @@ def test_download_no_range_end(tmpdir, srv, client): def test_download_holes(tmpdir, srv, client): - size = 1024 + size = KiB image = testutil.create_tempfile(tmpdir, "image", size=size) ticket = testutil.create_ticket(url="file://" + str(image), size=size) srv.auth.add(ticket) @@ -445,7 +446,7 @@ def test_download_holes(tmpdir, srv, client): def test_download_filename_in_ticket(tmpdir, srv, client): - size = 1024 + size = KiB filename = "\u05d0.raw" # hebrew aleph image = testutil.create_tempfile(tmpdir, "image", size=size) ticket = testutil.create_ticket(url="file://" + str(image), size=size, @@ -472,7 +473,7 @@ def test_download_out_of_range(tmpdir, srv, client, rng, end): def test_download_progress(tmpdir, srv, client, monkeypatch): # We need to read at least one buffer to update the transfered value. - monkeypatch.setattr(srv.config.backend_file, "buffer_size", 1024**2) + monkeypatch.setattr(srv.config.backend_file, "buffer_size", MiB) # And we need to request enough data so the server does not complete before # the client read all the data. @@ -511,7 +512,7 @@ def test_download_progress(tmpdir, srv, client, monkeypatch): def test_download_close_connection(tmpdir, srv, client): - image_size = 4096 + image_size = 4 * KiB image = testutil.create_tempfile(tmpdir, "image", b"a" * image_size) ticket = testutil.create_ticket( url="file://" + str(image), size=image_size) @@ -705,7 +706,7 @@ def test_options_all(srv, client): def test_options_read_write(srv, client, tmpdir): - size = 128 * 1024 + size = 128 * KiB image = testutil.create_tempfile(tmpdir, "image", size=size) ticket = testutil.create_ticket( url="file://" + str(image), size=size, ops=["read", "write"]) @@ -721,7 +722,7 @@ def test_options_read_write(srv, client, tmpdir): def test_options_read(srv, client, tmpdir): - size = 128 * 1024 + size = 128 * KiB image = testutil.create_tempfile(tmpdir, "image", size=size) ticket = testutil.create_ticket( url="file://" + str(image), size=size, ops=["read"]) @@ -737,7 +738,7 @@ def test_options_read(srv, client, tmpdir): def test_options_write(srv, client, tmpdir): - size = 128 * 1024 + size = 128 * KiB image = testutil.create_tempfile(tmpdir, "image", size=size) ticket = testutil.create_ticket( url="file://" + str(image), size=size, ops=["write"]) @@ -754,7 +755,7 @@ def test_options_write(srv, client, tmpdir): def test_options_extends_ticket(srv, client, tmpdir, fake_time): - size = 128 * 1024 + size = 128 * KiB image = testutil.create_tempfile(tmpdir, "image", size=size) ticket = testutil.create_ticket(url="file://" + str(image), size=size) srv.auth.add(ticket) @@ -786,7 +787,7 @@ def test_options_for_nonexistent_ticket(srv, client): def test_options_ticket_expired(srv, client, tmpdir, fake_time): - size = 128 * 1024 + size = 128 * KiB image = testutil.create_tempfile(tmpdir, "image", size=size) ticket = testutil.create_ticket( url="file://" + str(image), size=size, timeout=300) @@ -828,9 +829,9 @@ def test_response_version_error(tmpdir, srv, client): ]) def test_keep_alive_connection_on_success(tmpdir, srv, client, method, body): # After successful request the connection should remain open. - image = testutil.create_tempfile(tmpdir, "image", size=1024) + image = testutil.create_tempfile(tmpdir, "image", size=KiB) ticket = testutil.create_ticket(url="file://" + str(image), - size=1024) + size=KiB) srv.auth.add(ticket) uri = "/images/%(uuid)s" % ticket # Disabling auto_open so we can test if a connection was closed. @@ -915,7 +916,7 @@ def test_cors_options_all(srv, client): def test_cors_get_ok(tmpdir, srv, client): - size = 8192 + size = 8 * KiB image = testutil.create_tempfile(tmpdir, "image", size=size) ticket = testutil.create_ticket(url="file://" + str(image), size=size) srv.auth.add(ticket) @@ -944,7 +945,7 @@ def test_cors_get_error(srv, client): def test_cors_put_ok(tmpdir, srv, client): - size = 8192 + size = 8 * KiB image = testutil.create_tempfile(tmpdir, "image", size=size) ticket = testutil.create_ticket(url="file://" + str(image), size=size) srv.auth.add(ticket) From 51cd6d1c73eb7b0adbe981ee61f51860d947d5a4 Mon Sep 17 00:00:00 2001 From: Albert Esteve Date: Mon, 8 Aug 2022 10:09:59 +0200 Subject: [PATCH 2/3] images.md: fix extents typo Fix small typo on extents example. Signed-off-by: Albert Esteve --- docs/images.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/images.md b/docs/images.md index 74ec19c1..854beb34 100644 --- a/docs/images.md +++ b/docs/images.md @@ -268,7 +268,7 @@ Since 2.0 Request zero extents: - GET /images/{ticket-id}/extent + GET /images/{ticket-id}/extents Response: @@ -280,7 +280,7 @@ Response: Request dirty extents: - GET /images/{ticket-id}/extent?context=dirty + GET /images/{ticket-id}/extents?context=dirty Response: From 32e65736bfa69e9e404a4269e6b1f976463caabb Mon Sep 17 00:00:00 2001 From: Nir Soffer Date: Fri, 23 Jul 2021 22:49:15 +0300 Subject: [PATCH 3/3] options: Report image format and virtual size Report image virtual size in OPTIONS so clients can get the image size without a possibly slow extent call. Report the format since OPTIONS can report the virtual size only when the backend provides raw format. This is used when using the http backend to report OPTIONS to the client. Reporting virtual size is easy with the nbd and memory backends since they always use raw format. When using file backend and qcow2 image, we don't have access to the image virtual size, and this size is not helpful to the user uploading or downloading data. Currently we don't know about the image format since engine does not report it in the ticket. The http backend reports the info from the remote server, so it depends on the backend used by the remote server, and on having new server reporting the format and size. To keep code and the API simple, we report virtual size only when using the nbd and memory backends. When engine will report the image format for the file backend, we can also report the size for raw images access via the file backend. Change-Id: I89118301c98dc2d11c25a4d1e7ef83df26336f01 Related: #67 Bug-Url: https://bugzilla.redhat.com/1924945 Signed-off-by: Nir Soffer --- docs/images.md | 34 ++++++-- ovirt_imageio/_internal/backends/file.py | 7 ++ ovirt_imageio/_internal/backends/http.py | 36 +++++++-- ovirt_imageio/_internal/backends/memory.py | 4 + ovirt_imageio/_internal/backends/nbd.py | 4 + ovirt_imageio/_internal/handlers/images.py | 6 ++ test/handlers/images_test.py | 92 ++++++++++++++++++++-- 7 files changed, 164 insertions(+), 19 deletions(-) diff --git a/docs/images.md b/docs/images.md index 854beb34..af91de11 100644 --- a/docs/images.md +++ b/docs/images.md @@ -84,6 +84,25 @@ When using multiple writers, each writer should modify a distinct byte range. If two writers modify the same byte range concurrently they will overwrite each other data. +### transfer_format + +The transfer data format. Always available when using the `nbd` and +`memory` backends, not available when using the `file` backend. When +using the `http` backend `transfer_format` is available only if the +remote server reports it. + +Since 2.4.6. + +### virtual_size + +The underlying image virtual size. Available only when the backend is +using `raw` transfer format. Always available when using the `nbd` and +`memory` backends, not available when using the `file` backend. When +using the `http` backend the `virtual_size` is available only if the +remote server reports it. + +Since 2.4.6. + ### Errors Specific errors for OPTIONS request: @@ -112,9 +131,9 @@ Response: Content-Length: 122 {"unix_socket": "\u0000/org/ovirt/imageio", "features": ["extents", "zero", "flush"], - "max_readers": 8, "max_writers": 8} + "max_readers": 8, "max_writers": 8, "transfer_format": "raw", "virtual_size": 53687091200} -Get options for ticket-id with read-write access using nbd backend: +Get options for ticket-id with read-write access using `nbd` backend: $ curl -k -X OPTIONS https://server:54322/images/{ticket-id} | jq { @@ -125,11 +144,13 @@ Get options for ticket-id with read-write access using nbd backend: "flush" ], "max_readers": 8, - "max_writers": 8 + "max_writers": 8, + "transfer_format": "raw", + "virtual_size": 53687091200 } -The nbd backend is used when specifying the "raw" transfer format when -creating an image transfer in oVirt API. +The nbd backend is used when creating an image transfer with +format="raw" in oVirt API. Get options for ticket-id with read-only access using file backend: @@ -143,6 +164,9 @@ Get options for ticket-id with read-only access using file backend: "max_writers": 1 } +Note that `virtual_size` and `transfer_format` are not reported, and this +backend does not support multiple writers. + Get all available options for the special `*` ticket: $ curl -sk -X OPTIONS 'https://server:54322/images/*' | jq diff --git a/ovirt_imageio/_internal/backends/file.py b/ovirt_imageio/_internal/backends/file.py index 03626c6e..66edaf24 100644 --- a/ovirt_imageio/_internal/backends/file.py +++ b/ovirt_imageio/_internal/backends/file.py @@ -74,6 +74,13 @@ def __init__(self, fio, sparse=False, max_connections=8): def max_readers(self): return self._max_connections + @property + def format(self): + """ + Currently we don't have access to the underlying disk format. + """ + return None + # io.FileIO interface def readinto(self, buf): diff --git a/ovirt_imageio/_internal/backends/http.py b/ovirt_imageio/_internal/backends/http.py index 93335a0d..3e6d109d 100644 --- a/ovirt_imageio/_internal/backends/http.py +++ b/ovirt_imageio/_internal/backends/http.py @@ -63,7 +63,6 @@ def __init__(self, url, cafile=None, secure=True, connect_timeout=10, self._connect_timeout = connect_timeout self._read_timeout = read_timeout self._position = 0 - self._size = None self._extents = {} # Initlized during connection. @@ -74,6 +73,8 @@ def __init__(self, url, cafile=None, secure=True, connect_timeout=10, self._can_flush = False self._max_readers = 1 self._max_writers = 1 + self._format = None + self._size = None if connect: self._connect() @@ -104,8 +105,10 @@ def clone(self): backend._can_flush = self._can_flush backend._max_readers = self._max_readers backend._max_writers = self._max_writers + backend._format = self._format - # Copy size and extents to save expensive EXTENTS calls. + # Copy extents and size to save expensive EXTENTS calls with old + # imageio servers. backend._size = self._size for ctx in list(self._extents): backend._extents[ctx] = self._extents[ctx].copy() @@ -133,6 +136,10 @@ def _connect(self): # max_writers does not support multiple writers. self._max_writers = options.get("max_writers", 1) + # Newer server reports also transfer_format and virtual_size. + self._format = options.get("transfer_format") + self._size = options.get("virtual_size") + self._optimize_connection(options.get("unix_socket")) except Exception: self._con.close() @@ -155,6 +162,14 @@ def max_readers(self): def max_writers(self): return self._max_writers + @property + def format(self): + """ + Will be None for old server (imageio < 2.4.6) or when server is using + file backend. + """ + return self._format + # Preferred interface. def read_from(self, reader, length, buf): @@ -325,10 +340,17 @@ def seek(self, n, how=os.SEEK_SET): return self._position def size(self): - # We have 2 bad options: - # - Get last extent, may be slow, and may not be neded otherwise. - # - Emulate HEAD request, logging tracebacks in the remote server. - # Getting extents is more polite, so lets use it if we can. + """ + Return backend size in bytes. + + With newer server (imageio >= 2.4.6) reporting the virtual size the + size is intialized in connect(). + + Otherwise we have 2 bad options: + - Get the last extent, may be slow, and may not be neded otherwise. + - Emulate HEAD request, logging tracebacks in the remote server. + Getting extents is more polite, so lets use it if we can. + """ if self._size is None: if self._can_extents: last = list(self.extents())[-1] @@ -454,7 +476,7 @@ def _put_header(self, length): self._con.putheader("content-length", length) self._con.putheader("content-type", "application/octet-stream") self._con.putheader("content-range", "bytes {}-{}/*".format( - self._position, self._position + length - 1)) + self._position, self._position + length - 1)) self._con.endheaders() diff --git a/ovirt_imageio/_internal/backends/memory.py b/ovirt_imageio/_internal/backends/memory.py index 9f54259d..e15e52ea 100644 --- a/ovirt_imageio/_internal/backends/memory.py +++ b/ovirt_imageio/_internal/backends/memory.py @@ -72,6 +72,10 @@ def max_writers(self): # support more than one writer concurrently. return 1 + @property + def format(self): + return "raw" + # io.BaseIO interface def readinto(self, buf): diff --git a/ovirt_imageio/_internal/backends/nbd.py b/ovirt_imageio/_internal/backends/nbd.py index 3ee683af..5fc156ca 100644 --- a/ovirt_imageio/_internal/backends/nbd.py +++ b/ovirt_imageio/_internal/backends/nbd.py @@ -95,6 +95,10 @@ def max_readers(self): def max_writers(self): return self._max_connections + @property + def format(self): + return "raw" + # Backend interface def readinto(self, buf): diff --git a/ovirt_imageio/_internal/handlers/images.py b/ovirt_imageio/_internal/handlers/images.py index cade4e5e..2f4f68ef 100644 --- a/ovirt_imageio/_internal/handlers/images.py +++ b/ovirt_imageio/_internal/handlers/images.py @@ -268,5 +268,11 @@ def options(self, req, resp, ticket_id): options["max_readers"] = ctx.backend.max_readers options["max_writers"] = ctx.backend.max_writers + # Optional backend options. + if ctx.backend.format is not None: + options["transfer_format"] = ctx.backend.format + if ctx.backend.format == "raw": + options["virtual_size"] = ctx.backend.size() + resp.headers["allow"] = ",".join(allow) resp.send_json(options) diff --git a/test/handlers/images_test.py b/test/handlers/images_test.py index c6ceb03d..d38325bc 100644 --- a/test/handlers/images_test.py +++ b/test/handlers/images_test.py @@ -14,8 +14,9 @@ import pytest from ovirt_imageio._internal import config +from ovirt_imageio._internal import qemu_img from ovirt_imageio._internal import server -from ovirt_imageio._internal.units import KiB, MiB +from ovirt_imageio._internal.units import KiB, MiB, GiB from .. import testutil from .. import http @@ -705,7 +706,7 @@ def test_options_all(srv, client): assert "max_writers" not in options -def test_options_read_write(srv, client, tmpdir): +def test_options_file_read_write(srv, client, tmpdir): size = 128 * KiB image = testutil.create_tempfile(tmpdir, "image", size=size) ticket = testutil.create_ticket( @@ -717,11 +718,15 @@ def test_options_read_write(srv, client, tmpdir): assert set(res.getheader("allow").split(',')) == allows options = json.loads(res.read()) assert set(options["features"]) == ALL_FEATURES + + # File backend specific options. assert options["max_readers"] == srv.config.daemon.max_connections - assert options["max_writers"] == 1 # Using file backend. + assert options["max_writers"] == 1 + assert "transfer_format" not in options + assert "virtual_size" not in options -def test_options_read(srv, client, tmpdir): +def test_options_file_read(srv, client, tmpdir): size = 128 * KiB image = testutil.create_tempfile(tmpdir, "image", size=size) ticket = testutil.create_ticket( @@ -733,11 +738,15 @@ def test_options_read(srv, client, tmpdir): assert set(res.getheader("allow").split(',')) == allows options = json.loads(res.read()) assert set(options["features"]) == BASE_FEATURES + + # File backend specific options. assert options["max_readers"] == srv.config.daemon.max_connections - assert options["max_writers"] == 1 # Using file backend. + assert options["max_writers"] == 1 + assert "transfer_format" not in options + assert "virtual_size" not in options -def test_options_write(srv, client, tmpdir): +def test_options_file_write(srv, client, tmpdir): size = 128 * KiB image = testutil.create_tempfile(tmpdir, "image", size=size) ticket = testutil.create_ticket( @@ -750,8 +759,77 @@ def test_options_write(srv, client, tmpdir): assert set(res.getheader("allow").split(',')) == allows options = json.loads(res.read()) assert set(options["features"]) == ALL_FEATURES + + # File backend specific options. + assert options["max_readers"] == srv.config.daemon.max_connections + assert options["max_writers"] == 1 + assert "transfer_format" not in options + assert "virtual_size" not in options + + +@pytest.mark.parametrize("fmt", ["raw", "qcow2"]) +def test_options_nbd_read(srv, client, tmpdir, nbd_server, fmt): + # Create disk. + size = GiB + disk = str(tmpdir.join(f"disk.{fmt}")) + qemu_img.create(disk, fmt, size=size) + + # Start nbd server exporting the disk. + nbd_server.image = disk + nbd_server.fmt = fmt + nbd_server.read_only = True + nbd_server.start() + + # Add ticket using nbd server url. + ticket = testutil.create_ticket( + url=nbd_server.sock.url(), size=size, ops=["read"]) + srv.auth.add(ticket) + + # Get OPTIONS. + res = client.options("/images/" + ticket["uuid"]) + allows = {"OPTIONS", "GET"} + assert res.status == 200 + assert set(res.getheader("allow").split(',')) == allows + options = json.loads(res.read()) + assert set(options["features"]) == BASE_FEATURES + + # NBD backend specific options. + assert options["max_readers"] == srv.config.daemon.max_connections + assert options["max_writers"] == srv.config.daemon.max_connections + assert options["transfer_format"] == "raw" + assert options["virtual_size"] == size + + +@pytest.mark.parametrize("fmt", ["raw", "qcow2"]) +def test_options_nbd_write(srv, client, tmpdir, nbd_server, fmt): + # Create disk. + size = GiB + disk = str(tmpdir.join(f"disk.{fmt}")) + qemu_img.create(disk, fmt, size=size) + + # Start nbd server exporting the disk. + nbd_server.image = disk + nbd_server.fmt = fmt + nbd_server.start() + + # Add ticket using nbd server url. + ticket = testutil.create_ticket( + url=nbd_server.sock.url(), size=size, ops=["write"]) + srv.auth.add(ticket) + + # Get OPTIONS. + res = client.options("/images/" + ticket["uuid"]) + allows = {"OPTIONS", "GET", "PUT", "PATCH"} + assert res.status == 200 + assert set(res.getheader("allow").split(',')) == allows + options = json.loads(res.read()) + assert set(options["features"]) == ALL_FEATURES + + # NBD backend specific options. assert options["max_readers"] == srv.config.daemon.max_connections - assert options["max_writers"] == 1 # Using file backend. + assert options["max_writers"] == srv.config.daemon.max_connections + assert options["transfer_format"] == "raw" + assert options["virtual_size"] == size def test_options_extends_ticket(srv, client, tmpdir, fake_time):