Few more (here and inline):
1. Add can-do-action for network name with name 'none'. This is not must - if we change none with N/A in the network drop-down list.
Instead of "none" there will be an empty cell, to avoid confusion if a network name "none" will be added (I"ve updated the wiki). But still, the canDoAction have to be modified to support null network.
2. In "plugged --> plugged" - updateVmInteface should be replaced with updateVmDevice. done.
On 11/20/2012 09:40 AM, Livnat Peer wrote:
Hi Alona, I reviewed the DD of wired/unwired and I have a few questions/comments -
1. update vnic while the VM is running should be limited to cluster 3.2 (except for plug/unplug)
2. RunVM - why do we need to pass the 'linkState' property to VDSM? I think that if the link is down we should pass none as the network and if the link is up we should pass the network name, like we did so far. the link-state is internal implementation detail in the engine.
Isn't there any consideration from current 3.2 VDSM versions built from source ? I agree that stable/formal VDSM which supports 3.2 should be capable of handling empty network name. If decided to pass the linkState - it must be sent with a default value (true), same goes for createVM.
3. looking on the VDSM API - * why do we need 'type' ? * why do we need 'device'? * why do we need 'alias'? * As I wrote I think we don't need 'linkState'
4. I am missing the error handling description when the user performs - Update Vnic while changing the type for example. What is the behavior if we succeed to unplug but not plug after that, etc.
5. Updated APIs section -This section is based on the fact that we pass linkstate to vdsm which is not needed IMO, so if we change that this section should be updated accordingly.
6. The EVENT section is empty, what events should be issued to audit log? I think we should have an even for update vnic and if plug/unplug is required a second event should be issued.
If we keep using internally the ActivateDeactivateVmNic command by the UpdateVmNic command - each command should create the relevant event-log.
7. why does link state is varchar in DB? you expect to support other option in addition to up/Down (which can be represented by 1 bit), maybe n/a status?
+1
8. About the open issues-
* About deprecation of ActivateDeactivateVmNic command - I think that we should remove this command from the backend to avoid duplicate flows and bugs etc. for Backwards compatibility we should update the clients to use the new UpdateVmNetworkInterface. the down side for that is we have to make the can do action validation more complicated (according to cluster level etc.)
It will make the command execution more complex as well - since the ActivateDeactivateVmNic command is non-transactive - so the internal implementation should be copied to certain flows of the UpdateVmNetworkInterface (which involves VDSM calls).
In any case - UpdateVmNetworkInterface should be marked as non-transactive.
* If we remove ActivateDeactivateVmNic there is no need to rename it ;)
In case we decide to keep ActivateDeactivateVmNic - according to the design changes it will pass also the linkState to VDSM, therefore i won't modify its name, as it performs wire/unwire in addition to plug/unplug.
Thanks, Livnat _______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel