incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chip Childers <chip.child...@sungard.com>
Subject Re: Review Request: API commands to add or remove NIC from a VM
Date Mon, 28 Jan 2013 15:59:31 GMT
IMO - we should get the pending reviews in.  There are many of them,
and they are mostly for 4.1 features (this included).

However, as I asked on the other thread, waiting for Alex to comment
on the timing for Javelin.

On Mon, Jan 28, 2013 at 10:44 AM, Marcus Sorensen <shadowsor@gmail.com> wrote:
> FYI, we'd like to merge this soon, but we're waiting to see what's
> going on with javelin, because I think we'll need to make some
> changes. We merge cleanly with master now, but not with javelin. We've
> addressed the issues that came up in the discussion.
>
> On Fri, Jan 25, 2013 at 5: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
>>
>>>
>>>
>>>> 2. The functional specification is rather sparse -- I.e., it is not of
>>>> much use for a QA person to develop a comprehensive set of test cases. I
>>>> recommend taking a look at
>>>>
>>>> https://cwiki.apache.org/confluence/display/CLOUDSTACK/Design+Document+Temp
>>>> late to see what more information one could put in there. For example,
>>>> edge cases (remove the last nic / exceed nic limit), configuration
>>>> parameters.
>>
>> I have added more details to the functional spec.  Probably still needs more
>> work...
>>
>>>> 3. There's todos in the code: either this code is important  and cannot
>>>> ship without the TODO or is a nice to have. It is hard for someone to
>>>> figure out which case it is. Regardless if the TODOs are not fixed, they
>>>> should have bugs
>>>>
>>>
>>> I'll have to revisit the code, but I know at least a few that were
>>> inherited from the existing code base (things that were borrowed). Also I
>>> know of one or two that were just notes to myself of things to
>>> investigate,
>>> I thought I had removed those but I'll double check.
>>
>> Some of these have been addressed I'll work on the ones that still exist.
>>
>>>
>>>
>>>> 4. Questions in my mind:
>>>> A. Do you update instance metadata during this operation?
>>>> B. Verify IP address is not already used / in range / etc
>>>>
>>>
>>> We don't assign the IP, we call the same code that creates nics in the
>>> first place, and the IP is assigned via the same functionality that
>>> currently exists. If there's a problem with using IPs that are already
>>> used, it's a problem for deployVirtualMachine as well.
>>>
>>>
>>>> C. If you remove a default nic, what happens?
>>>>
>>>
>>> You can't.
>>>
>>>
>>>> D. Can you remove the last nic?
>>>>
>>>
>>> No, because you have to have a default nic, and you can't remove your
>>> default.
>>>
>>> E. Verify live migration after operation?
>>>>
>>>>
>>>
>>> This has been done for KVM, could be tested for Xen, but the VM definition
>>> looks good.
>>
>> I have tested this on XEN devcloud as well
>>
>>>
>>>
>>>> F. Operation blocks / fails during live migration / HA / start / stop
>>>>
>>>
>>> Brian recently changed some of the code related to this, but previously
>>> the
>>> VM state had to be 'stopped', not 'starting', 'stopping', 'running', or
>>> 'migrating'.
>>
>> The API calls will fail if the VM is not in a Running or Stopped state
>>
>>>
>>>
>>>> G. Usage events for remove / add operations
>>>>
>>>
>>> I'm unfamilar with usage events. The usages for new nics are tracked,
>>> though.
>>>
>>>
>>>> H. Interaction of removeNic with extant LB / PF / Static NAT/ FW rules
>>>>
>>>
>>> This is a good one to verify, does removing a nic orphan a port forwarding
>>> rule, etc, or does removing the nic clean up the rule?
>>>
>>>
>>>> I. Interaction of addNic / updateNic with PF/Static NAT/Source NAT --
>>>> e.g., the default route might remain the same inside the VM leading to
>>>> head scratching as to why a specific PF/NAT rule does not work.
>>>>
>>>
>>> This is a non-issue, in my opinion. It's similar to attaching a volume and
>>> then wondering why it's not formatted and mounted for us right where we
>>> wanted it. People have all sorts of varying uses for multiple networks,
>>> and
>>> it's up to them to configure what they intend. It would be good to make a
>>> note in the documentation though, people should know that we don't have
>>> much control over their VM's OS config and they need to treat it like any
>>> other server.
>>>
>>>
>>>>
>>>> Thanks
>>>>
>>>> On 1/24/13 3:45 PM, "Brian Angus" <blangus@betterservers.com> wrote:
>>>>
>>>>> The add_remove_nics branch has been updated.  It now allows hot swapping
>>>>> of NICs and you can now add multiple NICs from the same network.  The
>>>>> feature spec has been updated as the API calls have changed:
>>>>>
>>>>
>>>> https://cwiki.apache.org/confluence/display/CLOUDSTACK/Add+Remove+Networks
>>>>>
>>>>> +to+VMs
>>>>>
>>>>> -Brian
>>>>> On 01/21/2013 04:34 PM, Brian Angus wrote:
>>>>>>
>>>>>> On 01/21/2013 02:02 PM, Marcus Sorensen wrote:
>>>>>>>
>>>>>>> On Mon, Jan 21, 2013 at 12:30 AM, Hari Kannan
>>>>>>> <hari.kannan@citrix.com>wrote:
>>>>>>>
>>>>>>> I think you're right on #1, that if you create a VM via API you
can
>>>>>>> add
>>>>>>> multiple nics to the same network so long as you're not using
a VPC. I
>>>>>>> don't know for sure off the top of my head though
>>>>>>>
>>>>>> I tested this out and it is correct that you can add multiple NICs
with
>>>>>> the same network using the deployVirtualMachine api call.  When I
was
>>>>>> implementing this I thought cloudstack only allowed one NIC per network
>>>>>> because the com.cloud.network.NetworkManager.createNicForVm function
>>>>>> does not create a NIC if there is already a NIC that exists on the
>>>>>> Network (is that the desired behavior or is it a bug?).
>>>>>>>
>>>>>>>
>>>>>>>> 2. Earlier, Marcus had stated that " It doesn't strictly
require a
>>>>>>>> reboot,
>>>>>>>> but it does require that you apply a network config. The
nic will hot
>>>>>>>> plug
>>>>>>>> but you either have to reboot or manually trigger a dhcp
query " -
>>>>>>>> wish to
>>>>>>>> reconfirm this to be the case - also, can you please provide
a short
>>>>>>>> note
>>>>>>>> on how to do this, so we can add it to the documentation?
>>>>>>>>
>>>>>>>
>>>>>>> We could give some basic examples, but it's largely going to
come down
>>>>>>> to
>>>>>>> specific OS, for example with linux, it could change by distribution
>>>>>>> based
>>>>>>> on the udev rules, the config file locations, dhcp client differences,
>>>>>>> etc.
>>>>>>> It would be up to the admin of the VM guest to know how to set
up a
>>>>>>> network
>>>>>>> card on their guest OS.
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> Right now the code in the add_remove_nics branch only works when
the VM
>>>>>> is in a Stopped state.  I'm currently working on getting it working
>>>>>> while the VM is in a Running state.
>>>>>>
>>>>>>>> 3. Can you please confirm this is possible for any OS
>>>>>>>> (windows/Linux)?
>>>>>>>>
>>>>>>>
>>>>>>> Worst case is that the OS doesn't recognize the new NIC without
a
>>>>>>> reboot.
>>>>>>> So either you make them power the VM off always before adding
a NIC,
>>>>>>> or let
>>>>>>> them try to add the NIC and they can reboot if the OS doesn't
support
>>>>>>> it.
>>>>>>>
>>>>>>>
>>>>>>>> 4. is there any UI component developed for this?
>>>>>>>>
>>>>>>>
>>>>>>> No, I think there is a team who specializes in this. I was thinking
>>>>>>> the
>>>>>>> other day that the UI will likely always lag behind the API in
>>>>>>> features,
>>>>>>> because new features won't get time to be implemented in UI on
each
>>>>>>> release.
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>

Mime
View raw message