[sheepdog] [PATCH v4] dog/vdi: bug fix for voting majority, add test case to test it

[Hitoshi Mitake]

On Tue, Apr 8, 2014 at 6:52 PM, Ruoyu <liangry at ucweb.com> wrote:
> There is a critical error in the function vote_majority_object.
> When all the three copies are different, the original logic still
> choose the last one as majority.
> The root cause is step 2 of the voting algorithm was missed.
>
> Signed-off-by: Ruoyu <liangry at ucweb.com>
> ---
>  dog/vdi.c                | 22 +++++++++++++---
>  tests/functional/077     | 65 ++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/functional/077.out | 45 +++++++++++++++++++++++++++++++++
>  3 files changed, 129 insertions(+), 3 deletions(-)

Looks good to me, thanks for your fix.
Reviewed-by: Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp>

>
> diff --git a/dog/vdi.c b/dog/vdi.c
> index 999d4be..db5fc58 100644
> --- a/dog/vdi.c
> +++ b/dog/vdi.c
> @@ -1578,6 +1578,7 @@ static void vote_majority_object(struct vdi_check_info *info)
>         int count = 0, nr_live_copies = 0;
>         struct vdi_check_work *majority = NULL;
>
> +       /* step 1 */
>         for (int i = 0; i < info->nr_copies; i++) {
>                 struct vdi_check_work *vcw = &info->vcw[i];
>
> @@ -1594,14 +1595,29 @@ static void vote_majority_object(struct vdi_check_info *info)
>                         count--;
>         }
>
> +       /* step 2 */
> +       if (count > 0 && count <= nr_live_copies / 2) {
> +               count = 0;
> +               for (int i = 0; i < info->nr_copies; i++) {
> +                       struct vdi_check_work *vcw = &info->vcw[i];
> +
> +                       if (!vcw->object_found)
> +                               continue;
> +                       if (!memcmp(majority->hash, vcw->hash,
> +                                               sizeof(vcw->hash)))
> +                               count++;
> +               }
> +       }
> +
>         if (!majority)
>                 info->result = VDI_CHECK_NO_OBJ_FOUND;
> -       else if (count < nr_live_copies / 2) {
> +       else if (count > nr_live_copies / 2)
> +               info->result = VDI_CHECK_SUCCESS;
> +       else {
>                 /* no majority found */
>                 majority = NULL;
>                 info->result = VDI_CHECK_NO_MAJORITY_FOUND;
> -       } else
> -               info->result = VDI_CHECK_SUCCESS;
> +       }
>
>         info->majority = majority;
>  }
> diff --git a/tests/functional/077 b/tests/functional/077
> index f2c2211..2ab8e4f 100755
> --- a/tests/functional/077
> +++ b/tests/functional/077
> @@ -82,3 +82,68 @@ _wait_for_sheep 3
>  $DOG vdi check test
>
>  $DOG cluster shutdown
> +
> +
> +
> +read_vdi_from_nodes()
> +{
> +       for i in `seq 0 $(($1-1))`; do
> +               $DOG vdi read test 0 12 -p $((7000+$i))
> +               echo ""
> +       done
> +}
> +
> +check_vdi_consistance()
> +{
> +       read_vdi_from_nodes $1
> +       $DOG vdi check test
> +       read_vdi_from_nodes $1
> +}
> +
> +echo ""
> +
> +for i in 0 1 2; do
> +    _start_sheep $i
> +done
> +_wait_for_sheep 3
> +
> +echo "yes" | $DOG cluster format -c 3
> +$DOG vdi create test 4M
> +echo "hello, copy0" | $DOG vdi write test 0 12
> +cp $STORE/0/obj/007c2b2500000000 $STORE/copy0
> +echo "hello, copy1" | $DOG vdi write test 0 12
> +cp $STORE/1/obj/007c2b2500000000 $STORE/copy1
> +echo "hello, copy2" | $DOG vdi write test 0 12
> +
> +# three copies, two broken objects, no majority
> +
> +cp $STORE/copy0 $STORE/0/obj/007c2b2500000000
> +cp $STORE/copy1 $STORE/1/obj/007c2b2500000000
> +check_vdi_consistance 3
> +
> +for i in 3 4; do
> +    _start_sheep $i
> +done
> +_wait_for_sheep 5
> +
> +echo "yes" | $DOG cluster format -c 5
> +$DOG vdi create test 4M
> +echo "hello, copy0" | $DOG vdi write test 0 12
> +cp $STORE/0/obj/007c2b2500000000 $STORE/copy0
> +echo "hello, copy1" | $DOG vdi write test 0 12
> +cp $STORE/1/obj/007c2b2500000000 $STORE/copy1
> +
> +# five copies, two broken objects(AABBB)
> +
> +cp $STORE/copy0 $STORE/0/obj/007c2b2500000000
> +cp $STORE/copy0 $STORE/1/obj/007c2b2500000000
> +check_vdi_consistance 5
> +
> +# five copies, two broken objects(BABAB)
> +
> +cp $STORE/copy0 $STORE/1/obj/007c2b2500000000
> +cp $STORE/copy0 $STORE/3/obj/007c2b2500000000
> +check_vdi_consistance 5
> +
> +$DOG cluster shutdown
> +
> diff --git a/tests/functional/077.out b/tests/functional/077.out
> index be8c8d7..c966bbf 100644
> --- a/tests/functional/077.out
> +++ b/tests/functional/077.out
> @@ -14,3 +14,48 @@ finish check&repair test
>  original data
>  no majority of 7c2b2500000000
>  finish check&repair test
> +
> +    __
> +   ()'`;
> +   /\|`
> +  /  |   Caution! The cluster is not empty.
> +(/_)_|_  Are you sure you want to continue? [yes/no]: using backend plain store
> +hello, copy0
> +hello, copy1
> +hello, copy2
> +no majority of 7c2b2500000000
> +finish check&repair test
> +hello, copy0
> +hello, copy1
> +hello, copy2
> +    __
> +   ()'`;
> +   /\|`
> +  /  |   Caution! The cluster is not empty.
> +(/_)_|_  Are you sure you want to continue? [yes/no]: using backend plain store
> +hello, copy0
> +hello, copy0
> +hello, copy1
> +hello, copy1
> +hello, copy1
> +fixed replica 7c2b2500000000
> +fixed replica 7c2b2500000000
> +finish check&repair test
> +hello, copy1
> +hello, copy1
> +hello, copy1
> +hello, copy1
> +hello, copy1
> +hello, copy1
> +hello, copy0
> +hello, copy1
> +hello, copy0
> +hello, copy1
> +fixed replica 7c2b2500000000
> +fixed replica 7c2b2500000000
> +finish check&repair test
> +hello, copy1
> +hello, copy1
> +hello, copy1
> +hello, copy1
> +hello, copy1
> --
> 1.8.3.2
>
>
> --
> sheepdog mailing list
> sheepdog at lists.wpkg.org
> http://lists.wpkg.org/mailman/listinfo/sheepdog