You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Brian Angus <bl...@betterservers.com> on 2013/01/17 20:39:02 UTC
Review Request: API commands to add or remove NIC from a VM
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9011/
-----------------------------------------------------------
Review request for cloudstack and Marcus Sorensen.
Description
-------
This needs to be used to create a new feature branch.
Feature Spec: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Add+Remove+Networks+to+VMs
Jira Request: https://issues.apache.org/jira/browse/CLOUDSTACK-645
Added addNicToVirtualMachine, removeNicFromVirtualMachine, and updateDefaultNicForVirtualMachine api commands.
This addresses bug CLOUDSTACK-645.
Diffs
-----
api/src/com/cloud/vm/UserVmService.java b1ebe10
api/src/org/apache/cloudstack/api/command/user/vm/AddNicToVMCmd.java PRE-CREATION
api/src/org/apache/cloudstack/api/command/user/vm/RemoveNicFromVMCmd.java PRE-CREATION
api/src/org/apache/cloudstack/api/command/user/vm/UpdateDefaultNicForVMCmd.java PRE-CREATION
client/tomcatconf/commands.properties.in 182cbd8
server/src/com/cloud/network/NetworkManagerImpl.java 0a1fcd7
server/src/com/cloud/vm/UserVmManagerImpl.java a6fbdb1
server/src/com/cloud/vm/VirtualMachineManager.java 4f04617
server/src/com/cloud/vm/VirtualMachineManagerImpl.java 4d94fc5
server/test/com/cloud/vm/MockUserVmManagerImpl.java 27508b1
server/test/com/cloud/vm/MockVirtualMachineManagerImpl.java 6723198
Diff: https://reviews.apache.org/r/9011/diff/
Testing
-------
This was tested and works on devcloud and devcloud-kvm. I plan to add some automated tests as well.
Thanks,
Brian Angus
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Marcus Sorensen <sh...@gmail.com>.
> On Jan. 17, 2013, 8:02 p.m., Marcus Sorensen wrote:
> > Ship It!
added feature branch add_remove_nics, close this review request.
- Marcus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9011/#review15462
-----------------------------------------------------------
On Jan. 17, 2013, 7:38 p.m., Brian Angus wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9011/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2013, 7:38 p.m.)
>
>
> Review request for cloudstack and Marcus Sorensen.
>
>
> Description
> -------
>
> This needs to be used to create a new feature branch.
>
> Feature Spec: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Add+Remove+Networks+to+VMs
> Jira Request: https://issues.apache.org/jira/browse/CLOUDSTACK-645
>
> Added addNicToVirtualMachine, removeNicFromVirtualMachine, and updateDefaultNicForVirtualMachine api commands.
>
>
> This addresses bug CLOUDSTACK-645.
>
>
> Diffs
> -----
>
> api/src/com/cloud/vm/UserVmService.java b1ebe10
> api/src/org/apache/cloudstack/api/command/user/vm/AddNicToVMCmd.java PRE-CREATION
> api/src/org/apache/cloudstack/api/command/user/vm/RemoveNicFromVMCmd.java PRE-CREATION
> api/src/org/apache/cloudstack/api/command/user/vm/UpdateDefaultNicForVMCmd.java PRE-CREATION
> client/tomcatconf/commands.properties.in 182cbd8
> server/src/com/cloud/network/NetworkManagerImpl.java 0a1fcd7
> server/src/com/cloud/vm/UserVmManagerImpl.java a6fbdb1
> server/src/com/cloud/vm/VirtualMachineManager.java 4f04617
> server/src/com/cloud/vm/VirtualMachineManagerImpl.java 4d94fc5
> server/test/com/cloud/vm/MockUserVmManagerImpl.java 27508b1
> server/test/com/cloud/vm/MockVirtualMachineManagerImpl.java 6723198
>
> Diff: https://reviews.apache.org/r/9011/diff/
>
>
> Testing
> -------
>
> This was tested and works on devcloud and devcloud-kvm. I plan to add some automated tests as well.
>
>
> Thanks,
>
> Brian Angus
>
>
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Marcus Sorensen <sh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9011/#review15462
-----------------------------------------------------------
Ship it!
Ship It!
- Marcus Sorensen
On Jan. 17, 2013, 7:38 p.m., Brian Angus wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9011/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2013, 7:38 p.m.)
>
>
> Review request for cloudstack and Marcus Sorensen.
>
>
> Description
> -------
>
> This needs to be used to create a new feature branch.
>
> Feature Spec: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Add+Remove+Networks+to+VMs
> Jira Request: https://issues.apache.org/jira/browse/CLOUDSTACK-645
>
> Added addNicToVirtualMachine, removeNicFromVirtualMachine, and updateDefaultNicForVirtualMachine api commands.
>
>
> This addresses bug CLOUDSTACK-645.
>
>
> Diffs
> -----
>
> api/src/com/cloud/vm/UserVmService.java b1ebe10
> api/src/org/apache/cloudstack/api/command/user/vm/AddNicToVMCmd.java PRE-CREATION
> api/src/org/apache/cloudstack/api/command/user/vm/RemoveNicFromVMCmd.java PRE-CREATION
> api/src/org/apache/cloudstack/api/command/user/vm/UpdateDefaultNicForVMCmd.java PRE-CREATION
> client/tomcatconf/commands.properties.in 182cbd8
> server/src/com/cloud/network/NetworkManagerImpl.java 0a1fcd7
> server/src/com/cloud/vm/UserVmManagerImpl.java a6fbdb1
> server/src/com/cloud/vm/VirtualMachineManager.java 4f04617
> server/src/com/cloud/vm/VirtualMachineManagerImpl.java 4d94fc5
> server/test/com/cloud/vm/MockUserVmManagerImpl.java 27508b1
> server/test/com/cloud/vm/MockVirtualMachineManagerImpl.java 6723198
>
> Diff: https://reviews.apache.org/r/9011/diff/
>
>
> Testing
> -------
>
> This was tested and works on devcloud and devcloud-kvm. I plan to add some automated tests as well.
>
>
> Thanks,
>
> Brian Angus
>
>
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Marcus Sorensen <sh...@gmail.com>.
Pushed update.
On Fri, Jan 18, 2013 at 5:12 PM, Marcus Sorensen <sh...@gmail.com>wrote:
> Chiradeep, I sucked this into a development branch. I pulled master into
> that and he just sent me a patch that fixes it.
>
>
> On Fri, Jan 18, 2013 at 5:04 PM, Chiradeep Vittal <ch...@gmail.com>wrote:
>
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/9011/
>>
>> Can you rebase against current master?
>>
>>
>> - Chiradeep
>>
>> On January 17th, 2013, 7:38 p.m., Brian Angus wrote:
>> Review request for cloudstack and Marcus Sorensen.
>> By Brian Angus.
>>
>> *Updated Jan. 17, 2013, 7:38 p.m.*
>> Description
>>
>> This needs to be used to create a new feature branch.
>>
>> Feature Spec: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Add+Remove+Networks+to+VMs
>> Jira Request: https://issues.apache.org/jira/browse/CLOUDSTACK-645
>>
>> Added addNicToVirtualMachine, removeNicFromVirtualMachine, and updateDefaultNicForVirtualMachine api commands.
>>
>> Testing
>>
>> This was tested and works on devcloud and devcloud-kvm. I plan to add some automated tests as well.
>>
>> *Bugs: * CLOUDSTACK-645
>> Diffs
>>
>> - api/src/com/cloud/vm/UserVmService.java (b1ebe10)
>> - api/src/org/apache/cloudstack/api/command/user/vm/AddNicToVMCmd.java
>> (PRE-CREATION)
>> - api/src/org/apache/cloudstack/api/command/user/vm/RemoveNicFromVMCmd.java
>> (PRE-CREATION)
>> - api/src/org/apache/cloudstack/api/command/user/vm/UpdateDefaultNicForVMCmd.java
>> (PRE-CREATION)
>> - client/tomcatconf/commands.properties.in (182cbd8)
>> - server/src/com/cloud/network/NetworkManagerImpl.java (0a1fcd7)
>> - server/src/com/cloud/vm/UserVmManagerImpl.java (a6fbdb1)
>> - server/src/com/cloud/vm/VirtualMachineManager.java (4f04617)
>> - server/src/com/cloud/vm/VirtualMachineManagerImpl.java (4d94fc5)
>> - server/test/com/cloud/vm/MockUserVmManagerImpl.java (27508b1)
>> - server/test/com/cloud/vm/MockVirtualMachineManagerImpl.java
>> (6723198)
>>
>> View Diff <https://reviews.apache.org/r/9011/diff/>
>>
>
>
RE: Review Request: API commands to add or remove NIC from a VM
Posted by Simon Waterhouse <Si...@eu.citrix.com>.
Both Windows Server 2008 and Windows 7 support hot plugging NIC on XenServer.
-----Original Message-----
From: Hari Kannan [mailto:hari.kannan@citrix.com]
Sent: 21 January 2013 22:17
To: cloudstack-dev@incubator.apache.org
Cc: Chiradeep Vittal; Brian Angus
Subject: RE: Review Request: API commands to add or remove NIC from a VM
Both XS and vSphere say they support it on Windows, so I suspect Windows must support it..
Hari
-----Original Message-----
From: Marcus Sorensen [mailto:shadowsor@gmail.com]
Sent: Monday, January 21, 2013 1:32 PM
To: cloudstack-dev@incubator.apache.org
Cc: Chiradeep Vittal; Brian Angus
Subject: Re: Review Request: API commands to add or remove NIC from a VM
I don't know if nic hotplug works on windows. Certainly a reboot would.
Anyone with a windows VM want to try adding a nic manually to a running VM?
On Mon, Jan 21, 2013 at 2:18 PM, Hari Kannan <ha...@citrix.com> wrote:
> Thank you Marcus -
>
> Regarding documentation, if we can provide at least a high level blurb
> on one or two sample OS-es, it would be great Regarding OS support,
> just wanted to check if it is possible on Windows OS also?
>
> Hari
>
> -----Original Message-----
> From: Marcus Sorensen [mailto:shadowsor@gmail.com]
> Sent: Monday, January 21, 2013 1:03 PM
> To: cloudstack-dev@incubator.apache.org
> Cc: Chiradeep Vittal; Brian Angus
> Subject: Re: Review Request: API commands to add or remove NIC from a
> VM
>
> 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
>
>
> > 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.
>
>
> > 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.
>
RE: Review Request: API commands to add or remove NIC from a VM
Posted by Hari Kannan <ha...@citrix.com>.
Both XS and vSphere say they support it on Windows, so I suspect Windows must support it..
Hari
-----Original Message-----
From: Marcus Sorensen [mailto:shadowsor@gmail.com]
Sent: Monday, January 21, 2013 1:32 PM
To: cloudstack-dev@incubator.apache.org
Cc: Chiradeep Vittal; Brian Angus
Subject: Re: Review Request: API commands to add or remove NIC from a VM
I don't know if nic hotplug works on windows. Certainly a reboot would.
Anyone with a windows VM want to try adding a nic manually to a running VM?
On Mon, Jan 21, 2013 at 2:18 PM, Hari Kannan <ha...@citrix.com> wrote:
> Thank you Marcus -
>
> Regarding documentation, if we can provide at least a high level blurb
> on one or two sample OS-es, it would be great Regarding OS support,
> just wanted to check if it is possible on Windows OS also?
>
> Hari
>
> -----Original Message-----
> From: Marcus Sorensen [mailto:shadowsor@gmail.com]
> Sent: Monday, January 21, 2013 1:03 PM
> To: cloudstack-dev@incubator.apache.org
> Cc: Chiradeep Vittal; Brian Angus
> Subject: Re: Review Request: API commands to add or remove NIC from a
> VM
>
> 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
>
>
> > 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.
>
>
> > 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.
>
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Marcus Sorensen <sh...@gmail.com>.
I don't know if nic hotplug works on windows. Certainly a reboot would.
Anyone with a windows VM want to try adding a nic manually to a running VM?
On Mon, Jan 21, 2013 at 2:18 PM, Hari Kannan <ha...@citrix.com> wrote:
> Thank you Marcus -
>
> Regarding documentation, if we can provide at least a high level blurb on
> one or two sample OS-es, it would be great
> Regarding OS support, just wanted to check if it is possible on Windows OS
> also?
>
> Hari
>
> -----Original Message-----
> From: Marcus Sorensen [mailto:shadowsor@gmail.com]
> Sent: Monday, January 21, 2013 1:03 PM
> To: cloudstack-dev@incubator.apache.org
> Cc: Chiradeep Vittal; Brian Angus
> Subject: Re: Review Request: API commands to add or remove NIC from a VM
>
> 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
>
>
> > 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.
>
>
> > 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.
>
RE: Review Request: API commands to add or remove NIC from a VM
Posted by Hari Kannan <ha...@citrix.com>.
Thank you Marcus -
Regarding documentation, if we can provide at least a high level blurb on one or two sample OS-es, it would be great
Regarding OS support, just wanted to check if it is possible on Windows OS also?
Hari
-----Original Message-----
From: Marcus Sorensen [mailto:shadowsor@gmail.com]
Sent: Monday, January 21, 2013 1:03 PM
To: cloudstack-dev@incubator.apache.org
Cc: Chiradeep Vittal; Brian Angus
Subject: Re: Review Request: API commands to add or remove NIC from a VM
On Mon, Jan 21, 2013 at 12:30 AM, Hari Kannan <ha...@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
> 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.
> 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.
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Chiradeep Vittal <Ch...@citrix.com>.
Yay!
On 1/30/13 4:48 PM, "Marcus Sorensen" <sh...@gmail.com> wrote:
>On Wed, Jan 30, 2013 at 12:17 PM, Marcus Sorensen <sh...@gmail.com>
>wrote:
>> On Wed, Jan 30, 2013 at 10:58 AM, Chiradeep Vittal
>> <Ch...@citrix.com> wrote:
>>>
>>>
>>> On 1/29/13 3:36 PM, "Marcus Sorensen" <sh...@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).
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Marcus Sorensen <sh...@gmail.com>.
On Wed, Jan 30, 2013 at 12:17 PM, Marcus Sorensen <sh...@gmail.com> wrote:
> On Wed, Jan 30, 2013 at 10:58 AM, Chiradeep Vittal
> <Ch...@citrix.com> wrote:
>>
>>
>> On 1/29/13 3:36 PM, "Marcus Sorensen" <sh...@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).
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Marcus Sorensen <sh...@gmail.com>.
On Wed, Jan 30, 2013 at 10:58 AM, Chiradeep Vittal
<Ch...@citrix.com> wrote:
>
>
> On 1/29/13 3:36 PM, "Marcus Sorensen" <sh...@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.
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Marcus Sorensen <sh...@gmail.com>.
On Tue, Jan 29, 2013 at 4:36 PM, Marcus Sorensen <sh...@gmail.com> wrote:
> On Mon, Jan 28, 2013 at 5:25 PM, Marcus Sorensen <sh...@gmail.com> wrote:
>> On Mon, Jan 28, 2013 at 5:21 PM, Chiradeep Vittal
>> <Ch...@citrix.com> wrote:
>>>
>>>
>>> On 1/28/13 12:38 PM, "Marcus Sorensen" <sh...@gmail.com> wrote:
>>>
>>>>On Mon, Jan 28, 2013 at 12:44 PM, Marcus Sorensen <sh...@gmail.com>
>>>>wrote:
>>>>> On Mon, Jan 28, 2013 at 12:31 PM, Chiradeep Vittal
>>>>> <Ch...@citrix.com> wrote:
>>>>>> Inline
>>>>>>
>>>>>> On 1/25/13 4:03 PM, "Brian Angus" <bl...@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.
>
>
> 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.
>
> 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.
>
> 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 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?
Oh, and I should mention that the code in question is also the
existing code used to add nics to virtual routers. So I'm a little
skiddish about changing it to wipe out configs if the plug fails, as I
don't know what effect it would have on the other code that calls it.
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Chiradeep Vittal <Ch...@citrix.com>.
On 1/29/13 3:36 PM, "Marcus Sorensen" <sh...@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
>
>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.
>
>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.
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Marcus Sorensen <sh...@gmail.com>.
On Mon, Jan 28, 2013 at 5:25 PM, Marcus Sorensen <sh...@gmail.com> wrote:
> On Mon, Jan 28, 2013 at 5:21 PM, Chiradeep Vittal
> <Ch...@citrix.com> wrote:
>>
>>
>> On 1/28/13 12:38 PM, "Marcus Sorensen" <sh...@gmail.com> wrote:
>>
>>>On Mon, Jan 28, 2013 at 12:44 PM, Marcus Sorensen <sh...@gmail.com>
>>>wrote:
>>>> On Mon, Jan 28, 2013 at 12:31 PM, Chiradeep Vittal
>>>> <Ch...@citrix.com> wrote:
>>>>> Inline
>>>>>
>>>>> On 1/25/13 4:03 PM, "Brian Angus" <bl...@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.
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.
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.
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 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?
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Marcus Sorensen <sh...@gmail.com>.
On Mon, Jan 28, 2013 at 5:21 PM, Chiradeep Vittal
<Ch...@citrix.com> wrote:
>
>
> On 1/28/13 12:38 PM, "Marcus Sorensen" <sh...@gmail.com> wrote:
>
>>On Mon, Jan 28, 2013 at 12:44 PM, Marcus Sorensen <sh...@gmail.com>
>>wrote:
>>> On Mon, Jan 28, 2013 at 12:31 PM, Chiradeep Vittal
>>> <Ch...@citrix.com> wrote:
>>>> Inline
>>>>
>>>> On 1/25/13 4:03 PM, "Brian Angus" <bl...@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.
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Chiradeep Vittal <Ch...@citrix.com>.
On 1/28/13 12:38 PM, "Marcus Sorensen" <sh...@gmail.com> wrote:
>On Mon, Jan 28, 2013 at 12:44 PM, Marcus Sorensen <sh...@gmail.com>
>wrote:
>> On Mon, Jan 28, 2013 at 12:31 PM, Chiradeep Vittal
>> <Ch...@citrix.com> wrote:
>>> Inline
>>>
>>> On 1/25/13 4:03 PM, "Brian Angus" <bl...@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.
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Marcus Sorensen <sh...@gmail.com>.
On Mon, Jan 28, 2013 at 12:44 PM, Marcus Sorensen <sh...@gmail.com> wrote:
> On Mon, Jan 28, 2013 at 12:31 PM, Chiradeep Vittal
> <Ch...@citrix.com> wrote:
>> Inline
>>
>> On 1/25/13 4:03 PM, "Brian Angus" <bl...@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?
>
>>
>>>>
>>>>
>>>>> 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+T
>>>>>emp
>>>>> 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...
>>
>> Yes, please add any information about limits, usage data, logs, failure
>> conditions and any OS dependencies.
>>
>>>>> 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.
>>>>
>>
>> If so, then it is a bug for both use cases. Doesn't it mean we shouldn't
>> fix at least one of the use cases? For example, if the network subnet is
>> 10.1.1.0/24 and I try to create a nic with IP 75.75.75.75/32? I am
>> referring to the 'ipaddress' parameter in the AddNicToVMCmd.
>
> We'll test this. My point was that our code will react the same way to
> your example as deployVirtualMachine does when you give it a bogus IP.
> If there's an issue there it's probably best handled as a bugfix in
> the upstream functions than patching a special fix for this branch.
This isn't currently an issue for either scenario. Bogus IP returns this:
Insufficient capacity when adding NIC to VM[User|testVM]:
com.cloud.exception.InsufficientVirtualNetworkCapcityException: Unable
to acquire Guest IP address for network
Ntwk[205|Guest|8]Scope=interface com.cloud.dc.DataCenter; id=1
>
>>
>>
>>>> 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
>>
>> I didn't know you could do live migration on devcloud?
>
> Why not? bring up another devcloud, add it as a second host.
>
>>
>>>>
>>>>> 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.
>>
>> That closes some holes. But since the async operation does not have a
>> state machine, it is still possible for someone to migrate / stop the VM
>> while the nic attach is in progress. It may not matter (and might actually
>> work). I would note this as a limitation (bug) and hope that Simon takes
>> care of this in his feature.
>>
>>>>
>>>>
>>>>> 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?
>>
>> Unlikely that any clean up is going to happen.
>
> There's actually a lot of cleanup related stuff in current master that
> needs to be addressed, e.g. bridges don't go away when they're no
> longer used, and VM names aren't removed from VR's hosts table when
> VMs are removed (maybe this has been recently fixed? I'll look into
> it). We'll get it taken care of if that bug exists.
>
>>
>>>>
>>>>
>>>>> 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.
>>
>> The FS is a good place to document this.
>>
>> Thanks
>> --
>> Chiradeep
>>
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.
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Marcus Sorensen <sh...@gmail.com>.
On Mon, Jan 28, 2013 at 12:31 PM, Chiradeep Vittal
<Ch...@citrix.com> wrote:
> Inline
>
> On 1/25/13 4:03 PM, "Brian Angus" <bl...@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?
>
>>>
>>>
>>>> 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+T
>>>>emp
>>>> 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...
>
> Yes, please add any information about limits, usage data, logs, failure
> conditions and any OS dependencies.
>
>>>> 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.
>>>
>
> If so, then it is a bug for both use cases. Doesn't it mean we shouldn't
> fix at least one of the use cases? For example, if the network subnet is
> 10.1.1.0/24 and I try to create a nic with IP 75.75.75.75/32? I am
> referring to the 'ipaddress' parameter in the AddNicToVMCmd.
We'll test this. My point was that our code will react the same way to
your example as deployVirtualMachine does when you give it a bogus IP.
If there's an issue there it's probably best handled as a bugfix in
the upstream functions than patching a special fix for this branch.
>
>
>>> 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
>
> I didn't know you could do live migration on devcloud?
Why not? bring up another devcloud, add it as a second host.
>
>>>
>>>> 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.
>
> That closes some holes. But since the async operation does not have a
> state machine, it is still possible for someone to migrate / stop the VM
> while the nic attach is in progress. It may not matter (and might actually
> work). I would note this as a limitation (bug) and hope that Simon takes
> care of this in his feature.
>
>>>
>>>
>>>> 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?
>
> Unlikely that any clean up is going to happen.
There's actually a lot of cleanup related stuff in current master that
needs to be addressed, e.g. bridges don't go away when they're no
longer used, and VM names aren't removed from VR's hosts table when
VMs are removed (maybe this has been recently fixed? I'll look into
it). We'll get it taken care of if that bug exists.
>
>>>
>>>
>>>> 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.
>
> The FS is a good place to document this.
>
> Thanks
> --
> Chiradeep
>
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Chip Childers <ch...@sungard.com>.
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 <sh...@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 <bl...@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" <bl...@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
>>>>>>> <ha...@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.
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Marcus Sorensen <sh...@gmail.com>.
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 <bl...@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" <bl...@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
>>>>>> <ha...@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.
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Chiradeep Vittal <Ch...@citrix.com>.
Inline
On 1/25/13 4:03 PM, "Brian Angus" <bl...@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.
>>
>>
>>> 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+T
>>>emp
>>> 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...
Yes, please add any information about limits, usage data, logs, failure
conditions and any OS dependencies.
>>> 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.
>>
If so, then it is a bug for both use cases. Doesn't it mean we shouldn't
fix at least one of the use cases? For example, if the network subnet is
10.1.1.0/24 and I try to create a nic with IP 75.75.75.75/32? I am
referring to the 'ipaddress' parameter in the AddNicToVMCmd.
>> 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
I didn't know you could do live migration on devcloud?
>>
>>> 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.
That closes some holes. But since the async operation does not have a
state machine, it is still possible for someone to migrate / stop the VM
while the nic attach is in progress. It may not matter (and might actually
work). I would note this as a limitation (bug) and hope that Simon takes
care of this in his feature.
>>
>>
>>> 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?
Unlikely that any clean up is going to happen.
>>
>>
>>> 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.
The FS is a good place to document this.
Thanks
--
Chiradeep
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Brian Angus <bl...@betterservers.com>.
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" <bl...@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
>>>>> <ha...@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.
>>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Marcus Sorensen <sh...@gmail.com>.
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
> 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.
> 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.
> 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.
> 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'.
> 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" <bl...@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
> >>> <ha...@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.
> >>>
> >>
> >>
> >>
> >
>
>
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Chiradeep Vittal <Ch...@citrix.com>.
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
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.
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
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
C. If you remove a default nic, what happens?
D. Can you remove the last nic?
E. Verify live migration after operation?
F. Operation blocks / fails during live migration / HA / start / stop
G. Usage events for remove / add operations
H. Interaction of removeNic with extant LB / PF / Static NAT/ FW rules
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.
Thanks
On 1/24/13 3:45 PM, "Brian Angus" <bl...@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
>>> <ha...@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.
>>>
>>
>>
>>
>
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Brian Angus <bl...@betterservers.com>.
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
>> <ha...@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.
>>
>
>
>
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Brian Angus <bl...@betterservers.com>.
On 01/21/2013 02:02 PM, Marcus Sorensen wrote:
> On Mon, Jan 21, 2013 at 12:30 AM, Hari Kannan <ha...@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.
>
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Marcus Sorensen <sh...@gmail.com>.
On Mon, Jan 21, 2013 at 12:30 AM, Hari Kannan <ha...@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
> 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.
> 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.
RE: Review Request: API commands to add or remove NIC from a VM
Posted by Hari Kannan <ha...@citrix.com>.
Can you please clarify a couple of points please -
1. The FS states that " Currently in cloudstack there can be only on NIC on a network in cloudstack." - this is not true right? We cannot add a network AFTER a VM is created, but you could have multiple nics when the VM is first created?
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?
3. Can you please confirm this is possible for any OS (windows/Linux)?
4. is there any UI component developed for this?
Hari
-----Original Message-----
From: Marcus Sorensen [mailto:shadowsor@gmail.com]
Sent: Friday, January 18, 2013 4:13 PM
To: Chiradeep Vittal
Cc: Brian Angus; cloudstack
Subject: Re: Review Request: API commands to add or remove NIC from a VM
Chiradeep, I sucked this into a development branch. I pulled master into that and he just sent me a patch that fixes it.
On Fri, Jan 18, 2013 at 5:04 PM, Chiradeep Vittal <ch...@gmail.com>wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9011/
>
> Can you rebase against current master?
>
>
> - Chiradeep
>
> On January 17th, 2013, 7:38 p.m., Brian Angus wrote:
> Review request for cloudstack and Marcus Sorensen.
> By Brian Angus.
>
> *Updated Jan. 17, 2013, 7:38 p.m.*
> Description
>
> This needs to be used to create a new feature branch.
>
> Feature Spec:
> https://cwiki.apache.org/confluence/display/CLOUDSTACK/Add+Remove+Netw
> orks+to+VMs Jira Request:
> https://issues.apache.org/jira/browse/CLOUDSTACK-645
>
> Added addNicToVirtualMachine, removeNicFromVirtualMachine, and updateDefaultNicForVirtualMachine api commands.
>
> Testing
>
> This was tested and works on devcloud and devcloud-kvm. I plan to add some automated tests as well.
>
> *Bugs: * CLOUDSTACK-645
> Diffs
>
> - api/src/com/cloud/vm/UserVmService.java (b1ebe10)
> - api/src/org/apache/cloudstack/api/command/user/vm/AddNicToVMCmd.java
> (PRE-CREATION)
> - api/src/org/apache/cloudstack/api/command/user/vm/RemoveNicFromVMCmd.java
> (PRE-CREATION)
> - api/src/org/apache/cloudstack/api/command/user/vm/UpdateDefaultNicForVMCmd.java
> (PRE-CREATION)
> - client/tomcatconf/commands.properties.in (182cbd8)
> - server/src/com/cloud/network/NetworkManagerImpl.java (0a1fcd7)
> - server/src/com/cloud/vm/UserVmManagerImpl.java (a6fbdb1)
> - server/src/com/cloud/vm/VirtualMachineManager.java (4f04617)
> - server/src/com/cloud/vm/VirtualMachineManagerImpl.java (4d94fc5)
> - server/test/com/cloud/vm/MockUserVmManagerImpl.java (27508b1)
> - server/test/com/cloud/vm/MockVirtualMachineManagerImpl.java
> (6723198)
>
> View Diff <https://reviews.apache.org/r/9011/diff/>
>
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Marcus Sorensen <sh...@gmail.com>.
Chiradeep, I sucked this into a development branch. I pulled master into
that and he just sent me a patch that fixes it.
On Fri, Jan 18, 2013 at 5:04 PM, Chiradeep Vittal <ch...@gmail.com>wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9011/
>
> Can you rebase against current master?
>
>
> - Chiradeep
>
> On January 17th, 2013, 7:38 p.m., Brian Angus wrote:
> Review request for cloudstack and Marcus Sorensen.
> By Brian Angus.
>
> *Updated Jan. 17, 2013, 7:38 p.m.*
> Description
>
> This needs to be used to create a new feature branch.
>
> Feature Spec: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Add+Remove+Networks+to+VMs
> Jira Request: https://issues.apache.org/jira/browse/CLOUDSTACK-645
>
> Added addNicToVirtualMachine, removeNicFromVirtualMachine, and updateDefaultNicForVirtualMachine api commands.
>
> Testing
>
> This was tested and works on devcloud and devcloud-kvm. I plan to add some automated tests as well.
>
> *Bugs: * CLOUDSTACK-645
> Diffs
>
> - api/src/com/cloud/vm/UserVmService.java (b1ebe10)
> - api/src/org/apache/cloudstack/api/command/user/vm/AddNicToVMCmd.java
> (PRE-CREATION)
> - api/src/org/apache/cloudstack/api/command/user/vm/RemoveNicFromVMCmd.java
> (PRE-CREATION)
> - api/src/org/apache/cloudstack/api/command/user/vm/UpdateDefaultNicForVMCmd.java
> (PRE-CREATION)
> - client/tomcatconf/commands.properties.in (182cbd8)
> - server/src/com/cloud/network/NetworkManagerImpl.java (0a1fcd7)
> - server/src/com/cloud/vm/UserVmManagerImpl.java (a6fbdb1)
> - server/src/com/cloud/vm/VirtualMachineManager.java (4f04617)
> - server/src/com/cloud/vm/VirtualMachineManagerImpl.java (4d94fc5)
> - server/test/com/cloud/vm/MockUserVmManagerImpl.java (27508b1)
> - server/test/com/cloud/vm/MockVirtualMachineManagerImpl.java (6723198)
>
> View Diff <https://reviews.apache.org/r/9011/diff/>
>
Re: Review Request: API commands to add or remove NIC from a VM
Posted by Chiradeep Vittal <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9011/#review15509
-----------------------------------------------------------
Can you rebase against current master?
- Chiradeep Vittal
On Jan. 17, 2013, 7:38 p.m., Brian Angus wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9011/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2013, 7:38 p.m.)
>
>
> Review request for cloudstack and Marcus Sorensen.
>
>
> Description
> -------
>
> This needs to be used to create a new feature branch.
>
> Feature Spec: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Add+Remove+Networks+to+VMs
> Jira Request: https://issues.apache.org/jira/browse/CLOUDSTACK-645
>
> Added addNicToVirtualMachine, removeNicFromVirtualMachine, and updateDefaultNicForVirtualMachine api commands.
>
>
> This addresses bug CLOUDSTACK-645.
>
>
> Diffs
> -----
>
> api/src/com/cloud/vm/UserVmService.java b1ebe10
> api/src/org/apache/cloudstack/api/command/user/vm/AddNicToVMCmd.java PRE-CREATION
> api/src/org/apache/cloudstack/api/command/user/vm/RemoveNicFromVMCmd.java PRE-CREATION
> api/src/org/apache/cloudstack/api/command/user/vm/UpdateDefaultNicForVMCmd.java PRE-CREATION
> client/tomcatconf/commands.properties.in 182cbd8
> server/src/com/cloud/network/NetworkManagerImpl.java 0a1fcd7
> server/src/com/cloud/vm/UserVmManagerImpl.java a6fbdb1
> server/src/com/cloud/vm/VirtualMachineManager.java 4f04617
> server/src/com/cloud/vm/VirtualMachineManagerImpl.java 4d94fc5
> server/test/com/cloud/vm/MockUserVmManagerImpl.java 27508b1
> server/test/com/cloud/vm/MockVirtualMachineManagerImpl.java 6723198
>
> Diff: https://reviews.apache.org/r/9011/diff/
>
>
> Testing
> -------
>
> This was tested and works on devcloud and devcloud-kvm. I plan to add some automated tests as well.
>
>
> Thanks,
>
> Brian Angus
>
>