I have posted my review comments. Except for a few minor comments changes look good to me.
-Koushik On 02-Dec-2013, at 9:15 PM, Alex Ough <alex.o...@sungard.com> wrote: > All, > > It's been a while since this review was requested, so can anyone review > this to move on? > > Thanks in advance. > Alex Ough > > > On Fri, Nov 22, 2013 at 11:51 AM, Alex Ough <alex.o...@sungard.com> wrote: > >> Thanks to help from many developers, >> I sent my first review request in the cloudstack, >> https://reviews.apache.org/r/15763/, >> so please take a look at it and let me know if there is anything >> missing/incorrect. >> >> Thanks again. >> Alex Ough >> >> >> On Thu, Nov 21, 2013 at 10:09 AM, Alex Ough <alex.o...@sungard.com> wrote: >> >>> Hi Wido, >>> >>> Related with the 'disk_offering.cache_mode', >>> I found that there is a sql file to add that column, which is 'cloudstack/ >>> developer/target/db/db/schema-430to440.sql'. >>> But after running it manually, I have another error saying that the >>> 'cache_mode' cannot be null, so I just changed the column to allow null to >>> move on. >>> >>> So you may include the sql file and allow null in that column. >>> >>> Thanks >>> Alex Ough >>> >>> >>> On Wed, Nov 20, 2013 at 3:45 PM, Alex Huang <alex.hu...@citrix.com>wrote: >>> >>>> Wido, >>>> >>>> >>>> >>>> Looks like you didn’t update your schema file or forgot to add a schema >>>> upgrade file in master? See the error described by Alex below. >>>> >>>> >>>> >>>> @Alex >>>> >>>> You can probably remove the cachemode field in the DiskOfferingVO for now. >>>> Or revert 1edaa36 on your local repo to keep moving forward. >>>> >>>> >>>> >>>> --Alex >>>> >>>> >>>> >>>> *From:* Alex Ough [mailto:alex.o...@sungard.com] >>>> *Sent:* Wednesday, November 20, 2013 9:56 AM >>>> *To:* Alex Huang >>>> *Cc:* dev@cloudstack.apache.org >>>> >>>> *Subject:* Re: A question on vm migrations when hosts are set into a >>>> maintenance mode. >>>> >>>> >>>> >>>> Hi Alex, >>>> >>>> >>>> >>>> It looks like you moved the 'ExecuteInSequence' to the vm level from the >>>> management server, which seems to be ok, but at this time I cannot test it >>>> >>>> because when I try to start up the management server, it fails because >>>> of database schema error, which is 'Unknown column >>>> 'disk_offering.cache_mode' in 'field list' even if I re-built the database. >>>> >>>> >>>> >>>> Is the schema change part of your changes or something other developer >>>> changed? >>>> >>>> >>>> >>>> If that is from any other developer, can you fix this? >>>> >>>> >>>> >>>> Thanks >>>> >>>> Alex Ough >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> On Tue, Nov 19, 2013 at 7:25 PM, Alex Huang <alex.hu...@citrix.com> >>>> wrote: >>>> >>>> Alex, >>>> >>>> >>>> >>>> Can you do a pull from master and see if my fix fits your needs. >>>> Unfortunately, I’m on the road and couldn’t do an actual test of it. >>>> >>>> >>>> >>>> --Alex >>>> >>>> >>>> >>>> *From:* Alex Huang >>>> *Sent:* Tuesday, November 19, 2013 4:45 AM >>>> *To:* 'Alex Ough'; dev@cloudstack.apache.org >>>> *Subject:* RE: A question on vm migrations when hosts are set into a >>>> maintenance mode. >>>> >>>> >>>> >>>> Alex, >>>> >>>> >>>> >>>> Sorry for the late reply. Been travelling the last couple of weeks. >>>> I’ll look into this today. >>>> >>>> >>>> >>>> --Alex >>>> >>>> >>>> >>>> *From:* Alex Ough [mailto:alex.o...@sungard.com <alex.o...@sungard.com>] >>>> >>>> *Sent:* Monday, November 18, 2013 6:17 AM >>>> *To:* dev@cloudstack.apache.org >>>> *Cc:* Alex Huang >>>> *Subject:* Re: A question on vm migrations when hosts are set into a >>>> maintenance mode. >>>> >>>> >>>> >>>> Thank Parasanna & Sebastien, >>>> >>>> I also got his email and sent an email. >>>> >>>> Waiting for his reply... >>>> >>>> >>>> >>>> Thanks >>>> >>>> Alex Ough >>>> >>>> >>>> >>>> On Sat, Nov 16, 2013 at 3:05 PM, Sebastien Goasguen <run...@gmail.com> >>>> wrote: >>>> >>>> cc Alex Huang to get his attention: >>>> >>>> >>>> >>>> On Nov 15, 2013, at 10:17 PM, Prasanna Santhanam <t...@apache.org> wrote: >>>> >>>>> Alex, Could you just do a git blame on the file and copy the emails of >>>>> people who changed that bit of code? They may be able to help if Cc-ed >>>>> directly. >>>>> >>>>> Thanks, >>>>> >>>>> On Fri, Nov 15, 2013 at 01:49:07PM -0600, Alex Ough wrote: >>>>>> I hate to sending the same emails over and over again, but I really >>>> need to >>>>>> finalize this feature to be included in the next code freeze because >>>> this >>>>>> feature is very critical in our inside project. >>>>>> >>>>>> Anyone who can help, please? >>>>>> Thanks >>>>>> Alex Ough >>>>>> >>>>>> >>>>>> On Thu, Nov 14, 2013 at 1:27 PM, Alex Ough <alex.o...@sungard.com> >>>> wrote: >>>>>> >>>>>>> Not sure if Alex Huang checked this, but can anyone help to resolve >>>> this? >>>>>>> >>>>>>> Thanks >>>>>>> Alex Ough >>>>>>> >>>>>>> >>>>>>> On Wed, Nov 13, 2013 at 11:39 AM, Alex Ough <alex.o...@sungard.com> >>>> wrote: >>>>>>> >>>>>>>> It sounds a little scary... >>>>>>>> >>>>>>>> I looked at the history and found these. >>>>>>>> >>>>>>>> 8/9/ : file moved to engine by Alex Huang >>>>>>>> 9/16 : '_mgmtServer.getExecuteInSequence()' changed to >>>>>>>> 'getExecuteInSequence()' by Alex Huang >>>>>>>> >>>>>>>> >>>>>>>> Hi Alex Huang, >>>>>>>> I'm not sure if you're aware of this, but can you check this for me? >>>>>>>> >>>>>>>> Thanks >>>>>>>> Alex Ough >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Wed, Nov 13, 2013 at 11:18 AM, Marcus Sorensen < >>>> shadow...@gmail.com>wrote: >>>>>>>> >>>>>>>>> I'm not sure. I know in the past when I've seen files change >>>> locations >>>>>>>>> it has also clobbered updates to that file. Someone branched, did >>>> the >>>>>>>>> reorganization work, and merged, while in-between the original file >>>>>>>>> changed. >>>>>>>>> >>>>>>>>> On Wed, Nov 13, 2013 at 9:21 AM, Alex Ough <alex.o...@sungard.com> >>>>>>>>> wrote: >>>>>>>>>> All, >>>>>>>>>> >>>>>>>>>> While merging my changes to 4.3 branch, I found that the option, >>>>>>>>>> 'execute.in.sequence.hypervisor.commands' is NOT used in >>>>>>>>> Start/Stop/Copy >>>>>>>>>> commands in 'VirtualMachineManagerImpl.java' any more as below. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> *StopCommand stop = new StopCommand(vm, getExecuteInSequence());* >>>>>>>>>> >>>>>>>>>> *protected boolean getExecuteInSequence() {* >>>>>>>>>> * return false;* >>>>>>>>>> *}* >>>>>>>>>> >>>>>>>>>> As you see in the above, the function, 'getExecuteInSequence', >>>> just >>>>>>>>> returns >>>>>>>>>> false instead of getting the value from the global variable. >>>>>>>>>> >>>>>>>>>> And one more change is that the file has been moved to >>>>>>>>>> 'engine/orchestration/src/com/cloud/vm' from >>>> 'server/src/com/cloud/vm'. >>>>>>>>>> >>>>>>>>>> Am I missing something related with this or do we stop supporting >>>> this >>>>>>>>>> option in 4.3? >>>>>>>>>> I'm a little confused, so please help me resolve this. >>>>>>>>>> >>>>>>>>>> Thanks >>>>>>>>>> Alex Ough >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Tue, Nov 12, 2013 at 4:20 PM, Alex Ough <alex.o...@sungard.com >>>>> >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> Thanks a lot for your confirmation, Marcus. >>>>>>>>>>> I'll create a review request unless anyone has an objection. >>>>>>>>>>> >>>>>>>>>>> Thanks >>>>>>>>>>> Alex Ough >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Tue, Nov 12, 2013 at 3:37 PM, Marcus Sorensen < >>>> shadow...@gmail.com >>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>>> I have done parallel KVM migrations without issue, it's >>>> "supposed to >>>>>>>>>>>> work". Really I think it's in the same boat as parallel >>>> start/stop. >>>>>>>>> It >>>>>>>>>>>> should work, but the config option is there just in case. I >>>> think we >>>>>>>>>>>> should add it. >>>>>>>>>>>> >>>>>>>>>>>> On Thu, Oct 3, 2013 at 11:41 AM, Chip Childers >>>>>>>>>>>> <chip.child...@sungard.com> wrote: >>>>>>>>>>>>> On Thu, Oct 03, 2013 at 11:44:46AM -0500, Alex Ough wrote: >>>>>>>>>>>>>> I'm not sure what else commands 'MigrateCommand' actually >>>> execute >>>>>>>>> in >>>>>>>>>>>>>> addition to 'Start/Stop/CopyCommand', but can we include >>>>>>>>>>>> 'MigrateCommand' >>>>>>>>>>>>>> if it consists of only those 3 commands? >>>>>>>>>>>>>> >>>>>>>>>>>>>> Thanks >>>>>>>>>>>>>> Alex Ough >>>>>>>>>>>>> >>>>>>>>>>>>> In the case of VMware, the migrate command is executed via the >>>>>>>>>>>>> MigrateVMTask that's part of the VMware SDK (see >>>>>>>>>>>>> >>>>>>>>> >>>> vmware-base/src/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java). >>>>>>>>>>>>> >>>>>>>>>>>>> For VMware, I know that vCenter will queue and process >>>> concurrent >>>>>>>>>>>>> requests for migrations. Specifically, it will throttle the >>>>>>>>> migrations >>>>>>>>>>>>> happening, based on it's internal concurrency constraints, but >>>> the >>>>>>>>> task >>>>>>>>>>>>> queue will still accept more connections. Obviously the risk >>>> are >>>>>>>>> the >>>>>>>>>>>>> VMware layer tasks timing out if it takes too long for the task >>>>>>>>> queue to >>>>>>>>>>>>> complete. >>>>>>>>>>>>> >>>>>>>>>>>>> As for XenServer, it's happening in what appears to be a >>>> similar >>>>>>>>> way >>>>>>>>>>>>> (although the source host is the target for the migration API >>>>>>>>> call). >>>>>>>>>>>>> >>>>>>>>>>>>> Check >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>> >>>> plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java. >>>>>>>>>>>>> >>>>>>>>>>>>> I'm not familiar enough with XenServer's concurrency model for >>>>>>>>>>>>> migrations. Any experts know the answer to if it can handle >>>>>>>>> concurrency >>>>>>>>>>>>> in a stable way? >>>>>>>>>>>>> >>>>>>>>>>>>> With KVM, it's obviously executing via the agent. Similarly to >>>>>>>>>>>>> XenServer, I'm not familiar enough to know about concurrent >>>>>>>>> operations. >>>>>>>>>>>>> >>>>>>>>>>>>> So do the HV experts on the list have any opinions about >>>> XenServer >>>>>>>>> and >>>>>>>>>>>>> KVM migration concurrency? >>>>>>>>>>>>> >>>>>>>>>>>>> -chip >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>> >>>>> -- >>>>> Prasanna., >>>>> >>>>> ------------------------ >>>>> Powered by BigRock.com >>>>> >>>> >>>> >>>> >>>> >>>> >>> >>> >>
