On Mon, Jan 28, 2013 at 5:21 PM, Chiradeep Vittal <chiradeep.vit...@citrix.com> wrote: > > > On 1/28/13 12:38 PM, "Marcus Sorensen" <shadow...@gmail.com> wrote: > >>On Mon, Jan 28, 2013 at 12:44 PM, Marcus Sorensen <shadow...@gmail.com> >>wrote: >>> On Mon, Jan 28, 2013 at 12:31 PM, Chiradeep Vittal >>> <chiradeep.vit...@citrix.com> wrote: >>>> Inline >>>> >>>> On 1/25/13 4:03 PM, "Brian Angus" <blan...@betterservers.com> wrote: >>>> >>>>>My answers inline... >>>>> >>>>>On 01/24/2013 06:49 PM, Marcus Sorensen wrote: >>>>>> Brian is probably the right guy to answer some of these, but I'll >>>>>>chime >>>>>>in >>>>>> on a few. >>>>>> >>>>>> On Thu, Jan 24, 2013 at 5:58 PM, Chiradeep Vittal < >>>>>> chiradeep.vit...@citrix.com> wrote: >>>>>> >>>>>>> Thanks for this. >>>>>>> A few comments: >>>>>>> 1. These mutating operations have to be async and hence the API >>>>>>>commands >>>>>>> have to inherit from BaseAsyncCmd instead of BaseCmd >>>>>>> >>>>>> >>>>>> We can do that >>>>>This change has been pushed to the branch >>>> >>>> I saw this. We have 2 choices: extend from BaseAsyncCreateCmd or >>>> BaseAsyncCmd. I see that you have chosen the latter. If the former: you >>>> synchronously create the nic object first and obtain an id (uuid), and >>>> then asynchronously attach the nic. In the current implementation both >>>>the >>>> db entity creation and the attach are in the asynchronous workflow. If >>>>the >>>> attach fails in the backend, then there will be a nic object in the >>>> database theoretically attached to the vm, but not really attached. >>>> Choices: >>>> A) change the data model to model the fact that the nic is attached or >>>>not >>>> (and use the BaseAsyncCreateCmd approach) >>>> B) ensure that the nic is destroyed when the attach fails. >>>> >>>> Note that with (B) you have a potential race condition where an API >>>>client >>>> can retrieve the user vm before the attach has succeeded/failed and >>>>then >>>> potentially add a static nat rule. >>> >>> This is an interesting issue. If it fails to attach, we don't >>> necessarily want it to back out. In theory we only bother with the >>> attach because the VM is running, if it weren't then it would be >>> sufficient to just create the database entry. The existing code (e.g. >>> deployVirtualMachine) doesn't delete your nics if they fail to apply >>> when the VM starts up, correct? > > No, the VM will fail to start if any of the nics fail to attach/create. So > you will never have a VM that claims to have 3 nics attached but in > reality has only 2 attached. > >>>> >> >>We appreciate your feedback Chiradeep, and helping us to make sure we >>account for all scenarios. Do you think we're far enough along on this >>to get it into master? We are just finishing up a few tests that will >>be added into the branch but I hope we're to they point that any >>lingering outliers can be bugfixes post code freeze. > > If you fix the failing nic case, sure. >
Ok, thanks.
