9:06 a.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
>