Please use reply-all to make the discussion public.
---------- Forwarded message ---------
From:
Amit Bawer <abawer@redhat.com>Date: Tue, May 7, 2019 at 4:03 PM
Subject: Re: [PATCH 3/4] tests: Test wrong sector size with 4k storage
To: Nir Soffer <
nirsof@gmail.com>
Cc: Pavel Bar <
pbar@redhat.com>
When using 4k disk and sector=512, we expect sanlock to fail with
SanlockException(EINVAL) when writing or reading.
Two tests are marked a expected failure:
- write_resource() - not sure how sanlock can succeed with wrong sector
size - this may be a bug.
- read_resource_owners() - sanlock uses a fallback mechanism, hiding
wrong value from the user. I think we can make sanlock more strict.
Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
tests/python_test.py | 79 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 79 insertions(+)
diff --git a/tests/python_test.py b/tests/python_test.py
index f0ecd06..aa1f7f5 100644
--- a/tests/python_test.py
+++ b/tests/python_test.py
@@ -99,10 +99,35 @@ def test_write_lockspace_4k(sanlock_daemon, align):
# Check that sanlock did not write after the lockspace area.
f.seek(align)
assert f.read(4096) == b"X" * 4096
+@pytest.mark.skipif(
+ "SANLOCK_4K_PATH" not in os.environ,
+ reason="Requires user specified path using 4k block size")
+def test_write_lockspace_4k_invalid_sector_size(sanlock_daemon):
+ path = os.environ["SANLOCK_4K_PATH"]
since the 4k block is created outside the test itself manually,
maybe we should verify that 4k block size file actually exists in path before calling sanlock API
If the file does not exits, you should get a clear error when sanlock cannot access the file,
but sanlock error messages are sometimes confusing.
Checking for 4k sector size is possible with block device, but we can only guess the sector
when we have a file system, or depend on file system specific tools or properties. I think this
it is good enough to document the usage and assume that developers will read the docs.
However I think adding a check for existing file is very easy - can be something like:
@pytest.fixture
def user_4k_path():
path = os.environ.get("USER_4K_PATH")
if path is None:
pytest.skip("message...")
if not os.path.exist(path):
raise RuntimeError("message...")
return path
And now can use write these tests like this:
def test_foo(user_4k_path):
use path...
+
+ with pytest.raises(sanlock.SanlockException) as e:
+ sanlock.write_lockspace(
+ "name", path, iotimeout=1, sector=SECTOR_SIZE_512)
+ assert e.value.errno == errno.EINVAL
+
+
+@pytest.mark.skipif(
+ "SANLOCK_4K_PATH" not in os.environ,
+ reason="Requires user specified path using 4k block size")
+def test_read_lockspace_4k_invalid_sector_size(sanlock_daemon):
+ path = os.environ["SANLOCK_4K_PATH"]
since the 4k block is created outside the test itself manually,
maybe we should verify that 4k block size file actually exists in path before calling sanlock API.
There is no need to repeat the same comment more than once, but I gave bad example repeating
the code, so I cannot complain :-)
+
+ sanlock.write_lockspace("name", path, iotimeout=1, sector=SECTOR_SIZE_4K)
+
+ with pytest.raises(sanlock.SanlockException) as e:
+ sanlock.read_lockspace(path, sector=SECTOR_SIZE_512)
+ assert e.value.errno == errno.EINVAL
+
+
@pytest.mark.parametrize("size,offset", [
# Smallest offset.
(MIN_RES_SIZE, 0),
# Large offset.
(LARGE_FILE_SIZE, LARGE_FILE_SIZE - MIN_RES_SIZE),
@@ -185,10 +210,64 @@ def test_write_resource_4k(sanlock_daemon, align):
# Check that sanlock did not write after the resource area.
f.seek(align)
assert f.read(4096) == b"X" * 4096
+@pytest.mark.skipif(
+ "SANLOCK_4K_PATH" not in os.environ,
+ reason="Requires user specified path using 4k block size")
+@pytest.mark.xfail(reason="need to investigate why the call succeed")
+def test_write_resource_4k_invalid_sector_size(sanlock_daemon):
+ path = os.environ["SANLOCK_4K_PATH"]
since the 4k block is created outside the test itself manually,
maybe we should verify that 4k block size file actually exists in path before calling sanlock API.
+ disks = [(path, 0)]
+
+ with pytest.raises(sanlock.SanlockException) as e:
+ sanlock.write_resource(
+ "ls_name", "res_name", disks, sector=SECTOR_SIZE_512)
+ assert e.value.errno == errno.EINVAL
+
+
+@pytest.mark.skipif(
+ "SANLOCK_4K_PATH" not in os.environ,
+ reason="Requires user specified path using 4k block size")
+def test_read_resource_4k_invalid_sector_size(sanlock_daemon):
+ path = os.environ["SANLOCK_4K_PATH"]
since the 4k block is created outside the test itself manually,
maybe we should verify that 4k block size file actually exists in path before calling sanlock API.
+ disks = [(path, 0)]
+
+ sanlock.write_resource(
+ "ls_name",
+ "res_name",
+ disks,
+ align=ALIGNMENT_1M,
+ sector=SECTOR_SIZE_4K)
+
+ with pytest.raises(sanlock.SanlockException) as e:
+ sanlock.read_resource(path, sector=SECTOR_SIZE_512)
+ assert e.value.errno == errno.EINVAL
+
+
+@pytest.mark.skipif(
+ "SANLOCK_4K_PATH" not in os.environ,
+ reason="Requires user specified path using 4k block size")
+@pytest.mark.xfail(reason="fallback hides wrong value from caller")
+def test_read_resource_owners_4k_invalid_sector_size(sanlock_daemon):
+ path = os.environ["SANLOCK_4K_PATH"]
since the 4k block is created outside the test itself manually,
maybe we should verify that 4k block size file actually exists in path before calling sanlock API.
+ disks = [(path, 0)]
+
+ sanlock.write_resource(
+ "ls_name",
+ "res_name",
+ disks,
+ align=ALIGNMENT_1M,
+ sector=SECTOR_SIZE_4K)
+
+ with pytest.raises(sanlock.SanlockException) as e:
+ sanlock.read_resource_owners(
+ "ls_name", "res_name", disks, sector=SECTOR_SIZE_512)
+ assert e.value.errno == errno.EINVAL
+
+
@pytest.mark.parametrize("size,offset", [
# Smallest offset.
(MIN_RES_SIZE, 0),
# Large offset.
(LARGE_FILE_SIZE, LARGE_FILE_SIZE - MIN_RES_SIZE),
--
2.17.2