[Libguestfs] [PATCH] New API: btrfs_replace_start

[Cao jin]

Hi，

在 2015年06月12日 20:22, 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.

I am aware of your concern, based on the unsmooth experience of the 
command. The existed btrfs_device_add/delete tests in 
test-btrfs-devices.sh may also have the same problem(like no-op but just 
return 0, or suddenly do nothing)
> 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.
>

Ok, I will put the tests in a separate new bash, try to do the tests 
more accurately.

-- 
Yours Sincerely,

Cao Jin