[Engine-devel] network wiring DD questions/comments

[Livnat Peer]

On 20/11/12 17:06, Alona Kaplan wrote:
> 
>> 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.

true, we can enable changing the MAC and the type in 3.1.
I think the UI should reflect better what the user can or can't do.
leaving it only to the canDoAction is not good enough.

How about in 3.1 -

If the device is originally unplugged
 - everything is enabled.

If the device is originally plugged
 - everything is disabled except for
	* changing to unplugged
	* changing the type
	* changing the MAC address.
If the user changes to unplugged he can edit all fields,
If the user changes to plugged (back) then we reset everything except
for MAC and type.

It's not too complicated if you keep the original plugged value and
evaluate what should be done on any change from plugged to unplugged and
vice versa.


>>  2.3 I think for 3.0 or less, the update button should be disabled if
>>  the vm is not down. (Updated the wiki).

There is a request t see the details of the NIC

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

I agree it should stay unplugged, I would not perform roll back of the
type, MAC or any other field if the user will give it another try and
succeed the configuration would be as he wanted it to be.
IMO there is no right answer to that, I can think of use cases where
roll back is the right behavior from the user POV and I can also think
of scenarios where not rolling back is good for the user...


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

Great, adding this info to the wiki is useful for fully understanding
the feature, users know what to expect (although it did not change).

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

I agree we can use it internally but the API should not use it.

>>>
>>>
>>> Thanks, Livnat
>>>
>>