You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Kelven Yang <ke...@citrix.com> on 2013/06/17 18:59:00 UTC
[MERGE] Merge VMSync improvement branch into master
I'd like to kick off the official merge process. We will start the merge
process after the branch has passed necessary tests
Kelven
On 6/10/13 2:51 PM, "Kelven Yang" <ke...@citrix.com> wrote:
>Hi there,
>
>Alex Huang and I are targeting to finish the debugging process on VMsync
>improvement by the end of this week. I'd like to encourage those who are
>interested or having concerns about this project to review it as soon as
>possible on branch vmsync. We are going to propose the official merge
>request soon after it has passed our internal test.
>
>Some details have been posted to wiki a while ago at
>https://cwiki.apache.org/confluence/display/CLOUDSTACK/FS+-+VMSync+improve
>ment. Here is a summary about this change.
>
>
> 1. We changed the underlying VM state sync modeling.
>
>For those who are familiar with the old Microsoft COM model, they may
>recall "Free threading model" and "Thread-apartment model", VM state
>sync modeling change is similar to switching from free-threading to
>thread-apartment modeling. Previously, VM state changes are reported and
>processed in management server in a "free-threading" fashion, regardless
>whether or not there is active process with the subject VM, the state
>sync process is always executed in place. This approach has issues with
>the concurrency complexity by nature, since all sync-process has been
>concentrated into one place and caused complex code logic that is hard to
>change and maintain.
>
>A major modeling shift is introduced in this change, we now switch to an
>approach which we can call it "job-apartment" model, comparable to
>Microsoft's COM "thread-apartment model", that is, making the sync logic
>within the process context and de-centralize it across the board. This
>approach can simplify VM state sync logic individually and leave the
>complexity to underlying framework, which in the future, the framework
>can be optimized separately without affecting business layer (separating
>of concerns at architecture level)
>
>2. De-couple hypervisor resource agent from managing VM state in Cloud
>layer
>
>We also changed the way on how resource agent is involved in the overall
>VM state sync process. Previously, resource agent needs to participate VM
>state management in the Cloud layer closely, this requirement is removed
>and resource agent is no longer required to help maintain "delta" state
>in the overall VM state management, all it needs is to report what it
>knows about the VM state at virtualization layer, leaving all the
>handling to CloudStack management server.
>
>The reason for this change is to simplify the architecture between agent
>resource and management server, de-coupling in this way can lower the
>requirement for developers to write a new hypervisor resource agent and
>also give room for management server developers to optimize sync logic
>independently. (Again, separating of concerns at architecture level)
>
>
>3. Job framework has been improved
>
>To make the proposal possible, job framework has been refactored to
>support more explicit management of jobs, job joining, wake-up
>scheduling and serializing job execution has been added together with a
>topic-based message bus facility.
>
>4. Compile-time strong typing of Java generic usage in
>VirtualMachineManagerImpl
>
>Job scheduling change require more flexible run-time handling, however,
>previously VirtualMachineManagerImpl has a heavy-weight usage of Java
>generic to take advantage of compile-time strong typing provided by Java,
>this has brought some troubles with object serialization the occurs
>between boundaries of "job-apartments", VirtualMachineManagerImpl has
>been refactored because of that.
>
>Flames and Comment? all are welcome.
>
>Kelven
>
Re: [MERGE] Merge VMSync improvement branch into master
Posted by Chip Childers <ch...@sungard.com>.
On Mon, Jun 17, 2013 at 05:40:36PM +0000, Kelven Yang wrote:
> Low level classes were tested in unit tests(MessageBus, Job framework, Job
> dispatchers etc), interface layer changes are guarded through matching the
> old semantics, but changes are tested manually, we are planning to get
> this part of testing through BVT system after we have re-based the latest
> master.
>
> Kelven
Fantastic. BVT was what I was looking for primarily. Thanks Kelven!
>
> On 6/17/13 10:01 AM, "Chip Childers" <ch...@sungard.com> wrote:
>
> >On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang wrote:
> >> I'd like to kick off the official merge process. We will start the merge
> >> process after the branch has passed necessary tests
> >>
> >> Kelven
> >
> >Can you share what testing is being run against the branch?
>
>
RE: [MERGE] Merge VMSync improvement branch into master
Posted by Alex Huang <Al...@citrix.com>.
+1 to frequent patches and reviews.
I wish we started with chunk features and narrow patch scope. There are some circumstances (some unique to this situation) that led to this but I won't use that as an excuse. Thanks to all who are offering to lend a hand in reviewing and testing. It is much appreciated.
--Alex
> -----Original Message-----
> From: John Burwell [mailto:jburwell@basho.com]
> Sent: Thursday, June 27, 2013 11:33 AM
> To: dev@cloudstack.apache.org
> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>
> Hugo,
>
> I completely agree with this stance, and will add a -1 as well. It has been a
> significant challenge this cycle to complete high quality reviews due to the
> large patch size and short time turnaround time. Going forward, I am going
> to be more likely to follow Hugo's example, and -1 patches for which there is
> not adequate review time. I think the following techniques will help
> streamline the review process:
>
> 1. Narrow Patch Scope: Ensure that the patch is confined to a single
> enhancement or defect fix. If you doing refactoring as part of a feature,
> submit the refactoring work a separate commit. As a further suggestion, if
> you are performing multiple refactorings (e.g. package reorganization and
> utility method extraction), split each refactoring into separate patches.
> 2. Chunk Features: For each large feature being developed, logically chunk
> the functionality and submit each chunk as a separate patch. I recommend
> erring on the side of chunks being too small. Let the reviewer determine if
> the patch is not large enough to be representative of the feature. The worst
> that will happen is that a reviewer will provide feedback on the work
> completed and simply ask for more work to be done before it can be merged
> to master. For features while chunks can't be merged all the way to master,
> intermediate patches can be submitted to Review Board for review
> throughout the cycle -- allowing large merges to actually be the accumulation
> of several smaller reviews.
> 3. Identify Reviewers Early: For big features, solicit the reviewers whiling to
> review a feature from FS through merge from the mailing list. Reviews will
> go much more quickly if reviewers have been involved from the beginning
> because they will not help identify issues before coding starts, but also will
> not have to develop context late. To be clear, identifying reviewers early
> does not preclude a new reviewer from becoming involved later, but the
> reviews will greatly reduce the probability of a late review finding significant
> issues. Additionally, the early reviewers can help the late reviewers come up
> to speed -- reducing the burden on the contributor.
>
> Generally, my recommendation is to submit patches early and often.
> Ultimately, contributors determine how they wish to develop and deliver
> their contributions. These are only my recommendations as a reviewer to
> increase the probability that a feature will make a particular release train.
> Finally, it appears that reviewers are growing less and less tolerant of large
> patches that appear just before freeze dates, and those these patches face a
> much higher risk of being immediately receiving a -1 for the reasons stated
> above.
>
> Thanks,
> -John
>
> On Jun 26, 2013, at 8:45 PM, Hugo Trippaers <hu...@trippaers.nl> wrote:
>
> > Hey Alex, Kelven,
> >
> > I've been looking though the code and thanks for the very insightfull
> conversation and follow-up email. With merges of this magnitude it's
> essential to help people understand what is going on.
> >
> > Purely technical this is a merge i'm really pleased with. It will for sure
> improve our handling of virtual machines.
> >
> > However the timing of the merge request is not ideal. We are very close to
> the end of the already extended feature freeze. We have a consensus within
> the dev community to do large architectural changes in the beginning of the
> cycle and not at the very end. This not only means a lot of extra testing effort
> and other developers will have to get used to the changes introduced right at
> the moment when everybody should be focussed on bug fixing,
> documentation and stability. Without wanting to rip open old wounds, i can
> imagine we all want to avoid a javelin incident.
> >
> > I respect the work going into this and the effort it will take to keep this up
> to date with the current speed of master, but still i'm going to veto this with a
> -1 for inclusion before we cut of the 4.2 branch.
> >
> > Cheers,
> >
> > Hugo
> >
> > On Jun 26, 2013, at 5:32 PM, Alex Huang <Al...@citrix.com> wrote:
> >
> >> Hi All,
> >>
> >> Kelven had an emergency so I'm submitting the changes on vmsync for
> >> him. The patch are on https://reviews.apache.org/r/12126.
> >>
> >> Hugo took a look and already had some questions on why so many files
> were changed and added/deleted, So I like to explain a bit in this email.
> >>
> >> As part of the vmsync change, we try to move the files that were relevant
> to vm orchestration into the right places so that others can properly view the
> different jars and understand the relationship between the jars. This
> process is ongoing and will continue in other changes.
> >>
> >> - Work related to jobs management have been put under cloud-
> framework-jobs as handling jobs is not really part of our server but is a library.
> By doing it this way, changes in it can be properly reviewed and there's a
> good separation from things that utilizes jobs and jobs itself.
> >> - VirtualMachine orchestration has been moved from cloud-server to
> cloud-engine-orchestration. Cloud-engine-orchestration then only depends
> on cloud-engine-schema and cloud-api. This creates a clear compilation
> boundary between the self-service server implementation and orchestration
> implementation.
> >>
> >> As part of these changes, then we surfaced many problems because we
> setup the proper compilation boundaries and fixed all of these problems.
> For example, HostAllocator interface is something people can write plugins
> for but it was buried inside cloud-server package. We've moved a majority of
> these changes to cloud-api. There's quite a bit of file movements based on
> this change. For vmsync changes itself, the changes are centered around
> VirtualMachineManagerImpl.java so you can review that file instead.
> >>
> >> Thanks.
> >>
> >> --Alex
> >>
> >>
> >>> -----Original Message-----
> >>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
> >>> Sent: Tuesday, June 18, 2013 3:38 PM
> >>> To: dev@cloudstack.apache.org
> >>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
> >>>
> >>>
> >>>
> >>> On 6/17/13 7:43 PM, "Mice Xia" <mi...@tcloudcomputing.com>
> wrote:
> >>>
> >>>> Kelven,
> >>>>
> >>>> After the refactoring, will CS still restart HA enabled VM when it
> >>>> is power off externally (e.g. using xencenter) or internally (user
> >>>> initiated shutdown or crash)?
> >>>
> >>>
> >>> If HA functionality is provided by external manager (i.e., vCenter),
> >>> CS won't try to restart it, but if HA is provided by CloudStack, we
> >>> will restore the legacy logic. The hook up with old HA manager
> >>> (between
> >>> HighAvailabilityManager/VirtualMachineManager) is in the wrap-up
> >>> process
> >>>
> >>>
> >>>>
> >>>> Is seems the method HighAvailabilityManagerImpl .scheduleRestart()
> >>>> is not called when VM's actual state is stopped while expected
> >>>> state is
> >>> running.
> >>>>
> >>>>
> >>>> Regards
> >>>> Mice
> >>>>
> >>>> -----Original Message-----
> >>>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
> >>>> Sent: Tuesday, June 18, 2013 5:21 AM
> >>>> To: dev@cloudstack.apache.org
> >>>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
> >>>>
> >>>> Haven't created a patch yet, will do it soon after some last wrap-ups.
> >>>>
> >>>> Kelven
> >>>>
> >>>> On 6/17/13 12:03 PM, "John Burwell" <jb...@basho.com> wrote:
> >>>>
> >>>>> Kelven,
> >>>>>
> >>>>> Did this patch get pushed to Review Board? If so, what is the URL?
> >>>>>
> >>>>> Thanks.
> >>>>> -John
> >>>>>
> >>>>> On Jun 17, 2013, at 1:40 PM, Kelven Yang <ke...@citrix.com>
> wrote:
> >>>>>
> >>>>>> Low level classes were tested in unit tests(MessageBus, Job
> >>>>>> framework, Job dispatchers etc), interface layer changes are
> >>>>>> guarded through matching the old semantics, but changes are
> >>>>>> tested manually, we are planning to get this part of testing
> >>>>>> through BVT system after we have re-based the latest master.
> >>>>>>
> >>>>>> Kelven
> >>>>>>
> >>>>>> On 6/17/13 10:01 AM, "Chip Childers" <ch...@sungard.com>
> >>> wrote:
> >>>>>>
> >>>>>>> On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang wrote:
> >>>>>>>> I'd like to kick off the official merge process. We will start
> >>>>>>>> the merge process after the branch has passed necessary tests
> >>>>>>>>
> >>>>>>>> Kelven
> >>>>>>>
> >>>>>>> Can you share what testing is being run against the branch?
> >>>>>>
> >>>>>
> >>>>
> >>
> >
RE: [MERGE] Merge VMSync improvement branch into master
Posted by Suresh Sadhu <Su...@citrix.com>.
+1
I will also contribute and will test this feature thoroughly.
Regards
Sadhu
-----Original Message-----
From: Musayev, Ilya [mailto:imusayev@webmd.net]
Sent: Friday, June 28, 2013 1:47 AM
To: dev@cloudstack.apache.org
Subject: RE: [MERGE] Merge VMSync improvement branch into master
John and Hugo,
I see and understand your concerns, however, this feature is really needed by many users - I've met with several large users at CCC13 who were looking forward to this work, blocking this merge - would delay cloudstack adoption and make ACS look inferior to other cloud platforms. Realistically, this is going to put us back at least 8+ months away until 4.3 comes out.
If needed, I can dedicate QA cycles and work with Kelven to rule out all the bugs and issues - through as many scenarios as possible on my end.
Thanks
ilya
> -----Original Message-----
> From: John Burwell [mailto:jburwell@basho.com]
> Sent: Thursday, June 27, 2013 2:33 PM
> To: dev@cloudstack.apache.org
> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>
> Hugo,
>
> I completely agree with this stance, and will add a -1 as well. It
> has been a significant challenge this cycle to complete high quality
> reviews due to the large patch size and short time turnaround time.
> Going forward, I am going to be more likely to follow Hugo's example,
> and -1 patches for which there is not adequate review time. I think
> the following techniques will help streamline the review process:
>
> 1. Narrow Patch Scope: Ensure that the patch is confined to a single
> enhancement or defect fix. If you doing refactoring as part of a
> feature, submit the refactoring work a separate commit. As a further
> suggestion, if you are performing multiple refactorings (e.g. package
> reorganization and utility method extraction), split each refactoring into separate patches.
> 2. Chunk Features: For each large feature being developed, logically
> chunk the functionality and submit each chunk as a separate patch. I
> recommend erring on the side of chunks being too small. Let the
> reviewer determine if the patch is not large enough to be
> representative of the feature. The worst that will happen is that a
> reviewer will provide feedback on the work completed and simply ask
> for more work to be done before it can be merged to master. For
> features while chunks can't be merged all the way to master,
> intermediate patches can be submitted to Review Board for review
> throughout the cycle -- allowing large merges to actually be the accumulation of several smaller reviews.
> 3. Identify Reviewers Early: For big features, solicit the reviewers
> whiling to review a feature from FS through merge from the mailing
> list. Reviews will go much more quickly if reviewers have been
> involved from the beginning because they will not help identify issues
> before coding starts, but also will not have to develop context late.
> To be clear, identifying reviewers early does not preclude a new
> reviewer from becoming involved later, but the reviews will greatly
> reduce the probability of a late review finding significant issues.
> Additionally, the early reviewers can help the late reviewers come up to speed -- reducing the burden on the contributor.
>
> Generally, my recommendation is to submit patches early and often.
> Ultimately, contributors determine how they wish to develop and
> deliver their contributions. These are only my recommendations as a
> reviewer to increase the probability that a feature will make a particular release train.
> Finally, it appears that reviewers are growing less and less tolerant
> of large patches that appear just before freeze dates, and those these
> patches face a much higher risk of being immediately receiving a -1
> for the reasons stated above.
>
> Thanks,
> -John
>
> On Jun 26, 2013, at 8:45 PM, Hugo Trippaers <hu...@trippaers.nl> wrote:
>
> > Hey Alex, Kelven,
> >
> > I've been looking though the code and thanks for the very
> > insightfull
> conversation and follow-up email. With merges of this magnitude it's
> essential to help people understand what is going on.
> >
> > Purely technical this is a merge i'm really pleased with. It will
> > for sure
> improve our handling of virtual machines.
> >
> > However the timing of the merge request is not ideal. We are very
> > close to
> the end of the already extended feature freeze. We have a consensus
> within the dev community to do large architectural changes in the
> beginning of the cycle and not at the very end. This not only means a
> lot of extra testing effort and other developers will have to get used
> to the changes introduced right at the moment when everybody should be
> focussed on bug fixing, documentation and stability. Without wanting
> to rip open old wounds, i can imagine we all want to avoid a javelin incident.
> >
> > I respect the work going into this and the effort it will take to
> > keep this up
> to date with the current speed of master, but still i'm going to veto
> this with a
> -1 for inclusion before we cut of the 4.2 branch.
> >
> > Cheers,
> >
> > Hugo
> >
> > On Jun 26, 2013, at 5:32 PM, Alex Huang <Al...@citrix.com> wrote:
> >
> >> Hi All,
> >>
> >> Kelven had an emergency so I'm submitting the changes on vmsync for
> >> him. The patch are on https://reviews.apache.org/r/12126.
> >>
> >> Hugo took a look and already had some questions on why so many
> >> files
> were changed and added/deleted, So I like to explain a bit in this email.
> >>
> >> As part of the vmsync change, we try to move the files that were
> >> relevant
> to vm orchestration into the right places so that others can properly
> view the different jars and understand the relationship between the
> jars. This process is ongoing and will continue in other changes.
> >>
> >> - Work related to jobs management have been put under cloud-
> framework-jobs as handling jobs is not really part of our server but
> is a library. By doing it this way, changes in it can be properly
> reviewed and there's a good separation from things that utilizes jobs and jobs itself.
> >> - VirtualMachine orchestration has been moved from cloud-server to
> cloud-engine-orchestration. Cloud-engine-orchestration then only
> depends on cloud-engine-schema and cloud-api. This creates a clear
> compilation boundary between the self-service server implementation
> and orchestration implementation.
> >>
> >> As part of these changes, then we surfaced many problems because we
> setup the proper compilation boundaries and fixed all of these problems.
> For example, HostAllocator interface is something people can write
> plugins for but it was buried inside cloud-server package. We've
> moved a majority of these changes to cloud-api. There's quite a bit
> of file movements based on this change. For vmsync changes itself,
> the changes are centered around VirtualMachineManagerImpl.java so you can review that file instead.
> >>
> >> Thanks.
> >>
> >> --Alex
> >>
> >>
> >>> -----Original Message-----
> >>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
> >>> Sent: Tuesday, June 18, 2013 3:38 PM
> >>> To: dev@cloudstack.apache.org
> >>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
> >>>
> >>>
> >>>
> >>> On 6/17/13 7:43 PM, "Mice Xia" <mi...@tcloudcomputing.com>
> wrote:
> >>>
> >>>> Kelven,
> >>>>
> >>>> After the refactoring, will CS still restart HA enabled VM when
> >>>> it is power off externally (e.g. using xencenter) or internally
> >>>> (user initiated shutdown or crash)?
> >>>
> >>>
> >>> If HA functionality is provided by external manager (i.e.,
> >>> vCenter), CS won't try to restart it, but if HA is provided by
> >>> CloudStack, we will restore the legacy logic. The hook up with old
> >>> HA manager (between
> >>> HighAvailabilityManager/VirtualMachineManager) is in the wrap-up
> >>> process
> >>>
> >>>
> >>>>
> >>>> Is seems the method HighAvailabilityManagerImpl
> >>>> .scheduleRestart() is not called when VM's actual state is
> >>>> stopped while expected state is
> >>> running.
> >>>>
> >>>>
> >>>> Regards
> >>>> Mice
> >>>>
> >>>> -----Original Message-----
> >>>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
> >>>> Sent: Tuesday, June 18, 2013 5:21 AM
> >>>> To: dev@cloudstack.apache.org
> >>>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
> >>>>
> >>>> Haven't created a patch yet, will do it soon after some last wrap-ups.
> >>>>
> >>>> Kelven
> >>>>
> >>>> On 6/17/13 12:03 PM, "John Burwell" <jb...@basho.com> wrote:
> >>>>
> >>>>> Kelven,
> >>>>>
> >>>>> Did this patch get pushed to Review Board? If so, what is the URL?
> >>>>>
> >>>>> Thanks.
> >>>>> -John
> >>>>>
> >>>>> On Jun 17, 2013, at 1:40 PM, Kelven Yang
> >>>>> <ke...@citrix.com>
> wrote:
> >>>>>
> >>>>>> Low level classes were tested in unit tests(MessageBus, Job
> >>>>>> framework, Job dispatchers etc), interface layer changes are
> >>>>>> guarded through matching the old semantics, but changes are
> >>>>>> tested manually, we are planning to get this part of testing
> >>>>>> through BVT system after we have re-based the latest master.
> >>>>>>
> >>>>>> Kelven
> >>>>>>
> >>>>>> On 6/17/13 10:01 AM, "Chip Childers"
> >>>>>> <ch...@sungard.com>
> >>> wrote:
> >>>>>>
> >>>>>>> On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang wrote:
> >>>>>>>> I'd like to kick off the official merge process. We will
> >>>>>>>> start the merge process after the branch has passed
> >>>>>>>> necessary tests
> >>>>>>>>
> >>>>>>>> Kelven
> >>>>>>>
> >>>>>>> Can you share what testing is being run against the branch?
> >>>>>>
> >>>>>
> >>>>
> >>
> >
RE: [MERGE] Merge VMSync improvement branch into master
Posted by Animesh Chaturvedi <an...@citrix.com>.
> -----Original Message-----
> From: John Burwell [mailto:jburwell@basho.com]
> Sent: Thursday, June 27, 2013 12:55 PM
> To: dev@cloudstack.apache.org
> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>
> Ilya,
>
> I understand your concern. However, this patch is large and
> complicated. Without adequate review, we run that the risk getting it
> wrong which would be worse than not having it. If you feel it is a must
> have, you can propose extending the freeze to allow time to perform the
> review.
[Animesh>] IMO I would not like to extend the freeze date second time around and sticking to time based release. Ilya if this does not make to 4.2 then next opportunity would be 4.3 which will be 4 months apart not 8 as you mention. May be we should consider if there are important logical chunks that can be included for 4.2 rather than all of it.
>
> Thanks,
> -John
>
> On Jun 27, 2013, at 3:49 PM, "Musayev, Ilya" <im...@webmd.net> wrote:
>
> > John and Hugo,
> >
> > I see and understand your concerns, however, this feature is really
> needed by many users - I've met with several large users at CCC13 who
> were looking forward to this work, blocking this merge - would delay
> cloudstack adoption and make ACS look inferior to other cloud platforms.
> Realistically, this is going to put us back at least 8+ months away
> until 4.3 comes out.
> >
> > If needed, I can dedicate QA cycles and work with Kelven to rule out
> all the bugs and issues - through as many scenarios as possible on my
> end.
> >
> > Thanks
> > ilya
> >
> >> -----Original Message-----
> >> From: John Burwell [mailto:jburwell@basho.com]
> >> Sent: Thursday, June 27, 2013 2:33 PM
> >> To: dev@cloudstack.apache.org
> >> Subject: Re: [MERGE] Merge VMSync improvement branch into master
> >>
> >> Hugo,
> >>
> >> I completely agree with this stance, and will add a -1 as well. It
> >> has been a significant challenge this cycle to complete high quality
> >> reviews due to the large patch size and short time turnaround time.
> >> Going forward, I am going to be more likely to follow Hugo's example,
> >> and -1 patches for which there is not adequate review time. I think
> >> the following techniques will help streamline the review process:
> >>
> >> 1. Narrow Patch Scope: Ensure that the patch is confined to a single
> >> enhancement or defect fix. If you doing refactoring as part of a
> >> feature, submit the refactoring work a separate commit. As a further
> >> suggestion, if you are performing multiple refactorings (e.g. package
> >> reorganization and utility method extraction), split each refactoring
> into separate patches.
> >> 2. Chunk Features: For each large feature being developed, logically
> >> chunk the functionality and submit each chunk as a separate patch. I
> >> recommend erring on the side of chunks being too small. Let the
> >> reviewer determine if the patch is not large enough to be
> >> representative of the feature. The worst that will happen is that a
> >> reviewer will provide feedback on the work completed and simply ask
> >> for more work to be done before it can be merged to master. For
> >> features while chunks can't be merged all the way to master,
> >> intermediate patches can be submitted to Review Board for review
> >> throughout the cycle -- allowing large merges to actually be the
> accumulation of several smaller reviews.
> >> 3. Identify Reviewers Early: For big features, solicit the reviewers
> >> whiling to review a feature from FS through merge from the mailing
> >> list. Reviews will go much more quickly if reviewers have been
> >> involved from the beginning because they will not help identify
> >> issues before coding starts, but also will not have to develop
> >> context late. To be clear, identifying reviewers early does not
> >> preclude a new reviewer from becoming involved later, but the reviews
> >> will greatly reduce the probability of a late review finding
> >> significant issues. Additionally, the early reviewers can help the
> late reviewers come up to speed -- reducing the burden on the
> contributor.
> >>
> >> Generally, my recommendation is to submit patches early and often.
> >> Ultimately, contributors determine how they wish to develop and
> >> deliver their contributions. These are only my recommendations as a
> >> reviewer to increase the probability that a feature will make a
> particular release train.
> >> Finally, it appears that reviewers are growing less and less tolerant
> >> of large patches that appear just before freeze dates, and those
> >> these patches face a much higher risk of being immediately receiving
> >> a -1 for the reasons stated above.
> >>
> >> Thanks,
> >> -John
> >>
> >> On Jun 26, 2013, at 8:45 PM, Hugo Trippaers <hu...@trippaers.nl>
> wrote:
> >>
> >>> Hey Alex, Kelven,
> >>>
> >>> I've been looking though the code and thanks for the very
> >>> insightfull
> >> conversation and follow-up email. With merges of this magnitude it's
> >> essential to help people understand what is going on.
> >>>
> >>> Purely technical this is a merge i'm really pleased with. It will
> >>> for sure
> >> improve our handling of virtual machines.
> >>>
> >>> However the timing of the merge request is not ideal. We are very
> >>> close to
> >> the end of the already extended feature freeze. We have a consensus
> >> within the dev community to do large architectural changes in the
> >> beginning of the cycle and not at the very end. This not only means a
> >> lot of extra testing effort and other developers will have to get
> >> used to the changes introduced right at the moment when everybody
> >> should be focussed on bug fixing, documentation and stability.
> >> Without wanting to rip open old wounds, i can imagine we all want to
> avoid a javelin incident.
> >>>
> >>> I respect the work going into this and the effort it will take to
> >>> keep this up
> >> to date with the current speed of master, but still i'm going to veto
> >> this with a
> >> -1 for inclusion before we cut of the 4.2 branch.
> >>>
> >>> Cheers,
> >>>
> >>> Hugo
> >>>
> >>> On Jun 26, 2013, at 5:32 PM, Alex Huang <Al...@citrix.com>
> wrote:
> >>>
> >>>> Hi All,
> >>>>
> >>>> Kelven had an emergency so I'm submitting the changes on vmsync for
> >>>> him. The patch are on https://reviews.apache.org/r/12126.
> >>>>
> >>>> Hugo took a look and already had some questions on why so many
> >>>> files
> >> were changed and added/deleted, So I like to explain a bit in this
> email.
> >>>>
> >>>> As part of the vmsync change, we try to move the files that were
> >>>> relevant
> >> to vm orchestration into the right places so that others can properly
> >> view the different jars and understand the relationship between the
> >> jars. This process is ongoing and will continue in other changes.
> >>>>
> >>>> - Work related to jobs management have been put under cloud-
> >> framework-jobs as handling jobs is not really part of our server but
> >> is a library. By doing it this way, changes in it can be properly
> >> reviewed and there's a good separation from things that utilizes jobs
> and jobs itself.
> >>>> - VirtualMachine orchestration has been moved from cloud-server to
> >> cloud-engine-orchestration. Cloud-engine-orchestration then only
> >> depends on cloud-engine-schema and cloud-api. This creates a clear
> >> compilation boundary between the self-service server implementation
> >> and orchestration implementation.
> >>>>
> >>>> As part of these changes, then we surfaced many problems because we
> >> setup the proper compilation boundaries and fixed all of these
> problems.
> >> For example, HostAllocator interface is something people can write
> >> plugins for but it was buried inside cloud-server package. We've
> >> moved a majority of these changes to cloud-api. There's quite a bit
> >> of file movements based on this change. For vmsync changes itself,
> >> the changes are centered around VirtualMachineManagerImpl.java so you
> can review that file instead.
> >>>>
> >>>> Thanks.
> >>>>
> >>>> --Alex
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
> >>>>> Sent: Tuesday, June 18, 2013 3:38 PM
> >>>>> To: dev@cloudstack.apache.org
> >>>>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 6/17/13 7:43 PM, "Mice Xia" <mi...@tcloudcomputing.com>
> >> wrote:
> >>>>>
> >>>>>> Kelven,
> >>>>>>
> >>>>>> After the refactoring, will CS still restart HA enabled VM when
> >>>>>> it is power off externally (e.g. using xencenter) or internally
> >>>>>> (user initiated shutdown or crash)?
> >>>>>
> >>>>>
> >>>>> If HA functionality is provided by external manager (i.e.,
> >>>>> vCenter), CS won't try to restart it, but if HA is provided by
> >>>>> CloudStack, we will restore the legacy logic. The hook up with old
> >>>>> HA manager (between
> >>>>> HighAvailabilityManager/VirtualMachineManager) is in the wrap-up
> >>>>> process
> >>>>>
> >>>>>
> >>>>>>
> >>>>>> Is seems the method HighAvailabilityManagerImpl
> >>>>>> .scheduleRestart() is not called when VM's actual state is
> >>>>>> stopped while expected state is
> >>>>> running.
> >>>>>>
> >>>>>>
> >>>>>> Regards
> >>>>>> Mice
> >>>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
> >>>>>> Sent: Tuesday, June 18, 2013 5:21 AM
> >>>>>> To: dev@cloudstack.apache.org
> >>>>>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
> >>>>>>
> >>>>>> Haven't created a patch yet, will do it soon after some last
> wrap-ups.
> >>>>>>
> >>>>>> Kelven
> >>>>>>
> >>>>>> On 6/17/13 12:03 PM, "John Burwell" <jb...@basho.com> wrote:
> >>>>>>
> >>>>>>> Kelven,
> >>>>>>>
> >>>>>>> Did this patch get pushed to Review Board? If so, what is the
> URL?
> >>>>>>>
> >>>>>>> Thanks.
> >>>>>>> -John
> >>>>>>>
> >>>>>>> On Jun 17, 2013, at 1:40 PM, Kelven Yang
> >>>>>>> <ke...@citrix.com>
> >> wrote:
> >>>>>>>
> >>>>>>>> Low level classes were tested in unit tests(MessageBus, Job
> >>>>>>>> framework, Job dispatchers etc), interface layer changes are
> >>>>>>>> guarded through matching the old semantics, but changes are
> >>>>>>>> tested manually, we are planning to get this part of testing
> >>>>>>>> through BVT system after we have re-based the latest master.
> >>>>>>>>
> >>>>>>>> Kelven
> >>>>>>>>
> >>>>>>>> On 6/17/13 10:01 AM, "Chip Childers"
> >>>>>>>> <ch...@sungard.com>
> >>>>> wrote:
> >>>>>>>>
> >>>>>>>>> On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang wrote:
> >>>>>>>>>> I'd like to kick off the official merge process. We will
> >>>>>>>>>> start the merge process after the branch has passed
> >>>>>>>>>> necessary tests
> >>>>>>>>>>
> >>>>>>>>>> Kelven
> >>>>>>>>>
> >>>>>>>>> Can you share what testing is being run against the branch?
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>
> >>>
> >
> >
Re: [MERGE] Merge VMSync improvement branch into master
Posted by Kelven Yang <ke...@citrix.com>.
I recently had an urgent family matter to take care and thanks for Alex to
help me out on this matter. I think at this moment, without a previous
100% rate of BVT past rate, it will be hard to tell a direct link between
a particular change with a BVT failure. It is a good idea to resolve this
measurement tool issue first.
Kelven
On 6/27/13 4:50 PM, "Alex Huang" <Al...@citrix.com> wrote:
>Dave,
>
>Chip has asked this before and we also stated specifically that we won't
>merge it in unless we see equivalent pass rate on the BVT as master.
>We're doing that right now.
>
>--Alex
>
>> -----Original Message-----
>> From: David Nalley [mailto:david@gnsa.us]
>> Sent: Thursday, June 27, 2013 4:34 PM
>> To: dev@cloudstack.apache.org
>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>>
>> On Thu, Jun 27, 2013 at 5:51 PM, Hugo Trippaers <hu...@trippaers.nl>
>>wrote:
>> >
>> > I think Ilya offers is great, my current stance is also to see how we
>>can bring
>> this forward.
>> >
>> > I've had the opportunity to meet with several people at the Citrix
>>office in
>> Santa Clara, i'm actually working from their office at this moment. I
>>think it's
>> also the responsibility of someone who put in a -1 to work with the
>>original
>> committer to get the situation resolved. So i'll invest the time to
>>help with
>> the review as well.
>> >
>> > It would be great if Alex or Kelven could take the time to explain
>>how this
>> feature has been tested. That would give the community some insight as
>> well.
>> >
>> > My main technical problem with this merge is that stuff is moving all
>>over
>> the place without having even the slightest idea why. Now having
>>discussed
>> this with Alex in person i get the general idea of this merge, so can
>>actually
>> try to review it.
>> >
>> > I think that John have nicely explained what we could do to prevent
>> situations like this in advance. I fully understand that big features
>>or rewrites
>> don't happen overnight and might show up near the end of the release
>>cycle.
>> With the time based release cycle it's always a risk that some feature
>>might
>> not make it in on time. Getting more people involved and chunking the
>> commits into master will greatly speed up the reviewing process.
>> >
>> > I'll get back to this after spending some time on reviewing the
>>actual patch.
>> In fact i would like to ask more people to have a look at this patch
>>and reply
>> to this thread with comments or remarks.
>> >
>> > Cheers,
>> >
>> > Hugo
>> >
>>
>> So the problem in my mind, is that we don't have a way of verifying that
>> master isn't broken, and won't be broken by any given merge. I look at
>>even
>> the minimal level of automated testing that I see today, and ~20% of
>> integration tests are failing[1]. The regression set of tests (which
>>isn't running
>> as often) is seeing 75% of tests failing[2]. Heaping on more change
>>when we
>> are demonstrably already failing in many places is not behaving
>>responsibly
>> IMO.
>> The question I'd pose is this - running the various automated tests is
>>pretty
>> cheap - whats the output of that compared to the current test output on
>> master? Better or worse? If it hasn't been done, why not?
>> I desperately want these features, but not necessarily at the cost of
>>further
>> destabilizing what we have now in master - we can't continue accruing
>> technical debt.
>>
>> --David
>>
>> [1] http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-smoke-
>> matrix/lastCompletedBuild/testReport/
>> [2]
>>http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-regression-
>> matrix/28/testReport/
RE: [MERGE] Merge VMSync improvement branch into master
Posted by Alex Huang <Al...@citrix.com>.
Dave,
Chip has asked this before and we also stated specifically that we won't merge it in unless we see equivalent pass rate on the BVT as master. We're doing that right now.
--Alex
> -----Original Message-----
> From: David Nalley [mailto:david@gnsa.us]
> Sent: Thursday, June 27, 2013 4:34 PM
> To: dev@cloudstack.apache.org
> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>
> On Thu, Jun 27, 2013 at 5:51 PM, Hugo Trippaers <hu...@trippaers.nl> wrote:
> >
> > I think Ilya offers is great, my current stance is also to see how we can bring
> this forward.
> >
> > I've had the opportunity to meet with several people at the Citrix office in
> Santa Clara, i'm actually working from their office at this moment. I think it's
> also the responsibility of someone who put in a -1 to work with the original
> committer to get the situation resolved. So i'll invest the time to help with
> the review as well.
> >
> > It would be great if Alex or Kelven could take the time to explain how this
> feature has been tested. That would give the community some insight as
> well.
> >
> > My main technical problem with this merge is that stuff is moving all over
> the place without having even the slightest idea why. Now having discussed
> this with Alex in person i get the general idea of this merge, so can actually
> try to review it.
> >
> > I think that John have nicely explained what we could do to prevent
> situations like this in advance. I fully understand that big features or rewrites
> don't happen overnight and might show up near the end of the release cycle.
> With the time based release cycle it's always a risk that some feature might
> not make it in on time. Getting more people involved and chunking the
> commits into master will greatly speed up the reviewing process.
> >
> > I'll get back to this after spending some time on reviewing the actual patch.
> In fact i would like to ask more people to have a look at this patch and reply
> to this thread with comments or remarks.
> >
> > Cheers,
> >
> > Hugo
> >
>
> So the problem in my mind, is that we don't have a way of verifying that
> master isn't broken, and won't be broken by any given merge. I look at even
> the minimal level of automated testing that I see today, and ~20% of
> integration tests are failing[1]. The regression set of tests (which isn't running
> as often) is seeing 75% of tests failing[2]. Heaping on more change when we
> are demonstrably already failing in many places is not behaving responsibly
> IMO.
> The question I'd pose is this - running the various automated tests is pretty
> cheap - whats the output of that compared to the current test output on
> master? Better or worse? If it hasn't been done, why not?
> I desperately want these features, but not necessarily at the cost of further
> destabilizing what we have now in master - we can't continue accruing
> technical debt.
>
> --David
>
> [1] http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-smoke-
> matrix/lastCompletedBuild/testReport/
> [2] http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-regression-
> matrix/28/testReport/
Re: [MERGE] Merge VMSync improvement branch into master
Posted by Prasanna Santhanam <ts...@apache.org>.
> > On Jun 27, 2013, at 7:35 PM, David Nalley <da...@gnsa.us> wrote:
> >>
> >> So the problem in my mind, is that we don't have a way of verifying
> >> that master isn't broken, and won't be broken by any given merge. I
> >> look at even the minimal level of automated testing that I see today,
> >> and ~20% of integration tests are failing[1]. The regression set of
> >> tests (which isn't running as often) is seeing 75% of tests
> >> failing[2]. Heaping on more change when we are demonstrably already
> >> failing in many places is not behaving responsibly IMO.
> >> The question I'd pose is this - running the various automated tests is
> >> pretty cheap - whats the output of that compared to the current test
> >> output on master? Better or worse? If it hasn't been done, why not?
> >> I desperately want these features, but not necessarily at the cost of
> >> further destabilizing what we have now in master - we can't continue
> >> accruing technical debt.
> >>
> >> --David
> >>
> >> [1] http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-smoke-matrix/lastCompletedBuild/testReport/
> >> [2] http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-regression-matrix/28/testReport/
*We* need to fix those tests. I wouldn't be thrown off by the numbers
there since many are failing because of mismatched environments and/or
script failures. Getting the same pass rate across the vmsync and
master branches is a _decent_ indicator but not the best one because
coverage is poor. Reviews are absolutely *essential*
In fact it's not easy to automate failures in a standard way for
vmsync like functionality.
--
Prasanna.,
------------------------
Powered by BigRock.com
Re: [MERGE] Merge VMSync improvement branch into master
Posted by John Burwell <jb...@basho.com>.
Alex,
+1 for breaking up into smaller patches. I missed an opportunity to express my admiration for your withdrawal of the patch for 4.2. I imagine it was hard decision given all of the work you poured into it, and I think your action stands a great example of engineering integrity and quality focus for the community.
Thanks,
-John
On Jul 3, 2013, at 12:22 PM, Alex Huang <Al...@citrix.com> wrote:
> While I'm working on the BVT issues to be resolved. I will attempt to pull some of the less architectural changes over to master in pieces. That way when people do have to review the VMSync code, the code difference will be much smaller. Hopefully, the merges will be smaller too.
>
> For these changes, I will not be asking for review requests. They're refactoring done by Eclipse..
>
> --Alex
>
>> -----Original Message-----
>> From: Alex Huang [mailto:Alex.Huang@citrix.com]
>> Sent: Monday, July 1, 2013 10:20 AM
>> To: dev@cloudstack.apache.org
>> Subject: RE: [MERGE] Merge VMSync improvement branch into master
>>
>> Ilya,
>>
>> Thanks! The current problem on this branch is we're trying to distinguish
>> what's actually broken on master and what's broken by introducing this
>> change. At this point, the branch is basically frozen and only applying bug
>> fixes once BVT found problems.
>>
>> --Alex
>>
>>> -----Original Message-----
>>> From: Musayev, Ilya [mailto:imusayev@webmd.net]
>>> Sent: Monday, July 1, 2013 10:16 AM
>>> To: dev@cloudstack.apache.org
>>> Subject: RE: [MERGE] Merge VMSync improvement branch into master
>>>
>>> Alex,
>>>
>>> I completely understand. Please keep us in the loop on the progress.
>>> If its functional to some extent - i.e. at least build succeeds
>>> without errors and major functionality is there, I can merge the code
>>> into CloudSand CS distro - to test it out and share it with whoever wants to
>> try it.
>>>
>>> Thanks
>>> ilya
>>>
>>>> -----Original Message-----
>>>> From: Alex Huang [mailto:Alex.Huang@citrix.com]
>>>> Sent: Friday, June 28, 2013 9:16 PM
>>>> To: dev@cloudstack.apache.org
>>>> Subject: RE: [MERGE] Merge VMSync improvement branch into master
>>>>
>>>> Given the current state of BVT, I don't think we can reliably merge
>>>> this into master. It will have to wait for 4.3. I apologize to
>>>> those who really want to see this feature in. I myself have slaved
>>>> over this for some weeks, including missing the collab conference,
>>>> but I just cannot conscientiously push it in under the current
>> circumstances.
>>>>
>>>> --Alex
>>>>
>>>>> -----Original Message-----
>>>>> From: Sudha Ponnaganti [mailto:sudha.ponnaganti@citrix.com]
>>>>> Sent: Friday, June 28, 2013 7:11 AM
>>>>> To: dev@cloudstack.apache.org
>>>>> Subject: RE: [MERGE] Merge VMSync improvement branch into master
>>>>>
>>>>> Ideally I would like to see all validation to be done on feature
>>>>> branch - BVTs and also manual validation of the P1 test cases from
>>>>> VM Sync test plan just like object store validation - sadhu has
>>>>> signed up for it along with Ilya. This would help us to identify
>>>>> feature
>>> specific issues.
>>>>>
>>>>> We are running BVTs right now which have high failure rate.
>>>>> Focusing on this part right now to reach parity with Master branch.
>>>>>
>>>>>
>>>>> -----Original Message-----
>>>>> From: Trippie [mailto:trippie@gmail.com] On Behalf Of Hugo
>>>>> Trippaers
>>>>> Sent: Thursday, June 27, 2013 5:32 PM
>>>>> To: dev@cloudstack.apache.org
>>>>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>>>>>
>>>>> I agree with John that a change like this is very hard to test in
>>>>> an automated fashion. Still i have been looking at the numbers for
>>>>> the code coverage with cobertura. I was a bit disappointed to find
>>>>> that we have not made any progress with this merge with regards to
>>>>> unit tests and
>>>> total code coverage.
>>>>> We do not seem to be in a worse shape than before.
>>>>>
>>>>> @Sudha, it would be nice if you could add your view on this patch
>>>>> from the QA perspective? How would this patch affect your planning
>>>>> for
>>>> example?
>>>>>
>>>>> Cheers,
>>>>>
>>>>> Hugo
>>>>>
>>>>> On Jun 27, 2013, at 5:12 PM, John Burwell <jb...@basho.com>
>> wrote:
>>>>>
>>>>>> @David The types of concurrency changes introduced in this patch
>>>>>> are extremely difficult to completely test in an automated fashion.
>>>>>> Therefore, code review for correctness is critical to ensure quality.
>>>>>> To be clear, I am not questioning the value of automated testing.
>>>>>> I am just noting that it's next to impossible to achieve full
>>>>>> coverage, and code review is an critical supplement.
>>>>>>
>>>>>> @Ilya I plan to review this patch, but I will be able to start
>>>>>> until next week. I am also still reviewing object_store (a
>>>>>> separate procedural issue for another thread), and need to
>>>>>> complete
>>> solidfire.
>>>>>> This backlog is precisely why need to be reviewing iteratively
>>>>>> throughout the dev cycle.
>>>>>>
>>>>>> Thanks,
>>>>>> -John
>>>>>>
>>>>>> On Jun 27, 2013, at 7:35 PM, David Nalley <da...@gnsa.us> wrote:
>>>>>>
>>>>>>> On Thu, Jun 27, 2013 at 5:51 PM, Hugo Trippaers
>>>>>>> <hu...@trippaers.nl>
>>>>> wrote:
>>>>>>>>
>>>>>>>> I think Ilya offers is great, my current stance is also to see
>>>>>>>> how we can
>>>>> bring this forward.
>>>>>>>>
>>>>>>>> I've had the opportunity to meet with several people at the
>>>>>>>> Citrix office
>>>>> in Santa Clara, i'm actually working from their office at this moment.
>>>>> I think it's also the responsibility of someone who put in a -1 to
>>>>> work with the original committer to get the situation resolved. So
>>>>> i'll invest the time to help with the review as well.
>>>>>>>>
>>>>>>>> It would be great if Alex or Kelven could take the time to
>>>>>>>> explain how
>>>>> this feature has been tested. That would give the community some
>>>>> insight as well.
>>>>>>>>
>>>>>>>> My main technical problem with this merge is that stuff is
>>>>>>>> moving all over
>>>>> the place without having even the slightest idea why. Now having
>>>>> discussed this with Alex in person i get the general idea of this
>>>>> merge, so can actually try to review it.
>>>>>>>>
>>>>>>>> I think that John have nicely explained what we could do to
>>>>>>>> prevent
>>>>> situations like this in advance. I fully understand that big
>>>>> features or rewrites don't happen overnight and might show up near
>>>>> the end of the
>>>> release cycle.
>>>>> With the time based release cycle it's always a risk that some
>>>>> feature might not make it in on time. Getting more people involved
>>>>> and chunking the commits into master will greatly speed up the
>>>>> reviewing
>>>> process.
>>>>>>>>
>>>>>>>> I'll get back to this after spending some time on reviewing
>>>>>>>> the actual
>>>>> patch. In fact i would like to ask more people to have a look at
>>>>> this patch and reply to this thread with comments or remarks.
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>>
>>>>>>>> Hugo
>>>>>>>
>>>>>>> So the problem in my mind, is that we don't have a way of
>>>>>>> verifying that master isn't broken, and won't be broken by any
>>>>>>> given merge. I look at even the minimal level of automated
>>>>>>> testing that I see today, and ~20% of integration tests are
>>>>>>> failing[1]. The regression set of tests (which isn't running as
>>>>>>> often) is seeing 75% of tests failing[2]. Heaping on more
>>>>>>> change when we are demonstrably already failing in many places
>>>>>>> is not
>>> behaving responsibly IMO.
>>>>>>> The question I'd pose is this - running the various automated
>>>>>>> tests is pretty cheap - whats the output of that compared to
>>>>>>> the current test output on master? Better or worse? If it
>>>>>>> hasn't been done, why
>>>> not?
>>>>>>> I desperately want these features, but not necessarily at the
>>>>>>> cost of further destabilizing what we have now in master - we
>>>>>>> can't continue accruing technical debt.
>>>>>>>
>>>>>>> --David
>>>>>>>
>>>>>>> [1]
>>>>>>> http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-smok
>>>>>>> e- ma tr ix/lastCompletedBuild/testReport/ [2]
>>>>>>> http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-regr
>>>>>>> es
>>>>>>> si
>>>>>>> on
>>>>>>> -matrix/28/testReport/
>>>>
>>>
>
RE: [MERGE] Merge VMSync improvement branch into master
Posted by Alex Huang <Al...@citrix.com>.
While I'm working on the BVT issues to be resolved. I will attempt to pull some of the less architectural changes over to master in pieces. That way when people do have to review the VMSync code, the code difference will be much smaller. Hopefully, the merges will be smaller too.
For these changes, I will not be asking for review requests. They're refactoring done by Eclipse..
--Alex
> -----Original Message-----
> From: Alex Huang [mailto:Alex.Huang@citrix.com]
> Sent: Monday, July 1, 2013 10:20 AM
> To: dev@cloudstack.apache.org
> Subject: RE: [MERGE] Merge VMSync improvement branch into master
>
> Ilya,
>
> Thanks! The current problem on this branch is we're trying to distinguish
> what's actually broken on master and what's broken by introducing this
> change. At this point, the branch is basically frozen and only applying bug
> fixes once BVT found problems.
>
> --Alex
>
> > -----Original Message-----
> > From: Musayev, Ilya [mailto:imusayev@webmd.net]
> > Sent: Monday, July 1, 2013 10:16 AM
> > To: dev@cloudstack.apache.org
> > Subject: RE: [MERGE] Merge VMSync improvement branch into master
> >
> > Alex,
> >
> > I completely understand. Please keep us in the loop on the progress.
> > If its functional to some extent - i.e. at least build succeeds
> > without errors and major functionality is there, I can merge the code
> > into CloudSand CS distro - to test it out and share it with whoever wants to
> try it.
> >
> > Thanks
> > ilya
> >
> > > -----Original Message-----
> > > From: Alex Huang [mailto:Alex.Huang@citrix.com]
> > > Sent: Friday, June 28, 2013 9:16 PM
> > > To: dev@cloudstack.apache.org
> > > Subject: RE: [MERGE] Merge VMSync improvement branch into master
> > >
> > > Given the current state of BVT, I don't think we can reliably merge
> > > this into master. It will have to wait for 4.3. I apologize to
> > > those who really want to see this feature in. I myself have slaved
> > > over this for some weeks, including missing the collab conference,
> > > but I just cannot conscientiously push it in under the current
> circumstances.
> > >
> > > --Alex
> > >
> > > > -----Original Message-----
> > > > From: Sudha Ponnaganti [mailto:sudha.ponnaganti@citrix.com]
> > > > Sent: Friday, June 28, 2013 7:11 AM
> > > > To: dev@cloudstack.apache.org
> > > > Subject: RE: [MERGE] Merge VMSync improvement branch into master
> > > >
> > > > Ideally I would like to see all validation to be done on feature
> > > > branch - BVTs and also manual validation of the P1 test cases from
> > > > VM Sync test plan just like object store validation - sadhu has
> > > > signed up for it along with Ilya. This would help us to identify
> > > > feature
> > specific issues.
> > > >
> > > > We are running BVTs right now which have high failure rate.
> > > > Focusing on this part right now to reach parity with Master branch.
> > > >
> > > >
> > > > -----Original Message-----
> > > > From: Trippie [mailto:trippie@gmail.com] On Behalf Of Hugo
> > > > Trippaers
> > > > Sent: Thursday, June 27, 2013 5:32 PM
> > > > To: dev@cloudstack.apache.org
> > > > Subject: Re: [MERGE] Merge VMSync improvement branch into master
> > > >
> > > > I agree with John that a change like this is very hard to test in
> > > > an automated fashion. Still i have been looking at the numbers for
> > > > the code coverage with cobertura. I was a bit disappointed to find
> > > > that we have not made any progress with this merge with regards to
> > > > unit tests and
> > > total code coverage.
> > > > We do not seem to be in a worse shape than before.
> > > >
> > > > @Sudha, it would be nice if you could add your view on this patch
> > > > from the QA perspective? How would this patch affect your planning
> > > > for
> > > example?
> > > >
> > > > Cheers,
> > > >
> > > > Hugo
> > > >
> > > > On Jun 27, 2013, at 5:12 PM, John Burwell <jb...@basho.com>
> wrote:
> > > >
> > > > > @David The types of concurrency changes introduced in this patch
> > > > > are extremely difficult to completely test in an automated fashion.
> > > > > Therefore, code review for correctness is critical to ensure quality.
> > > > > To be clear, I am not questioning the value of automated testing.
> > > > > I am just noting that it's next to impossible to achieve full
> > > > > coverage, and code review is an critical supplement.
> > > > >
> > > > > @Ilya I plan to review this patch, but I will be able to start
> > > > > until next week. I am also still reviewing object_store (a
> > > > > separate procedural issue for another thread), and need to
> > > > > complete
> > solidfire.
> > > > > This backlog is precisely why need to be reviewing iteratively
> > > > > throughout the dev cycle.
> > > > >
> > > > > Thanks,
> > > > > -John
> > > > >
> > > > > On Jun 27, 2013, at 7:35 PM, David Nalley <da...@gnsa.us> wrote:
> > > > >
> > > > >> On Thu, Jun 27, 2013 at 5:51 PM, Hugo Trippaers
> > > > >> <hu...@trippaers.nl>
> > > > wrote:
> > > > >>>
> > > > >>> I think Ilya offers is great, my current stance is also to see
> > > > >>> how we can
> > > > bring this forward.
> > > > >>>
> > > > >>> I've had the opportunity to meet with several people at the
> > > > >>> Citrix office
> > > > in Santa Clara, i'm actually working from their office at this moment.
> > > > I think it's also the responsibility of someone who put in a -1 to
> > > > work with the original committer to get the situation resolved. So
> > > > i'll invest the time to help with the review as well.
> > > > >>>
> > > > >>> It would be great if Alex or Kelven could take the time to
> > > > >>> explain how
> > > > this feature has been tested. That would give the community some
> > > > insight as well.
> > > > >>>
> > > > >>> My main technical problem with this merge is that stuff is
> > > > >>> moving all over
> > > > the place without having even the slightest idea why. Now having
> > > > discussed this with Alex in person i get the general idea of this
> > > > merge, so can actually try to review it.
> > > > >>>
> > > > >>> I think that John have nicely explained what we could do to
> > > > >>> prevent
> > > > situations like this in advance. I fully understand that big
> > > > features or rewrites don't happen overnight and might show up near
> > > > the end of the
> > > release cycle.
> > > > With the time based release cycle it's always a risk that some
> > > > feature might not make it in on time. Getting more people involved
> > > > and chunking the commits into master will greatly speed up the
> > > > reviewing
> > > process.
> > > > >>>
> > > > >>> I'll get back to this after spending some time on reviewing
> > > > >>> the actual
> > > > patch. In fact i would like to ask more people to have a look at
> > > > this patch and reply to this thread with comments or remarks.
> > > > >>>
> > > > >>> Cheers,
> > > > >>>
> > > > >>> Hugo
> > > > >>
> > > > >> So the problem in my mind, is that we don't have a way of
> > > > >> verifying that master isn't broken, and won't be broken by any
> > > > >> given merge. I look at even the minimal level of automated
> > > > >> testing that I see today, and ~20% of integration tests are
> > > > >> failing[1]. The regression set of tests (which isn't running as
> > > > >> often) is seeing 75% of tests failing[2]. Heaping on more
> > > > >> change when we are demonstrably already failing in many places
> > > > >> is not
> > behaving responsibly IMO.
> > > > >> The question I'd pose is this - running the various automated
> > > > >> tests is pretty cheap - whats the output of that compared to
> > > > >> the current test output on master? Better or worse? If it
> > > > >> hasn't been done, why
> > > not?
> > > > >> I desperately want these features, but not necessarily at the
> > > > >> cost of further destabilizing what we have now in master - we
> > > > >> can't continue accruing technical debt.
> > > > >>
> > > > >> --David
> > > > >>
> > > > >> [1]
> > > > >> http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-smok
> > > > >> e- ma tr ix/lastCompletedBuild/testReport/ [2]
> > > > >> http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-regr
> > > > >> es
> > > > >> si
> > > > >> on
> > > > >> -matrix/28/testReport/
> > >
> >
RE: [MERGE] Merge VMSync improvement branch into master
Posted by Alex Huang <Al...@citrix.com>.
Ilya,
Thanks! The current problem on this branch is we're trying to distinguish what's actually broken on master and what's broken by introducing this change. At this point, the branch is basically frozen and only applying bug fixes once BVT found problems.
--Alex
> -----Original Message-----
> From: Musayev, Ilya [mailto:imusayev@webmd.net]
> Sent: Monday, July 1, 2013 10:16 AM
> To: dev@cloudstack.apache.org
> Subject: RE: [MERGE] Merge VMSync improvement branch into master
>
> Alex,
>
> I completely understand. Please keep us in the loop on the progress. If its
> functional to some extent - i.e. at least build succeeds without errors and
> major functionality is there, I can merge the code into CloudSand CS distro -
> to test it out and share it with whoever wants to try it.
>
> Thanks
> ilya
>
> > -----Original Message-----
> > From: Alex Huang [mailto:Alex.Huang@citrix.com]
> > Sent: Friday, June 28, 2013 9:16 PM
> > To: dev@cloudstack.apache.org
> > Subject: RE: [MERGE] Merge VMSync improvement branch into master
> >
> > Given the current state of BVT, I don't think we can reliably merge
> > this into master. It will have to wait for 4.3. I apologize to those
> > who really want to see this feature in. I myself have slaved over
> > this for some weeks, including missing the collab conference, but I
> > just cannot conscientiously push it in under the current circumstances.
> >
> > --Alex
> >
> > > -----Original Message-----
> > > From: Sudha Ponnaganti [mailto:sudha.ponnaganti@citrix.com]
> > > Sent: Friday, June 28, 2013 7:11 AM
> > > To: dev@cloudstack.apache.org
> > > Subject: RE: [MERGE] Merge VMSync improvement branch into master
> > >
> > > Ideally I would like to see all validation to be done on feature
> > > branch - BVTs and also manual validation of the P1 test cases from
> > > VM Sync test plan just like object store validation - sadhu has
> > > signed up for it along with Ilya. This would help us to identify feature
> specific issues.
> > >
> > > We are running BVTs right now which have high failure rate. Focusing
> > > on this part right now to reach parity with Master branch.
> > >
> > >
> > > -----Original Message-----
> > > From: Trippie [mailto:trippie@gmail.com] On Behalf Of Hugo Trippaers
> > > Sent: Thursday, June 27, 2013 5:32 PM
> > > To: dev@cloudstack.apache.org
> > > Subject: Re: [MERGE] Merge VMSync improvement branch into master
> > >
> > > I agree with John that a change like this is very hard to test in an
> > > automated fashion. Still i have been looking at the numbers for the
> > > code coverage with cobertura. I was a bit disappointed to find that
> > > we have not made any progress with this merge with regards to unit
> > > tests and
> > total code coverage.
> > > We do not seem to be in a worse shape than before.
> > >
> > > @Sudha, it would be nice if you could add your view on this patch
> > > from the QA perspective? How would this patch affect your planning
> > > for
> > example?
> > >
> > > Cheers,
> > >
> > > Hugo
> > >
> > > On Jun 27, 2013, at 5:12 PM, John Burwell <jb...@basho.com> wrote:
> > >
> > > > @David The types of concurrency changes introduced in this patch
> > > > are extremely difficult to completely test in an automated fashion.
> > > > Therefore, code review for correctness is critical to ensure quality.
> > > > To be clear, I am not questioning the value of automated testing.
> > > > I am just noting that it's next to impossible to achieve full
> > > > coverage, and code review is an critical supplement.
> > > >
> > > > @Ilya I plan to review this patch, but I will be able to start
> > > > until next week. I am also still reviewing object_store (a
> > > > separate procedural issue for another thread), and need to complete
> solidfire.
> > > > This backlog is precisely why need to be reviewing iteratively
> > > > throughout the dev cycle.
> > > >
> > > > Thanks,
> > > > -John
> > > >
> > > > On Jun 27, 2013, at 7:35 PM, David Nalley <da...@gnsa.us> wrote:
> > > >
> > > >> On Thu, Jun 27, 2013 at 5:51 PM, Hugo Trippaers
> > > >> <hu...@trippaers.nl>
> > > wrote:
> > > >>>
> > > >>> I think Ilya offers is great, my current stance is also to see
> > > >>> how we can
> > > bring this forward.
> > > >>>
> > > >>> I've had the opportunity to meet with several people at the
> > > >>> Citrix office
> > > in Santa Clara, i'm actually working from their office at this moment.
> > > I think it's also the responsibility of someone who put in a -1 to
> > > work with the original committer to get the situation resolved. So
> > > i'll invest the time to help with the review as well.
> > > >>>
> > > >>> It would be great if Alex or Kelven could take the time to
> > > >>> explain how
> > > this feature has been tested. That would give the community some
> > > insight as well.
> > > >>>
> > > >>> My main technical problem with this merge is that stuff is
> > > >>> moving all over
> > > the place without having even the slightest idea why. Now having
> > > discussed this with Alex in person i get the general idea of this
> > > merge, so can actually try to review it.
> > > >>>
> > > >>> I think that John have nicely explained what we could do to
> > > >>> prevent
> > > situations like this in advance. I fully understand that big
> > > features or rewrites don't happen overnight and might show up near
> > > the end of the
> > release cycle.
> > > With the time based release cycle it's always a risk that some
> > > feature might not make it in on time. Getting more people involved
> > > and chunking the commits into master will greatly speed up the
> > > reviewing
> > process.
> > > >>>
> > > >>> I'll get back to this after spending some time on reviewing the
> > > >>> actual
> > > patch. In fact i would like to ask more people to have a look at
> > > this patch and reply to this thread with comments or remarks.
> > > >>>
> > > >>> Cheers,
> > > >>>
> > > >>> Hugo
> > > >>
> > > >> So the problem in my mind, is that we don't have a way of
> > > >> verifying that master isn't broken, and won't be broken by any
> > > >> given merge. I look at even the minimal level of automated
> > > >> testing that I see today, and ~20% of integration tests are
> > > >> failing[1]. The regression set of tests (which isn't running as
> > > >> often) is seeing 75% of tests failing[2]. Heaping on more change
> > > >> when we are demonstrably already failing in many places is not
> behaving responsibly IMO.
> > > >> The question I'd pose is this - running the various automated
> > > >> tests is pretty cheap - whats the output of that compared to the
> > > >> current test output on master? Better or worse? If it hasn't been
> > > >> done, why
> > not?
> > > >> I desperately want these features, but not necessarily at the
> > > >> cost of further destabilizing what we have now in master - we
> > > >> can't continue accruing technical debt.
> > > >>
> > > >> --David
> > > >>
> > > >> [1]
> > > >> http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-smoke-
> > > >> ma tr ix/lastCompletedBuild/testReport/ [2]
> > > >> http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-regres
> > > >> si
> > > >> on
> > > >> -matrix/28/testReport/
> >
>
RE: [MERGE] Merge VMSync improvement branch into master
Posted by "Musayev, Ilya" <im...@webmd.net>.
Alex,
I completely understand. Please keep us in the loop on the progress. If its functional to some extent - i.e. at least build succeeds without errors and major functionality is there, I can merge the code into CloudSand CS distro - to test it out and share it with whoever wants to try it.
Thanks
ilya
> -----Original Message-----
> From: Alex Huang [mailto:Alex.Huang@citrix.com]
> Sent: Friday, June 28, 2013 9:16 PM
> To: dev@cloudstack.apache.org
> Subject: RE: [MERGE] Merge VMSync improvement branch into master
>
> Given the current state of BVT, I don't think we can reliably merge this into
> master. It will have to wait for 4.3. I apologize to those who really want to
> see this feature in. I myself have slaved over this for some weeks, including
> missing the collab conference, but I just cannot conscientiously push it in
> under the current circumstances.
>
> --Alex
>
> > -----Original Message-----
> > From: Sudha Ponnaganti [mailto:sudha.ponnaganti@citrix.com]
> > Sent: Friday, June 28, 2013 7:11 AM
> > To: dev@cloudstack.apache.org
> > Subject: RE: [MERGE] Merge VMSync improvement branch into master
> >
> > Ideally I would like to see all validation to be done on feature
> > branch - BVTs and also manual validation of the P1 test cases from VM
> > Sync test plan just like object store validation - sadhu has signed up
> > for it along with Ilya. This would help us to identify feature specific issues.
> >
> > We are running BVTs right now which have high failure rate. Focusing
> > on this part right now to reach parity with Master branch.
> >
> >
> > -----Original Message-----
> > From: Trippie [mailto:trippie@gmail.com] On Behalf Of Hugo Trippaers
> > Sent: Thursday, June 27, 2013 5:32 PM
> > To: dev@cloudstack.apache.org
> > Subject: Re: [MERGE] Merge VMSync improvement branch into master
> >
> > I agree with John that a change like this is very hard to test in an
> > automated fashion. Still i have been looking at the numbers for the
> > code coverage with cobertura. I was a bit disappointed to find that we
> > have not made any progress with this merge with regards to unit tests and
> total code coverage.
> > We do not seem to be in a worse shape than before.
> >
> > @Sudha, it would be nice if you could add your view on this patch from
> > the QA perspective? How would this patch affect your planning for
> example?
> >
> > Cheers,
> >
> > Hugo
> >
> > On Jun 27, 2013, at 5:12 PM, John Burwell <jb...@basho.com> wrote:
> >
> > > @David The types of concurrency changes introduced in this patch are
> > > extremely difficult to completely test in an automated fashion.
> > > Therefore, code review for correctness is critical to ensure quality.
> > > To be clear, I am not questioning the value of automated testing. I
> > > am just noting that it's next to impossible to achieve full
> > > coverage, and code review is an critical supplement.
> > >
> > > @Ilya I plan to review this patch, but I will be able to start until
> > > next week. I am also still reviewing object_store (a separate
> > > procedural issue for another thread), and need to complete solidfire.
> > > This backlog is precisely why need to be reviewing iteratively
> > > throughout the dev cycle.
> > >
> > > Thanks,
> > > -John
> > >
> > > On Jun 27, 2013, at 7:35 PM, David Nalley <da...@gnsa.us> wrote:
> > >
> > >> On Thu, Jun 27, 2013 at 5:51 PM, Hugo Trippaers <hu...@trippaers.nl>
> > wrote:
> > >>>
> > >>> I think Ilya offers is great, my current stance is also to see how
> > >>> we can
> > bring this forward.
> > >>>
> > >>> I've had the opportunity to meet with several people at the Citrix
> > >>> office
> > in Santa Clara, i'm actually working from their office at this moment.
> > I think it's also the responsibility of someone who put in a -1 to
> > work with the original committer to get the situation resolved. So
> > i'll invest the time to help with the review as well.
> > >>>
> > >>> It would be great if Alex or Kelven could take the time to explain
> > >>> how
> > this feature has been tested. That would give the community some
> > insight as well.
> > >>>
> > >>> My main technical problem with this merge is that stuff is moving
> > >>> all over
> > the place without having even the slightest idea why. Now having
> > discussed this with Alex in person i get the general idea of this
> > merge, so can actually try to review it.
> > >>>
> > >>> I think that John have nicely explained what we could do to
> > >>> prevent
> > situations like this in advance. I fully understand that big features
> > or rewrites don't happen overnight and might show up near the end of the
> release cycle.
> > With the time based release cycle it's always a risk that some feature
> > might not make it in on time. Getting more people involved and
> > chunking the commits into master will greatly speed up the reviewing
> process.
> > >>>
> > >>> I'll get back to this after spending some time on reviewing the
> > >>> actual
> > patch. In fact i would like to ask more people to have a look at this
> > patch and reply to this thread with comments or remarks.
> > >>>
> > >>> Cheers,
> > >>>
> > >>> Hugo
> > >>
> > >> So the problem in my mind, is that we don't have a way of verifying
> > >> that master isn't broken, and won't be broken by any given merge. I
> > >> look at even the minimal level of automated testing that I see
> > >> today, and ~20% of integration tests are failing[1]. The regression
> > >> set of tests (which isn't running as often) is seeing 75% of tests
> > >> failing[2]. Heaping on more change when we are demonstrably already
> > >> failing in many places is not behaving responsibly IMO.
> > >> The question I'd pose is this - running the various automated tests
> > >> is pretty cheap - whats the output of that compared to the current
> > >> test output on master? Better or worse? If it hasn't been done, why
> not?
> > >> I desperately want these features, but not necessarily at the cost
> > >> of further destabilizing what we have now in master - we can't
> > >> continue accruing technical debt.
> > >>
> > >> --David
> > >>
> > >> [1]
> > >> http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-smoke-ma
> > >> tr ix/lastCompletedBuild/testReport/ [2]
> > >> http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-regressi
> > >> on
> > >> -matrix/28/testReport/
>
RE: [MERGE] Merge VMSync improvement branch into master
Posted by Alex Huang <Al...@citrix.com>.
Given the current state of BVT, I don't think we can reliably merge this into master. It will have to wait for 4.3. I apologize to those who really want to see this feature in. I myself have slaved over this for some weeks, including missing the collab conference, but I just cannot conscientiously push it in under the current circumstances.
--Alex
> -----Original Message-----
> From: Sudha Ponnaganti [mailto:sudha.ponnaganti@citrix.com]
> Sent: Friday, June 28, 2013 7:11 AM
> To: dev@cloudstack.apache.org
> Subject: RE: [MERGE] Merge VMSync improvement branch into master
>
> Ideally I would like to see all validation to be done on feature branch - BVTs
> and also manual validation of the P1 test cases from VM Sync test plan just
> like object store validation - sadhu has signed up for it along with Ilya. This
> would help us to identify feature specific issues.
>
> We are running BVTs right now which have high failure rate. Focusing on this
> part right now to reach parity with Master branch.
>
>
> -----Original Message-----
> From: Trippie [mailto:trippie@gmail.com] On Behalf Of Hugo Trippaers
> Sent: Thursday, June 27, 2013 5:32 PM
> To: dev@cloudstack.apache.org
> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>
> I agree with John that a change like this is very hard to test in an automated
> fashion. Still i have been looking at the numbers for the code coverage with
> cobertura. I was a bit disappointed to find that we have not made any
> progress with this merge with regards to unit tests and total code coverage.
> We do not seem to be in a worse shape than before.
>
> @Sudha, it would be nice if you could add your view on this patch from the
> QA perspective? How would this patch affect your planning for example?
>
> Cheers,
>
> Hugo
>
> On Jun 27, 2013, at 5:12 PM, John Burwell <jb...@basho.com> wrote:
>
> > @David The types of concurrency changes introduced in this patch are
> > extremely difficult to completely test in an automated fashion.
> > Therefore, code review for correctness is critical to ensure quality.
> > To be clear, I am not questioning the value of automated testing. I
> > am just noting that it's next to impossible to achieve full coverage,
> > and code review is an critical supplement.
> >
> > @Ilya I plan to review this patch, but I will be able to start until
> > next week. I am also still reviewing object_store (a separate
> > procedural issue for another thread), and need to complete solidfire.
> > This backlog is precisely why need to be reviewing iteratively
> > throughout the dev cycle.
> >
> > Thanks,
> > -John
> >
> > On Jun 27, 2013, at 7:35 PM, David Nalley <da...@gnsa.us> wrote:
> >
> >> On Thu, Jun 27, 2013 at 5:51 PM, Hugo Trippaers <hu...@trippaers.nl>
> wrote:
> >>>
> >>> I think Ilya offers is great, my current stance is also to see how we can
> bring this forward.
> >>>
> >>> I've had the opportunity to meet with several people at the Citrix office
> in Santa Clara, i'm actually working from their office at this moment. I think
> it's also the responsibility of someone who put in a -1 to work with the
> original committer to get the situation resolved. So i'll invest the time to help
> with the review as well.
> >>>
> >>> It would be great if Alex or Kelven could take the time to explain how
> this feature has been tested. That would give the community some insight as
> well.
> >>>
> >>> My main technical problem with this merge is that stuff is moving all over
> the place without having even the slightest idea why. Now having discussed
> this with Alex in person i get the general idea of this merge, so can actually
> try to review it.
> >>>
> >>> I think that John have nicely explained what we could do to prevent
> situations like this in advance. I fully understand that big features or rewrites
> don't happen overnight and might show up near the end of the release cycle.
> With the time based release cycle it's always a risk that some feature might
> not make it in on time. Getting more people involved and chunking the
> commits into master will greatly speed up the reviewing process.
> >>>
> >>> I'll get back to this after spending some time on reviewing the actual
> patch. In fact i would like to ask more people to have a look at this patch and
> reply to this thread with comments or remarks.
> >>>
> >>> Cheers,
> >>>
> >>> Hugo
> >>
> >> So the problem in my mind, is that we don't have a way of verifying
> >> that master isn't broken, and won't be broken by any given merge. I
> >> look at even the minimal level of automated testing that I see today,
> >> and ~20% of integration tests are failing[1]. The regression set of
> >> tests (which isn't running as often) is seeing 75% of tests
> >> failing[2]. Heaping on more change when we are demonstrably already
> >> failing in many places is not behaving responsibly IMO.
> >> The question I'd pose is this - running the various automated tests
> >> is pretty cheap - whats the output of that compared to the current
> >> test output on master? Better or worse? If it hasn't been done, why not?
> >> I desperately want these features, but not necessarily at the cost of
> >> further destabilizing what we have now in master - we can't continue
> >> accruing technical debt.
> >>
> >> --David
> >>
> >> [1]
> >> http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-smoke-matr
> >> ix/lastCompletedBuild/testReport/ [2]
> >> http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-regression
> >> -matrix/28/testReport/
RE: [MERGE] Merge VMSync improvement branch into master
Posted by Sudha Ponnaganti <su...@citrix.com>.
Ideally I would like to see all validation to be done on feature branch - BVTs and also manual validation of the P1 test cases from VM Sync test plan just like object store validation - sadhu has signed up for it along with Ilya. This would help us to identify feature specific issues.
We are running BVTs right now which have high failure rate. Focusing on this part right now to reach parity with Master branch.
-----Original Message-----
From: Trippie [mailto:trippie@gmail.com] On Behalf Of Hugo Trippaers
Sent: Thursday, June 27, 2013 5:32 PM
To: dev@cloudstack.apache.org
Subject: Re: [MERGE] Merge VMSync improvement branch into master
I agree with John that a change like this is very hard to test in an automated fashion. Still i have been looking at the numbers for the code coverage with cobertura. I was a bit disappointed to find that we have not made any progress with this merge with regards to unit tests and total code coverage. We do not seem to be in a worse shape than before.
@Sudha, it would be nice if you could add your view on this patch from the QA perspective? How would this patch affect your planning for example?
Cheers,
Hugo
On Jun 27, 2013, at 5:12 PM, John Burwell <jb...@basho.com> wrote:
> @David The types of concurrency changes introduced in this patch are
> extremely difficult to completely test in an automated fashion.
> Therefore, code review for correctness is critical to ensure quality.
> To be clear, I am not questioning the value of automated testing. I
> am just noting that it's next to impossible to achieve full coverage,
> and code review is an critical supplement.
>
> @Ilya I plan to review this patch, but I will be able to start until
> next week. I am also still reviewing object_store (a separate
> procedural issue for another thread), and need to complete solidfire.
> This backlog is precisely why need to be reviewing iteratively
> throughout the dev cycle.
>
> Thanks,
> -John
>
> On Jun 27, 2013, at 7:35 PM, David Nalley <da...@gnsa.us> wrote:
>
>> On Thu, Jun 27, 2013 at 5:51 PM, Hugo Trippaers <hu...@trippaers.nl> wrote:
>>>
>>> I think Ilya offers is great, my current stance is also to see how we can bring this forward.
>>>
>>> I've had the opportunity to meet with several people at the Citrix office in Santa Clara, i'm actually working from their office at this moment. I think it's also the responsibility of someone who put in a -1 to work with the original committer to get the situation resolved. So i'll invest the time to help with the review as well.
>>>
>>> It would be great if Alex or Kelven could take the time to explain how this feature has been tested. That would give the community some insight as well.
>>>
>>> My main technical problem with this merge is that stuff is moving all over the place without having even the slightest idea why. Now having discussed this with Alex in person i get the general idea of this merge, so can actually try to review it.
>>>
>>> I think that John have nicely explained what we could do to prevent situations like this in advance. I fully understand that big features or rewrites don't happen overnight and might show up near the end of the release cycle. With the time based release cycle it's always a risk that some feature might not make it in on time. Getting more people involved and chunking the commits into master will greatly speed up the reviewing process.
>>>
>>> I'll get back to this after spending some time on reviewing the actual patch. In fact i would like to ask more people to have a look at this patch and reply to this thread with comments or remarks.
>>>
>>> Cheers,
>>>
>>> Hugo
>>
>> So the problem in my mind, is that we don't have a way of verifying
>> that master isn't broken, and won't be broken by any given merge. I
>> look at even the minimal level of automated testing that I see today,
>> and ~20% of integration tests are failing[1]. The regression set of
>> tests (which isn't running as often) is seeing 75% of tests
>> failing[2]. Heaping on more change when we are demonstrably already
>> failing in many places is not behaving responsibly IMO.
>> The question I'd pose is this - running the various automated tests
>> is pretty cheap - whats the output of that compared to the current
>> test output on master? Better or worse? If it hasn't been done, why not?
>> I desperately want these features, but not necessarily at the cost of
>> further destabilizing what we have now in master - we can't continue
>> accruing technical debt.
>>
>> --David
>>
>> [1]
>> http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-smoke-matr
>> ix/lastCompletedBuild/testReport/ [2]
>> http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-regression
>> -matrix/28/testReport/
Re: [MERGE] Merge VMSync improvement branch into master
Posted by Hugo Trippaers <hu...@trippaers.nl>.
I agree with John that a change like this is very hard to test in an automated fashion. Still i have been looking at the numbers for the code coverage with cobertura. I was a bit disappointed to find that we have not made any progress with this merge with regards to unit tests and total code coverage. We do not seem to be in a worse shape than before.
@Sudha, it would be nice if you could add your view on this patch from the QA perspective? How would this patch affect your planning for example?
Cheers,
Hugo
On Jun 27, 2013, at 5:12 PM, John Burwell <jb...@basho.com> wrote:
> @David The types of concurrency changes introduced in this patch are
> extremely difficult to completely test in an automated fashion.
> Therefore, code review for correctness is critical to ensure quality.
> To be clear, I am not questioning the value of automated testing. I
> am just noting that it's next to impossible to achieve full coverage,
> and code review is an critical supplement.
>
> @Ilya I plan to review this patch, but I will be able to start until
> next week. I am also still reviewing object_store (a separate
> procedural issue for another thread), and need to complete solidfire.
> This backlog is precisely why need to be reviewing iteratively
> throughout the dev cycle.
>
> Thanks,
> -John
>
> On Jun 27, 2013, at 7:35 PM, David Nalley <da...@gnsa.us> wrote:
>
>> On Thu, Jun 27, 2013 at 5:51 PM, Hugo Trippaers <hu...@trippaers.nl> wrote:
>>>
>>> I think Ilya offers is great, my current stance is also to see how we can bring this forward.
>>>
>>> I've had the opportunity to meet with several people at the Citrix office in Santa Clara, i'm actually working from their office at this moment. I think it's also the responsibility of someone who put in a -1 to work with the original committer to get the situation resolved. So i'll invest the time to help with the review as well.
>>>
>>> It would be great if Alex or Kelven could take the time to explain how this feature has been tested. That would give the community some insight as well.
>>>
>>> My main technical problem with this merge is that stuff is moving all over the place without having even the slightest idea why. Now having discussed this with Alex in person i get the general idea of this merge, so can actually try to review it.
>>>
>>> I think that John have nicely explained what we could do to prevent situations like this in advance. I fully understand that big features or rewrites don't happen overnight and might show up near the end of the release cycle. With the time based release cycle it's always a risk that some feature might not make it in on time. Getting more people involved and chunking the commits into master will greatly speed up the reviewing process.
>>>
>>> I'll get back to this after spending some time on reviewing the actual patch. In fact i would like to ask more people to have a look at this patch and reply to this thread with comments or remarks.
>>>
>>> Cheers,
>>>
>>> Hugo
>>
>> So the problem in my mind, is that we don't have a way of verifying
>> that master isn't broken, and won't be broken by any given merge. I
>> look at even the minimal level of automated testing that I see today,
>> and ~20% of integration tests are failing[1]. The regression set of
>> tests (which isn't running as often) is seeing 75% of tests
>> failing[2]. Heaping on more change when we are demonstrably already
>> failing in many places is not behaving responsibly IMO.
>> The question I'd pose is this - running the various automated tests is
>> pretty cheap - whats the output of that compared to the current test
>> output on master? Better or worse? If it hasn't been done, why not?
>> I desperately want these features, but not necessarily at the cost of
>> further destabilizing what we have now in master - we can't continue
>> accruing technical debt.
>>
>> --David
>>
>> [1] http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-smoke-matrix/lastCompletedBuild/testReport/
>> [2] http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-regression-matrix/28/testReport/
Re: [MERGE] Merge VMSync improvement branch into master
Posted by John Burwell <jb...@basho.com>.
@David The types of concurrency changes introduced in this patch are
extremely difficult to completely test in an automated fashion.
Therefore, code review for correctness is critical to ensure quality.
To be clear, I am not questioning the value of automated testing. I
am just noting that it's next to impossible to achieve full coverage,
and code review is an critical supplement.
@Ilya I plan to review this patch, but I will be able to start until
next week. I am also still reviewing object_store (a separate
procedural issue for another thread), and need to complete solidfire.
This backlog is precisely why need to be reviewing iteratively
throughout the dev cycle.
Thanks,
-John
On Jun 27, 2013, at 7:35 PM, David Nalley <da...@gnsa.us> wrote:
> On Thu, Jun 27, 2013 at 5:51 PM, Hugo Trippaers <hu...@trippaers.nl> wrote:
>>
>> I think Ilya offers is great, my current stance is also to see how we can bring this forward.
>>
>> I've had the opportunity to meet with several people at the Citrix office in Santa Clara, i'm actually working from their office at this moment. I think it's also the responsibility of someone who put in a -1 to work with the original committer to get the situation resolved. So i'll invest the time to help with the review as well.
>>
>> It would be great if Alex or Kelven could take the time to explain how this feature has been tested. That would give the community some insight as well.
>>
>> My main technical problem with this merge is that stuff is moving all over the place without having even the slightest idea why. Now having discussed this with Alex in person i get the general idea of this merge, so can actually try to review it.
>>
>> I think that John have nicely explained what we could do to prevent situations like this in advance. I fully understand that big features or rewrites don't happen overnight and might show up near the end of the release cycle. With the time based release cycle it's always a risk that some feature might not make it in on time. Getting more people involved and chunking the commits into master will greatly speed up the reviewing process.
>>
>> I'll get back to this after spending some time on reviewing the actual patch. In fact i would like to ask more people to have a look at this patch and reply to this thread with comments or remarks.
>>
>> Cheers,
>>
>> Hugo
>
> So the problem in my mind, is that we don't have a way of verifying
> that master isn't broken, and won't be broken by any given merge. I
> look at even the minimal level of automated testing that I see today,
> and ~20% of integration tests are failing[1]. The regression set of
> tests (which isn't running as often) is seeing 75% of tests
> failing[2]. Heaping on more change when we are demonstrably already
> failing in many places is not behaving responsibly IMO.
> The question I'd pose is this - running the various automated tests is
> pretty cheap - whats the output of that compared to the current test
> output on master? Better or worse? If it hasn't been done, why not?
> I desperately want these features, but not necessarily at the cost of
> further destabilizing what we have now in master - we can't continue
> accruing technical debt.
>
> --David
>
> [1] http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-smoke-matrix/lastCompletedBuild/testReport/
> [2] http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-regression-matrix/28/testReport/
Re: [MERGE] Merge VMSync improvement branch into master
Posted by David Nalley <da...@gnsa.us>.
On Thu, Jun 27, 2013 at 5:51 PM, Hugo Trippaers <hu...@trippaers.nl> wrote:
>
> I think Ilya offers is great, my current stance is also to see how we can bring this forward.
>
> I've had the opportunity to meet with several people at the Citrix office in Santa Clara, i'm actually working from their office at this moment. I think it's also the responsibility of someone who put in a -1 to work with the original committer to get the situation resolved. So i'll invest the time to help with the review as well.
>
> It would be great if Alex or Kelven could take the time to explain how this feature has been tested. That would give the community some insight as well.
>
> My main technical problem with this merge is that stuff is moving all over the place without having even the slightest idea why. Now having discussed this with Alex in person i get the general idea of this merge, so can actually try to review it.
>
> I think that John have nicely explained what we could do to prevent situations like this in advance. I fully understand that big features or rewrites don't happen overnight and might show up near the end of the release cycle. With the time based release cycle it's always a risk that some feature might not make it in on time. Getting more people involved and chunking the commits into master will greatly speed up the reviewing process.
>
> I'll get back to this after spending some time on reviewing the actual patch. In fact i would like to ask more people to have a look at this patch and reply to this thread with comments or remarks.
>
> Cheers,
>
> Hugo
>
So the problem in my mind, is that we don't have a way of verifying
that master isn't broken, and won't be broken by any given merge. I
look at even the minimal level of automated testing that I see today,
and ~20% of integration tests are failing[1]. The regression set of
tests (which isn't running as often) is seeing 75% of tests
failing[2]. Heaping on more change when we are demonstrably already
failing in many places is not behaving responsibly IMO.
The question I'd pose is this - running the various automated tests is
pretty cheap - whats the output of that compared to the current test
output on master? Better or worse? If it hasn't been done, why not?
I desperately want these features, but not necessarily at the cost of
further destabilizing what we have now in master - we can't continue
accruing technical debt.
--David
[1] http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-smoke-matrix/lastCompletedBuild/testReport/
[2] http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-regression-matrix/28/testReport/
RE: [MERGE] Merge VMSync improvement branch into master
Posted by Alex Huang <Al...@citrix.com>.
Thanks Hugo.
Let me bring some reasoning to these changes.
AsyncJobManager previously was embedded as part of the server package. It only served api jobs. In Kelven's design, all vm operations relies on asyncjob to order the operation so that it will no longer have any concurrentoperationexceptions for example. All HA jobs will also be funneled to the asyncjob. For that reason, asyncjob becomes a much more generic entity. For that we moved it into it's own jar package and remove all traces of api that's embedded inside asyncjobmanager. For each client of asyncjobmanager, they have to define their own dispatcher which does the serialization and deserialization of their jobs themselves. The dispatcher for api is called ApiAsyncJobDispatcher.java. The dispatcher for vm work is now called VmWorkJobDispatcher.java.
Due to this change most of the cmd files in api package have to be changed because asyncjob topics and asyncjob command types were universally included in the *cmd files. The actual command execution were not changed.
As for import, that's a function of my eclipse settings. It organizes our imports to this order: Java, javax, org, com, org.apache.cloudstack, com.cloud. This puts the cloudstack imports to the bottom. I don't think any review needs to be done on the imports.
Please throw any questions you have on the list. Will be happy to explain.
--Alex
> -----Original Message-----
> From: Trippie [mailto:trippie@gmail.com] On Behalf Of Hugo Trippaers
> Sent: Thursday, June 27, 2013 4:27 PM
> To: dev@cloudstack.apache.org
> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>
> Let's get it done :-)
>
>
> I'm at about 15% of the entire commit, reading through it line by line...
>
> My notes so far (this is just a copy of my notepad fyi)
> There are a lot of changes to BaseCmd and Async command. Do they
> impact the api in any way. I can't tell yet but this might impact api
> compatibility by fixing some of the current issues in the api. (Some external
> programs like libcloud and jclouds have work-arounds for issues with the
> current api)
>
> Lot of cleanup done for spelling errors and re-ordering of imports.
> Typically something that we should push in as separate commits in master.
> No risk and they make the patch unnecessarily bloated.
>
> UserContext has been replaced by CallContext and is used more
> consistently. GoodThing((tm))
>
> Netapp support enabled in the component context. Seems
> unrelated to this particular patch?
>
> usageServerMonitor disabled in the component context. Also seems
> unrelated to this particular patch?
>
>
> I'll be updating this with my progress.
>
> Cheers,
>
> Hugo
>
>
> On Jun 27, 2013, at 4:05 PM, "Musayev, Ilya" <im...@webmd.net> wrote:
>
> > Thanks Hugo,
> >
> > I'm all for it if John can help us with his -1 vote :)
> >
> >
> >> -----Original Message-----
> >> From: Trippie [mailto:trippie@gmail.com] On Behalf Of Hugo Trippaers
> >> Sent: Thursday, June 27, 2013 5:51 PM
> >> To: dev@cloudstack.apache.org
> >> Subject: Re: [MERGE] Merge VMSync improvement branch into master
> >>
> >>
> >> I think Ilya offers is great, my current stance is also to see how we
> >> can bring this forward.
> >>
> >> I've had the opportunity to meet with several people at the Citrix
> >> office in Santa Clara, i'm actually working from their office at this
> >> moment. I think it's also the responsibility of someone who put in a
> >> -1 to work with the original committer to get the situation resolved.
> >> So i'll invest the time to help with the review as well.
> >>
> >> It would be great if Alex or Kelven could take the time to explain
> >> how this feature has been tested. That would give the community some
> >> insight as well.
> >>
> >> My main technical problem with this merge is that stuff is moving all
> >> over the place without having even the slightest idea why. Now having
> >> discussed this with Alex in person i get the general idea of this
> >> merge, so can actually try to review it.
> >>
> >> I think that John have nicely explained what we could do to prevent
> >> situations like this in advance. I fully understand that big features
> >> or rewrites don't happen overnight and might show up near the end of
> the release cycle.
> >> With the time based release cycle it's always a risk that some
> >> feature might not make it in on time. Getting more people involved
> >> and chunking the commits into master will greatly speed up the reviewing
> process.
> >>
> >> I'll get back to this after spending some time on reviewing the
> >> actual patch. In fact i would like to ask more people to have a look
> >> at this patch and reply to this thread with comments or remarks.
> >>
> >> Cheers,
> >>
> >> Hugo
> >>
> >>
> >> On Jun 27, 2013, at 12:55 PM, John Burwell <jb...@basho.com> wrote:
> >>
> >>> Ilya,
> >>>
> >>> I understand your concern. However, this patch is large and complicated.
> >> Without adequate review, we run that the risk getting it wrong which
> >> would be worse than not having it. If you feel it is a must have,
> >> you can propose extending the freeze to allow time to perform the
> review.
> >>>
> >>> Thanks,
> >>> -John
> >>>
> >>> On Jun 27, 2013, at 3:49 PM, "Musayev, Ilya" <im...@webmd.net>
> >> wrote:
> >>>
> >>>> John and Hugo,
> >>>>
> >>>> I see and understand your concerns, however, this feature is really
> >> needed by many users - I've met with several large users at CCC13 who
> >> were looking forward to this work, blocking this merge - would delay
> >> cloudstack adoption and make ACS look inferior to other cloud
> >> platforms. Realistically, this is going to put us back at least 8+ months away
> until 4.3 comes out.
> >>>>
> >>>> If needed, I can dedicate QA cycles and work with Kelven to rule
> >>>> out all
> >> the bugs and issues - through as many scenarios as possible on my end.
> >>>>
> >>>> Thanks
> >>>> ilya
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: John Burwell [mailto:jburwell@basho.com]
> >>>>> Sent: Thursday, June 27, 2013 2:33 PM
> >>>>> To: dev@cloudstack.apache.org
> >>>>> Subject: Re: [MERGE] Merge VMSync improvement branch into
> master
> >>>>>
> >>>>> Hugo,
> >>>>>
> >>>>> I completely agree with this stance, and will add a -1 as well.
> >>>>> It has been a significant challenge this cycle to complete high
> >>>>> quality reviews due to the large patch size and short time turnaround
> time.
> >>>>> Going forward, I am going to be more likely to follow Hugo's
> >>>>> example, and -1 patches for which there is not adequate review time.
> >>>>> I think the following techniques will help streamline the review
> process:
> >>>>>
> >>>>> 1. Narrow Patch Scope: Ensure that the patch is confined to a
> >>>>> single enhancement or defect fix. If you doing refactoring as
> >>>>> part of a feature, submit the refactoring work a separate commit.
> >>>>> As a further suggestion, if you are performing multiple
> >>>>> refactorings (e.g. package reorganization and utility method
> >>>>> extraction), split each
> >> refactoring into separate patches.
> >>>>> 2. Chunk Features: For each large feature being developed,
> >>>>> logically chunk the functionality and submit each chunk as a separate
> patch.
> >>>>> I recommend erring on the side of chunks being too small. Let the
> >>>>> reviewer determine if the patch is not large enough to be
> >>>>> representative of the feature. The worst that will happen is
> >>>>> that a reviewer will provide feedback on the work completed and
> >>>>> simply ask for more work to be done before it can be merged to
> >>>>> master. For features while chunks can't be merged all the way to
> >>>>> master, intermediate patches can be submitted to Review Board for
> >>>>> review throughout the cycle -- allowing large merges to actually
> >>>>> be the
> >> accumulation of several smaller reviews.
> >>>>> 3. Identify Reviewers Early: For big features, solicit the
> >>>>> reviewers whiling to review a feature from FS through merge from
> >>>>> the mailing list. Reviews will go much more quickly if reviewers
> >>>>> have been involved from the beginning because they will not help
> >>>>> identify issues before coding starts, but also will not have to
> >>>>> develop context late. To be clear, identifying reviewers early
> >>>>> does not preclude a new reviewer from becoming involved later, but
> >>>>> the reviews will greatly reduce the probability of a late review
> >>>>> finding significant issues. Additionally, the early reviewers can
> >>>>> help the late
> >> reviewers come up to speed -- reducing the burden on the contributor.
> >>>>>
> >>>>> Generally, my recommendation is to submit patches early and often.
> >>>>> Ultimately, contributors determine how they wish to develop and
> >>>>> deliver their contributions. These are only my recommendations as
> >>>>> a reviewer to increase the probability that a feature will make a
> >>>>> particular
> >> release train.
> >>>>> Finally, it appears that reviewers are growing less and less
> >>>>> tolerant of large patches that appear just before freeze dates,
> >>>>> and those these patches face a much higher risk of being
> >>>>> immediately receiving a -1 for the reasons stated above.
> >>>>>
> >>>>> Thanks,
> >>>>> -John
> >>>>>
> >>>>> On Jun 26, 2013, at 8:45 PM, Hugo Trippaers <hu...@trippaers.nl>
> wrote:
> >>>>>
> >>>>>> Hey Alex, Kelven,
> >>>>>>
> >>>>>> I've been looking though the code and thanks for the very
> >>>>>> insightfull
> >>>>> conversation and follow-up email. With merges of this magnitude
> >>>>> it's essential to help people understand what is going on.
> >>>>>>
> >>>>>> Purely technical this is a merge i'm really pleased with. It will
> >>>>>> for sure
> >>>>> improve our handling of virtual machines.
> >>>>>>
> >>>>>> However the timing of the merge request is not ideal. We are very
> >>>>>> close to
> >>>>> the end of the already extended feature freeze. We have a
> >>>>> consensus within the dev community to do large architectural
> >>>>> changes in the beginning of the cycle and not at the very end.
> >>>>> This not only means a lot of extra testing effort and other
> >>>>> developers will have to get used to the changes introduced right
> >>>>> at the moment when everybody should be focussed on bug fixing,
> documentation and stability.
> >>>>> Without wanting to rip open old wounds, i can imagine we all want
> >>>>> to
> >> avoid a javelin incident.
> >>>>>>
> >>>>>> I respect the work going into this and the effort it will take to
> >>>>>> keep this up
> >>>>> to date with the current speed of master, but still i'm going to
> >>>>> veto this with a
> >>>>> -1 for inclusion before we cut of the 4.2 branch.
> >>>>>>
> >>>>>> Cheers,
> >>>>>>
> >>>>>> Hugo
> >>>>>>
> >>>>>> On Jun 26, 2013, at 5:32 PM, Alex Huang <Al...@citrix.com>
> >> wrote:
> >>>>>>
> >>>>>>> Hi All,
> >>>>>>>
> >>>>>>> Kelven had an emergency so I'm submitting the changes on vmsync
> >>>>>>> for him. The patch are on https://reviews.apache.org/r/12126.
> >>>>>>>
> >>>>>>> Hugo took a look and already had some questions on why so many
> >>>>>>> files
> >>>>> were changed and added/deleted, So I like to explain a bit in this
> email.
> >>>>>>>
> >>>>>>> As part of the vmsync change, we try to move the files that were
> >>>>>>> relevant
> >>>>> to vm orchestration into the right places so that others can
> >>>>> properly view the different jars and understand the relationship
> >>>>> between the jars. This process is ongoing and will continue in
> >>>>> other
> >> changes.
> >>>>>>>
> >>>>>>> - Work related to jobs management have been put under cloud-
> >>>>> framework-jobs as handling jobs is not really part of our server
> >>>>> but is a library. By doing it this way, changes in it can be
> >>>>> properly reviewed and there's a good separation from things that
> >>>>> utilizes jobs and
> >> jobs itself.
> >>>>>>> - VirtualMachine orchestration has been moved from cloud-server
> >>>>>>> to
> >>>>> cloud-engine-orchestration. Cloud-engine-orchestration then only
> >>>>> depends on cloud-engine-schema and cloud-api. This creates a
> >>>>> clear compilation boundary between the self-service server
> >>>>> implementation and orchestration implementation.
> >>>>>>>
> >>>>>>> As part of these changes, then we surfaced many problems
> because
> >>>>>>> we
> >>>>> setup the proper compilation boundaries and fixed all of these
> >> problems.
> >>>>> For example, HostAllocator interface is something people can write
> >>>>> plugins for but it was buried inside cloud-server package. We've
> >>>>> moved a majority of these changes to cloud-api. There's quite a
> >>>>> bit of file movements based on this change. For vmsync changes
> >>>>> itself, the changes are centered around
> >>>>> VirtualMachineManagerImpl.java so
> >> you can review that file instead.
> >>>>>>>
> >>>>>>> Thanks.
> >>>>>>>
> >>>>>>> --Alex
> >>>>>>>
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
> >>>>>>>> Sent: Tuesday, June 18, 2013 3:38 PM
> >>>>>>>> To: dev@cloudstack.apache.org
> >>>>>>>> Subject: Re: [MERGE] Merge VMSync improvement branch into
> >> master
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 6/17/13 7:43 PM, "Mice Xia" <mi...@tcloudcomputing.com>
> >>>>> wrote:
> >>>>>>>>
> >>>>>>>>> Kelven,
> >>>>>>>>>
> >>>>>>>>> After the refactoring, will CS still restart HA enabled VM
> >>>>>>>>> when it is power off externally (e.g. using xencenter) or
> >>>>>>>>> internally (user initiated shutdown or crash)?
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> If HA functionality is provided by external manager (i.e.,
> >>>>>>>> vCenter), CS won't try to restart it, but if HA is provided by
> >>>>>>>> CloudStack, we will restore the legacy logic. The hook up with
> >>>>>>>> old HA manager (between
> >>>>>>>> HighAvailabilityManager/VirtualMachineManager) is in the
> >>>>>>>> wrap-up process
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Is seems the method HighAvailabilityManagerImpl
> >>>>>>>>> .scheduleRestart() is not called when VM's actual state is
> >>>>>>>>> stopped while expected state is
> >>>>>>>> running.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Regards
> >>>>>>>>> Mice
> >>>>>>>>>
> >>>>>>>>> -----Original Message-----
> >>>>>>>>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
> >>>>>>>>> Sent: Tuesday, June 18, 2013 5:21 AM
> >>>>>>>>> To: dev@cloudstack.apache.org
> >>>>>>>>> Subject: Re: [MERGE] Merge VMSync improvement branch into
> >> master
> >>>>>>>>>
> >>>>>>>>> Haven't created a patch yet, will do it soon after some last
> >>>>>>>>> wrap-
> >> ups.
> >>>>>>>>>
> >>>>>>>>> Kelven
> >>>>>>>>>
> >>>>>>>>> On 6/17/13 12:03 PM, "John Burwell" <jb...@basho.com>
> >> wrote:
> >>>>>>>>>
> >>>>>>>>>> Kelven,
> >>>>>>>>>>
> >>>>>>>>>> Did this patch get pushed to Review Board? If so, what is
> >>>>>>>>>> the
> >> URL?
> >>>>>>>>>>
> >>>>>>>>>> Thanks.
> >>>>>>>>>> -John
> >>>>>>>>>>
> >>>>>>>>>> On Jun 17, 2013, at 1:40 PM, Kelven Yang
> >>>>>>>>>> <ke...@citrix.com>
> >>>>> wrote:
> >>>>>>>>>>
> >>>>>>>>>>> Low level classes were tested in unit tests(MessageBus, Job
> >>>>>>>>>>> framework, Job dispatchers etc), interface layer changes
> >>>>>>>>>>> are guarded through matching the old semantics, but changes
> >>>>>>>>>>> are tested manually, we are planning to get this part of
> >>>>>>>>>>> testing through BVT system after we have re-based the latest
> master.
> >>>>>>>>>>>
> >>>>>>>>>>> Kelven
> >>>>>>>>>>>
> >>>>>>>>>>> On 6/17/13 10:01 AM, "Chip Childers"
> >>>>>>>>>>> <ch...@sungard.com>
> >>>>>>>> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>> On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang
> wrote:
> >>>>>>>>>>>>> I'd like to kick off the official merge process. We will
> >>>>>>>>>>>>> start the merge process after the branch has passed
> >>>>>>>>>>>>> necessary tests
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Kelven
> >>>>>>>>>>>>
> >>>>>>>>>>>> Can you share what testing is being run against the branch?
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>
> >>>>
> >>>
> >>
> >
> >
Re: [MERGE] Merge VMSync improvement branch into master
Posted by Hugo Trippaers <hu...@trippaers.nl>.
Let's get it done :-)
I'm at about 15% of the entire commit, reading through it line by line...
My notes so far (this is just a copy of my notepad fyi)
There are a lot of changes to BaseCmd and Async command. Do they impact the api in any way. I can't tell yet but this might impact api compatibility by fixing some of the current issues in the api. (Some external programs like libcloud and jclouds have work-arounds for issues with the current api)
Lot of cleanup done for spelling errors and re-ordering of imports. Typically something that we should push in as separate commits in master. No risk and they make the patch unnecessarily bloated.
UserContext has been replaced by CallContext and is used more consistently. GoodThing(™)
Netapp support enabled in the component context. Seems unrelated to this particular patch?
usageServerMonitor disabled in the component context. Also seems unrelated to this particular patch?
I'll be updating this with my progress.
Cheers,
Hugo
On Jun 27, 2013, at 4:05 PM, "Musayev, Ilya" <im...@webmd.net> wrote:
> Thanks Hugo,
>
> I'm all for it if John can help us with his -1 vote :)
>
>
>> -----Original Message-----
>> From: Trippie [mailto:trippie@gmail.com] On Behalf Of Hugo Trippaers
>> Sent: Thursday, June 27, 2013 5:51 PM
>> To: dev@cloudstack.apache.org
>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>>
>>
>> I think Ilya offers is great, my current stance is also to see how we can bring
>> this forward.
>>
>> I've had the opportunity to meet with several people at the Citrix office in
>> Santa Clara, i'm actually working from their office at this moment. I think it's
>> also the responsibility of someone who put in a -1 to work with the original
>> committer to get the situation resolved. So i'll invest the time to help with
>> the review as well.
>>
>> It would be great if Alex or Kelven could take the time to explain how this
>> feature has been tested. That would give the community some insight as
>> well.
>>
>> My main technical problem with this merge is that stuff is moving all over the
>> place without having even the slightest idea why. Now having discussed this
>> with Alex in person i get the general idea of this merge, so can actually try to
>> review it.
>>
>> I think that John have nicely explained what we could do to prevent
>> situations like this in advance. I fully understand that big features or rewrites
>> don't happen overnight and might show up near the end of the release cycle.
>> With the time based release cycle it's always a risk that some feature might
>> not make it in on time. Getting more people involved and chunking the
>> commits into master will greatly speed up the reviewing process.
>>
>> I'll get back to this after spending some time on reviewing the actual patch. In
>> fact i would like to ask more people to have a look at this patch and reply to
>> this thread with comments or remarks.
>>
>> Cheers,
>>
>> Hugo
>>
>>
>> On Jun 27, 2013, at 12:55 PM, John Burwell <jb...@basho.com> wrote:
>>
>>> Ilya,
>>>
>>> I understand your concern. However, this patch is large and complicated.
>> Without adequate review, we run that the risk getting it wrong which would
>> be worse than not having it. If you feel it is a must have, you can propose
>> extending the freeze to allow time to perform the review.
>>>
>>> Thanks,
>>> -John
>>>
>>> On Jun 27, 2013, at 3:49 PM, "Musayev, Ilya" <im...@webmd.net>
>> wrote:
>>>
>>>> John and Hugo,
>>>>
>>>> I see and understand your concerns, however, this feature is really
>> needed by many users - I've met with several large users at CCC13 who were
>> looking forward to this work, blocking this merge - would delay cloudstack
>> adoption and make ACS look inferior to other cloud platforms. Realistically,
>> this is going to put us back at least 8+ months away until 4.3 comes out.
>>>>
>>>> If needed, I can dedicate QA cycles and work with Kelven to rule out all
>> the bugs and issues - through as many scenarios as possible on my end.
>>>>
>>>> Thanks
>>>> ilya
>>>>
>>>>> -----Original Message-----
>>>>> From: John Burwell [mailto:jburwell@basho.com]
>>>>> Sent: Thursday, June 27, 2013 2:33 PM
>>>>> To: dev@cloudstack.apache.org
>>>>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>>>>>
>>>>> Hugo,
>>>>>
>>>>> I completely agree with this stance, and will add a -1 as well. It
>>>>> has been a significant challenge this cycle to complete high quality
>>>>> reviews due to the large patch size and short time turnaround time.
>>>>> Going forward, I am going to be more likely to follow Hugo's
>>>>> example, and -1 patches for which there is not adequate review time.
>>>>> I think the following techniques will help streamline the review process:
>>>>>
>>>>> 1. Narrow Patch Scope: Ensure that the patch is confined to a
>>>>> single enhancement or defect fix. If you doing refactoring as part
>>>>> of a feature, submit the refactoring work a separate commit. As a
>>>>> further suggestion, if you are performing multiple refactorings
>>>>> (e.g. package reorganization and utility method extraction), split each
>> refactoring into separate patches.
>>>>> 2. Chunk Features: For each large feature being developed, logically
>>>>> chunk the functionality and submit each chunk as a separate patch.
>>>>> I recommend erring on the side of chunks being too small. Let the
>>>>> reviewer determine if the patch is not large enough to be
>>>>> representative of the feature. The worst that will happen is that
>>>>> a reviewer will provide feedback on the work completed and simply
>>>>> ask for more work to be done before it can be merged to master. For
>>>>> features while chunks can't be merged all the way to master,
>>>>> intermediate patches can be submitted to Review Board for review
>>>>> throughout the cycle -- allowing large merges to actually be the
>> accumulation of several smaller reviews.
>>>>> 3. Identify Reviewers Early: For big features, solicit the reviewers
>>>>> whiling to review a feature from FS through merge from the mailing
>>>>> list. Reviews will go much more quickly if reviewers have been
>>>>> involved from the beginning because they will not help identify
>>>>> issues before coding starts, but also will not have to develop
>>>>> context late. To be clear, identifying reviewers early does not
>>>>> preclude a new reviewer from becoming involved later, but the
>>>>> reviews will greatly reduce the probability of a late review finding
>>>>> significant issues. Additionally, the early reviewers can help the late
>> reviewers come up to speed -- reducing the burden on the contributor.
>>>>>
>>>>> Generally, my recommendation is to submit patches early and often.
>>>>> Ultimately, contributors determine how they wish to develop and
>>>>> deliver their contributions. These are only my recommendations as a
>>>>> reviewer to increase the probability that a feature will make a particular
>> release train.
>>>>> Finally, it appears that reviewers are growing less and less
>>>>> tolerant of large patches that appear just before freeze dates, and
>>>>> those these patches face a much higher risk of being immediately
>>>>> receiving a -1 for the reasons stated above.
>>>>>
>>>>> Thanks,
>>>>> -John
>>>>>
>>>>> On Jun 26, 2013, at 8:45 PM, Hugo Trippaers <hu...@trippaers.nl> wrote:
>>>>>
>>>>>> Hey Alex, Kelven,
>>>>>>
>>>>>> I've been looking though the code and thanks for the very
>>>>>> insightfull
>>>>> conversation and follow-up email. With merges of this magnitude it's
>>>>> essential to help people understand what is going on.
>>>>>>
>>>>>> Purely technical this is a merge i'm really pleased with. It will
>>>>>> for sure
>>>>> improve our handling of virtual machines.
>>>>>>
>>>>>> However the timing of the merge request is not ideal. We are very
>>>>>> close to
>>>>> the end of the already extended feature freeze. We have a consensus
>>>>> within the dev community to do large architectural changes in the
>>>>> beginning of the cycle and not at the very end. This not only means
>>>>> a lot of extra testing effort and other developers will have to get
>>>>> used to the changes introduced right at the moment when everybody
>>>>> should be focussed on bug fixing, documentation and stability.
>>>>> Without wanting to rip open old wounds, i can imagine we all want to
>> avoid a javelin incident.
>>>>>>
>>>>>> I respect the work going into this and the effort it will take to
>>>>>> keep this up
>>>>> to date with the current speed of master, but still i'm going to
>>>>> veto this with a
>>>>> -1 for inclusion before we cut of the 4.2 branch.
>>>>>>
>>>>>> Cheers,
>>>>>>
>>>>>> Hugo
>>>>>>
>>>>>> On Jun 26, 2013, at 5:32 PM, Alex Huang <Al...@citrix.com>
>> wrote:
>>>>>>
>>>>>>> Hi All,
>>>>>>>
>>>>>>> Kelven had an emergency so I'm submitting the changes on vmsync
>>>>>>> for him. The patch are on https://reviews.apache.org/r/12126.
>>>>>>>
>>>>>>> Hugo took a look and already had some questions on why so many
>>>>>>> files
>>>>> were changed and added/deleted, So I like to explain a bit in this email.
>>>>>>>
>>>>>>> As part of the vmsync change, we try to move the files that were
>>>>>>> relevant
>>>>> to vm orchestration into the right places so that others can
>>>>> properly view the different jars and understand the relationship
>>>>> between the jars. This process is ongoing and will continue in other
>> changes.
>>>>>>>
>>>>>>> - Work related to jobs management have been put under cloud-
>>>>> framework-jobs as handling jobs is not really part of our server but
>>>>> is a library. By doing it this way, changes in it can be properly
>>>>> reviewed and there's a good separation from things that utilizes jobs and
>> jobs itself.
>>>>>>> - VirtualMachine orchestration has been moved from cloud-server to
>>>>> cloud-engine-orchestration. Cloud-engine-orchestration then only
>>>>> depends on cloud-engine-schema and cloud-api. This creates a clear
>>>>> compilation boundary between the self-service server implementation
>>>>> and orchestration implementation.
>>>>>>>
>>>>>>> As part of these changes, then we surfaced many problems because
>>>>>>> we
>>>>> setup the proper compilation boundaries and fixed all of these
>> problems.
>>>>> For example, HostAllocator interface is something people can write
>>>>> plugins for but it was buried inside cloud-server package. We've
>>>>> moved a majority of these changes to cloud-api. There's quite a bit
>>>>> of file movements based on this change. For vmsync changes itself,
>>>>> the changes are centered around VirtualMachineManagerImpl.java so
>> you can review that file instead.
>>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>>> --Alex
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
>>>>>>>> Sent: Tuesday, June 18, 2013 3:38 PM
>>>>>>>> To: dev@cloudstack.apache.org
>>>>>>>> Subject: Re: [MERGE] Merge VMSync improvement branch into
>> master
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 6/17/13 7:43 PM, "Mice Xia" <mi...@tcloudcomputing.com>
>>>>> wrote:
>>>>>>>>
>>>>>>>>> Kelven,
>>>>>>>>>
>>>>>>>>> After the refactoring, will CS still restart HA enabled VM when
>>>>>>>>> it is power off externally (e.g. using xencenter) or internally
>>>>>>>>> (user initiated shutdown or crash)?
>>>>>>>>
>>>>>>>>
>>>>>>>> If HA functionality is provided by external manager (i.e.,
>>>>>>>> vCenter), CS won't try to restart it, but if HA is provided by
>>>>>>>> CloudStack, we will restore the legacy logic. The hook up with
>>>>>>>> old HA manager (between
>>>>>>>> HighAvailabilityManager/VirtualMachineManager) is in the wrap-up
>>>>>>>> process
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Is seems the method HighAvailabilityManagerImpl
>>>>>>>>> .scheduleRestart() is not called when VM's actual state is
>>>>>>>>> stopped while expected state is
>>>>>>>> running.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Regards
>>>>>>>>> Mice
>>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
>>>>>>>>> Sent: Tuesday, June 18, 2013 5:21 AM
>>>>>>>>> To: dev@cloudstack.apache.org
>>>>>>>>> Subject: Re: [MERGE] Merge VMSync improvement branch into
>> master
>>>>>>>>>
>>>>>>>>> Haven't created a patch yet, will do it soon after some last wrap-
>> ups.
>>>>>>>>>
>>>>>>>>> Kelven
>>>>>>>>>
>>>>>>>>> On 6/17/13 12:03 PM, "John Burwell" <jb...@basho.com>
>> wrote:
>>>>>>>>>
>>>>>>>>>> Kelven,
>>>>>>>>>>
>>>>>>>>>> Did this patch get pushed to Review Board? If so, what is the
>> URL?
>>>>>>>>>>
>>>>>>>>>> Thanks.
>>>>>>>>>> -John
>>>>>>>>>>
>>>>>>>>>> On Jun 17, 2013, at 1:40 PM, Kelven Yang
>>>>>>>>>> <ke...@citrix.com>
>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Low level classes were tested in unit tests(MessageBus, Job
>>>>>>>>>>> framework, Job dispatchers etc), interface layer changes are
>>>>>>>>>>> guarded through matching the old semantics, but changes are
>>>>>>>>>>> tested manually, we are planning to get this part of testing
>>>>>>>>>>> through BVT system after we have re-based the latest master.
>>>>>>>>>>>
>>>>>>>>>>> Kelven
>>>>>>>>>>>
>>>>>>>>>>> On 6/17/13 10:01 AM, "Chip Childers"
>>>>>>>>>>> <ch...@sungard.com>
>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang wrote:
>>>>>>>>>>>>> I'd like to kick off the official merge process. We will
>>>>>>>>>>>>> start the merge process after the branch has passed
>>>>>>>>>>>>> necessary tests
>>>>>>>>>>>>>
>>>>>>>>>>>>> Kelven
>>>>>>>>>>>>
>>>>>>>>>>>> Can you share what testing is being run against the branch?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>>
>>>
>>
>
>
RE: [MERGE] Merge VMSync improvement branch into master
Posted by "Musayev, Ilya" <im...@webmd.net>.
Thanks Hugo,
I'm all for it if John can help us with his -1 vote :)
> -----Original Message-----
> From: Trippie [mailto:trippie@gmail.com] On Behalf Of Hugo Trippaers
> Sent: Thursday, June 27, 2013 5:51 PM
> To: dev@cloudstack.apache.org
> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>
>
> I think Ilya offers is great, my current stance is also to see how we can bring
> this forward.
>
> I've had the opportunity to meet with several people at the Citrix office in
> Santa Clara, i'm actually working from their office at this moment. I think it's
> also the responsibility of someone who put in a -1 to work with the original
> committer to get the situation resolved. So i'll invest the time to help with
> the review as well.
>
> It would be great if Alex or Kelven could take the time to explain how this
> feature has been tested. That would give the community some insight as
> well.
>
> My main technical problem with this merge is that stuff is moving all over the
> place without having even the slightest idea why. Now having discussed this
> with Alex in person i get the general idea of this merge, so can actually try to
> review it.
>
> I think that John have nicely explained what we could do to prevent
> situations like this in advance. I fully understand that big features or rewrites
> don't happen overnight and might show up near the end of the release cycle.
> With the time based release cycle it's always a risk that some feature might
> not make it in on time. Getting more people involved and chunking the
> commits into master will greatly speed up the reviewing process.
>
> I'll get back to this after spending some time on reviewing the actual patch. In
> fact i would like to ask more people to have a look at this patch and reply to
> this thread with comments or remarks.
>
> Cheers,
>
> Hugo
>
>
> On Jun 27, 2013, at 12:55 PM, John Burwell <jb...@basho.com> wrote:
>
> > Ilya,
> >
> > I understand your concern. However, this patch is large and complicated.
> Without adequate review, we run that the risk getting it wrong which would
> be worse than not having it. If you feel it is a must have, you can propose
> extending the freeze to allow time to perform the review.
> >
> > Thanks,
> > -John
> >
> > On Jun 27, 2013, at 3:49 PM, "Musayev, Ilya" <im...@webmd.net>
> wrote:
> >
> >> John and Hugo,
> >>
> >> I see and understand your concerns, however, this feature is really
> needed by many users - I've met with several large users at CCC13 who were
> looking forward to this work, blocking this merge - would delay cloudstack
> adoption and make ACS look inferior to other cloud platforms. Realistically,
> this is going to put us back at least 8+ months away until 4.3 comes out.
> >>
> >> If needed, I can dedicate QA cycles and work with Kelven to rule out all
> the bugs and issues - through as many scenarios as possible on my end.
> >>
> >> Thanks
> >> ilya
> >>
> >>> -----Original Message-----
> >>> From: John Burwell [mailto:jburwell@basho.com]
> >>> Sent: Thursday, June 27, 2013 2:33 PM
> >>> To: dev@cloudstack.apache.org
> >>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
> >>>
> >>> Hugo,
> >>>
> >>> I completely agree with this stance, and will add a -1 as well. It
> >>> has been a significant challenge this cycle to complete high quality
> >>> reviews due to the large patch size and short time turnaround time.
> >>> Going forward, I am going to be more likely to follow Hugo's
> >>> example, and -1 patches for which there is not adequate review time.
> >>> I think the following techniques will help streamline the review process:
> >>>
> >>> 1. Narrow Patch Scope: Ensure that the patch is confined to a
> >>> single enhancement or defect fix. If you doing refactoring as part
> >>> of a feature, submit the refactoring work a separate commit. As a
> >>> further suggestion, if you are performing multiple refactorings
> >>> (e.g. package reorganization and utility method extraction), split each
> refactoring into separate patches.
> >>> 2. Chunk Features: For each large feature being developed, logically
> >>> chunk the functionality and submit each chunk as a separate patch.
> >>> I recommend erring on the side of chunks being too small. Let the
> >>> reviewer determine if the patch is not large enough to be
> >>> representative of the feature. The worst that will happen is that
> >>> a reviewer will provide feedback on the work completed and simply
> >>> ask for more work to be done before it can be merged to master. For
> >>> features while chunks can't be merged all the way to master,
> >>> intermediate patches can be submitted to Review Board for review
> >>> throughout the cycle -- allowing large merges to actually be the
> accumulation of several smaller reviews.
> >>> 3. Identify Reviewers Early: For big features, solicit the reviewers
> >>> whiling to review a feature from FS through merge from the mailing
> >>> list. Reviews will go much more quickly if reviewers have been
> >>> involved from the beginning because they will not help identify
> >>> issues before coding starts, but also will not have to develop
> >>> context late. To be clear, identifying reviewers early does not
> >>> preclude a new reviewer from becoming involved later, but the
> >>> reviews will greatly reduce the probability of a late review finding
> >>> significant issues. Additionally, the early reviewers can help the late
> reviewers come up to speed -- reducing the burden on the contributor.
> >>>
> >>> Generally, my recommendation is to submit patches early and often.
> >>> Ultimately, contributors determine how they wish to develop and
> >>> deliver their contributions. These are only my recommendations as a
> >>> reviewer to increase the probability that a feature will make a particular
> release train.
> >>> Finally, it appears that reviewers are growing less and less
> >>> tolerant of large patches that appear just before freeze dates, and
> >>> those these patches face a much higher risk of being immediately
> >>> receiving a -1 for the reasons stated above.
> >>>
> >>> Thanks,
> >>> -John
> >>>
> >>> On Jun 26, 2013, at 8:45 PM, Hugo Trippaers <hu...@trippaers.nl> wrote:
> >>>
> >>>> Hey Alex, Kelven,
> >>>>
> >>>> I've been looking though the code and thanks for the very
> >>>> insightfull
> >>> conversation and follow-up email. With merges of this magnitude it's
> >>> essential to help people understand what is going on.
> >>>>
> >>>> Purely technical this is a merge i'm really pleased with. It will
> >>>> for sure
> >>> improve our handling of virtual machines.
> >>>>
> >>>> However the timing of the merge request is not ideal. We are very
> >>>> close to
> >>> the end of the already extended feature freeze. We have a consensus
> >>> within the dev community to do large architectural changes in the
> >>> beginning of the cycle and not at the very end. This not only means
> >>> a lot of extra testing effort and other developers will have to get
> >>> used to the changes introduced right at the moment when everybody
> >>> should be focussed on bug fixing, documentation and stability.
> >>> Without wanting to rip open old wounds, i can imagine we all want to
> avoid a javelin incident.
> >>>>
> >>>> I respect the work going into this and the effort it will take to
> >>>> keep this up
> >>> to date with the current speed of master, but still i'm going to
> >>> veto this with a
> >>> -1 for inclusion before we cut of the 4.2 branch.
> >>>>
> >>>> Cheers,
> >>>>
> >>>> Hugo
> >>>>
> >>>> On Jun 26, 2013, at 5:32 PM, Alex Huang <Al...@citrix.com>
> wrote:
> >>>>
> >>>>> Hi All,
> >>>>>
> >>>>> Kelven had an emergency so I'm submitting the changes on vmsync
> >>>>> for him. The patch are on https://reviews.apache.org/r/12126.
> >>>>>
> >>>>> Hugo took a look and already had some questions on why so many
> >>>>> files
> >>> were changed and added/deleted, So I like to explain a bit in this email.
> >>>>>
> >>>>> As part of the vmsync change, we try to move the files that were
> >>>>> relevant
> >>> to vm orchestration into the right places so that others can
> >>> properly view the different jars and understand the relationship
> >>> between the jars. This process is ongoing and will continue in other
> changes.
> >>>>>
> >>>>> - Work related to jobs management have been put under cloud-
> >>> framework-jobs as handling jobs is not really part of our server but
> >>> is a library. By doing it this way, changes in it can be properly
> >>> reviewed and there's a good separation from things that utilizes jobs and
> jobs itself.
> >>>>> - VirtualMachine orchestration has been moved from cloud-server to
> >>> cloud-engine-orchestration. Cloud-engine-orchestration then only
> >>> depends on cloud-engine-schema and cloud-api. This creates a clear
> >>> compilation boundary between the self-service server implementation
> >>> and orchestration implementation.
> >>>>>
> >>>>> As part of these changes, then we surfaced many problems because
> >>>>> we
> >>> setup the proper compilation boundaries and fixed all of these
> problems.
> >>> For example, HostAllocator interface is something people can write
> >>> plugins for but it was buried inside cloud-server package. We've
> >>> moved a majority of these changes to cloud-api. There's quite a bit
> >>> of file movements based on this change. For vmsync changes itself,
> >>> the changes are centered around VirtualMachineManagerImpl.java so
> you can review that file instead.
> >>>>>
> >>>>> Thanks.
> >>>>>
> >>>>> --Alex
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
> >>>>>> Sent: Tuesday, June 18, 2013 3:38 PM
> >>>>>> To: dev@cloudstack.apache.org
> >>>>>> Subject: Re: [MERGE] Merge VMSync improvement branch into
> master
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 6/17/13 7:43 PM, "Mice Xia" <mi...@tcloudcomputing.com>
> >>> wrote:
> >>>>>>
> >>>>>>> Kelven,
> >>>>>>>
> >>>>>>> After the refactoring, will CS still restart HA enabled VM when
> >>>>>>> it is power off externally (e.g. using xencenter) or internally
> >>>>>>> (user initiated shutdown or crash)?
> >>>>>>
> >>>>>>
> >>>>>> If HA functionality is provided by external manager (i.e.,
> >>>>>> vCenter), CS won't try to restart it, but if HA is provided by
> >>>>>> CloudStack, we will restore the legacy logic. The hook up with
> >>>>>> old HA manager (between
> >>>>>> HighAvailabilityManager/VirtualMachineManager) is in the wrap-up
> >>>>>> process
> >>>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> Is seems the method HighAvailabilityManagerImpl
> >>>>>>> .scheduleRestart() is not called when VM's actual state is
> >>>>>>> stopped while expected state is
> >>>>>> running.
> >>>>>>>
> >>>>>>>
> >>>>>>> Regards
> >>>>>>> Mice
> >>>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
> >>>>>>> Sent: Tuesday, June 18, 2013 5:21 AM
> >>>>>>> To: dev@cloudstack.apache.org
> >>>>>>> Subject: Re: [MERGE] Merge VMSync improvement branch into
> master
> >>>>>>>
> >>>>>>> Haven't created a patch yet, will do it soon after some last wrap-
> ups.
> >>>>>>>
> >>>>>>> Kelven
> >>>>>>>
> >>>>>>> On 6/17/13 12:03 PM, "John Burwell" <jb...@basho.com>
> wrote:
> >>>>>>>
> >>>>>>>> Kelven,
> >>>>>>>>
> >>>>>>>> Did this patch get pushed to Review Board? If so, what is the
> URL?
> >>>>>>>>
> >>>>>>>> Thanks.
> >>>>>>>> -John
> >>>>>>>>
> >>>>>>>> On Jun 17, 2013, at 1:40 PM, Kelven Yang
> >>>>>>>> <ke...@citrix.com>
> >>> wrote:
> >>>>>>>>
> >>>>>>>>> Low level classes were tested in unit tests(MessageBus, Job
> >>>>>>>>> framework, Job dispatchers etc), interface layer changes are
> >>>>>>>>> guarded through matching the old semantics, but changes are
> >>>>>>>>> tested manually, we are planning to get this part of testing
> >>>>>>>>> through BVT system after we have re-based the latest master.
> >>>>>>>>>
> >>>>>>>>> Kelven
> >>>>>>>>>
> >>>>>>>>> On 6/17/13 10:01 AM, "Chip Childers"
> >>>>>>>>> <ch...@sungard.com>
> >>>>>> wrote:
> >>>>>>>>>
> >>>>>>>>>> On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang wrote:
> >>>>>>>>>>> I'd like to kick off the official merge process. We will
> >>>>>>>>>>> start the merge process after the branch has passed
> >>>>>>>>>>> necessary tests
> >>>>>>>>>>>
> >>>>>>>>>>> Kelven
> >>>>>>>>>>
> >>>>>>>>>> Can you share what testing is being run against the branch?
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>
> >>>>
> >>
> >>
> >
>
Re: [MERGE] Merge VMSync improvement branch into master
Posted by Hugo Trippaers <hu...@trippaers.nl>.
I think Ilya offers is great, my current stance is also to see how we can bring this forward.
I've had the opportunity to meet with several people at the Citrix office in Santa Clara, i'm actually working from their office at this moment. I think it's also the responsibility of someone who put in a -1 to work with the original committer to get the situation resolved. So i'll invest the time to help with the review as well.
It would be great if Alex or Kelven could take the time to explain how this feature has been tested. That would give the community some insight as well.
My main technical problem with this merge is that stuff is moving all over the place without having even the slightest idea why. Now having discussed this with Alex in person i get the general idea of this merge, so can actually try to review it.
I think that John have nicely explained what we could do to prevent situations like this in advance. I fully understand that big features or rewrites don't happen overnight and might show up near the end of the release cycle. With the time based release cycle it's always a risk that some feature might not make it in on time. Getting more people involved and chunking the commits into master will greatly speed up the reviewing process.
I'll get back to this after spending some time on reviewing the actual patch. In fact i would like to ask more people to have a look at this patch and reply to this thread with comments or remarks.
Cheers,
Hugo
On Jun 27, 2013, at 12:55 PM, John Burwell <jb...@basho.com> wrote:
> Ilya,
>
> I understand your concern. However, this patch is large and complicated. Without adequate review, we run that the risk getting it wrong which would be worse than not having it. If you feel it is a must have, you can propose extending the freeze to allow time to perform the review.
>
> Thanks,
> -John
>
> On Jun 27, 2013, at 3:49 PM, "Musayev, Ilya" <im...@webmd.net> wrote:
>
>> John and Hugo,
>>
>> I see and understand your concerns, however, this feature is really needed by many users - I've met with several large users at CCC13 who were looking forward to this work, blocking this merge - would delay cloudstack adoption and make ACS look inferior to other cloud platforms. Realistically, this is going to put us back at least 8+ months away until 4.3 comes out.
>>
>> If needed, I can dedicate QA cycles and work with Kelven to rule out all the bugs and issues - through as many scenarios as possible on my end.
>>
>> Thanks
>> ilya
>>
>>> -----Original Message-----
>>> From: John Burwell [mailto:jburwell@basho.com]
>>> Sent: Thursday, June 27, 2013 2:33 PM
>>> To: dev@cloudstack.apache.org
>>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>>>
>>> Hugo,
>>>
>>> I completely agree with this stance, and will add a -1 as well. It has been a
>>> significant challenge this cycle to complete high quality reviews due to the
>>> large patch size and short time turnaround time. Going forward, I am going
>>> to be more likely to follow Hugo's example, and -1 patches for which there is
>>> not adequate review time. I think the following techniques will help
>>> streamline the review process:
>>>
>>> 1. Narrow Patch Scope: Ensure that the patch is confined to a single
>>> enhancement or defect fix. If you doing refactoring as part of a feature,
>>> submit the refactoring work a separate commit. As a further suggestion, if
>>> you are performing multiple refactorings (e.g. package reorganization and
>>> utility method extraction), split each refactoring into separate patches.
>>> 2. Chunk Features: For each large feature being developed, logically chunk
>>> the functionality and submit each chunk as a separate patch. I recommend
>>> erring on the side of chunks being too small. Let the reviewer determine if
>>> the patch is not large enough to be representative of the feature. The worst
>>> that will happen is that a reviewer will provide feedback on the work
>>> completed and simply ask for more work to be done before it can be merged
>>> to master. For features while chunks can't be merged all the way to master,
>>> intermediate patches can be submitted to Review Board for review
>>> throughout the cycle -- allowing large merges to actually be the accumulation
>>> of several smaller reviews.
>>> 3. Identify Reviewers Early: For big features, solicit the reviewers whiling to
>>> review a feature from FS through merge from the mailing list. Reviews will
>>> go much more quickly if reviewers have been involved from the beginning
>>> because they will not help identify issues before coding starts, but also will
>>> not have to develop context late. To be clear, identifying reviewers early
>>> does not preclude a new reviewer from becoming involved later, but the
>>> reviews will greatly reduce the probability of a late review finding significant
>>> issues. Additionally, the early reviewers can help the late reviewers come up
>>> to speed -- reducing the burden on the contributor.
>>>
>>> Generally, my recommendation is to submit patches early and often.
>>> Ultimately, contributors determine how they wish to develop and deliver
>>> their contributions. These are only my recommendations as a reviewer to
>>> increase the probability that a feature will make a particular release train.
>>> Finally, it appears that reviewers are growing less and less tolerant of large
>>> patches that appear just before freeze dates, and those these patches face a
>>> much higher risk of being immediately receiving a -1 for the reasons stated
>>> above.
>>>
>>> Thanks,
>>> -John
>>>
>>> On Jun 26, 2013, at 8:45 PM, Hugo Trippaers <hu...@trippaers.nl> wrote:
>>>
>>>> Hey Alex, Kelven,
>>>>
>>>> I've been looking though the code and thanks for the very insightfull
>>> conversation and follow-up email. With merges of this magnitude it's
>>> essential to help people understand what is going on.
>>>>
>>>> Purely technical this is a merge i'm really pleased with. It will for sure
>>> improve our handling of virtual machines.
>>>>
>>>> However the timing of the merge request is not ideal. We are very close to
>>> the end of the already extended feature freeze. We have a consensus within
>>> the dev community to do large architectural changes in the beginning of the
>>> cycle and not at the very end. This not only means a lot of extra testing effort
>>> and other developers will have to get used to the changes introduced right at
>>> the moment when everybody should be focussed on bug fixing,
>>> documentation and stability. Without wanting to rip open old wounds, i can
>>> imagine we all want to avoid a javelin incident.
>>>>
>>>> I respect the work going into this and the effort it will take to keep this up
>>> to date with the current speed of master, but still i'm going to veto this with a
>>> -1 for inclusion before we cut of the 4.2 branch.
>>>>
>>>> Cheers,
>>>>
>>>> Hugo
>>>>
>>>> On Jun 26, 2013, at 5:32 PM, Alex Huang <Al...@citrix.com> wrote:
>>>>
>>>>> Hi All,
>>>>>
>>>>> Kelven had an emergency so I'm submitting the changes on vmsync for
>>>>> him. The patch are on https://reviews.apache.org/r/12126.
>>>>>
>>>>> Hugo took a look and already had some questions on why so many files
>>> were changed and added/deleted, So I like to explain a bit in this email.
>>>>>
>>>>> As part of the vmsync change, we try to move the files that were relevant
>>> to vm orchestration into the right places so that others can properly view the
>>> different jars and understand the relationship between the jars. This
>>> process is ongoing and will continue in other changes.
>>>>>
>>>>> - Work related to jobs management have been put under cloud-
>>> framework-jobs as handling jobs is not really part of our server but is a
>>> library. By doing it this way, changes in it can be properly reviewed and
>>> there's a good separation from things that utilizes jobs and jobs itself.
>>>>> - VirtualMachine orchestration has been moved from cloud-server to
>>> cloud-engine-orchestration. Cloud-engine-orchestration then only depends
>>> on cloud-engine-schema and cloud-api. This creates a clear compilation
>>> boundary between the self-service server implementation and orchestration
>>> implementation.
>>>>>
>>>>> As part of these changes, then we surfaced many problems because we
>>> setup the proper compilation boundaries and fixed all of these problems.
>>> For example, HostAllocator interface is something people can write plugins
>>> for but it was buried inside cloud-server package. We've moved a majority of
>>> these changes to cloud-api. There's quite a bit of file movements based on
>>> this change. For vmsync changes itself, the changes are centered around
>>> VirtualMachineManagerImpl.java so you can review that file instead.
>>>>>
>>>>> Thanks.
>>>>>
>>>>> --Alex
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
>>>>>> Sent: Tuesday, June 18, 2013 3:38 PM
>>>>>> To: dev@cloudstack.apache.org
>>>>>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 6/17/13 7:43 PM, "Mice Xia" <mi...@tcloudcomputing.com>
>>> wrote:
>>>>>>
>>>>>>> Kelven,
>>>>>>>
>>>>>>> After the refactoring, will CS still restart HA enabled VM when it
>>>>>>> is power off externally (e.g. using xencenter) or internally (user
>>>>>>> initiated shutdown or crash)?
>>>>>>
>>>>>>
>>>>>> If HA functionality is provided by external manager (i.e., vCenter),
>>>>>> CS won't try to restart it, but if HA is provided by CloudStack, we
>>>>>> will restore the legacy logic. The hook up with old HA manager
>>>>>> (between
>>>>>> HighAvailabilityManager/VirtualMachineManager) is in the wrap-up
>>>>>> process
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Is seems the method HighAvailabilityManagerImpl .scheduleRestart()
>>>>>>> is not called when VM's actual state is stopped while expected
>>>>>>> state is
>>>>>> running.
>>>>>>>
>>>>>>>
>>>>>>> Regards
>>>>>>> Mice
>>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
>>>>>>> Sent: Tuesday, June 18, 2013 5:21 AM
>>>>>>> To: dev@cloudstack.apache.org
>>>>>>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>>>>>>>
>>>>>>> Haven't created a patch yet, will do it soon after some last wrap-ups.
>>>>>>>
>>>>>>> Kelven
>>>>>>>
>>>>>>> On 6/17/13 12:03 PM, "John Burwell" <jb...@basho.com> wrote:
>>>>>>>
>>>>>>>> Kelven,
>>>>>>>>
>>>>>>>> Did this patch get pushed to Review Board? If so, what is the URL?
>>>>>>>>
>>>>>>>> Thanks.
>>>>>>>> -John
>>>>>>>>
>>>>>>>> On Jun 17, 2013, at 1:40 PM, Kelven Yang <ke...@citrix.com>
>>> wrote:
>>>>>>>>
>>>>>>>>> Low level classes were tested in unit tests(MessageBus, Job
>>>>>>>>> framework, Job dispatchers etc), interface layer changes are
>>>>>>>>> guarded through matching the old semantics, but changes are
>>>>>>>>> tested manually, we are planning to get this part of testing
>>>>>>>>> through BVT system after we have re-based the latest master.
>>>>>>>>>
>>>>>>>>> Kelven
>>>>>>>>>
>>>>>>>>> On 6/17/13 10:01 AM, "Chip Childers" <ch...@sungard.com>
>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang wrote:
>>>>>>>>>>> I'd like to kick off the official merge process. We will start
>>>>>>>>>>> the merge process after the branch has passed necessary tests
>>>>>>>>>>>
>>>>>>>>>>> Kelven
>>>>>>>>>>
>>>>>>>>>> Can you share what testing is being run against the branch?
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>
>>
>
Re: [MERGE] Merge VMSync improvement branch into master
Posted by John Burwell <jb...@basho.com>.
Ilya,
I understand your concern. However, this patch is large and complicated. Without adequate review, we run that the risk getting it wrong which would be worse than not having it. If you feel it is a must have, you can propose extending the freeze to allow time to perform the review.
Thanks,
-John
On Jun 27, 2013, at 3:49 PM, "Musayev, Ilya" <im...@webmd.net> wrote:
> John and Hugo,
>
> I see and understand your concerns, however, this feature is really needed by many users - I've met with several large users at CCC13 who were looking forward to this work, blocking this merge - would delay cloudstack adoption and make ACS look inferior to other cloud platforms. Realistically, this is going to put us back at least 8+ months away until 4.3 comes out.
>
> If needed, I can dedicate QA cycles and work with Kelven to rule out all the bugs and issues - through as many scenarios as possible on my end.
>
> Thanks
> ilya
>
>> -----Original Message-----
>> From: John Burwell [mailto:jburwell@basho.com]
>> Sent: Thursday, June 27, 2013 2:33 PM
>> To: dev@cloudstack.apache.org
>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>>
>> Hugo,
>>
>> I completely agree with this stance, and will add a -1 as well. It has been a
>> significant challenge this cycle to complete high quality reviews due to the
>> large patch size and short time turnaround time. Going forward, I am going
>> to be more likely to follow Hugo's example, and -1 patches for which there is
>> not adequate review time. I think the following techniques will help
>> streamline the review process:
>>
>> 1. Narrow Patch Scope: Ensure that the patch is confined to a single
>> enhancement or defect fix. If you doing refactoring as part of a feature,
>> submit the refactoring work a separate commit. As a further suggestion, if
>> you are performing multiple refactorings (e.g. package reorganization and
>> utility method extraction), split each refactoring into separate patches.
>> 2. Chunk Features: For each large feature being developed, logically chunk
>> the functionality and submit each chunk as a separate patch. I recommend
>> erring on the side of chunks being too small. Let the reviewer determine if
>> the patch is not large enough to be representative of the feature. The worst
>> that will happen is that a reviewer will provide feedback on the work
>> completed and simply ask for more work to be done before it can be merged
>> to master. For features while chunks can't be merged all the way to master,
>> intermediate patches can be submitted to Review Board for review
>> throughout the cycle -- allowing large merges to actually be the accumulation
>> of several smaller reviews.
>> 3. Identify Reviewers Early: For big features, solicit the reviewers whiling to
>> review a feature from FS through merge from the mailing list. Reviews will
>> go much more quickly if reviewers have been involved from the beginning
>> because they will not help identify issues before coding starts, but also will
>> not have to develop context late. To be clear, identifying reviewers early
>> does not preclude a new reviewer from becoming involved later, but the
>> reviews will greatly reduce the probability of a late review finding significant
>> issues. Additionally, the early reviewers can help the late reviewers come up
>> to speed -- reducing the burden on the contributor.
>>
>> Generally, my recommendation is to submit patches early and often.
>> Ultimately, contributors determine how they wish to develop and deliver
>> their contributions. These are only my recommendations as a reviewer to
>> increase the probability that a feature will make a particular release train.
>> Finally, it appears that reviewers are growing less and less tolerant of large
>> patches that appear just before freeze dates, and those these patches face a
>> much higher risk of being immediately receiving a -1 for the reasons stated
>> above.
>>
>> Thanks,
>> -John
>>
>> On Jun 26, 2013, at 8:45 PM, Hugo Trippaers <hu...@trippaers.nl> wrote:
>>
>>> Hey Alex, Kelven,
>>>
>>> I've been looking though the code and thanks for the very insightfull
>> conversation and follow-up email. With merges of this magnitude it's
>> essential to help people understand what is going on.
>>>
>>> Purely technical this is a merge i'm really pleased with. It will for sure
>> improve our handling of virtual machines.
>>>
>>> However the timing of the merge request is not ideal. We are very close to
>> the end of the already extended feature freeze. We have a consensus within
>> the dev community to do large architectural changes in the beginning of the
>> cycle and not at the very end. This not only means a lot of extra testing effort
>> and other developers will have to get used to the changes introduced right at
>> the moment when everybody should be focussed on bug fixing,
>> documentation and stability. Without wanting to rip open old wounds, i can
>> imagine we all want to avoid a javelin incident.
>>>
>>> I respect the work going into this and the effort it will take to keep this up
>> to date with the current speed of master, but still i'm going to veto this with a
>> -1 for inclusion before we cut of the 4.2 branch.
>>>
>>> Cheers,
>>>
>>> Hugo
>>>
>>> On Jun 26, 2013, at 5:32 PM, Alex Huang <Al...@citrix.com> wrote:
>>>
>>>> Hi All,
>>>>
>>>> Kelven had an emergency so I'm submitting the changes on vmsync for
>>>> him. The patch are on https://reviews.apache.org/r/12126.
>>>>
>>>> Hugo took a look and already had some questions on why so many files
>> were changed and added/deleted, So I like to explain a bit in this email.
>>>>
>>>> As part of the vmsync change, we try to move the files that were relevant
>> to vm orchestration into the right places so that others can properly view the
>> different jars and understand the relationship between the jars. This
>> process is ongoing and will continue in other changes.
>>>>
>>>> - Work related to jobs management have been put under cloud-
>> framework-jobs as handling jobs is not really part of our server but is a
>> library. By doing it this way, changes in it can be properly reviewed and
>> there's a good separation from things that utilizes jobs and jobs itself.
>>>> - VirtualMachine orchestration has been moved from cloud-server to
>> cloud-engine-orchestration. Cloud-engine-orchestration then only depends
>> on cloud-engine-schema and cloud-api. This creates a clear compilation
>> boundary between the self-service server implementation and orchestration
>> implementation.
>>>>
>>>> As part of these changes, then we surfaced many problems because we
>> setup the proper compilation boundaries and fixed all of these problems.
>> For example, HostAllocator interface is something people can write plugins
>> for but it was buried inside cloud-server package. We've moved a majority of
>> these changes to cloud-api. There's quite a bit of file movements based on
>> this change. For vmsync changes itself, the changes are centered around
>> VirtualMachineManagerImpl.java so you can review that file instead.
>>>>
>>>> Thanks.
>>>>
>>>> --Alex
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
>>>>> Sent: Tuesday, June 18, 2013 3:38 PM
>>>>> To: dev@cloudstack.apache.org
>>>>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>>>>>
>>>>>
>>>>>
>>>>> On 6/17/13 7:43 PM, "Mice Xia" <mi...@tcloudcomputing.com>
>> wrote:
>>>>>
>>>>>> Kelven,
>>>>>>
>>>>>> After the refactoring, will CS still restart HA enabled VM when it
>>>>>> is power off externally (e.g. using xencenter) or internally (user
>>>>>> initiated shutdown or crash)?
>>>>>
>>>>>
>>>>> If HA functionality is provided by external manager (i.e., vCenter),
>>>>> CS won't try to restart it, but if HA is provided by CloudStack, we
>>>>> will restore the legacy logic. The hook up with old HA manager
>>>>> (between
>>>>> HighAvailabilityManager/VirtualMachineManager) is in the wrap-up
>>>>> process
>>>>>
>>>>>
>>>>>>
>>>>>> Is seems the method HighAvailabilityManagerImpl .scheduleRestart()
>>>>>> is not called when VM's actual state is stopped while expected
>>>>>> state is
>>>>> running.
>>>>>>
>>>>>>
>>>>>> Regards
>>>>>> Mice
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
>>>>>> Sent: Tuesday, June 18, 2013 5:21 AM
>>>>>> To: dev@cloudstack.apache.org
>>>>>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>>>>>>
>>>>>> Haven't created a patch yet, will do it soon after some last wrap-ups.
>>>>>>
>>>>>> Kelven
>>>>>>
>>>>>> On 6/17/13 12:03 PM, "John Burwell" <jb...@basho.com> wrote:
>>>>>>
>>>>>>> Kelven,
>>>>>>>
>>>>>>> Did this patch get pushed to Review Board? If so, what is the URL?
>>>>>>>
>>>>>>> Thanks.
>>>>>>> -John
>>>>>>>
>>>>>>> On Jun 17, 2013, at 1:40 PM, Kelven Yang <ke...@citrix.com>
>> wrote:
>>>>>>>
>>>>>>>> Low level classes were tested in unit tests(MessageBus, Job
>>>>>>>> framework, Job dispatchers etc), interface layer changes are
>>>>>>>> guarded through matching the old semantics, but changes are
>>>>>>>> tested manually, we are planning to get this part of testing
>>>>>>>> through BVT system after we have re-based the latest master.
>>>>>>>>
>>>>>>>> Kelven
>>>>>>>>
>>>>>>>> On 6/17/13 10:01 AM, "Chip Childers" <ch...@sungard.com>
>>>>> wrote:
>>>>>>>>
>>>>>>>>> On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang wrote:
>>>>>>>>>> I'd like to kick off the official merge process. We will start
>>>>>>>>>> the merge process after the branch has passed necessary tests
>>>>>>>>>>
>>>>>>>>>> Kelven
>>>>>>>>>
>>>>>>>>> Can you share what testing is being run against the branch?
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>
>
>
RE: [MERGE] Merge VMSync improvement branch into master
Posted by "Musayev, Ilya" <im...@webmd.net>.
John and Hugo,
I see and understand your concerns, however, this feature is really needed by many users - I've met with several large users at CCC13 who were looking forward to this work, blocking this merge - would delay cloudstack adoption and make ACS look inferior to other cloud platforms. Realistically, this is going to put us back at least 8+ months away until 4.3 comes out.
If needed, I can dedicate QA cycles and work with Kelven to rule out all the bugs and issues - through as many scenarios as possible on my end.
Thanks
ilya
> -----Original Message-----
> From: John Burwell [mailto:jburwell@basho.com]
> Sent: Thursday, June 27, 2013 2:33 PM
> To: dev@cloudstack.apache.org
> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>
> Hugo,
>
> I completely agree with this stance, and will add a -1 as well. It has been a
> significant challenge this cycle to complete high quality reviews due to the
> large patch size and short time turnaround time. Going forward, I am going
> to be more likely to follow Hugo's example, and -1 patches for which there is
> not adequate review time. I think the following techniques will help
> streamline the review process:
>
> 1. Narrow Patch Scope: Ensure that the patch is confined to a single
> enhancement or defect fix. If you doing refactoring as part of a feature,
> submit the refactoring work a separate commit. As a further suggestion, if
> you are performing multiple refactorings (e.g. package reorganization and
> utility method extraction), split each refactoring into separate patches.
> 2. Chunk Features: For each large feature being developed, logically chunk
> the functionality and submit each chunk as a separate patch. I recommend
> erring on the side of chunks being too small. Let the reviewer determine if
> the patch is not large enough to be representative of the feature. The worst
> that will happen is that a reviewer will provide feedback on the work
> completed and simply ask for more work to be done before it can be merged
> to master. For features while chunks can't be merged all the way to master,
> intermediate patches can be submitted to Review Board for review
> throughout the cycle -- allowing large merges to actually be the accumulation
> of several smaller reviews.
> 3. Identify Reviewers Early: For big features, solicit the reviewers whiling to
> review a feature from FS through merge from the mailing list. Reviews will
> go much more quickly if reviewers have been involved from the beginning
> because they will not help identify issues before coding starts, but also will
> not have to develop context late. To be clear, identifying reviewers early
> does not preclude a new reviewer from becoming involved later, but the
> reviews will greatly reduce the probability of a late review finding significant
> issues. Additionally, the early reviewers can help the late reviewers come up
> to speed -- reducing the burden on the contributor.
>
> Generally, my recommendation is to submit patches early and often.
> Ultimately, contributors determine how they wish to develop and deliver
> their contributions. These are only my recommendations as a reviewer to
> increase the probability that a feature will make a particular release train.
> Finally, it appears that reviewers are growing less and less tolerant of large
> patches that appear just before freeze dates, and those these patches face a
> much higher risk of being immediately receiving a -1 for the reasons stated
> above.
>
> Thanks,
> -John
>
> On Jun 26, 2013, at 8:45 PM, Hugo Trippaers <hu...@trippaers.nl> wrote:
>
> > Hey Alex, Kelven,
> >
> > I've been looking though the code and thanks for the very insightfull
> conversation and follow-up email. With merges of this magnitude it's
> essential to help people understand what is going on.
> >
> > Purely technical this is a merge i'm really pleased with. It will for sure
> improve our handling of virtual machines.
> >
> > However the timing of the merge request is not ideal. We are very close to
> the end of the already extended feature freeze. We have a consensus within
> the dev community to do large architectural changes in the beginning of the
> cycle and not at the very end. This not only means a lot of extra testing effort
> and other developers will have to get used to the changes introduced right at
> the moment when everybody should be focussed on bug fixing,
> documentation and stability. Without wanting to rip open old wounds, i can
> imagine we all want to avoid a javelin incident.
> >
> > I respect the work going into this and the effort it will take to keep this up
> to date with the current speed of master, but still i'm going to veto this with a
> -1 for inclusion before we cut of the 4.2 branch.
> >
> > Cheers,
> >
> > Hugo
> >
> > On Jun 26, 2013, at 5:32 PM, Alex Huang <Al...@citrix.com> wrote:
> >
> >> Hi All,
> >>
> >> Kelven had an emergency so I'm submitting the changes on vmsync for
> >> him. The patch are on https://reviews.apache.org/r/12126.
> >>
> >> Hugo took a look and already had some questions on why so many files
> were changed and added/deleted, So I like to explain a bit in this email.
> >>
> >> As part of the vmsync change, we try to move the files that were relevant
> to vm orchestration into the right places so that others can properly view the
> different jars and understand the relationship between the jars. This
> process is ongoing and will continue in other changes.
> >>
> >> - Work related to jobs management have been put under cloud-
> framework-jobs as handling jobs is not really part of our server but is a
> library. By doing it this way, changes in it can be properly reviewed and
> there's a good separation from things that utilizes jobs and jobs itself.
> >> - VirtualMachine orchestration has been moved from cloud-server to
> cloud-engine-orchestration. Cloud-engine-orchestration then only depends
> on cloud-engine-schema and cloud-api. This creates a clear compilation
> boundary between the self-service server implementation and orchestration
> implementation.
> >>
> >> As part of these changes, then we surfaced many problems because we
> setup the proper compilation boundaries and fixed all of these problems.
> For example, HostAllocator interface is something people can write plugins
> for but it was buried inside cloud-server package. We've moved a majority of
> these changes to cloud-api. There's quite a bit of file movements based on
> this change. For vmsync changes itself, the changes are centered around
> VirtualMachineManagerImpl.java so you can review that file instead.
> >>
> >> Thanks.
> >>
> >> --Alex
> >>
> >>
> >>> -----Original Message-----
> >>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
> >>> Sent: Tuesday, June 18, 2013 3:38 PM
> >>> To: dev@cloudstack.apache.org
> >>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
> >>>
> >>>
> >>>
> >>> On 6/17/13 7:43 PM, "Mice Xia" <mi...@tcloudcomputing.com>
> wrote:
> >>>
> >>>> Kelven,
> >>>>
> >>>> After the refactoring, will CS still restart HA enabled VM when it
> >>>> is power off externally (e.g. using xencenter) or internally (user
> >>>> initiated shutdown or crash)?
> >>>
> >>>
> >>> If HA functionality is provided by external manager (i.e., vCenter),
> >>> CS won't try to restart it, but if HA is provided by CloudStack, we
> >>> will restore the legacy logic. The hook up with old HA manager
> >>> (between
> >>> HighAvailabilityManager/VirtualMachineManager) is in the wrap-up
> >>> process
> >>>
> >>>
> >>>>
> >>>> Is seems the method HighAvailabilityManagerImpl .scheduleRestart()
> >>>> is not called when VM's actual state is stopped while expected
> >>>> state is
> >>> running.
> >>>>
> >>>>
> >>>> Regards
> >>>> Mice
> >>>>
> >>>> -----Original Message-----
> >>>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
> >>>> Sent: Tuesday, June 18, 2013 5:21 AM
> >>>> To: dev@cloudstack.apache.org
> >>>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
> >>>>
> >>>> Haven't created a patch yet, will do it soon after some last wrap-ups.
> >>>>
> >>>> Kelven
> >>>>
> >>>> On 6/17/13 12:03 PM, "John Burwell" <jb...@basho.com> wrote:
> >>>>
> >>>>> Kelven,
> >>>>>
> >>>>> Did this patch get pushed to Review Board? If so, what is the URL?
> >>>>>
> >>>>> Thanks.
> >>>>> -John
> >>>>>
> >>>>> On Jun 17, 2013, at 1:40 PM, Kelven Yang <ke...@citrix.com>
> wrote:
> >>>>>
> >>>>>> Low level classes were tested in unit tests(MessageBus, Job
> >>>>>> framework, Job dispatchers etc), interface layer changes are
> >>>>>> guarded through matching the old semantics, but changes are
> >>>>>> tested manually, we are planning to get this part of testing
> >>>>>> through BVT system after we have re-based the latest master.
> >>>>>>
> >>>>>> Kelven
> >>>>>>
> >>>>>> On 6/17/13 10:01 AM, "Chip Childers" <ch...@sungard.com>
> >>> wrote:
> >>>>>>
> >>>>>>> On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang wrote:
> >>>>>>>> I'd like to kick off the official merge process. We will start
> >>>>>>>> the merge process after the branch has passed necessary tests
> >>>>>>>>
> >>>>>>>> Kelven
> >>>>>>>
> >>>>>>> Can you share what testing is being run against the branch?
> >>>>>>
> >>>>>
> >>>>
> >>
> >
Re: [MERGE] Merge VMSync improvement branch into master
Posted by John Burwell <jb...@basho.com>.
Hugo,
I completely agree with this stance, and will add a -1 as well. It has been a significant challenge this cycle to complete high quality reviews due to the large patch size and short time turnaround time. Going forward, I am going to be more likely to follow Hugo's example, and -1 patches for which there is not adequate review time. I think the following techniques will help streamline the review process:
1. Narrow Patch Scope: Ensure that the patch is confined to a single enhancement or defect fix. If you doing refactoring as part of a feature, submit the refactoring work a separate commit. As a further suggestion, if you are performing multiple refactorings (e.g. package reorganization and utility method extraction), split each refactoring into separate patches.
2. Chunk Features: For each large feature being developed, logically chunk the functionality and submit each chunk as a separate patch. I recommend erring on the side of chunks being too small. Let the reviewer determine if the patch is not large enough to be representative of the feature. The worst that will happen is that a reviewer will provide feedback on the work completed and simply ask for more work to be done before it can be merged to master. For features while chunks can't be merged all the way to master, intermediate patches can be submitted to Review Board for review throughout the cycle -- allowing large merges to actually be the accumulation of several smaller reviews.
3. Identify Reviewers Early: For big features, solicit the reviewers whiling to review a feature from FS through merge from the mailing list. Reviews will go much more quickly if reviewers have been involved from the beginning because they will not help identify issues before coding starts, but also will not have to develop context late. To be clear, identifying reviewers early does not preclude a new reviewer from becoming involved later, but the reviews will greatly reduce the probability of a late review finding significant issues. Additionally, the early reviewers can help the late reviewers come up to speed -- reducing the burden on the contributor.
Generally, my recommendation is to submit patches early and often. Ultimately, contributors determine how they wish to develop and deliver their contributions. These are only my recommendations as a reviewer to increase the probability that a feature will make a particular release train. Finally, it appears that reviewers are growing less and less tolerant of large patches that appear just before freeze dates, and those these patches face a much higher risk of being immediately receiving a -1 for the reasons stated above.
Thanks,
-John
On Jun 26, 2013, at 8:45 PM, Hugo Trippaers <hu...@trippaers.nl> wrote:
> Hey Alex, Kelven,
>
> I've been looking though the code and thanks for the very insightfull conversation and follow-up email. With merges of this magnitude it's essential to help people understand what is going on.
>
> Purely technical this is a merge i'm really pleased with. It will for sure improve our handling of virtual machines.
>
> However the timing of the merge request is not ideal. We are very close to the end of the already extended feature freeze. We have a consensus within the dev community to do large architectural changes in the beginning of the cycle and not at the very end. This not only means a lot of extra testing effort and other developers will have to get used to the changes introduced right at the moment when everybody should be focussed on bug fixing, documentation and stability. Without wanting to rip open old wounds, i can imagine we all want to avoid a javelin incident.
>
> I respect the work going into this and the effort it will take to keep this up to date with the current speed of master, but still i'm going to veto this with a -1 for inclusion before we cut of the 4.2 branch.
>
> Cheers,
>
> Hugo
>
> On Jun 26, 2013, at 5:32 PM, Alex Huang <Al...@citrix.com> wrote:
>
>> Hi All,
>>
>> Kelven had an emergency so I'm submitting the changes on vmsync for him. The patch are on
>> https://reviews.apache.org/r/12126.
>>
>> Hugo took a look and already had some questions on why so many files were changed and added/deleted, So I like to explain a bit in this email.
>>
>> As part of the vmsync change, we try to move the files that were relevant to vm orchestration into the right places so that others can properly view the different jars and understand the relationship between the jars. This process is ongoing and will continue in other changes.
>>
>> - Work related to jobs management have been put under cloud-framework-jobs as handling jobs is not really part of our server but is a library. By doing it this way, changes in it can be properly reviewed and there's a good separation from things that utilizes jobs and jobs itself.
>> - VirtualMachine orchestration has been moved from cloud-server to cloud-engine-orchestration. Cloud-engine-orchestration then only depends on cloud-engine-schema and cloud-api. This creates a clear compilation boundary between the self-service server implementation and orchestration implementation.
>>
>> As part of these changes, then we surfaced many problems because we setup the proper compilation boundaries and fixed all of these problems. For example, HostAllocator interface is something people can write plugins for but it was buried inside cloud-server package. We've moved a majority of these changes to cloud-api. There's quite a bit of file movements based on this change. For vmsync changes itself, the changes are centered around VirtualMachineManagerImpl.java so you can review that file instead.
>>
>> Thanks.
>>
>> --Alex
>>
>>
>>> -----Original Message-----
>>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
>>> Sent: Tuesday, June 18, 2013 3:38 PM
>>> To: dev@cloudstack.apache.org
>>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>>>
>>>
>>>
>>> On 6/17/13 7:43 PM, "Mice Xia" <mi...@tcloudcomputing.com> wrote:
>>>
>>>> Kelven,
>>>>
>>>> After the refactoring, will CS still restart HA enabled VM when it is
>>>> power off externally (e.g. using xencenter) or internally (user
>>>> initiated shutdown or crash)?
>>>
>>>
>>> If HA functionality is provided by external manager (i.e., vCenter), CS won't
>>> try to restart it, but if HA is provided by CloudStack, we will restore the legacy
>>> logic. The hook up with old HA manager (between
>>> HighAvailabilityManager/VirtualMachineManager) is in the wrap-up process
>>>
>>>
>>>>
>>>> Is seems the method HighAvailabilityManagerImpl .scheduleRestart() is
>>>> not called when VM's actual state is stopped while expected state is
>>> running.
>>>>
>>>>
>>>> Regards
>>>> Mice
>>>>
>>>> -----Original Message-----
>>>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
>>>> Sent: Tuesday, June 18, 2013 5:21 AM
>>>> To: dev@cloudstack.apache.org
>>>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>>>>
>>>> Haven't created a patch yet, will do it soon after some last wrap-ups.
>>>>
>>>> Kelven
>>>>
>>>> On 6/17/13 12:03 PM, "John Burwell" <jb...@basho.com> wrote:
>>>>
>>>>> Kelven,
>>>>>
>>>>> Did this patch get pushed to Review Board? If so, what is the URL?
>>>>>
>>>>> Thanks.
>>>>> -John
>>>>>
>>>>> On Jun 17, 2013, at 1:40 PM, Kelven Yang <ke...@citrix.com> wrote:
>>>>>
>>>>>> Low level classes were tested in unit tests(MessageBus, Job
>>>>>> framework, Job dispatchers etc), interface layer changes are guarded
>>>>>> through matching the old semantics, but changes are tested manually,
>>>>>> we are planning to get this part of testing through BVT system after
>>>>>> we have re-based the latest master.
>>>>>>
>>>>>> Kelven
>>>>>>
>>>>>> On 6/17/13 10:01 AM, "Chip Childers" <ch...@sungard.com>
>>> wrote:
>>>>>>
>>>>>>> On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang wrote:
>>>>>>>> I'd like to kick off the official merge process. We will start the
>>>>>>>> merge process after the branch has passed necessary tests
>>>>>>>>
>>>>>>>> Kelven
>>>>>>>
>>>>>>> Can you share what testing is being run against the branch?
>>>>>>
>>>>>
>>>>
>>
>
Re: [MERGE] Merge VMSync improvement branch into master
Posted by Hugo Trippaers <hu...@trippaers.nl>.
Hey Alex, Kelven,
I've been looking though the code and thanks for the very insightfull conversation and follow-up email. With merges of this magnitude it's essential to help people understand what is going on.
Purely technical this is a merge i'm really pleased with. It will for sure improve our handling of virtual machines.
However the timing of the merge request is not ideal. We are very close to the end of the already extended feature freeze. We have a consensus within the dev community to do large architectural changes in the beginning of the cycle and not at the very end. This not only means a lot of extra testing effort and other developers will have to get used to the changes introduced right at the moment when everybody should be focussed on bug fixing, documentation and stability. Without wanting to rip open old wounds, i can imagine we all want to avoid a javelin incident.
I respect the work going into this and the effort it will take to keep this up to date with the current speed of master, but still i'm going to veto this with a -1 for inclusion before we cut of the 4.2 branch.
Cheers,
Hugo
On Jun 26, 2013, at 5:32 PM, Alex Huang <Al...@citrix.com> wrote:
> Hi All,
>
> Kelven had an emergency so I'm submitting the changes on vmsync for him. The patch are on
> https://reviews.apache.org/r/12126.
>
> Hugo took a look and already had some questions on why so many files were changed and added/deleted, So I like to explain a bit in this email.
>
> As part of the vmsync change, we try to move the files that were relevant to vm orchestration into the right places so that others can properly view the different jars and understand the relationship between the jars. This process is ongoing and will continue in other changes.
>
> - Work related to jobs management have been put under cloud-framework-jobs as handling jobs is not really part of our server but is a library. By doing it this way, changes in it can be properly reviewed and there's a good separation from things that utilizes jobs and jobs itself.
> - VirtualMachine orchestration has been moved from cloud-server to cloud-engine-orchestration. Cloud-engine-orchestration then only depends on cloud-engine-schema and cloud-api. This creates a clear compilation boundary between the self-service server implementation and orchestration implementation.
>
> As part of these changes, then we surfaced many problems because we setup the proper compilation boundaries and fixed all of these problems. For example, HostAllocator interface is something people can write plugins for but it was buried inside cloud-server package. We've moved a majority of these changes to cloud-api. There's quite a bit of file movements based on this change. For vmsync changes itself, the changes are centered around VirtualMachineManagerImpl.java so you can review that file instead.
>
> Thanks.
>
> --Alex
>
>
>> -----Original Message-----
>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
>> Sent: Tuesday, June 18, 2013 3:38 PM
>> To: dev@cloudstack.apache.org
>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>>
>>
>>
>> On 6/17/13 7:43 PM, "Mice Xia" <mi...@tcloudcomputing.com> wrote:
>>
>>> Kelven,
>>>
>>> After the refactoring, will CS still restart HA enabled VM when it is
>>> power off externally (e.g. using xencenter) or internally (user
>>> initiated shutdown or crash)?
>>
>>
>> If HA functionality is provided by external manager (i.e., vCenter), CS won't
>> try to restart it, but if HA is provided by CloudStack, we will restore the legacy
>> logic. The hook up with old HA manager (between
>> HighAvailabilityManager/VirtualMachineManager) is in the wrap-up process
>>
>>
>>>
>>> Is seems the method HighAvailabilityManagerImpl .scheduleRestart() is
>>> not called when VM's actual state is stopped while expected state is
>> running.
>>>
>>>
>>> Regards
>>> Mice
>>>
>>> -----Original Message-----
>>> From: Kelven Yang [mailto:kelven.yang@citrix.com]
>>> Sent: Tuesday, June 18, 2013 5:21 AM
>>> To: dev@cloudstack.apache.org
>>> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>>>
>>> Haven't created a patch yet, will do it soon after some last wrap-ups.
>>>
>>> Kelven
>>>
>>> On 6/17/13 12:03 PM, "John Burwell" <jb...@basho.com> wrote:
>>>
>>>> Kelven,
>>>>
>>>> Did this patch get pushed to Review Board? If so, what is the URL?
>>>>
>>>> Thanks.
>>>> -John
>>>>
>>>> On Jun 17, 2013, at 1:40 PM, Kelven Yang <ke...@citrix.com> wrote:
>>>>
>>>>> Low level classes were tested in unit tests(MessageBus, Job
>>>>> framework, Job dispatchers etc), interface layer changes are guarded
>>>>> through matching the old semantics, but changes are tested manually,
>>>>> we are planning to get this part of testing through BVT system after
>>>>> we have re-based the latest master.
>>>>>
>>>>> Kelven
>>>>>
>>>>> On 6/17/13 10:01 AM, "Chip Childers" <ch...@sungard.com>
>> wrote:
>>>>>
>>>>>> On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang wrote:
>>>>>>> I'd like to kick off the official merge process. We will start the
>>>>>>> merge process after the branch has passed necessary tests
>>>>>>>
>>>>>>> Kelven
>>>>>>
>>>>>> Can you share what testing is being run against the branch?
>>>>>
>>>>
>>>
>
RE: [MERGE] Merge VMSync improvement branch into master
Posted by Alex Huang <Al...@citrix.com>.
Hi All,
Kelven had an emergency so I'm submitting the changes on vmsync for him. The patch are on
https://reviews.apache.org/r/12126.
Hugo took a look and already had some questions on why so many files were changed and added/deleted, So I like to explain a bit in this email.
As part of the vmsync change, we try to move the files that were relevant to vm orchestration into the right places so that others can properly view the different jars and understand the relationship between the jars. This process is ongoing and will continue in other changes.
- Work related to jobs management have been put under cloud-framework-jobs as handling jobs is not really part of our server but is a library. By doing it this way, changes in it can be properly reviewed and there's a good separation from things that utilizes jobs and jobs itself.
- VirtualMachine orchestration has been moved from cloud-server to cloud-engine-orchestration. Cloud-engine-orchestration then only depends on cloud-engine-schema and cloud-api. This creates a clear compilation boundary between the self-service server implementation and orchestration implementation.
As part of these changes, then we surfaced many problems because we setup the proper compilation boundaries and fixed all of these problems. For example, HostAllocator interface is something people can write plugins for but it was buried inside cloud-server package. We've moved a majority of these changes to cloud-api. There's quite a bit of file movements based on this change. For vmsync changes itself, the changes are centered around VirtualMachineManagerImpl.java so you can review that file instead.
Thanks.
--Alex
> -----Original Message-----
> From: Kelven Yang [mailto:kelven.yang@citrix.com]
> Sent: Tuesday, June 18, 2013 3:38 PM
> To: dev@cloudstack.apache.org
> Subject: Re: [MERGE] Merge VMSync improvement branch into master
>
>
>
> On 6/17/13 7:43 PM, "Mice Xia" <mi...@tcloudcomputing.com> wrote:
>
> >Kelven,
> >
> >After the refactoring, will CS still restart HA enabled VM when it is
> >power off externally (e.g. using xencenter) or internally (user
> >initiated shutdown or crash)?
>
>
> If HA functionality is provided by external manager (i.e., vCenter), CS won't
> try to restart it, but if HA is provided by CloudStack, we will restore the legacy
> logic. The hook up with old HA manager (between
> HighAvailabilityManager/VirtualMachineManager) is in the wrap-up process
>
>
> >
> >Is seems the method HighAvailabilityManagerImpl .scheduleRestart() is
> >not called when VM's actual state is stopped while expected state is
> running.
> >
> >
> >Regards
> >Mice
> >
> >-----Original Message-----
> >From: Kelven Yang [mailto:kelven.yang@citrix.com]
> >Sent: Tuesday, June 18, 2013 5:21 AM
> >To: dev@cloudstack.apache.org
> >Subject: Re: [MERGE] Merge VMSync improvement branch into master
> >
> >Haven't created a patch yet, will do it soon after some last wrap-ups.
> >
> >Kelven
> >
> >On 6/17/13 12:03 PM, "John Burwell" <jb...@basho.com> wrote:
> >
> >>Kelven,
> >>
> >>Did this patch get pushed to Review Board? If so, what is the URL?
> >>
> >>Thanks.
> >>-John
> >>
> >>On Jun 17, 2013, at 1:40 PM, Kelven Yang <ke...@citrix.com> wrote:
> >>
> >>> Low level classes were tested in unit tests(MessageBus, Job
> >>>framework, Job dispatchers etc), interface layer changes are guarded
> >>>through matching the old semantics, but changes are tested manually,
> >>>we are planning to get this part of testing through BVT system after
> >>>we have re-based the latest master.
> >>>
> >>> Kelven
> >>>
> >>> On 6/17/13 10:01 AM, "Chip Childers" <ch...@sungard.com>
> wrote:
> >>>
> >>>> On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang wrote:
> >>>>> I'd like to kick off the official merge process. We will start the
> >>>>>merge process after the branch has passed necessary tests
> >>>>>
> >>>>> Kelven
> >>>>
> >>>> Can you share what testing is being run against the branch?
> >>>
> >>
> >
Re: [MERGE] Merge VMSync improvement branch into master
Posted by Kelven Yang <ke...@citrix.com>.
On 6/17/13 7:43 PM, "Mice Xia" <mi...@tcloudcomputing.com> wrote:
>Kelven,
>
>After the refactoring, will CS still restart HA enabled VM when it is
>power off externally (e.g. using xencenter) or internally (user initiated
>shutdown or crash)?
If HA functionality is provided by external manager (i.e., vCenter), CS
won't try to restart it, but if HA is provided by CloudStack, we will
restore the legacy logic. The hook up with old HA manager (between
HighAvailabilityManager/VirtualMachineManager) is in the wrap-up process
>
>Is seems the method HighAvailabilityManagerImpl .scheduleRestart() is not
>called when VM's actual state is stopped while expected state is running.
>
>
>Regards
>Mice
>
>-----Original Message-----
>From: Kelven Yang [mailto:kelven.yang@citrix.com]
>Sent: Tuesday, June 18, 2013 5:21 AM
>To: dev@cloudstack.apache.org
>Subject: Re: [MERGE] Merge VMSync improvement branch into master
>
>Haven't created a patch yet, will do it soon after some last wrap-ups.
>
>Kelven
>
>On 6/17/13 12:03 PM, "John Burwell" <jb...@basho.com> wrote:
>
>>Kelven,
>>
>>Did this patch get pushed to Review Board? If so, what is the URL?
>>
>>Thanks.
>>-John
>>
>>On Jun 17, 2013, at 1:40 PM, Kelven Yang <ke...@citrix.com> wrote:
>>
>>> Low level classes were tested in unit tests(MessageBus, Job
>>>framework, Job dispatchers etc), interface layer changes are guarded
>>>through matching the old semantics, but changes are tested manually,
>>>we are planning to get this part of testing through BVT system after
>>>we have re-based the latest master.
>>>
>>> Kelven
>>>
>>> On 6/17/13 10:01 AM, "Chip Childers" <ch...@sungard.com> wrote:
>>>
>>>> On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang wrote:
>>>>> I'd like to kick off the official merge process. We will start the
>>>>>merge process after the branch has passed necessary tests
>>>>>
>>>>> Kelven
>>>>
>>>> Can you share what testing is being run against the branch?
>>>
>>
>
RE: [MERGE] Merge VMSync improvement branch into master
Posted by Mice Xia <mi...@tcloudcomputing.com>.
Kelven,
After the refactoring, will CS still restart HA enabled VM when it is power off externally (e.g. using xencenter) or internally (user initiated shutdown or crash)?
Is seems the method HighAvailabilityManagerImpl .scheduleRestart() is not called when VM's actual state is stopped while expected state is running.
Regards
Mice
-----Original Message-----
From: Kelven Yang [mailto:kelven.yang@citrix.com]
Sent: Tuesday, June 18, 2013 5:21 AM
To: dev@cloudstack.apache.org
Subject: Re: [MERGE] Merge VMSync improvement branch into master
Haven't created a patch yet, will do it soon after some last wrap-ups.
Kelven
On 6/17/13 12:03 PM, "John Burwell" <jb...@basho.com> wrote:
>Kelven,
>
>Did this patch get pushed to Review Board? If so, what is the URL?
>
>Thanks.
>-John
>
>On Jun 17, 2013, at 1:40 PM, Kelven Yang <ke...@citrix.com> wrote:
>
>> Low level classes were tested in unit tests(MessageBus, Job
>>framework, Job dispatchers etc), interface layer changes are guarded
>>through matching the old semantics, but changes are tested manually,
>>we are planning to get this part of testing through BVT system after
>>we have re-based the latest master.
>>
>> Kelven
>>
>> On 6/17/13 10:01 AM, "Chip Childers" <ch...@sungard.com> wrote:
>>
>>> On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang wrote:
>>>> I'd like to kick off the official merge process. We will start the
>>>>merge process after the branch has passed necessary tests
>>>>
>>>> Kelven
>>>
>>> Can you share what testing is being run against the branch?
>>
>
Re: [MERGE] Merge VMSync improvement branch into master
Posted by Kelven Yang <ke...@citrix.com>.
Haven't created a patch yet, will do it soon after some last wrap-ups.
Kelven
On 6/17/13 12:03 PM, "John Burwell" <jb...@basho.com> wrote:
>Kelven,
>
>Did this patch get pushed to Review Board? If so, what is the URL?
>
>Thanks.
>-John
>
>On Jun 17, 2013, at 1:40 PM, Kelven Yang <ke...@citrix.com> wrote:
>
>> Low level classes were tested in unit tests(MessageBus, Job framework,
>>Job
>> dispatchers etc), interface layer changes are guarded through matching
>>the
>> old semantics, but changes are tested manually, we are planning to get
>> this part of testing through BVT system after we have re-based the
>>latest
>> master.
>>
>> Kelven
>>
>> On 6/17/13 10:01 AM, "Chip Childers" <ch...@sungard.com> wrote:
>>
>>> On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang wrote:
>>>> I'd like to kick off the official merge process. We will start the
>>>>merge
>>>> process after the branch has passed necessary tests
>>>>
>>>> Kelven
>>>
>>> Can you share what testing is being run against the branch?
>>
>
Re: [MERGE] Merge VMSync improvement branch into master
Posted by John Burwell <jb...@basho.com>.
Kelven,
Did this patch get pushed to Review Board? If so, what is the URL?
Thanks.
-John
On Jun 17, 2013, at 1:40 PM, Kelven Yang <ke...@citrix.com> wrote:
> Low level classes were tested in unit tests(MessageBus, Job framework, Job
> dispatchers etc), interface layer changes are guarded through matching the
> old semantics, but changes are tested manually, we are planning to get
> this part of testing through BVT system after we have re-based the latest
> master.
>
> Kelven
>
> On 6/17/13 10:01 AM, "Chip Childers" <ch...@sungard.com> wrote:
>
>> On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang wrote:
>>> I'd like to kick off the official merge process. We will start the merge
>>> process after the branch has passed necessary tests
>>>
>>> Kelven
>>
>> Can you share what testing is being run against the branch?
>
Re: [MERGE] Merge VMSync improvement branch into master
Posted by Kelven Yang <ke...@citrix.com>.
Low level classes were tested in unit tests(MessageBus, Job framework, Job
dispatchers etc), interface layer changes are guarded through matching the
old semantics, but changes are tested manually, we are planning to get
this part of testing through BVT system after we have re-based the latest
master.
Kelven
On 6/17/13 10:01 AM, "Chip Childers" <ch...@sungard.com> wrote:
>On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang wrote:
>> I'd like to kick off the official merge process. We will start the merge
>> process after the branch has passed necessary tests
>>
>> Kelven
>
>Can you share what testing is being run against the branch?
Re: [MERGE] Merge VMSync improvement branch into master
Posted by Chip Childers <ch...@sungard.com>.
On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang wrote:
> I'd like to kick off the official merge process. We will start the merge
> process after the branch has passed necessary tests
>
> Kelven
Can you share what testing is being run against the branch?