OSD abstract class

OSD abstract class

[Loic Dachary]

[-- Attachment #1: Type: text/plain, Size: 1117 bytes --]

Hi,

In the context of the implementation of

http://wiki.ceph.com/01Planning/02Blueprints/Dumpling/Erasure_encoding_as_a_storage_backend

I'm preparing tests to assert that the modifications that will be made to the existing PG and ReplicatedPG will not introduce regressions. Unless I'm mistaken, there are no unit tests or functional tests for PG or ReplicatedPG at the moment. I thought that it might be useful to add an abstract OSD class from which OSD is derived, to allow a test to derive from it to implement synthetic behavior. 


I gave it a try and it passes make check successfully. It is a little hairy because I commented out the code instead of removing it to help with rebasing. 

https://github.com/dachary/ceph/commit/a9eb690a3537c3f844b47e76cde048b084e314eb

I'll now try to use it to implement some tests for ReplicatedPG. 

I would very much appreciate if someone has an advice on the best way to proceed. Even if it's to say : "you're crazy, don't go there ! you're wasting your time, there is a much simpler way !" ;-)

Cheers
 
-- 
Loïc Dachary, Artisan Logiciel Libre


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: OSD abstract class

[Gregory Farnum]

On Thu, Apr 25, 2013 at 1:17 AM, Loic Dachary <loic@dachary.org> wrote:
> Hi,
>
> In the context of the implementation of
>
> http://wiki.ceph.com/01Planning/02Blueprints/Dumpling/Erasure_encoding_as_a_storage_backend
>
> I'm preparing tests to assert that the modifications that will be made to the existing PG and ReplicatedPG will not introduce regressions. Unless I'm mistaken, there are no unit tests or functional tests for PG or ReplicatedPG at the moment. I thought that it might be useful to add an abstract OSD class from which OSD is derived, to allow a test to derive from it to implement synthetic behavior.
>
>
> I gave it a try and it passes make check successfully. It is a little hairy because I commented out the code instead of removing it to help with rebasing.
>
> https://github.com/dachary/ceph/commit/a9eb690a3537c3f844b47e76cde048b084e314eb
>
> I'll now try to use it to implement some tests for ReplicatedPG.
>
> I would very much appreciate if someone has an advice on the best way to proceed. Even if it's to say : "you're crazy, don't go there ! you're wasting your time, there is a much simpler way !" ;-)

What's your plot here? The OSD is fairly large to be doing unit tests
on (although it's not insensible) — but if we want to form a new PG it
might be best to start out by making abstract PG implementations.
That'll be a bit easier since I believe I they only access the
OSDService now, and not the OSD proper.
-Greg
Software Engineer #42 @ http://inktank.com | http://ceph.com
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: OSD abstract class

[Loic Dachary]

[-- Attachment #1: Type: text/plain, Size: 2761 bytes --]



On 04/25/2013 06:45 PM, Gregory Farnum wrote:
> On Thu, Apr 25, 2013 at 1:17 AM, Loic Dachary <loic@dachary.org> wrote:
>> Hi,
>>
>> In the context of the implementation of
>>
>> http://wiki.ceph.com/01Planning/02Blueprints/Dumpling/Erasure_encoding_as_a_storage_backend
>>
>> I'm preparing tests to assert that the modifications that will be made to the existing PG and ReplicatedPG will not introduce regressions. Unless I'm mistaken, there are no unit tests or functional tests for PG or ReplicatedPG at the moment. I thought that it might be useful to add an abstract OSD class from which OSD is derived, to allow a test to derive from it to implement synthetic behavior.
>>
>>
>> I gave it a try and it passes make check successfully. It is a little hairy because I commented out the code instead of removing it to help with rebasing.
>>
>> https://github.com/dachary/ceph/commit/a9eb690a3537c3f844b47e76cde048b084e314eb
>>
>> I'll now try to use it to implement some tests for ReplicatedPG.
>>
>> I would very much appreciate if someone has an advice on the best way to proceed. Even if it's to say : "you're crazy, don't go there ! you're wasting your time, there is a much simpler way !" ;-)
> 
> What's your plot here? The OSD is fairly large to be doing unit tests
> on (although it's not insensible) — but if we want to form a new PG it
> might be best to start out by making abstract PG implementations.
> 

My reasoning was that to for a new PG it was enough to rely on

https://github.com/ceph/ceph/blob/master/src/osd/PG.h

and create a new ErasureEncodingPG.{cc,h} after 

https://github.com/ceph/ceph/blob/master/src/osd/ReplicatedPG.cc

Since ReplicatedPG is the only class derived from PG, I assumed there will be a need to move code from ReplicatedPG to PG and vice versa to make sure PG can actually be the base class of two different implementations. Are you suggesting that PG should be abstracted ? Is there a reason why it would not be a good base class for a new ErasureEncodedPG ? 

> That'll be a bit easier since I believe I they only access the
> OSDService now, and not the OSD proper.

Indeed :-) It will be a lot easier to write an OSDService abstract class to be derived for test purposes. It is much more complicated to do the same with OSD. And probably not necessary since PG's only access OSD thru OSDService. 

Thanks a thousand time for the tip :-)

> -Greg
> Software Engineer #42 @ http://inktank.com | http://ceph.com
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Loïc Dachary, Artisan Logiciel Libre


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: OSD abstract class

[Gregory Farnum]

On Thu, Apr 25, 2013 at 11:28 AM, Loic Dachary <loic@dachary.org> wrote:
>
>
> On 04/25/2013 06:45 PM, Gregory Farnum wrote:
>> On Thu, Apr 25, 2013 at 1:17 AM, Loic Dachary <loic@dachary.org> wrote:
>>> Hi,
>>>
>>> In the context of the implementation of
>>>
>>> http://wiki.ceph.com/01Planning/02Blueprints/Dumpling/Erasure_encoding_as_a_storage_backend
>>>
>>> I'm preparing tests to assert that the modifications that will be made to the existing PG and ReplicatedPG will not introduce regressions. Unless I'm mistaken, there are no unit tests or functional tests for PG or ReplicatedPG at the moment. I thought that it might be useful to add an abstract OSD class from which OSD is derived, to allow a test to derive from it to implement synthetic behavior.
>>>
>>>
>>> I gave it a try and it passes make check successfully. It is a little hairy because I commented out the code instead of removing it to help with rebasing.
>>>
>>> https://github.com/dachary/ceph/commit/a9eb690a3537c3f844b47e76cde048b084e314eb
>>>
>>> I'll now try to use it to implement some tests for ReplicatedPG.
>>>
>>> I would very much appreciate if someone has an advice on the best way to proceed. Even if it's to say : "you're crazy, don't go there ! you're wasting your time, there is a much simpler way !" ;-)
>>
>> What's your plot here? The OSD is fairly large to be doing unit tests
>> on (although it's not insensible) — but if we want to form a new PG it
>> might be best to start out by making abstract PG implementations.
>>
>
> My reasoning was that to for a new PG it was enough to rely on
>
> https://github.com/ceph/ceph/blob/master/src/osd/PG.h
>
> and create a new ErasureEncodingPG.{cc,h} after
>
> https://github.com/ceph/ceph/blob/master/src/osd/ReplicatedPG.cc
>
> Since ReplicatedPG is the only class derived from PG, I assumed there will be a need to move code from ReplicatedPG to PG and vice versa to make sure PG can actually be the base class of two different implementations. Are you suggesting that PG should be abstracted ? Is there a reason why it would not be a good base class for a new ErasureEncodedPG ?

Oh, it *should* be the base class, but the distinction between PG and
ReplicatedPG right now is...vanishing. I had a quick chat about this
with Sam a couple days ago and we agreed the first task here is going
to be properly slicing them up somehow (the current state of affairs
is not a good model, even, from what he's told me).


I'm not saying the OSD is a bad plan, just that I'm not sure what kind
of useful tests we could produce by abstracting it — whereas the unit
tests we can write against a PG seem a lot simpler and more obvious. I
could well be missing something, though.
-Greg
Software Engineer #42 @ http://inktank.com | http://ceph.com
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: OSD abstract class

[Loic Dachary]

[-- Attachment #1: Type: text/plain, Size: 3226 bytes --]



On 04/25/2013 09:47 PM, Gregory Farnum wrote:
> On Thu, Apr 25, 2013 at 11:28 AM, Loic Dachary <loic@dachary.org> wrote:
>>
>>
>> On 04/25/2013 06:45 PM, Gregory Farnum wrote:
>>> On Thu, Apr 25, 2013 at 1:17 AM, Loic Dachary <loic@dachary.org> wrote:
>>>> Hi,
>>>>
>>>> In the context of the implementation of
>>>>
>>>> http://wiki.ceph.com/01Planning/02Blueprints/Dumpling/Erasure_encoding_as_a_storage_backend
>>>>
>>>> I'm preparing tests to assert that the modifications that will be made to the existing PG and ReplicatedPG will not introduce regressions. Unless I'm mistaken, there are no unit tests or functional tests for PG or ReplicatedPG at the moment. I thought that it might be useful to add an abstract OSD class from which OSD is derived, to allow a test to derive from it to implement synthetic behavior.
>>>>
>>>>
>>>> I gave it a try and it passes make check successfully. It is a little hairy because I commented out the code instead of removing it to help with rebasing.
>>>>
>>>> https://github.com/dachary/ceph/commit/a9eb690a3537c3f844b47e76cde048b084e314eb
>>>>
>>>> I'll now try to use it to implement some tests for ReplicatedPG.
>>>>
>>>> I would very much appreciate if someone has an advice on the best way to proceed. Even if it's to say : "you're crazy, don't go there ! you're wasting your time, there is a much simpler way !" ;-)
>>>
>>> What's your plot here? The OSD is fairly large to be doing unit tests
>>> on (although it's not insensible) — but if we want to form a new PG it
>>> might be best to start out by making abstract PG implementations.
>>>
>>
>> My reasoning was that to for a new PG it was enough to rely on
>>
>> https://github.com/ceph/ceph/blob/master/src/osd/PG.h
>>
>> and create a new ErasureEncodingPG.{cc,h} after
>>
>> https://github.com/ceph/ceph/blob/master/src/osd/ReplicatedPG.cc
>>
>> Since ReplicatedPG is the only class derived from PG, I assumed there will be a need to move code from ReplicatedPG to PG and vice versa to make sure PG can actually be the base class of two different implementations. Are you suggesting that PG should be abstracted ? Is there a reason why it would not be a good base class for a new ErasureEncodedPG ?
> 
> Oh, it *should* be the base class, but the distinction between PG and
> ReplicatedPG right now is...vanishing. I had a quick chat about this
> with Sam a couple days ago and we agreed the first task here is going
> to be properly slicing them up somehow (the current state of affairs
> is not a good model, even, from what he's told me).

Understood :-) I'm quite sure this is out of my league.

> I'm not saying the OSD is a bad plan, just that I'm not sure what kind
> of useful tests we could produce by abstracting it — whereas the unit
> tests we can write against a PG seem a lot simpler and more obvious. I
> could well be missing something, though.

I'll focus on writing PG tests. It will help me understand the details of PG / ReplicatedPG and eventually help with the necessary refactoring.

Thanks again for the guidance :-)

> -Greg
> Software Engineer #42 @ http://inktank.com | http://ceph.com

-- 
Loïc Dachary, Artisan Logiciel Libre


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: OSD abstract class

[Samuel Just]

A good place to start would be to get some unit test coverage of the
PG::PriorSet logic.  It's already factored out to a degree.
osd_types.h::pg_interval_t::check_new_interval would also be a good
place for testing.

For something rather more ambitious, the
merge_log/rewind_dievergent_log and related methods could use better
test coverage, but that would require some significant refactoring.
-Sam

On Thu, Apr 25, 2013 at 1:20 PM, Loic Dachary <loic@dachary.org> wrote:
>
>
> On 04/25/2013 09:47 PM, Gregory Farnum wrote:
>> On Thu, Apr 25, 2013 at 11:28 AM, Loic Dachary <loic@dachary.org> wrote:
>>>
>>>
>>> On 04/25/2013 06:45 PM, Gregory Farnum wrote:
>>>> On Thu, Apr 25, 2013 at 1:17 AM, Loic Dachary <loic@dachary.org> wrote:
>>>>> Hi,
>>>>>
>>>>> In the context of the implementation of
>>>>>
>>>>> http://wiki.ceph.com/01Planning/02Blueprints/Dumpling/Erasure_encoding_as_a_storage_backend
>>>>>
>>>>> I'm preparing tests to assert that the modifications that will be made to the existing PG and ReplicatedPG will not introduce regressions. Unless I'm mistaken, there are no unit tests or functional tests for PG or ReplicatedPG at the moment. I thought that it might be useful to add an abstract OSD class from which OSD is derived, to allow a test to derive from it to implement synthetic behavior.
>>>>>
>>>>>
>>>>> I gave it a try and it passes make check successfully. It is a little hairy because I commented out the code instead of removing it to help with rebasing.
>>>>>
>>>>> https://github.com/dachary/ceph/commit/a9eb690a3537c3f844b47e76cde048b084e314eb
>>>>>
>>>>> I'll now try to use it to implement some tests for ReplicatedPG.
>>>>>
>>>>> I would very much appreciate if someone has an advice on the best way to proceed. Even if it's to say : "you're crazy, don't go there ! you're wasting your time, there is a much simpler way !" ;-)
>>>>
>>>> What's your plot here? The OSD is fairly large to be doing unit tests
>>>> on (although it's not insensible) — but if we want to form a new PG it
>>>> might be best to start out by making abstract PG implementations.
>>>>
>>>
>>> My reasoning was that to for a new PG it was enough to rely on
>>>
>>> https://github.com/ceph/ceph/blob/master/src/osd/PG.h
>>>
>>> and create a new ErasureEncodingPG.{cc,h} after
>>>
>>> https://github.com/ceph/ceph/blob/master/src/osd/ReplicatedPG.cc
>>>
>>> Since ReplicatedPG is the only class derived from PG, I assumed there will be a need to move code from ReplicatedPG to PG and vice versa to make sure PG can actually be the base class of two different implementations. Are you suggesting that PG should be abstracted ? Is there a reason why it would not be a good base class for a new ErasureEncodedPG ?
>>
>> Oh, it *should* be the base class, but the distinction between PG and
>> ReplicatedPG right now is...vanishing. I had a quick chat about this
>> with Sam a couple days ago and we agreed the first task here is going
>> to be properly slicing them up somehow (the current state of affairs
>> is not a good model, even, from what he's told me).
>
> Understood :-) I'm quite sure this is out of my league.
>
>> I'm not saying the OSD is a bad plan, just that I'm not sure what kind
>> of useful tests we could produce by abstracting it — whereas the unit
>> tests we can write against a PG seem a lot simpler and more obvious. I
>> could well be missing something, though.
>
> I'll focus on writing PG tests. It will help me understand the details of PG / ReplicatedPG and eventually help with the necessary refactoring.
>
> Thanks again for the guidance :-)
>
>> -Greg
>> Software Engineer #42 @ http://inktank.com | http://ceph.com
>
> --
> Loïc Dachary, Artisan Logiciel Libre
>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: OSD abstract class

[Loic Dachary]

[-- Attachment #1: Type: text/plain, Size: 4195 bytes --]



On 04/26/2013 03:05 AM, Samuel Just wrote:
> A good place to start would be to get some unit test coverage of the
> PG::PriorSet logic.  It's already factored out to a degree.
> osd_types.h::pg_interval_t::check_new_interval would also be a good
> place for testing.
> 
> For something rather more ambitious, the
> merge_log/rewind_dievergent_log and related methods could use better
> test coverage, but that would require some significant refactoring.

Hi Sam,

A short note to let you know that I appreciate the advice. I'll need to understand a little more about PG before I can act on it ;-)

Cheers

> -Sam
> 
> On Thu, Apr 25, 2013 at 1:20 PM, Loic Dachary <loic@dachary.org> wrote:
>>
>>
>> On 04/25/2013 09:47 PM, Gregory Farnum wrote:
>>> On Thu, Apr 25, 2013 at 11:28 AM, Loic Dachary <loic@dachary.org> wrote:
>>>>
>>>>
>>>> On 04/25/2013 06:45 PM, Gregory Farnum wrote:
>>>>> On Thu, Apr 25, 2013 at 1:17 AM, Loic Dachary <loic@dachary.org> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> In the context of the implementation of
>>>>>>
>>>>>> http://wiki.ceph.com/01Planning/02Blueprints/Dumpling/Erasure_encoding_as_a_storage_backend
>>>>>>
>>>>>> I'm preparing tests to assert that the modifications that will be made to the existing PG and ReplicatedPG will not introduce regressions. Unless I'm mistaken, there are no unit tests or functional tests for PG or ReplicatedPG at the moment. I thought that it might be useful to add an abstract OSD class from which OSD is derived, to allow a test to derive from it to implement synthetic behavior.
>>>>>>
>>>>>>
>>>>>> I gave it a try and it passes make check successfully. It is a little hairy because I commented out the code instead of removing it to help with rebasing.
>>>>>>
>>>>>> https://github.com/dachary/ceph/commit/a9eb690a3537c3f844b47e76cde048b084e314eb
>>>>>>
>>>>>> I'll now try to use it to implement some tests for ReplicatedPG.
>>>>>>
>>>>>> I would very much appreciate if someone has an advice on the best way to proceed. Even if it's to say : "you're crazy, don't go there ! you're wasting your time, there is a much simpler way !" ;-)
>>>>>
>>>>> What's your plot here? The OSD is fairly large to be doing unit tests
>>>>> on (although it's not insensible) — but if we want to form a new PG it
>>>>> might be best to start out by making abstract PG implementations.
>>>>>
>>>>
>>>> My reasoning was that to for a new PG it was enough to rely on
>>>>
>>>> https://github.com/ceph/ceph/blob/master/src/osd/PG.h
>>>>
>>>> and create a new ErasureEncodingPG.{cc,h} after
>>>>
>>>> https://github.com/ceph/ceph/blob/master/src/osd/ReplicatedPG.cc
>>>>
>>>> Since ReplicatedPG is the only class derived from PG, I assumed there will be a need to move code from ReplicatedPG to PG and vice versa to make sure PG can actually be the base class of two different implementations. Are you suggesting that PG should be abstracted ? Is there a reason why it would not be a good base class for a new ErasureEncodedPG ?
>>>
>>> Oh, it *should* be the base class, but the distinction between PG and
>>> ReplicatedPG right now is...vanishing. I had a quick chat about this
>>> with Sam a couple days ago and we agreed the first task here is going
>>> to be properly slicing them up somehow (the current state of affairs
>>> is not a good model, even, from what he's told me).
>>
>> Understood :-) I'm quite sure this is out of my league.
>>
>>> I'm not saying the OSD is a bad plan, just that I'm not sure what kind
>>> of useful tests we could produce by abstracting it — whereas the unit
>>> tests we can write against a PG seem a lot simpler and more obvious. I
>>> could well be missing something, though.
>>
>> I'll focus on writing PG tests. It will help me understand the details of PG / ReplicatedPG and eventually help with the necessary refactoring.
>>
>> Thanks again for the guidance :-)
>>
>>> -Greg
>>> Software Engineer #42 @ http://inktank.com | http://ceph.com
>>
>> --
>> Loïc Dachary, Artisan Logiciel Libre
>>

-- 
Loïc Dachary, Artisan Logiciel Libre
All that is necessary for the triumph of evil is that good people do nothing.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]