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 <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 > >> >> >>> 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" <blan...@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.kan...@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. >>>>>> >>>>> >>>>> >>>>> >>>> >>> >>> >> >
