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 <[email protected]> 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 <[email protected]> > 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 < >>> [email protected]> 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" <[email protected]> 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 >>>>>>> <[email protected]>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. >>>>>>> >>>>>> >>>>>> >>>>>> >>>>> >>>> >>>> >>> >> >
