incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marcus Sorensen <shadow...@gmail.com>
Subject Re: Review Request: API commands to add or remove NIC from a VM
Date Tue, 29 Jan 2013 00:25:06 GMT
On Mon, Jan 28, 2013 at 5:21 PM, Chiradeep Vittal
<Chiradeep.Vittal@citrix.com> wrote:
>
>
> On 1/28/13 12:38 PM, "Marcus Sorensen" <shadowsor@gmail.com> wrote:
>
>>On Mon, Jan 28, 2013 at 12:44 PM, Marcus Sorensen <shadowsor@gmail.com>
>>wrote:
>>> On Mon, Jan 28, 2013 at 12:31 PM, Chiradeep Vittal
>>> <Chiradeep.Vittal@citrix.com> wrote:
>>>> Inline
>>>>
>>>> On 1/25/13 4:03 PM, "Brian Angus" <blangus@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.Vittal@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.

Mime
View raw message