Re: [PATCH 3/4] tests: Test wrong sector size with 4k storage

[Nir Soffer]

On Wed, May 8, 2019 at 10:19 AM Pavel Bar <pbar(a)redhat.com&gt; wrote:

>  On Tue, May 7, 2019 at 1:38 AM Nir Soffer <nirsof(a)gmail.com&gt; 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&gt;
> > ---
> >  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=&quot;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=&quot;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
> >
> >