On Wed, Jan 30, 2013 at 12:17 PM, Marcus Sorensen <shadow...@gmail.com> wrote: > On Wed, Jan 30, 2013 at 10:58 AM, Chiradeep Vittal > <chiradeep.vit...@citrix.com> wrote: >> >> >> On 1/29/13 3:36 PM, "Marcus Sorensen" <shadow...@gmail.com> wrote: >>> >>> >>>I've been reviewing this, and I'm still not sure that backing out the >>>add nic db entry if the plug nic fails is the right way to go. We can >>>catch the failure and remove it from the database, sure. But consider >>>these scenarios: >>> >>>User runs deployVirtualMachine with parameter startvm=false. We don't >>>destroy their chosen VM config later when they launch the VM and a nic >>>or something else fails to provision. >> >> No, but we don't leave a VM running in an inconsistent state either. >> Either all the nics are created or the vm fails to start > > But we do leave virtual routers in inconsistent state, since the code > in question is used by that. I have seen virtual routers fail to plug > nics and simply not work (usually if there's a problem with a bridge, > for example in development environment). This is not a defense for > doing it in user VMs, but as mentioned, the issue is raised by > existing code we were leveraging, not new code we're adding. > >> >>> >>>User runs addNicToVirtualMachine when the VM is off. This essentially >>>edits the Virtual Machine's config to the same as had the >>>virtualmachine been deployed with the extra network. Again, if the >>>plug fails when they turn the VM on, we don't destroy their config. >> >> This should leave the VM not 'running'. >> >>> >>>All three of these, deployVirtualMachine, addNicToVirtualMachine when >>>VM is on, and addNicToVirtualMachine when vm is off, rely on >>>_networkMgr.createNicForVm to validate and apply for a nic. If we >>>trust that to create a valid config for our VMs, then we shouldn't go >>>back on that if there's a temporary problem. >> >> The question is not of the config, but telling the end-user that the >> addVmToNetwork failed but then leaving the nic entry in the db. An >> automated orchestration tool using the API would list the VM, show it is >> running and then see that there are N nics attached, without any idea that >> some of the nics are in fact not attached. > > Yeah, that's a valid concern, but I tend to think the plug will > succeed even if some things are broken, from what I've seen in > testing. I think the plug might only concern itself with whether it > can change the running VMs config, but not necessarily if it's > actually working as intended. If the plug fails, you're likely in > bigger trouble because other VMs and routers are failing to start, but > then post-issue you have no way to know of any lingering nic issues > that were plugged while the system was having problems. > >> >> >>> >>>The whole goal of this feature was to be able to edit the VM's network >>>config post-deploy. Doing it while the VM was running is a bonus, but >>>not strictly necessary. Would it be better to go back to only allowing >>>it while it's off (essentially the same thing, we won't know if the >>>plug works until they attempt to start the VM, at which point we >>>wouldn't remove it)? Or simply scrap this branch and move on to making >>>nics their own entity? >> >> I wouldn't scrap it. I am comfortable leaving it as is, or removing the >> ability to hot plug. >> > > I would like to leave it as-is and get it merged, and then I think > these concerns can be addressed in testing/bugfix phase. We've done a > lot of testing on it and it works well, though I do understand your > points about potential bugs. Worst case scenario, we can simply patch > it to only work on stopped VMs.
Change is merged. It looks like Brian updated the spec with expected failures (trying to remove default nic, etc).
