[Libguestfs] [PATCH] New API: btrfs_replace_start

[Pino Toscano]

Hi,

On Friday 12 June 2015 19:54:26 Cao Jin wrote:
> >>>> diff --git a/tests/btrfs/test-btrfs-devices.sh b/tests/btrfs/test-btrfs-devices.sh
> >>>> index 3935c60..463b0a8 100755
> >>>> --- a/tests/btrfs/test-btrfs-devices.sh
> >>>> +++ b/tests/btrfs/test-btrfs-devices.sh
> >>>> @@ -66,6 +66,8 @@ btrfs-device-add "/dev/sdc1 /dev/sdd1" /
> >>>>    btrfs-device-delete "/dev/sdb1" /
> >>>>    btrfs-device-add "/dev/sdb1" /
> >>>>    btrfs-device-delete "/dev/sdc1 /dev/sdd1" /
> >>>> +btrfs-replace-start "/dev/sda1" "/dev/sdd1" / force:true
> >>>> +btrfs-replace-start "/dev/sdd1" "/dev/sda1" / force:true
> >>>>
> >>>>    mkdir /data2
> >>>>    tar-in $srcdir/../data/filesanddirs-10M.tar.xz /data2 compress:xz
> >>>> @@ -74,6 +76,8 @@ btrfs-device-add "/dev/sdc1 /dev/sdd1" /
> >>>>    btrfs-device-delete "/dev/sdb1" /
> >>>>    btrfs-device-add "/dev/sdb1" /
> >>>>    btrfs-device-delete "/dev/sdc1 /dev/sdd1" /
> >>>> +btrfs-replace-start "/dev/sda1" "/dev/sdd1" / force:true
> >>>> +btrfs-replace-start "/dev/sdd1" "/dev/sda1" / force:true
> >>>>
> >>>>    mkdir /data3
> >>>>    tar-in $srcdir/../data/filesanddirs-10M.tar.xz /data3 compress:xz
> >>>> @@ -82,6 +86,8 @@ btrfs-device-add "/dev/sdc1 /dev/sdd1" /
> >>>>    btrfs-device-delete "/dev/sdb1" /
> >>>>    btrfs-device-add "/dev/sdb1" /
> >>>>    btrfs-device-delete "/dev/sdc1 /dev/sdd1" /
> >>>> +btrfs-replace-start "/dev/sda1" "/dev/sdd1" / force:true
> >>>> +btrfs-replace-start "/dev/sdd1" "/dev/sda1" / force:true
> >>>>
> >>>>    mkdir /data4
> >>>>    tar-in $srcdir/../data/filesanddirs-10M.tar.xz /data4 compress:xz
> >>>> @@ -90,6 +96,8 @@ btrfs-device-add "/dev/sdc1 /dev/sdd1" /
> >>>>    btrfs-device-delete "/dev/sdb1" /
> >>>>    btrfs-device-add "/dev/sdb1" /
> >>>>    btrfs-device-delete "/dev/sdc1 /dev/sdd1" /
> >>>> +btrfs-replace-start "/dev/sda1" "/dev/sdd1" / force:true
> >>>> +btrfs-replace-start "/dev/sdd1" "/dev/sda1" / force:true
> >>>
> >>> What are these tests supposed to check? Other than calling
> >>> btrfs-replace-start and checking it does not fail, how can the result
> >>> of this operation be actually checked?
> >>>
> >>
> >> These tests are used for test whether btrfs replace will success or not.
> >> The existed add/delete test cases may also have the "problem" you
> >> mentioned: don`t know how to actually check the result, like, is the
> >> device really added/deleted in the btrfs? I have tested the api both in
> >> guestfish and the test case script, in guestfish, it is easy to check
> >> whether the device is replaced or not, just mount and check(of course,
> >> the api worked). In test case, it is not convenient. But actually, in
> >> test case, if btrfs-replace-start fails, the script will exit with
> >> errors, I encountered this situation when debug this case.
> >> Just "replace sda with sdd" is not enough for the test case, but after
> >> adding "replace sdd with sda", I think it is pretty sure that the case
> >> can actually check the result. Because even if 1st replace exit without
> >> error but actually not replaced, the 2nd replace will exit with error.
> >> So the 2nd replace add assurance. Also, I have ran the case successfully.
> >
> > This is nice description, but still we need *automated* test cases,
> > otherwise checking that the API works is a nightmare.
> >
> Yes, the test cases should be automated.
> Maybe there is a litter mistake here. what I want say is: the tests I 
> wrote, is automated, and the result is capable of checking the result 
> actually. I mentioned a little about how it can actually check the result.
> 
> As you know, the test script under tests/btrfs/ are automated, my tests 
> are added in the test-btrfs-devices.sh, so, my api test is automated. 
> And the rest we should concern is that, could the tests check the result 
> actually? Answer is: Yes. Here is the thing:
> 
> the tests I wrote:
>      A: btrfs-replace-start "/dev/sda1" "/dev/sdd1" / force:true
>      B: btrfs-replace-start "/dev/sdd1" "/dev/sda1" / force:true
> 
> When I use the api without option "-B"(the api won`t work with "-B"), 
> the tests can`t pass, will fail at B, because A actally didn`t work(but 
> don`t fail at A), so B will fail. I add B, just for the purpose that it 
> can actually check the api`s result.
> 
> I mentioned I have tested the api in guestfish, means that I am sure 
> this api can work

While I appreciate the effort you are putting in writing and testing
the new API, and the explanation about the added calls in checks,
in my opinion the added tests are not (in they way they are added)
useful.

The problem is that if I, for example, make the btrfs_replace_start
implementation a no-op (for example by having just "return 0" in it),
the test would keep working as before, as the status-quo before and
after each block of btrfs-replace-start calls is the same, and nobody
would notice if not by running it.
Sure, making it a no-op is a stretch, sure, but image if
`btrfs replace` would suddenly do nothing: the result would be the same
as above. Currently, the only thing checked is that the
btrfs_replace_start calls don't fail, but they tell me nothing about
whether each of them did the right thing.

That's why I would rather prefer a separate test for
btrfs_replace_start, either a new guestfish calls in
test-btrfs-devices.sh (using the disks created there already) or as a
separate new bash/perl test altogether.

-- 
Pino Toscano