[PATCH v2 0/6] Complete 4k API
by Nir Soffer
Add the missing align and sector arguments to read_resource_owners() and
add infrastructue for testing 4k storage and initial tests.
The tests for using wrong sector size revealed that
read_resouce_owners() ignores wrong align and sector size. Make it more
strict to be consistent with other APIs.
Changes since v1:
- Update python/example.py (Vojta)
- Improve test infrastructure for testing 4k storage (Amit, Pavel)
- Improve comments and commit messages (Pavel)
- Make the validation code in read_resource_owners more clear (Pavel)
- Add test for read_resource_owners with invalid align value
- Don't override align_size if sector_size is not set
- Reject invalid align in sanlock_read_resource_owners()
v1 was here:
https://lists.fedorahosted.org/archives/list/sanlock-devel@lists.fedoraho...
Nir Soffer (6):
python: Add align and sector arguments to read_resource_owners()
tests: Start tests for 4k storage
tests: Test wrong sector size with 4k storage
sanlock: Fail read_resource_owners() with wrong sector size
tests: Add failing test for invalid align size
sanlock: Fail read_resource_owners with incorrect align size
README.dev | 44 +++++++++++++
python/example.py | 2 +-
python/sanlock.c | 24 +++++--
src/resource.c | 37 ++++++++---
tests/conftest.py | 36 +++++++++++
tests/python_test.py | 148 ++++++++++++++++++++++++++++++++++++++++++-
tests/util.py | 34 ++++++----
tox.ini | 2 +-
8 files changed, 299 insertions(+), 28 deletions(-)
--
2.17.2
4 years, 11 months
Re: [PATCH v2 2/6] tests: Start tests for 4k storage
by Nir Soffer
On Mon, May 13, 2019 at 4:28 PM Pavel Bar <pbar(a)redhat.com> wrote:
>
>
> On Sun, May 12, 2019 at 6:54 PM Nir Soffer <nirsof(a)gmail.com> wrote:
>
>> We added API for using 4k sector size and various alignments, but there
>> are no tests for the new APIs with disks using 4k sector size. Testing
>> this is complicated since creating a block device and mounting file
>> systems requires root, and we like to run the tests as a regular user.
>>
>> Add new tests for writing and reading lockspace and resource using 4k
>> storage provided by the user. If a path is not specified the tests are
>> skipped.
>>
>> To run the tests, you need to create a block device using 4k sector size
>> and optionally create a file system and mount it. The process is
>> explained in README.dev.
>>
>> Then pass the path of the block device or file on the new file system to
>> the tests:
>>
>> USER_4K_PATH=/dev/loop2 tox -e py27
>>
>> Adding and removing lockspace and acquiring resources is not tested yet
>> with 4k storage.
>>
>> Signed-off-by: Nir Soffer <nsoffer(a)redhat.com>
>> ---
>> README.dev | 44 ++++++++++++++++++++++++++++++
>> tests/conftest.py | 36 ++++++++++++++++++++++++
>> tests/python_test.py | 65 ++++++++++++++++++++++++++++++++++++++++++++
>> tests/util.py | 34 +++++++++++++++--------
>> tox.ini | 2 +-
>> 5 files changed, 169 insertions(+), 12 deletions(-)
>>
>> diff --git a/README.dev b/README.dev
>> index 3519993..bfb170c 100644
>> --- a/README.dev
>> +++ b/README.dev
>> @@ -22,5 +22,49 @@ To run only test from some modules:
>> $ tox tests/daemon_test.py
>>
>> To run only tests matching the substring "foo":
>>
>> $ tox -- -k foo
>> +
>> +
>> +Testing 4K support
>> +==================
>> +
>> +This requires manual setup for creating a block device using 4k sector
>> size,
>> +optionally creating and mounting a file system, and passing the test
>> path to
>> +tests. The setup must be done as root, but the tests can run later as the
>> +current user.
>> +
>> +Common setup
>> +------------
>> +
>> +1. Create a backing file:
>> +
>> + $ truncate -s 1G /var/tmp/backing
>> +
>> +2. Create a loop device with 4k sector size:
>> +
>> + $ sudo losetup -f /var/tmp/backing --show --sector-size=4096
>> + /dev/loop2
>> +
>> +3. Change the device (or mountpoint) owner to current user
>> +
>> + $ sudo chown $USER:$USER /dev/loop2
>> +
>> +Testing 4k block device
>> +-----------------------
>> +
>> +Run the tests with USER_4K_PATH environment variable:
>> +
>> + $ USER_4K_PATH=/dev/loop2 tox -e py27
>> +
>> +Testing 4k file system
>> +----------------------
>> +
>> +To test file system on top of 4k block device, create a file system on
>> the
>> +device, mount it, and create a file for testing lockspace or resources:
>> +
>> + $ sudo mkfs.xfs /dev/loop2
>> + $ sudo mount /dev/loop2 /tmp/sanlock-4k
>> + $ sudo chown $USER:$USER /tmp/sanlock-4k
>> + $ truncate -s 128m /tmp/sanlock-4k/disk
>> + $ USER_4K_PATH=/tmp/sanlock-4k/disk tox -e py27
>> diff --git a/tests/conftest.py b/tests/conftest.py
>> index 6b8b0e8..721ee41 100644
>> --- a/tests/conftest.py
>> +++ b/tests/conftest.py
>> @@ -1,9 +1,12 @@
>> """
>> Fixtures for sanlock testing.
>> """
>>
>> +import os
>> +import stat
>> +
>> import pytest
>>
>> from . import util
>>
>>
>> @@ -19,5 +22,38 @@ def sanlock_daemon():
>> finally:
>> # Killing sanlock allows terminating without reomving the
>> lockspace,
>> # which takes about 3 seconds, slowing down the tests.
>> p.kill()
>> p.wait()
>> +
>> +
>> +(a)pytest.fixture
>> +def user_4k_path():
>> + """
>> + A path to block device or file on file system on top of 4k block
>> device,
>> + provided by the user.
>> +
>> + The user must create the block device or the file system before
>> running the
>> + tests, and specify the path to the file in the USER_4K_PATH
>> environment
>> + variable.
>> +
>> + If USER_4K_PATH was not specified, tests using this fixture will be
>> skipped.
>> + If USER_4K_PATH was specified but does not exist, or is not a file
>> or block
>> + device, RuntimeError is raised.
>> +
>> + Return path to the user specifed file.
>>
>
> Typo: "specifed" --> "specified".
>
>
>> + """
>> + path = os.environ.get("USER_4K_PATH")
>> + if path is None:
>> + pytest.skip("USER_4K_PATH pointing to a 4k block device or file
>> was "
>> + "not specified")
>> +
>> + if not os.path.exists(path):
>> + raise RuntimeError("USER_4K_PATH {!r} does not
>> exist".format(path))
>> +
>> + mode = os.stat(path).st_mode
>> + if not (stat.S_ISBLK(mode) or stat.S_ISREG(mode)):
>> + raise RuntimeError(
>> + "USER_4K_PATH {!r} is not a block device or regular file"
>> + .format(path))
>> +
>> + return path
>> diff --git a/tests/python_test.py b/tests/python_test.py
>> index 1a6bbbd..0681570 100644
>> --- a/tests/python_test.py
>> +++ b/tests/python_test.py
>> @@ -23,10 +23,11 @@ LARGE_FILE_SIZE = 1024**4
>> LOCKSPACE_SIZE = 1024**2
>> MIN_RES_SIZE = 1024**2
>>
>> ALIGNMENT_1M = 1024**2
>> SECTOR_SIZE_512 = 512
>> +SECTOR_SIZE_4K = 4096
>>
>
> Need to remember change to constants from "units.py".
>
Please update your PR once we merge this.
>
>
>>
>>
>> @pytest.mark.parametrize("size,offset", [
>> # Smallest offset.
>> (LOCKSPACE_SIZE, 0),
>> @@ -64,10 +65,39 @@ def test_write_lockspace(tmpdir, sanlock_daemon,
>> size, offset):
>> # TODO: check more stuff here...
>>
>> util.check_guard(path, size)
>>
>>
>> +(a)pytest.mark.parametrize("align", sanlock.ALIGN_SIZE)
>> +def test_write_lockspace_4k(user_4k_path, sanlock_daemon, align):
>> +
>> + # Postion lockspace area, ensuring that previous tests will not
>> break this
>>
>
> Typo: "Postion" --> "Position"
>
Should be "poison" I'll fix this when I push.
>
>
>> + # test, and sanlock does not write beyond the lockspace area.
>> + with io.open(user_4k_path, "rb+") as f:
>> + f.write(align * b"x")
>> + util.write_guard(user_4k_path, align)
>> +
>> + sanlock.write_lockspace(
>> + "name", user_4k_path, iotimeout=1, align=align,
>> sector=SECTOR_SIZE_4K)
>> +
>> + ls = sanlock.read_lockspace(
>> + user_4k_path, align=align, sector=SECTOR_SIZE_4K)
>> +
>> + assert ls == {"iotimeout": 1, "lockspace": "name"}
>> +
>> + acquired = sanlock.inq_lockspace("name", 1, user_4k_path, wait=False)
>> + assert acquired is False
>> +
>> + # Verify that lockspace was written.
>> + with io.open(user_4k_path, "rb") as f:
>> + magic, = struct.unpack("< I", f.read(4))
>> + assert magic == constants.DELTA_DISK_MAGIC
>> +
>> + # Check that sanlock did not write beyond the lockspace area.
>> + util.check_guard(user_4k_path, align)
>> +
>> +
>> @pytest.mark.parametrize("size,offset", [
>> # Smallest offset.
>> (MIN_RES_SIZE, 0),
>> # Large offset.
>> (LARGE_FILE_SIZE, LARGE_FILE_SIZE - MIN_RES_SIZE),
>> @@ -111,10 +141,45 @@ def test_write_resource(tmpdir, sanlock_daemon,
>> size, offset):
>> # TODO: check more stuff here...
>>
>> util.check_guard(path, size)
>>
>>
>> +(a)pytest.mark.parametrize("align", sanlock.ALIGN_SIZE)
>> +def test_write_resource_4k(sanlock_daemon, user_4k_path, align):
>> + disks = [(user_4k_path, 0)]
>> +
>> + # Postion resource area, ensuring that previous tests will not
>> break this
>>
>
> Typo.
>
Yea, same.
>
>
>> + # test, and sanlock does not write beyond the lockspace area.
>> + with io.open(user_4k_path, "rb+") as f:
>> + f.write(align * b"x")
>>
>
> Is it possible to use "GUARD" from the "util.py"?
>
This is the poison byte "x", not the guard byte "X".
Maybe we should add later a constant or a helper like:
util.poison(path, offset, size)
>
>
>> + util.write_guard(user_4k_path, align)
>> +
>> + sanlock.write_resource(
>> + "ls_name", "res_name", disks, align=align, sector=SECTOR_SIZE_4K)
>> +
>> + res = sanlock.read_resource(
>> + user_4k_path, align=align, sector=SECTOR_SIZE_4K)
>> +
>> + assert res == {
>> + "lockspace": "ls_name",
>> + "resource": "res_name",
>> + "version": 0
>> + }
>> +
>> + owners = sanlock.read_resource_owners(
>> + "ls_name", "res_name", disks, align=align, sector=SECTOR_SIZE_4K)
>> + assert owners == []
>> +
>> + # Verify that resource was written.
>> + with io.open(user_4k_path, "rb") as f:
>> + magic, = struct.unpack("< I", f.read(4))
>> + assert magic == constants.PAXOS_DISK_MAGIC
>> +
>> + # Check that sanlock did not write beyond the lock space area.
>> + util.check_guard(user_4k_path, align)
>> +
>> +
>> @pytest.mark.parametrize("size,offset", [
>> # Smallest offset.
>> (MIN_RES_SIZE, 0),
>> # Large offset.
>> (LARGE_FILE_SIZE, LARGE_FILE_SIZE - MIN_RES_SIZE),
>> diff --git a/tests/util.py b/tests/util.py
>> index a6b53e3..0695d8c 100644
>> --- a/tests/util.py
>> +++ b/tests/util.py
>> @@ -11,10 +11,13 @@ import subprocess
>> import time
>>
>> TESTDIR = os.path.dirname(__file__)
>> SANLOCK = os.path.join(TESTDIR, os.pardir, "src", "sanlock")
>>
>> +GUARD = b"X"
>> +GUARD_SIZE = 4096
>>
>
> Remember to change to constant from "units.py": 4096 --> 4 * KiB
>
>
>> +
>>
>> class TimeoutExpired(Exception):
>> """ Raised when timeout expired """
>>
>>
>> @@ -95,32 +98,41 @@ def wait_for_termination(p, timeout):
>> if time.time() > deadline:
>> raise TimeoutExpired
>> time.sleep(0.05)
>>
>>
>> -def create_file(path, size, guard=b"X", guard_size=4096):
>>
>
> Can use "GUARD" instead of b"X"?
> Can use "GUARD_SIZE" instead of 4096?
>
>
>
>> +def create_file(path, size, guard=True):
>> """
>> Create sparse file of size bytes.
>>
>> - If guard is set, add a guard area after the end of the file and fill
>> it
>> - with guard bytes. This allows testing that the code under test do
>> not write
>> - anything after the end of the file.
>> + If guard is True, add a guard area beyond the end of the file.
>> """
>> with io.open(path, "wb") as f:
>> f.truncate(size)
>> - if guard:
>> - f.seek(size)
>> - f.write(guard * guard_size)
>> +
>> + if guard:
>> + write_guard(path, size)
>> +
>> +
>> +def write_guard(path, offset):
>> + """
>> + Write guard areas at offset and fill with guard byte.
>> +
>> + Use check_guard() to verify that nothing was written to the guard
>> area.
>> + """
>> + with io.open(path, "rb+") as f:
>> + f.seek(offset)
>> + f.write(GUARD * GUARD_SIZE)
>>
>>
>> -def check_guard(path, size, guard=b"X", guard_size=4096):
>> +def check_guard(path, offset):
>> """
>> - Assert that a file ends with a guard area filled with guard bytes.
>> + Assert that guard area at offset was not modified.
>> """
>> with io.open(path, "rb") as f:
>> - f.seek(size)
>> - assert f.read() == guard * guard_size
>> + f.seek(GUARD)
>> + assert f.read(GUARD_SIZE) == GUARD * GUARD_SIZE
>>
>>
>> def check_rindex_entry(entry, name, offset=None, flags=None):
>> # See src/ondisk.c rindex_entry_in()
>> e_offset, e_flags, e_unused, e_name = struct.unpack("<Q L L 48s",
>> entry)
>> diff --git a/tox.ini b/tox.ini
>> index 1c2349e..d21f2f3 100644
>> --- a/tox.ini
>> +++ b/tox.ini
>> @@ -7,11 +7,11 @@
>> envlist = py27,py36
>> skipsdist = True
>> skip_missing_interpreters = True
>>
>> [testenv]
>> -passenv = USER
>> +passenv = *
>> setenv =
>> LD_LIBRARY_PATH={env:PWD}/wdmd:{env:PWD}/src
>> SANLOCK_PRIVILEGED=0
>> SANLOCK_RUN_DIR=/tmp/sanlock
>> whitelist_externals = make
>> --
>> 2.17.2
>>
>>
4 years, 11 months
Re: [PATCH v2 1/6] python: Add align and sector arguments to read_resource_owners()
by Nir Soffer
On Mon, May 13, 2019 at 3:48 PM Pavel Bar <pbar(a)redhat.com> wrote:
>
>
> On Sun, May 12, 2019 at 6:54 PM Nir Soffer <nirsof(a)gmail.com> wrote:
>
>> We forgot to add the flags to this function, and it works with 4k
>> storage because sanlock implements a fallback mechanism. Since we
>> require align and sector arguments in read_resource(), we should also
>> require them for consistency in read_resource_owners().
>>
>> Signed-off-by: Nir Soffer <nsoffer(a)redhat.com>
>> ---
>> python/example.py | 2 +-
>> python/sanlock.c | 24 +++++++++++++++++++-----
>> 2 files changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/python/example.py b/python/example.py
>> index cb8f3e9..6ef9f7d 100644
>> --- a/python/example.py
>> +++ b/python/example.py
>> @@ -52,11 +52,11 @@ def main():
>> print "Lockspace '%s' hosts: " % LOCKSPACE_NAME,
>> hosts_list
>> break
>> time.sleep(5)
>> print "Resource '%s' owners: " % RESOURCE_NAME, \
>> sanlock.read_resource_owners(
>> - LOCKSPACE_NAME, RESOURCE_NAME, SNLK_DISKS)
>> + LOCKSPACE_NAME, RESOURCE_NAME, SNLK_DISKS, align=1048576,
>> sector=512)
>>
>
> Thus is "example.py". Is it important to change it to "MiB"?
>
I don't think so, the MiB constants are implementation detail of the tests,
not part of sanlock API.
>
>
>> print "Releasing '%s' on '%s'" % (RESOURCE_NAME, LOCKSPACE_NAME)
>> sanlock.release(LOCKSPACE_NAME, RESOURCE_NAME, SNLK_DISKS,
>> slkfd=fd)
>> except Exception as e:
>> print "Exception: ", e
>> finally:
>> diff --git a/python/sanlock.c b/python/sanlock.c
>> index d6841e8..4be9be2 100644
>> --- a/python/sanlock.c
>> +++ b/python/sanlock.c
>> @@ -1195,30 +1195,36 @@ exit_fail:
>> return NULL;
>> }
>>
>> /* read_resource_owners */
>> PyDoc_STRVAR(pydoc_read_resource_owners, "\
>> -read_resource_owners(lockspace, resource, disks) -> list\n\
>> +read_resource_owners(lockspace, resource, disks, align=1048576,
>> sector=512) \
>> +-> list\n\
>> Returns the list of hosts owning a resource, the list is not filtered
>> and\n\
>> it might contain hosts that are currently failing or dead. The hosts
>> are\n\
>> returned in the same format used by get_hosts.\n\
>> -The disks must be in the format: [(path, offset), ... ]");
>> +The disks must be in the format: [(path, offset), ... ].\n\
>> +Align can be one of (1048576, 2097152, 4194304, 8388608).\n\
>> +Sector can be one of (512, 4096).");
>>
>> static PyObject *
>> py_read_resource_owners(PyObject *self __unused, PyObject *args,
>> PyObject *keywds)
>> {
>> int rv, hss_count = 0;
>> + int sector = SECTOR_SIZE_512;
>> + long align = ALIGNMENT_1M;
>> const char *lockspace, *resource;
>> struct sanlk_resource *res = NULL;
>> struct sanlk_host *hss = NULL;
>> PyObject *disks, *ls_list = NULL;
>>
>> - static char *kwlist[] = {"lockspace", "resource", "disks", NULL};
>> + static char *kwlist[] = {"lockspace", "resource", "disks", "align",
>> + "sector", NULL};
>>
>> /* parse python tuple */
>> - if (!PyArg_ParseTupleAndKeywords(args, keywds, "ssO!", kwlist,
>> - &lockspace, &resource, &PyList_Type, &disks)) {
>> + if (!PyArg_ParseTupleAndKeywords(args, keywds, "ssO!|li", kwlist,
>> + &lockspace, &resource, &PyList_Type, &disks, &align, §or)) {
>> return NULL;
>> }
>>
>> /* parse and check sanlock resource */
>> if (__parse_resource(disks, &res) < 0) {
>> @@ -1227,10 +1233,18 @@ py_read_resource_owners(PyObject *self __unused,
>> PyObject *args, PyObject *keywd
>>
>> /* prepare sanlock names */
>> strncpy(res->lockspace_name, lockspace, SANLK_NAME_LEN);
>> strncpy(res->name, resource, SANLK_NAME_LEN);
>>
>> + /* set resource alignment/sector flags */
>>
>
> Suggestion: "alignment & sector".
>
I'll make it nicer before I push the patch.
>
>
>> +
>> + if (add_align_flag(align, &res->flags) == -1)
>> + goto exit_fail;
>> +
>> + if (add_sector_flag(sector, &res->flags) == -1)
>> + goto exit_fail;
>> +
>> /* read resource owners (gil disabled) */
>> Py_BEGIN_ALLOW_THREADS
>> rv = sanlock_read_resource_owners(res, 0, &hss, &hss_count);
>> Py_END_ALLOW_THREADS
>>
>> --
>> 2.17.2
>>
>>
4 years, 11 months
Re: [PATCH v2 2/6] tests: Start tests for 4k storage
by Vojtech Juranek
.
>
> > + # test, and sanlock does not write beyond the lockspace area.
> > + with io.open(user_4k_path, "rb+") as f:
> > + f.write(align * b"x")
>
> Is it possible to use "GUARD" from the "util.py"?
>
if I understand this correctly, it's not meant as a guard area but just as a
preparation for the test to rewrite eventual leftovers from previous tests.
Using GUARD here could be IMHO confusing.
4 years, 11 months
Re: [PATCH 3/4] tests: Test wrong sector size with 4k storage
by Nir Soffer
On Tue, May 7, 2019 at 4:04 PM Amit Bawer <abawer(a)redhat.com> wrote:
Please use reply-all to make the discussion public.
---------- Forwarded message ---------
> From: Amit Bawer <abawer(a)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(a)gmail.com>
> Cc: Pavel Bar <pbar(a)redhat.com>
>
>
>
>
> On Tue, May 7, 2019 at 1:38 AM Nir Soffer <nirsof(a)gmail.com> wrote:
>
>> 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(a)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
>>
>>
>> +(a)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
>> +
>> +
>> +(a)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
>>
>>
>> +(a)pytest.mark.skipif(
>> + "SANLOCK_4K_PATH" not in os.environ,
>> + reason="Requires user specified path using 4k block size")
>> +(a)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
>> +
>> +
>> +(a)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
>> +
>> +
>> +(a)pytest.mark.skipif(
>> + "SANLOCK_4K_PATH" not in os.environ,
>> + reason="Requires user specified path using 4k block size")
>> +(a)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
>>
>>
4 years, 11 months
Re: [PATCH 3/4] tests: Test wrong sector size with 4k storage
by Nir Soffer
On Wed, May 8, 2019 at 10:19 AM Pavel Bar <pbar(a)redhat.com> wrote:
>
>
> On Tue, May 7, 2019 at 1:38 AM Nir Soffer <nirsof(a)gmail.com> wrote:
>
>> 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:
>>
>
> Rephrase the part in yellow. Maybe you meant: "...marked with an
> expected..."?
>
It should be:
Two tests are marked as 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(a)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
>>
>
> Might be worth to define the 4K constant that in turn will use
> constants.KiB that comes with my PR.
>
I will add a proper constant, since we use it in multiple tests.
> +(a)pytest.mark.skipif(
>> + "SANLOCK_4K_PATH" not in os.environ,
>> + reason="Requires user specified path using 4k block size")
>>
>
> Suggestion to rephrase the reason text (added in *bold*): "Requires user
> specified *SanLock* path *when *using 4k block size"
>
The suggestion is wrong, and the original text is correct - the specified
path is should be
a block device using 4k sector size or a file on a file system on such
block device.
However the text does not make it clear that this was skipped because
SANLOCK_4K_PATH
was not specified, I will add it to the message.
> +def test_write_lockspace_4k_invalid_sector_size(sanlock_daemon):
>> + path = os.environ["SANLOCK_4K_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
>> +
>> +
>> +(a)pytest.mark.skipif(
>> + "SANLOCK_4K_PATH" not in os.environ,
>> + reason="Requires user specified path using 4k block size")
>>
>
> Suggestion to rephrase the reason text (added in *bold*): "Requires user
> specified *SanLock* path *when *using 4k block size"
>
There is no need to repeat the same comment again and again.
However this is my fault, repeating the mark again and again. We can define
the mark like this once:
requires_sanlock_4k_path = pytest.mark.skipif(
"SANLOCK_4K_PATH" not in os.environ,
reason="Requires SANLOCK_4K_PATH using 4k block size")
And then use the mark like this:
@requires_sanlock_4k_path
def test_foo():
> +def test_read_lockspace_4k_invalid_sector_size(sanlock_daemon):
>> + path = os.environ["SANLOCK_4K_PATH"]
>> +
>> + 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.
>>
>
> Suggestion to change in the above comment "after" --> "beyond"
>
Thanks, looks better.
>
>
>> f.seek(align)
>> assert f.read(4096) == b"X" * 4096
>>
>
> Might be worth to define the 4K constant that in turn will use
> constants.KiB that comes with my PR.
>
There is no need to repeat the same comment again and again.
+(a)pytest.mark.skipif(
>> + "SANLOCK_4K_PATH" not in os.environ,
>> + reason="Requires user specified path using 4k block size")
>>
>
> Suggestion to rephrase the reason text (added in *bold*): "Requires user
> specified *SanLock* path *when *using 4k block size"
>
>
>> +(a)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"]
>> + 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
>> +
>> +
>> +(a)pytest.mark.skipif(
>> + "SANLOCK_4K_PATH" not in os.environ,
>> + reason="Requires user specified path using 4k block size")
>>
>
> Suggestion to rephrase the reason text (added in *bold*): "Requires user
> specified *SanLock* path *when *using 4k block size"
>
>
>> +def test_read_resource_4k_invalid_sector_size(sanlock_daemon):
>> + path = os.environ["SANLOCK_4K_PATH"]
>> + 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
>> +
>> +
>> +(a)pytest.mark.skipif(
>> + "SANLOCK_4K_PATH" not in os.environ,
>> + reason="Requires user specified path using 4k block size")
>>
>
> Suggestion to rephrase the reason text (added in *bold*): "Requires user
> specified *SanLock* path *when *using 4k block size"
>
>
>> +(a)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"]
>> + 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
>>
>
Forgot to comment about this?
+
>> +
>> @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
>>
>>
4 years, 11 months
Re: [PATCH 4/4] sanlock: Fail read_resource_owners() with wrong
sector size
by Nir Soffer
On Wed, May 8, 2019 at 11:09 AM Pavel Bar <pbar(a)redhat.com> wrote:
Thanks for reviewing.
On Tue, May 7, 2019 at 1:38 AM Nir Soffer <nirsof(a)gmail.com> wrote:
>
>> If sanlock_read_resource_owners() is called with wrong sector size, fail
>> instead of falling back to actual sector size. This is more consistent
>> with sanlock_read_resource() and other other functions.
>>
>> Signed-off-by: Nir Soffer <nsoffer(a)redhat.com>
>> ---
>> src/resource.c | 17 ++++++++++++++---
>> tests/python_test.py | 1 -
>> 2 files changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/resource.c b/src/resource.c
>> index a783106..f75ea71 100644
>> --- a/src/resource.c
>> +++ b/src/resource.c
>> @@ -167,19 +167,19 @@ int read_resource_owners(struct task *task, struct
>> token *token,
>> uint64_t host_id;
>> uint32_t checksum;
>> char *lease_buf_dblock;
>> char *lease_buf = NULL;
>> char *hosts_buf = NULL;
>> + int sector_size = token->sector_size;
>> int align_size;
>> int host_count = 0;
>> int i, rv;
>>
>> disk = &token->disks[0];
>>
>> /*
>> - * We don't know the sector_size of the resource until the leader
>> - * record has been read, start with the smaller size.
>> + * If we don't know the sector_size start with the smaller size.
>> */
>>
>> if (!token->sector_size) {
>> token->sector_size = 512;
>> token->align_size = sector_size_to_align_size_old(512);
>> @@ -204,13 +204,24 @@ int read_resource_owners(struct task *task, struct
>> token *token,
>>
>> align_size = leader_align_size_from_flag(leader.flags);
>> if (!align_size)
>> align_size =
>> sector_size_to_align_size_old(leader.sector_size);
>>
>> + /* If caller specified value was incorrect, fail. */
>> + if (sector_size && sector_size != leader.sector_size) {
>>
>
> If "leader.sector_size" is non-zero, you can skip the first sub-condition
> "sector_size &&"
>
I think that leader.sector_size is always non-zero, but we cannot skip
sector_size check.
sector_size is initialized to the user specified sector size - what we set
in the python bindings
sector argument.
If sector_size is set, it must match the actual sector size on storage
(leader.sector_size)
written when a resource was created (write_resource). This is the only case
when using
the python bindings, because we use default value of 512 - keeping the old
behavior.
However if this function is called from the command line (e.g. sanlock
client ...), or directly
using sanlock_read_resource_owners() (e.g. someone writing application
using sanlock C API),
sector will be set only if you added SANLK_RES_SECTOR512 or
SANLK_RES_SECTOR4K
flags the res.flags. In this case sanlock try to be nice and guess the
value.
I'm not sure why David added this option, maybe to keep backward
compatibility.
>
> + log_errot(token, "read_resource_owners invalid
>> sector_size: %d actual: %d",
>>
>
> "log_erro*t*" - shouldn't it be "log_erro*r*"?
>
It looks like a typo, but this is the name of the macro :-)
// src/log.h
46 #define log_errot(token, fmt, args...) log_level(token->space_id,
token->res_id, NULL, LOG_ERR, fmt, ##args)
>
>
>> + sector_size, leader.sector_size);
>> + rv = -EINVAL;
>> + goto out;
>> + }
>> +
>> + /*
>> + * If the caller did not specify sector size and our guess was
>> wrong,
>> + * retry with the actual value
>> + */
>> if ((token->sector_size != leader.sector_size) ||
>> (token->align_size != align_size)) {
>> - /* initial sizes were wrong */
>> log_debug("read_resource_owners rereading with correct
>> sizses");
>>
>
> Although you didn't change this code, I see typo "sizses".
>
Why not send a patch fixing it?
>
> token->sector_size = leader.sector_size;
>> token->align_size = align_size;
>> free(lease_buf);
>> lease_buf = NULL;
>> diff --git a/tests/python_test.py b/tests/python_test.py
>> index aa1f7f5..e8a34c0 100644
>> --- a/tests/python_test.py
>> +++ b/tests/python_test.py
>> @@ -246,11 +246,10 @@ def
>> test_read_resource_4k_invalid_sector_size(sanlock_daemon):
>>
>>
>> @pytest.mark.skipif(
>> "SANLOCK_4K_PATH" not in os.environ,
>> reason="Requires user specified path using 4k block size")
>> -(a)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"]
>> disks = [(path, 0)]
>>
>> sanlock.write_resource(
>> --
>> 2.17.2
>>
>>
4 years, 11 months
Re: [PATCH 2/4] tests: Start tests for 4k storage
by Nir Soffer
On Tue, May 7, 2019 at 4:08 PM Pavel Bar <pbar(a)redhat.com> wrote:
>
>
> On Tue, May 7, 2019 at 1:38 AM Nir Soffer <nirsof(a)gmail.com> wrote:
>
>> We added API for using 4k sector size and various alignments, but there
>> are no tests for the new APIs with disks using 4k sector size. Testing
>> this is complicated since creating a block device and mounting file
>> systems requires root, and we like to run the tests as a regular user.
>>
>> Add new tests for writing and reading lockspace and resource using 4k
>> storage provided by the user. If a path is not specified the tests are
>> skipped.
>>
>> To run the tests, you need to create a block device using 4k sector size
>> and optionally create a file system and mount it. The process is
>> explained in README.dev.
>>
>> Then pass the path of the block device or file on the new file system to
>> the tests:
>>
>> SANLOCK_4K_PATH=/dev/loop2 tox -e py27
>>
>> Adding and removing lockspace and acquiring resources is not tested yet
>> with 4k storage.
>>
>> Signed-off-by: Nir Soffer <nsoffer(a)redhat.com>
>> ---
>> README.dev | 44 ++++++++++++++++++++++++++
>> tests/python_test.py | 74 ++++++++++++++++++++++++++++++++++++++++++++
>> tox.ini | 2 +-
>> 3 files changed, 119 insertions(+), 1 deletion(-)
>>
>> diff --git a/README.dev b/README.dev
>> index 3519993..04617aa 100644
>> --- a/README.dev
>> +++ b/README.dev
>> @@ -22,5 +22,49 @@ To run only test from some modules:
>> $ tox tests/daemon_test.py
>>
>> To run only tests matching the substring "foo":
>>
>> $ tox -- -k foo
>> +
>> +
>> +Testing 4K support
>> +==================
>> +
>> +This requires manual setup for creating a block device using 4k sector
>> size,
>> +optionally creating and mounting a file system, and passing the test
>> path to
>> +tests. The setup must be done as root, but the tests can run later as the
>> +current user.
>> +
>> +Common setup
>> +------------
>> +
>> +1. Create a backing file:
>> +
>> + $ truncate -s 1G /var/tmp/backing
>> +
>> +2. Create a loop device with 4k sector size:
>> +
>> + $ sudo losetup -f /var/tmp/backing --show --sector-size=4096
>> + /dev/loop2
>> +
>> +3. Change the device (or mountpoint) owner to current user
>> +
>> + $ sudo chown $USER:$USER /dev/loop2
>> +
>> +Testing 4k block device
>> +-----------------------
>> +
>> +Run the tests with SANLOCK_4K_PATH environment variable:
>> +
>> + $ SANLOCK_4K_PATH=/dev/loop2 tox -e py27
>> +
>> +Testing 4k file system
>> +----------------------
>> +
>> +To test file system on top of 4k block device, create a file system on
>> the
>> +device, mount it, and create a file for testing lockspace or resources:
>> +
>> + $ sudo mkfs.xfs /dev/loop2
>> + $ sudo mount /dev/loop2 /tmp/sanlock-4k
>> + $ sudo chown $USER:$USER /tmp/sanlock-4k
>> + $ truncate -s 128m /tmp/sanlock-4k/disk
>> + $ SANLOCK_4K_PATH=/tmp/sanlock-4k/disk tox -e py27
>> diff --git a/tests/python_test.py b/tests/python_test.py
>> index 1a6bbbd..f0ecd06 100644
>> --- a/tests/python_test.py
>> +++ b/tests/python_test.py
>> @@ -2,10 +2,11 @@
>> Test sanlock python binding with sanlock daemon.
>> """
>>
>> import errno
>> import io
>> +import os
>> import struct
>> import time
>>
>> import pytest
>>
>> @@ -23,10 +24,11 @@ LARGE_FILE_SIZE = 1024**4
>> LOCKSPACE_SIZE = 1024**2
>> MIN_RES_SIZE = 1024**2
>>
>> ALIGNMENT_1M = 1024**2
>> SECTOR_SIZE_512 = 512
>> +SECTOR_SIZE_4K = 4096
>>
>
> I already have a PR ready with new constants (for Kibibyte, Mebibyte,
> Gibibyte & Tebibyte). When my PR is merged, need to remember to change the
> "SECTOR_SIZE_4K" to use new constants.
>
> LOCKSPACE_SIZE = constants.MiB
> MIN_RES_SIZE = constants.MiB
>
> ALIGNMENT_1M = constants.MiB
> SECTOR_SIZE_512 = 512
> SECTOR_SIZE_4K = 4*constants.KiB <--- Remember to change
>
Cool, your PR on my queue.
>
>
>
>>
>> @pytest.mark.parametrize("size,offset", [
>> # Smallest offset.
>> (LOCKSPACE_SIZE, 0),
>> @@ -64,10 +66,43 @@ def test_write_lockspace(tmpdir, sanlock_daemon,
>> size, offset):
>> # TODO: check more stuff here...
>>
>> util.check_guard(path, size)
>>
>>
>> +(a)pytest.mark.skipif(
>> + "SANLOCK_4K_PATH" not in os.environ,
>> + reason="Requires user specified path using 4k block size")
>> +(a)pytest.mark.parametrize("align", sanlock.ALIGN_SIZE)
>> +def test_write_lockspace_4k(sanlock_daemon, align):
>> + path = os.environ["SANLOCK_4K_PATH"]
>> +
>> + # Postion lockspace area, ensuring that previous tests will not
>> break this
>> + # test, and sanlock does not write after the lockspace area.
>> + with io.open(path, "rb+") as f:
>> + f.write(align * b"x")
>> + f.write(4096 * b"X")
>> +
>> + sanlock.write_lockspace(
>> + "name", path, iotimeout=1, align=align, sector=SECTOR_SIZE_4K)
>> +
>> + ls = sanlock.read_lockspace(path, align=align, sector=SECTOR_SIZE_4K)
>> +
>> + assert ls == {"iotimeout": 1, "lockspace": "name"}
>> +
>> + acquired = sanlock.inq_lockspace("name", 1, path, wait=False)
>> + assert acquired is False
>> +
>> + with io.open(path, "rb") as f:
>> + # Verify that lockspace was written.
>> + magic, = struct.unpack("< I", f.read(4))
>> + assert magic == constants.DELTA_DISK_MAGIC
>> +
>> + # Check that sanlock did not write after the lockspace area.
>> + f.seek(align)
>> + assert f.read(4096) == b"X" * 4096
>> +
>> +
>> @pytest.mark.parametrize("size,offset", [
>> # Smallest offset.
>> (MIN_RES_SIZE, 0),
>> # Large offset.
>> (LARGE_FILE_SIZE, LARGE_FILE_SIZE - MIN_RES_SIZE),
>> @@ -111,10 +146,49 @@ def test_write_resource(tmpdir, sanlock_daemon,
>> size, offset):
>> # TODO: check more stuff here...
>>
>> util.check_guard(path, size)
>>
>>
>> +(a)pytest.mark.skipif(
>> + "SANLOCK_4K_PATH" not in os.environ,
>> + reason="Requires user specified path using 4k block size")
>> +(a)pytest.mark.parametrize("align", sanlock.ALIGN_SIZE)
>> +def test_write_resource_4k(sanlock_daemon, align):
>> + path = os.environ["SANLOCK_4K_PATH"]
>> + disks = [(path, 0)]
>> +
>> + # Postion resource area, ensuring that previous tests will not break
>> this
>> + # test, and sanlock does not write after the lockspace area.
>> + with io.open(path, "rb+") as f:
>> + f.write(align * b"x")
>> + f.write(4096 * b"X")
>> +
>> + sanlock.write_resource(
>> + "ls_name", "res_name", disks, align=align, sector=SECTOR_SIZE_4K)
>> +
>> + res = sanlock.read_resource(path, align=align, sector=SECTOR_SIZE_4K)
>> +
>> + assert res == {
>> + "lockspace": "ls_name",
>> + "resource": "res_name",
>> + "version": 0
>> + }
>> +
>> + owners = sanlock.read_resource_owners(
>> + "ls_name", "res_name", disks, align=align, sector=SECTOR_SIZE_4K)
>> + assert owners == []
>> +
>> + with io.open(path, "rb") as f:
>> + # Verify that resource was written.
>> + magic, = struct.unpack("< I", f.read(4))
>> + assert magic == constants.PAXOS_DISK_MAGIC
>> +
>> + # Check that sanlock did not write after the resource area.
>> + f.seek(align)
>> + assert f.read(4096) == b"X" * 4096
>> +
>> +
>> @pytest.mark.parametrize("size,offset", [
>> # Smallest offset.
>> (MIN_RES_SIZE, 0),
>> # Large offset.
>> (LARGE_FILE_SIZE, LARGE_FILE_SIZE - MIN_RES_SIZE),
>> diff --git a/tox.ini b/tox.ini
>> index 1c2349e..d21f2f3 100644
>> --- a/tox.ini
>> +++ b/tox.ini
>> @@ -7,11 +7,11 @@
>> envlist = py27,py36
>> skipsdist = True
>> skip_missing_interpreters = True
>>
>> [testenv]
>> -passenv = USER
>> +passenv = *
>> setenv =
>> LD_LIBRARY_PATH={env:PWD}/wdmd:{env:PWD}/src
>> SANLOCK_PRIVILEGED=0
>> SANLOCK_RUN_DIR=/tmp/sanlock
>> whitelist_externals = make
>> --
>> 2.17.2
>>
>>
4 years, 11 months
[PATCH 0/4] Complete 4k APIs
by Nir Soffer
Add the missing align and sector arguments to read_resource_owners() and
add initial tests for 4k storage.
The tests for using wrong sector size revealed that
read_resouce_owners() ignores wrong sector size. Make it more strict to
be consistent with other APIs.
Nir Soffer (4):
python: Add align and sector arguments to read_resource_owners()
tests: Start tests for 4k storage
tests: Test wrong sector size with 4k storage
sanlock: Fail read_resource_owners() with wrong sector size
README.dev | 44 +++++++++++++
python/sanlock.c | 24 +++++--
src/resource.c | 17 ++++-
tests/python_test.py | 152 +++++++++++++++++++++++++++++++++++++++++++
tox.ini | 2 +-
5 files changed, 230 insertions(+), 9 deletions(-)
--
2.17.2
4 years, 11 months
[PATCH] tests: Start tests for disks with 4k sector size
by Nir Soffer
We added API for using 4k sector size and various alignments, but there
are not tests for the new APIs yet. Testing 4k block size is more
complicated since creating a block device and mounting file systems
requires root, and we like to run the tests as a regular user.
Add new tests for writing and reading lockspace and resource using 4k
storage provided by the user. If a path is not specified the tests are
skipped.
To run the tests, you need to create a block device using 4k sector size
and optionally create a file system and mount it. The process is
explained in README.dev.
Then pass the path of the block device or file on the new file system to
the tests:
SANLOCK_4K_PATH=/dev/loop2 tox -e py26
Adding and removing lockspace and acquiring resources is not tested yet
with 4k.
Signed-off-by: Nir Soffer <nsoffer(a)redhat.com>
---
README.dev | 44 ++++++++++++++++++++++++++
tests/python_test.py | 73 ++++++++++++++++++++++++++++++++++++++++++++
tox.ini | 2 +-
3 files changed, 118 insertions(+), 1 deletion(-)
diff --git a/README.dev b/README.dev
index 3519993..04617aa 100644
--- a/README.dev
+++ b/README.dev
@@ -22,5 +22,49 @@ To run only test from some modules:
$ tox tests/daemon_test.py
To run only tests matching the substring "foo":
$ tox -- -k foo
+
+
+Testing 4K support
+==================
+
+This requires manual setup for creating a block device using 4k sector size,
+optionally creating and mounting a file system, and passing the test path to
+tests. The setup must be done as root, but the tests can run later as the
+current user.
+
+Common setup
+------------
+
+1. Create a backing file:
+
+ $ truncate -s 1G /var/tmp/backing
+
+2. Create a loop device with 4k sector size:
+
+ $ sudo losetup -f /var/tmp/backing --show --sector-size=4096
+ /dev/loop2
+
+3. Change the device (or mountpoint) owner to current user
+
+ $ sudo chown $USER:$USER /dev/loop2
+
+Testing 4k block device
+-----------------------
+
+Run the tests with SANLOCK_4K_PATH environment variable:
+
+ $ SANLOCK_4K_PATH=/dev/loop2 tox -e py27
+
+Testing 4k file system
+----------------------
+
+To test file system on top of 4k block device, create a file system on the
+device, mount it, and create a file for testing lockspace or resources:
+
+ $ sudo mkfs.xfs /dev/loop2
+ $ sudo mount /dev/loop2 /tmp/sanlock-4k
+ $ sudo chown $USER:$USER /tmp/sanlock-4k
+ $ truncate -s 128m /tmp/sanlock-4k/disk
+ $ SANLOCK_4K_PATH=/tmp/sanlock-4k/disk tox -e py27
diff --git a/tests/python_test.py b/tests/python_test.py
index a0474f1..290ac51 100644
--- a/tests/python_test.py
+++ b/tests/python_test.py
@@ -2,10 +2,11 @@
Test sanlock python binding with sanlock daemon.
"""
import errno
import io
+import os
import struct
import time
import pytest
@@ -23,10 +24,11 @@ LARGE_FILE_SIZE = 1024**4
LOCKSPACE_SIZE = 1024**2
MIN_RES_SIZE = 1024**2
ALIGNMENT_1M = 1024**2
SECTOR_SIZE_512 = 512
+SECTOR_SIZE_4K = 4096
@pytest.mark.parametrize("size,offset", [
# Smallest offset.
(LOCKSPACE_SIZE, 0),
@@ -64,10 +66,43 @@ def test_write_lockspace(tmpdir, sanlock_daemon, size, offset):
# TODO: check more stuff here...
util.check_guard(path, size)
+(a)pytest.mark.skipif(
+ "SANLOCK_4K_PATH" not in os.environ,
+ reason="Requires user specified path using 4k block size")
+(a)pytest.mark.parametrize("align", sanlock.ALIGN_SIZE)
+def test_write_lockspace_4k(sanlock_daemon, align):
+ path = os.environ["SANLOCK_4K_PATH"]
+
+ # Postion lockspace area, ensuring that previous tests will not break this
+ # test, and sanlock does not write after the lockspace area.
+ with io.open(path, "rb+") as f:
+ f.write(align * b"x")
+ f.write(4096 * b"X")
+
+ sanlock.write_lockspace(
+ "name", path, iotimeout=1, align=align, sector=SECTOR_SIZE_4K)
+
+ ls = sanlock.read_lockspace(path, align=align, sector=SECTOR_SIZE_4K)
+
+ assert ls == {"iotimeout": 1, "lockspace": "name"}
+
+ acquired = sanlock.inq_lockspace("name", 1, path, wait=False)
+ assert acquired is False
+
+ with io.open(path, "rb") as f:
+ # Verify that lockspace was written.
+ magic, = struct.unpack("< I", f.read(4))
+ assert magic == constants.DELTA_DISK_MAGIC
+
+ # Check that sanlock did not write after the lockspace area.
+ f.seek(align)
+ assert f.read(4096) == b"X" * 4096
+
+
@pytest.mark.parametrize("size,offset", [
# Smallest offset.
(MIN_RES_SIZE, 0),
# Large offset.
(LARGE_FILE_SIZE, LARGE_FILE_SIZE - MIN_RES_SIZE),
@@ -111,10 +146,48 @@ def test_write_resource(tmpdir, sanlock_daemon, size, offset):
# TODO: check more stuff here...
util.check_guard(path, size)
+(a)pytest.mark.skipif(
+ "SANLOCK_4K_PATH" not in os.environ,
+ reason="Requires user specified path using 4k block size")
+(a)pytest.mark.parametrize("align", sanlock.ALIGN_SIZE)
+def test_write_resource_4k(sanlock_daemon, align):
+ path = os.environ["SANLOCK_4K_PATH"]
+ disks = [(path, 0)]
+
+ # Postion resource area, ensuring that previous tests will not break this
+ # test, and sanlock does not write after the lockspace area.
+ with io.open(path, "rb+") as f:
+ f.write(align * b"x")
+ f.write(4096 * b"X")
+
+ sanlock.write_resource(
+ "ls_name", "res_name", disks, align=align, sector=SECTOR_SIZE_4K)
+
+ res = sanlock.read_resource(path, align=align, sector=SECTOR_SIZE_4K)
+
+ assert res == {
+ "lockspace": "ls_name",
+ "resource": "res_name",
+ "version": 0
+ }
+
+ owners = sanlock.read_resource_owners("ls_name", "res_name", disks)
+ assert owners == []
+
+ with io.open(path, "rb") as f:
+ # Verify that resource was written.
+ magic, = struct.unpack("< I", f.read(4))
+ assert magic == constants.PAXOS_DISK_MAGIC
+
+ # Check that sanlock did not write after the resource area.
+ f.seek(align)
+ assert f.read(4096) == b"X" * 4096
+
+
@pytest.mark.parametrize("size,offset", [
# Smallest offset.
(MIN_RES_SIZE, 0),
# Large offset.
(LARGE_FILE_SIZE, LARGE_FILE_SIZE - MIN_RES_SIZE),
diff --git a/tox.ini b/tox.ini
index 1c2349e..d21f2f3 100644
--- a/tox.ini
+++ b/tox.ini
@@ -7,11 +7,11 @@
envlist = py27,py36
skipsdist = True
skip_missing_interpreters = True
[testenv]
-passenv = USER
+passenv = *
setenv =
LD_LIBRARY_PATH={env:PWD}/wdmd:{env:PWD}/src
SANLOCK_PRIVILEGED=0
SANLOCK_RUN_DIR=/tmp/sanlock
whitelist_externals = make
--
2.17.2
4 years, 11 months
