20 Nov
2012
20 Nov
'12
4:06 p.m.
> Hi, > > I"ve added my answers as inline comments. > > Thanks, > Alona. > > > 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) > I"ve updated the detailed wiki- > 1. On Add- there will be a canDoAction error if the user will try to > change the network to "none" on 3.1 or less. > I don't think the "none" should be removed from the combo. The user > won't understand why sometimes he has "none" and sometimes not. > 2. On Update- there are two problematic cases with 3.1 or less- > 2.1 The user changes the network to "none" - should behave as add. > 2.2 The vnic is plugged (before and after the update) and the only > change is the network. > In this case an "UpdateDevice" verb should be sent to vdsm, so > if it is 3.1 or less the backend should throw a canDoAction. (I > updated the wiki we this scenario) > But, if mac address, type or plug were updated- we are sending > plug/unplug to the vdsm. So the update can be done although for > 3.1. > We talked about disabling all the fields (except "plug/unplug") > in the dialog if the nic is plugged and rollback the ui if the > user change from plugged to unplugged and then to plugged > again. > I don't think it is right. Because if the user wants just to > update the mac for example, we should allow it even if the nic > is plugged. > I think the canDoAction is enough. > 2.3 I think for 3.0 or less, the update button should be disabled if > the vm is not down. (Updated the wiki). > > > > 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. > > > I will talk to Simon. If he is ok with it, I will remove the link > state from the feature. > > > 3. looking on the VDSM API - > > * why do we need 'type' ? > Danken wanted UpdateDevice to be a common api for updating all the > devices. By the type the vdsm decides which device is updated. > > * why do we need 'device'? > You are right, it should be removed. (I updated the wiki) > > * why do we need 'alias'? > We don't send the mac. This is the "unique id" of the nic. > The libvirt sets a unique alias for each device(disk, interface...), > so it can be used as the identifier of the device. > > * 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. > > > When changing the type we are internally using the plug/unplug > actions. > If the unplug succeeds and the plug fails- the backend/ui will get an > error from the plug action. > At the end of such situation the vnic will be unplugged. > I"m not sure what we should do with the type- store the new type or > rollback to the old value in the db? > > > 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. > We already have events for update_vnic failure- > NETWORK_UPDATE_VM_INTERFACE_FAILED and success- > NETWORK_UPDATE_VM_INTERFACE. > And as we use the old plug/unplug actions we don't have to add new > events to the audit log. > I don't think we have to add extra code to the events. > > > > > 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? > You are right, should be boolean. (Updated the wiki). > (If Simon will agree to remove the link state from the feature, will > be removed...) > > > > 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.) > > > > * If we remove ActivateDeactivateVmNic there is no need to rename > > it > > ;) > We use it internally. > Also we have to keep it exposed for backward compatibility with Rest > API. > I"m not sure it should be renamed... > > > > > > Thanks, Livnat > > >