[Engine-devel] network wiring DD questions/comments

[Alona Kaplan]

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