You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Edison Su <Ed...@citrix.com> on 2013/02/15 00:13:27 UTC
[MERGE]Storage refactor branch
Hi All,
Here is the update from storage refactor branch since last week:
1. clean/hook up snapshot into new storage framework, separate taking snapshot and backup snapshot. Add a snapshotstrategy interface for people want to change or replace how snapshot is processed in cloudstack. Current snapshot code is little bit hacky, welcome to replace it for your need.
Since last week I created storage_refactor branch, seems no objection for what I am doing, so I want to merge the branch into master, by end of this week, due to the following reasons:
Maintain such big patch(more than 20K) outside of master, touch all the storage code, is not an easy task.
After merge into master, other developers who developing storage features can work on master directly, thus I don't need to rebase the branch regularly.
About test, I only tested basic features, like, stop/start vm, attach/detach volumes, take snapshots in devcloud. For sure, I'll break something. The more test from other people, will help to me to stabilize the code. About unit test, I have some unit test and integration test for devcloud, but both of them needs database and devcloud environment, so they are disabled by default and unit test themselves are broken also. I'll write more tests during the stabilization.
Re: [MERGE]Storage refactor branch
Posted by Wido den Hollander <wi...@widodh.nl>.
On 02/15/2013 08:03 PM, Edison Su wrote:
>
>
>> -----Original Message-----
>> From: Hugo Trippaers [mailto:HTrippaers@schubergphilis.com]
>> Sent: Friday, February 15, 2013 6:39 AM
>> To: cloudstack-dev@incubator.apache.org
>> Cc: John Burwell <jb...@basho.com> (jburwell@basho.com); Mike
>> Tutkowski (mike.tutkowski@solidfire.com)
>> Subject: RE: [MERGE]Storage refactor branch
>>
>> Hey Edison,
>>
>> Thanks for the update
>>
>>> -----Original Message-----
>>> From: Edison Su [mailto:Edison.su@citrix.com]
>>> Sent: Friday, February 15, 2013 12:13 AM
>>> To: cloudstack-dev@incubator.apache.org
>>> Cc: John Burwell <jb...@basho.com> (jburwell@basho.com); Mike
>>> Tutkowski (mike.tutkowski@solidfire.com)
>>> Subject: [MERGE]Storage refactor branch
>>>
>>> Hi All,
>>> Here is the update from storage refactor branch since last week:
>>>
>>> 1. clean/hook up snapshot into new storage framework, separate taking
>>> snapshot and backup snapshot. Add a snapshotstrategy interface for
>>> people want to change or replace how snapshot is processed in
>>> cloudstack. Current snapshot code is little bit hacky, welcome to replace it
>> for your need.
>>> Since last week I created storage_refactor branch, seems no
>>> objection for what I am doing, so I want to merge the branch into
>>> master, by end of this week, due to the following reasons:
>>> Maintain such big patch(more than 20K) outside of master,
>>> touch all the storage code, is not an easy task.
>>> After merge into master, other developers who developing
>>> storage features can work on master directly, thus I don't need to
>>> rebase the branch regularly.
>>> About test, I only tested basic features, like, stop/start vm,
>>> attach/detach volumes, take snapshots in devcloud. For sure, I'll
>>> break something. The more test from other people, will help to me to
>>> stabilize the code. About unit test, I have some unit test and
>>> integration test for devcloud, but both of them needs database and
>>> devcloud environment, so they are disabled by default and unit test
>>> themselves are broken also. I'll write more tests during the stabilization.
>>
>> I understand the need to merge into master, it is a serious pain to keep
>> updating the branch with that latest state of master. However this testing
>
> Thanks for understand the painful of rebaseing such big patch. There is another reason I want to merge this branch into master ASAP: all the new storage features are depended this branch: like zone-wide storage, make nfs secondary storage as optional, re-write s3/swift integration etc. Let's get it merged, adding more awesome features:)
>
I'm waiting on starting with the new RBD features until this is merged in.
I also understand what a hassle this is, got the same when developing
the original RBD without having commit rights.
Wido
>> concerns me a lot, we all agreed that we would focus on automated testing
>> both on the unittest level and on the functional level. This is the first merge
>> request since we had the last discussion on testing (related to merging the
>> javelin and other branches). The clear consensus was that commits would
>> have to be accompanied by unittests (at least for the changed pieces of code)
>> and preferable some automated functional test.
>>
>> Considering that, I think that a merge is not the right this to do at the
>> moment. Instead of merging into master and then fixing stuff, we should
>> focus on adding testing to this branch so we can merge a well-tested unit
>> later. Notice my use of 'we' here, we all said focus on testing, so we all should
>> help out. Unfortunately I'm pretty busy with packaging and the $dayjob, but
>> if you have something I can help you with regarding setting up unittests feel
>> free to let me know. You already mentioned that you have some, but the
>> databases need to be mocked?
>>
>> It would be great if you could spend some time to details what tests you
>> have in place, so we can fix those. With the help of the cobertura report we
>> can figure out where the risks are. Prassana and myself are also working on
>> the automated test framework, so if you have some marvin tests, we can
>> help to automate them.
>
> My branch is not a feature branch, while other features are depended on it. I didn't add any new feature on the branch, all the existing marvin automated tests should work. Instead of testing and fixing on my branch then merge, is it better to test and fix on master after the merge, using existing marvin test?
> Again, the rebase to master is painful for such branch.
>
>
>>
>> Anybody else willing to help put in some tests into the storage_refactor
>> branch?
>>
>> Cheers,
>>
>> Hugo
Re: [MERGE]Storage refactor branch
Posted by Hugo Trippaers <HT...@schubergphilis.com>.
Verstuurd vanaf mijn iPad
Op 22 feb. 2013 om 19:44 heeft "Edison Su" <Ed...@citrix.com> het volgende geschreven:
>
>
>> -----Original Message-----
>> From: Hugo Trippaers [mailto:HTrippaers@schubergphilis.com]
>> Sent: Friday, February 22, 2013 10:03 AM
>> To: Edison Su
>> Cc: cloudstack-dev@incubator.apache.org
>> Subject: Re: [MERGE]Storage refactor branch
>>
>>
>>
>> Verstuurd vanaf mijn iPad
>>
>> Op 22 feb. 2013 om 18:32 heeft "Edison Su" <Ed...@citrix.com> het
>> volgende geschreven:
>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Hugo Trippaers [mailto:HTrippaers@schubergphilis.com]
>>>> Sent: Friday, February 22, 2013 2:46 AM
>>>> To: cloudstack-dev@incubator.apache.org; Edison Su
>>>> Cc: John Burwell <jb...@basho.com> (jburwell@basho.com); Mike
>>>> Tutkowski (mike.tutkowski@solidfire.com)
>>>> Subject: RE: [MERGE]Storage refactor branch
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Chip Childers [mailto:chip.childers@sungard.com]
>>>>> Sent: donderdag 21 februari 2013 21:17
>>>>> To: cloudstack-dev@incubator.apache.org; Edison.su@citrix.com
>>>>> Cc: John Burwell <jb...@basho.com> (jburwell@basho.com); Mike
>>>>> Tutkowski (mike.tutkowski@solidfire.com)
>>>>> Subject: Re: [MERGE]Storage refactor branch
>>>>>
>>>>> On Wed, Feb 20, 2013 at 03:55:59PM -0500, Chip Childers wrote:
>>>>>> On Fri, Feb 15, 2013 at 11:03:27AM -0800, Edison Su wrote:
>>>>>>> My branch is not a feature branch, while other features are
>>>>>>> depended on
>>>>> it. I didn't add any new feature on the branch, all the existing
>>>>> marvin automated tests should work. Instead of testing and fixing on
>>>>> my branch then merge, is it better to test and fix on master after
>>>>> the merge, using existing marvin test?
>>>>>>
>>>>>> IMO, it's not ever good to intentionally to break master.
>>>>>
>>>>> Edison - I see that you merged this into master today. Is master
>>>>> now in a state where it's broken? Did you run the marvin tests
>>>>> against your branch prior to the merge?
>>>>
>>>> I'm pretty surprised by this merge. We have about three running
>>>> threads on the developer list regarding testing and the overall
>>>> quality of the master branch. This particular merge thread on the ML
>>>> has valid concerns for testing of the branch, which have not been
>>>> addressed. Yet all this is ignored and the branch is merged anyway?
>>>> This is not what we all agreed to do, Edison, could you please explain why
>> you did this?
>>>
>>> I send out the request, there is no objection in 72 hours, so I think I can
>> merge it in.
>>
>> I guess I should have been more clear in choosing my words, I didn't approve
>> of this to be merged without unit tests, but I think I should have stated this
>> more clearly.
>>
>>> I am also trying to setup a marvin test, but the smoke test is
>>> disabled:
>>> http://jenkins.cloudstack.org/view/cloudstack-qa/job/test-cloudstack-s
>>> moke/ There are few functions(like migration volume between pools,
>>> create template from snapshot, etc) I haven't tested by myself, but these
>> functions will not block developer's daily work, even if they don't work. The
>> reason I don't want to test these features, because, my next next task(after
>> zone-wide storage) is to refactor nfs secondary storage, so I'll change that
>> part of code again. Then the purpose of fully test at these stage has not
>> much value.
>>
>> We should never merge in incomplete or untested code into master. If you
>> have a certain piece of code that you didn't test, you should not commit it to
>> master. It's not about developers daily work not being blocked, it's about the
>> quality of the code people push into master. If you don't want to test the
>> code yet because of the next feature, fine, but don't merge it then. Testing
>> (and specifically unit testing) is about quality of the code. Proving that the
>> logic does what it is supposed to do and that it can handle errors by design.
>> It's more than just testing and seeing if something works. But I think enough
>> has been said about unit testing in other threads.
>>
>> We are so far behind in unit tests that any piece of the code we touch should
>> result in a unit test. So please plan for this when you are working on the next
>> feature and I think it would be a nice gesture if you could write and submit
>> the missing unit tests for this storage refactor before going to the next
>> feature.
> The change is so huge, in order to test all my changes, that will take long time. What I'll do, is gradually adding more unit tests after the merge.
> I wish I am just writing some new features, man, refactor is not an easy task....:)
Granted :-) looking forward to seeing more unit tests popping up in the near future. I'll gladly help if you point me in the right direction.
>
>>
>>
>>>
>>>>
>>>>>
>>>>> -chip
RE: [MERGE]Storage refactor branch
Posted by Edison Su <Ed...@citrix.com>.
> -----Original Message-----
> From: Hugo Trippaers [mailto:HTrippaers@schubergphilis.com]
> Sent: Friday, February 22, 2013 10:03 AM
> To: Edison Su
> Cc: cloudstack-dev@incubator.apache.org
> Subject: Re: [MERGE]Storage refactor branch
>
>
>
> Verstuurd vanaf mijn iPad
>
> Op 22 feb. 2013 om 18:32 heeft "Edison Su" <Ed...@citrix.com> het
> volgende geschreven:
>
> >
> >
> >> -----Original Message-----
> >> From: Hugo Trippaers [mailto:HTrippaers@schubergphilis.com]
> >> Sent: Friday, February 22, 2013 2:46 AM
> >> To: cloudstack-dev@incubator.apache.org; Edison Su
> >> Cc: John Burwell <jb...@basho.com> (jburwell@basho.com); Mike
> >> Tutkowski (mike.tutkowski@solidfire.com)
> >> Subject: RE: [MERGE]Storage refactor branch
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: Chip Childers [mailto:chip.childers@sungard.com]
> >>> Sent: donderdag 21 februari 2013 21:17
> >>> To: cloudstack-dev@incubator.apache.org; Edison.su@citrix.com
> >>> Cc: John Burwell <jb...@basho.com> (jburwell@basho.com); Mike
> >>> Tutkowski (mike.tutkowski@solidfire.com)
> >>> Subject: Re: [MERGE]Storage refactor branch
> >>>
> >>> On Wed, Feb 20, 2013 at 03:55:59PM -0500, Chip Childers wrote:
> >>>> On Fri, Feb 15, 2013 at 11:03:27AM -0800, Edison Su wrote:
> >>>>> My branch is not a feature branch, while other features are
> >>>>> depended on
> >>> it. I didn't add any new feature on the branch, all the existing
> >>> marvin automated tests should work. Instead of testing and fixing on
> >>> my branch then merge, is it better to test and fix on master after
> >>> the merge, using existing marvin test?
> >>>>
> >>>> IMO, it's not ever good to intentionally to break master.
> >>>
> >>> Edison - I see that you merged this into master today. Is master
> >>> now in a state where it's broken? Did you run the marvin tests
> >>> against your branch prior to the merge?
> >>
> >> I'm pretty surprised by this merge. We have about three running
> >> threads on the developer list regarding testing and the overall
> >> quality of the master branch. This particular merge thread on the ML
> >> has valid concerns for testing of the branch, which have not been
> >> addressed. Yet all this is ignored and the branch is merged anyway?
> >> This is not what we all agreed to do, Edison, could you please explain why
> you did this?
> >
> > I send out the request, there is no objection in 72 hours, so I think I can
> merge it in.
>
> I guess I should have been more clear in choosing my words, I didn't approve
> of this to be merged without unit tests, but I think I should have stated this
> more clearly.
>
> > I am also trying to setup a marvin test, but the smoke test is
> > disabled:
> > http://jenkins.cloudstack.org/view/cloudstack-qa/job/test-cloudstack-s
> > moke/ There are few functions(like migration volume between pools,
> > create template from snapshot, etc) I haven't tested by myself, but these
> functions will not block developer's daily work, even if they don't work. The
> reason I don't want to test these features, because, my next next task(after
> zone-wide storage) is to refactor nfs secondary storage, so I'll change that
> part of code again. Then the purpose of fully test at these stage has not
> much value.
>
> We should never merge in incomplete or untested code into master. If you
> have a certain piece of code that you didn't test, you should not commit it to
> master. It's not about developers daily work not being blocked, it's about the
> quality of the code people push into master. If you don't want to test the
> code yet because of the next feature, fine, but don't merge it then. Testing
> (and specifically unit testing) is about quality of the code. Proving that the
> logic does what it is supposed to do and that it can handle errors by design.
> It's more than just testing and seeing if something works. But I think enough
> has been said about unit testing in other threads.
>
> We are so far behind in unit tests that any piece of the code we touch should
> result in a unit test. So please plan for this when you are working on the next
> feature and I think it would be a nice gesture if you could write and submit
> the missing unit tests for this storage refactor before going to the next
> feature.
The change is so huge, in order to test all my changes, that will take long time. What I'll do, is gradually adding more unit tests after the merge.
I wish I am just writing some new features, man, refactor is not an easy task....:)
>
>
> >
> >>
> >>>
> >>> -chip
Re: [MERGE]Storage refactor branch
Posted by Hugo Trippaers <HT...@schubergphilis.com>.
Verstuurd vanaf mijn iPad
Op 22 feb. 2013 om 18:32 heeft "Edison Su" <Ed...@citrix.com> het volgende geschreven:
>
>
>> -----Original Message-----
>> From: Hugo Trippaers [mailto:HTrippaers@schubergphilis.com]
>> Sent: Friday, February 22, 2013 2:46 AM
>> To: cloudstack-dev@incubator.apache.org; Edison Su
>> Cc: John Burwell <jb...@basho.com> (jburwell@basho.com); Mike
>> Tutkowski (mike.tutkowski@solidfire.com)
>> Subject: RE: [MERGE]Storage refactor branch
>>
>>
>>
>>> -----Original Message-----
>>> From: Chip Childers [mailto:chip.childers@sungard.com]
>>> Sent: donderdag 21 februari 2013 21:17
>>> To: cloudstack-dev@incubator.apache.org; Edison.su@citrix.com
>>> Cc: John Burwell <jb...@basho.com> (jburwell@basho.com); Mike
>>> Tutkowski (mike.tutkowski@solidfire.com)
>>> Subject: Re: [MERGE]Storage refactor branch
>>>
>>> On Wed, Feb 20, 2013 at 03:55:59PM -0500, Chip Childers wrote:
>>>> On Fri, Feb 15, 2013 at 11:03:27AM -0800, Edison Su wrote:
>>>>> My branch is not a feature branch, while other features are
>>>>> depended on
>>> it. I didn't add any new feature on the branch, all the existing
>>> marvin automated tests should work. Instead of testing and fixing on
>>> my branch then merge, is it better to test and fix on master after the
>>> merge, using existing marvin test?
>>>>
>>>> IMO, it's not ever good to intentionally to break master.
>>>
>>> Edison - I see that you merged this into master today. Is master now
>>> in a state where it's broken? Did you run the marvin tests against
>>> your branch prior to the merge?
>>
>> I'm pretty surprised by this merge. We have about three running threads on
>> the developer list regarding testing and the overall quality of the master
>> branch. This particular merge thread on the ML has valid concerns for testing
>> of the branch, which have not been addressed. Yet all this is ignored and the
>> branch is merged anyway? This is not what we all agreed to do, Edison, could
>> you please explain why you did this?
>
> I send out the request, there is no objection in 72 hours, so I think I can merge it in.
I guess I should have been more clear in choosing my words, I didn't approve of this to be merged without unit tests, but I think I should have stated this more clearly.
> I am also trying to setup a marvin test, but the smoke test is disabled: http://jenkins.cloudstack.org/view/cloudstack-qa/job/test-cloudstack-smoke/
> There are few functions(like migration volume between pools, create template from snapshot, etc) I haven't tested by myself, but these functions will not block developer's daily work, even if they don't work. The reason I don't want to test these features, because, my next next task(after zone-wide storage) is to refactor nfs secondary storage, so I'll change that part of code again. Then the purpose of fully test at these stage has not much value.
We should never merge in incomplete or untested code into master. If you have a certain piece of code that you didn't test, you should not commit it to master. It's not about developers daily work not being blocked, it's about the quality of the code people push into master. If you don't want to test the code yet because of the next feature, fine, but don't merge it then. Testing (and specifically unit testing) is about quality of the code. Proving that the logic does what it is supposed to do and that it can handle errors by design. It's more than just testing and seeing if something works. But I think enough has been said about unit testing in other threads.
We are so far behind in unit tests that any piece of the code we touch should result in a unit test. So please plan for this when you are working on the next feature and I think it would be a nice gesture if you could write and submit the missing unit tests for this storage refactor before going to the next feature.
>
>>
>>>
>>> -chip
RE: [MERGE]Storage refactor branch
Posted by Edison Su <Ed...@citrix.com>.
> -----Original Message-----
> From: Hugo Trippaers [mailto:HTrippaers@schubergphilis.com]
> Sent: Friday, February 22, 2013 2:46 AM
> To: cloudstack-dev@incubator.apache.org; Edison Su
> Cc: John Burwell <jb...@basho.com> (jburwell@basho.com); Mike
> Tutkowski (mike.tutkowski@solidfire.com)
> Subject: RE: [MERGE]Storage refactor branch
>
>
>
> > -----Original Message-----
> > From: Chip Childers [mailto:chip.childers@sungard.com]
> > Sent: donderdag 21 februari 2013 21:17
> > To: cloudstack-dev@incubator.apache.org; Edison.su@citrix.com
> > Cc: John Burwell <jb...@basho.com> (jburwell@basho.com); Mike
> > Tutkowski (mike.tutkowski@solidfire.com)
> > Subject: Re: [MERGE]Storage refactor branch
> >
> > On Wed, Feb 20, 2013 at 03:55:59PM -0500, Chip Childers wrote:
> > > On Fri, Feb 15, 2013 at 11:03:27AM -0800, Edison Su wrote:
> > > > My branch is not a feature branch, while other features are
> > > > depended on
> > it. I didn't add any new feature on the branch, all the existing
> > marvin automated tests should work. Instead of testing and fixing on
> > my branch then merge, is it better to test and fix on master after the
> > merge, using existing marvin test?
> > >
> > > IMO, it's not ever good to intentionally to break master.
> > >
> >
> > Edison - I see that you merged this into master today. Is master now
> > in a state where it's broken? Did you run the marvin tests against
> > your branch prior to the merge?
>
> I'm pretty surprised by this merge. We have about three running threads on
> the developer list regarding testing and the overall quality of the master
> branch. This particular merge thread on the ML has valid concerns for testing
> of the branch, which have not been addressed. Yet all this is ignored and the
> branch is merged anyway? This is not what we all agreed to do, Edison, could
> you please explain why you did this?
I send out the request, there is no objection in 72 hours, so I think I can merge it in.
I am also trying to setup a marvin test, but the smoke test is disabled: http://jenkins.cloudstack.org/view/cloudstack-qa/job/test-cloudstack-smoke/
There are few functions(like migration volume between pools, create template from snapshot, etc) I haven't tested by myself, but these functions will not block developer's daily work, even if they don't work. The reason I don't want to test these features, because, my next next task(after zone-wide storage) is to refactor nfs secondary storage, so I'll change that part of code again. Then the purpose of fully test at these stage has not much value.
>
> >
> > -chip
RE: [MERGE]Storage refactor branch
Posted by Hugo Trippaers <HT...@schubergphilis.com>.
> -----Original Message-----
> From: Chip Childers [mailto:chip.childers@sungard.com]
> Sent: donderdag 21 februari 2013 21:17
> To: cloudstack-dev@incubator.apache.org; Edison.su@citrix.com
> Cc: John Burwell <jb...@basho.com> (jburwell@basho.com); Mike
> Tutkowski (mike.tutkowski@solidfire.com)
> Subject: Re: [MERGE]Storage refactor branch
>
> On Wed, Feb 20, 2013 at 03:55:59PM -0500, Chip Childers wrote:
> > On Fri, Feb 15, 2013 at 11:03:27AM -0800, Edison Su wrote:
> > > My branch is not a feature branch, while other features are depended on
> it. I didn't add any new feature on the branch, all the existing marvin
> automated tests should work. Instead of testing and fixing on my branch
> then merge, is it better to test and fix on master after the merge, using
> existing marvin test?
> >
> > IMO, it's not ever good to intentionally to break master.
> >
>
> Edison - I see that you merged this into master today. Is master now in a
> state where it's broken? Did you run the marvin tests against your branch
> prior to the merge?
I'm pretty surprised by this merge. We have about three running threads on the developer list regarding testing and the overall quality of the master branch. This particular merge thread on the ML has valid concerns for testing of the branch, which have not been addressed. Yet all this is ignored and the branch is merged anyway? This is not what we all agreed to do, Edison, could you please explain why you did this?
>
> -chip
Re: [MERGE]Storage refactor branch
Posted by Chip Childers <ch...@sungard.com>.
On Wed, Feb 20, 2013 at 03:55:59PM -0500, Chip Childers wrote:
> On Fri, Feb 15, 2013 at 11:03:27AM -0800, Edison Su wrote:
> > My branch is not a feature branch, while other features are depended on it. I didn't add any new feature on the branch, all the existing marvin automated tests should work. Instead of testing and fixing on my branch then merge, is it better to test and fix on master after the merge, using existing marvin test?
>
> IMO, it's not ever good to intentionally to break master.
>
Edison - I see that you merged this into master today. Is master now in
a state where it's broken? Did you run the marvin tests against your
branch prior to the merge?
-chip
Re: [MERGE]Storage refactor branch
Posted by Chip Childers <ch...@sungard.com>.
On Fri, Feb 15, 2013 at 11:03:27AM -0800, Edison Su wrote:
>
>
> > -----Original Message-----
> > From: Hugo Trippaers [mailto:HTrippaers@schubergphilis.com]
> > Sent: Friday, February 15, 2013 6:39 AM
> > To: cloudstack-dev@incubator.apache.org
> > Cc: John Burwell <jb...@basho.com> (jburwell@basho.com); Mike
> > Tutkowski (mike.tutkowski@solidfire.com)
> > Subject: RE: [MERGE]Storage refactor branch
> >
> > Hey Edison,
> >
> > Thanks for the update
> >
> > > -----Original Message-----
> > > From: Edison Su [mailto:Edison.su@citrix.com]
> > > Sent: Friday, February 15, 2013 12:13 AM
> > > To: cloudstack-dev@incubator.apache.org
> > > Cc: John Burwell <jb...@basho.com> (jburwell@basho.com); Mike
> > > Tutkowski (mike.tutkowski@solidfire.com)
> > > Subject: [MERGE]Storage refactor branch
> > >
> > > Hi All,
> > > Here is the update from storage refactor branch since last week:
> > >
> > > 1. clean/hook up snapshot into new storage framework, separate taking
> > > snapshot and backup snapshot. Add a snapshotstrategy interface for
> > > people want to change or replace how snapshot is processed in
> > > cloudstack. Current snapshot code is little bit hacky, welcome to replace it
> > for your need.
> > > Since last week I created storage_refactor branch, seems no
> > > objection for what I am doing, so I want to merge the branch into
> > > master, by end of this week, due to the following reasons:
> > > Maintain such big patch(more than 20K) outside of master,
> > > touch all the storage code, is not an easy task.
> > > After merge into master, other developers who developing
> > > storage features can work on master directly, thus I don't need to
> > > rebase the branch regularly.
> > > About test, I only tested basic features, like, stop/start vm,
> > > attach/detach volumes, take snapshots in devcloud. For sure, I'll
> > > break something. The more test from other people, will help to me to
> > > stabilize the code. About unit test, I have some unit test and
> > > integration test for devcloud, but both of them needs database and
> > > devcloud environment, so they are disabled by default and unit test
> > > themselves are broken also. I'll write more tests during the stabilization.
> >
> > I understand the need to merge into master, it is a serious pain to keep
> > updating the branch with that latest state of master. However this testing
>
> Thanks for understand the painful of rebaseing such big patch. There is another reason I want to merge this branch into master ASAP: all the new storage features are depended this branch: like zone-wide storage, make nfs secondary storage as optional, re-write s3/swift integration etc. Let's get it merged, adding more awesome features:)
>
> > concerns me a lot, we all agreed that we would focus on automated testing
> > both on the unittest level and on the functional level. This is the first merge
> > request since we had the last discussion on testing (related to merging the
> > javelin and other branches). The clear consensus was that commits would
> > have to be accompanied by unittests (at least for the changed pieces of code)
> > and preferable some automated functional test.
> >
> > Considering that, I think that a merge is not the right this to do at the
> > moment. Instead of merging into master and then fixing stuff, we should
> > focus on adding testing to this branch so we can merge a well-tested unit
> > later. Notice my use of 'we' here, we all said focus on testing, so we all should
> > help out. Unfortunately I'm pretty busy with packaging and the $dayjob, but
> > if you have something I can help you with regarding setting up unittests feel
> > free to let me know. You already mentioned that you have some, but the
> > databases need to be mocked?
> >
> > It would be great if you could spend some time to details what tests you
> > have in place, so we can fix those. With the help of the cobertura report we
> > can figure out where the risks are. Prassana and myself are also working on
> > the automated test framework, so if you have some marvin tests, we can
> > help to automate them.
>
> My branch is not a feature branch, while other features are depended on it. I didn't add any new feature on the branch, all the existing marvin automated tests should work. Instead of testing and fixing on my branch then merge, is it better to test and fix on master after the merge, using existing marvin test?
IMO, it's not ever good to intentionally to break master.
RE: [MERGE]Storage refactor branch
Posted by Edison Su <Ed...@citrix.com>.
> -----Original Message-----
> From: Hugo Trippaers [mailto:HTrippaers@schubergphilis.com]
> Sent: Friday, February 15, 2013 6:39 AM
> To: cloudstack-dev@incubator.apache.org
> Cc: John Burwell <jb...@basho.com> (jburwell@basho.com); Mike
> Tutkowski (mike.tutkowski@solidfire.com)
> Subject: RE: [MERGE]Storage refactor branch
>
> Hey Edison,
>
> Thanks for the update
>
> > -----Original Message-----
> > From: Edison Su [mailto:Edison.su@citrix.com]
> > Sent: Friday, February 15, 2013 12:13 AM
> > To: cloudstack-dev@incubator.apache.org
> > Cc: John Burwell <jb...@basho.com> (jburwell@basho.com); Mike
> > Tutkowski (mike.tutkowski@solidfire.com)
> > Subject: [MERGE]Storage refactor branch
> >
> > Hi All,
> > Here is the update from storage refactor branch since last week:
> >
> > 1. clean/hook up snapshot into new storage framework, separate taking
> > snapshot and backup snapshot. Add a snapshotstrategy interface for
> > people want to change or replace how snapshot is processed in
> > cloudstack. Current snapshot code is little bit hacky, welcome to replace it
> for your need.
> > Since last week I created storage_refactor branch, seems no
> > objection for what I am doing, so I want to merge the branch into
> > master, by end of this week, due to the following reasons:
> > Maintain such big patch(more than 20K) outside of master,
> > touch all the storage code, is not an easy task.
> > After merge into master, other developers who developing
> > storage features can work on master directly, thus I don't need to
> > rebase the branch regularly.
> > About test, I only tested basic features, like, stop/start vm,
> > attach/detach volumes, take snapshots in devcloud. For sure, I'll
> > break something. The more test from other people, will help to me to
> > stabilize the code. About unit test, I have some unit test and
> > integration test for devcloud, but both of them needs database and
> > devcloud environment, so they are disabled by default and unit test
> > themselves are broken also. I'll write more tests during the stabilization.
>
> I understand the need to merge into master, it is a serious pain to keep
> updating the branch with that latest state of master. However this testing
Thanks for understand the painful of rebaseing such big patch. There is another reason I want to merge this branch into master ASAP: all the new storage features are depended this branch: like zone-wide storage, make nfs secondary storage as optional, re-write s3/swift integration etc. Let's get it merged, adding more awesome features:)
> concerns me a lot, we all agreed that we would focus on automated testing
> both on the unittest level and on the functional level. This is the first merge
> request since we had the last discussion on testing (related to merging the
> javelin and other branches). The clear consensus was that commits would
> have to be accompanied by unittests (at least for the changed pieces of code)
> and preferable some automated functional test.
>
> Considering that, I think that a merge is not the right this to do at the
> moment. Instead of merging into master and then fixing stuff, we should
> focus on adding testing to this branch so we can merge a well-tested unit
> later. Notice my use of 'we' here, we all said focus on testing, so we all should
> help out. Unfortunately I'm pretty busy with packaging and the $dayjob, but
> if you have something I can help you with regarding setting up unittests feel
> free to let me know. You already mentioned that you have some, but the
> databases need to be mocked?
>
> It would be great if you could spend some time to details what tests you
> have in place, so we can fix those. With the help of the cobertura report we
> can figure out where the risks are. Prassana and myself are also working on
> the automated test framework, so if you have some marvin tests, we can
> help to automate them.
My branch is not a feature branch, while other features are depended on it. I didn't add any new feature on the branch, all the existing marvin automated tests should work. Instead of testing and fixing on my branch then merge, is it better to test and fix on master after the merge, using existing marvin test?
Again, the rebase to master is painful for such branch.
>
> Anybody else willing to help put in some tests into the storage_refactor
> branch?
>
> Cheers,
>
> Hugo
RE: [MERGE]Storage refactor branch
Posted by Hugo Trippaers <HT...@schubergphilis.com>.
Hey Edison,
Thanks for the update
> -----Original Message-----
> From: Edison Su [mailto:Edison.su@citrix.com]
> Sent: Friday, February 15, 2013 12:13 AM
> To: cloudstack-dev@incubator.apache.org
> Cc: John Burwell <jb...@basho.com> (jburwell@basho.com); Mike
> Tutkowski (mike.tutkowski@solidfire.com)
> Subject: [MERGE]Storage refactor branch
>
> Hi All,
> Here is the update from storage refactor branch since last week:
>
> 1. clean/hook up snapshot into new storage framework, separate taking
> snapshot and backup snapshot. Add a snapshotstrategy interface for people
> want to change or replace how snapshot is processed in cloudstack. Current
> snapshot code is little bit hacky, welcome to replace it for your need.
> Since last week I created storage_refactor branch, seems no objection for
> what I am doing, so I want to merge the branch into master, by end of this
> week, due to the following reasons:
> Maintain such big patch(more than 20K) outside of master, touch all the
> storage code, is not an easy task.
> After merge into master, other developers who developing storage
> features can work on master directly, thus I don't need to rebase the branch
> regularly.
> About test, I only tested basic features, like, stop/start vm, attach/detach
> volumes, take snapshots in devcloud. For sure, I'll break something. The
> more test from other people, will help to me to stabilize the code. About unit
> test, I have some unit test and integration test for devcloud, but both of
> them needs database and devcloud environment, so they are disabled by
> default and unit test themselves are broken also. I'll write more tests during
> the stabilization.
I understand the need to merge into master, it is a serious pain to keep updating the branch with that latest state of master. However this testing concerns me a lot, we all agreed that we would focus on automated testing both on the unittest level and on the functional level. This is the first merge request since we had the last discussion on testing (related to merging the javelin and other branches). The clear consensus was that commits would have to be accompanied by unittests (at least for the changed pieces of code) and preferable some automated functional test.
Considering that, I think that a merge is not the right this to do at the moment. Instead of merging into master and then fixing stuff, we should focus on adding testing to this branch so we can merge a well-tested unit later. Notice my use of 'we' here, we all said focus on testing, so we all should help out. Unfortunately I'm pretty busy with packaging and the $dayjob, but if you have something I can help you with regarding setting up unittests feel free to let me know. You already mentioned that you have some, but the databases need to be mocked?
It would be great if you could spend some time to details what tests you have in place, so we can fix those. With the help of the cobertura report we can figure out where the risks are. Prassana and myself are also working on the automated test framework, so if you have some marvin tests, we can help to automate them.
Anybody else willing to help put in some tests into the storage_refactor branch?
Cheers,
Hugo