Re: [Engine-devel] network wiring DD questions/comments

> 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 >> >