cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Musayev, Ilya" <imusa...@webmd.net>
Subject RE: [MERGE] Merge VMSync improvement branch into master
Date Thu, 27 Jun 2013 23:05:28 GMT
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 <jburwell@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" <imusayev@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 <hugo@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 <Alex.Huang@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" <mice_xia@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" <jburwell@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
> >>>>>>>> <kelven.yang@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"
> >>>>>>>>> <chip.childers@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?
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>
> >>>>
> >>
> >>
> >
> 



Mime
View raw message