On Wed, Dec 05, 2012 at 02:40:44PM +0200, Livnat Peer wrote:
> On 05/12/12 14:21, Dan Kenigsberg wrote:
> > On Tue, Dec 04, 2012 at 03:10:00PM -0600, Adam Litke wrote:
> >> On Tue, Dec 04, 2012 at 12:32:34PM -0500, Antoni Segura Puimedon wrote:
> >>> Hi list!
> >>>
> >>> We are working on the new 3.2 feature for adding support for updating VM
> >>> devices, more specifically at the moment network devices.
> >>>
> >>> There is one point of the design which is not yet consensual and we'd 
> >>> need to agree on a proper and clean design that would satisfy us all:
> >>>
> >>> My current proposal, as reflected by patch:
> >>>    http://gerrit.ovirt.org/#/c/9560/5/vdsm_api/vdsmapi-schema.json
> >>> and its parent is to have a linkActive boolean that is true for link
> >>> status 'up' and false for link status 'down'.
> >>>
> >>> We want to support a none (dummy) network that is used to dissociate vnics
> >>> from any real network. The semantics, as you can see in the patch are that
> >>> unless you specify a network, updateDevice will place the interface on 
> >>> that
> >>> network. However, Adam Litke argues that not specifying a network should
> >>> keep the vnic on the network it currently is, as network is an optional
> >>> parameter and 'linkActive' is also optional and has this "preserve current
> >>> state" semantics.
> >>>
> >>> I can certainly see the merit of what Adam proposes, and the 
> >>> implementation
> >>> would be that linkActive becomes an enum like so:
> >>>
> >>> {'enum': 'linkState'/* or linkActive */ , 'data': ['up', 'down', 
> >>> 'disconnected']}
> >>>
> >>> With this change, network would only be changed if one different than the 
> >>> current
> >>> one is specified and the vnic would be taken to the dummy bridge when the 
> >>> linkState
> >>> would be set to 'disconnected'.
> >>>
> >>> There is also an objection, raised by Adam about the semantics of 
> >>> portMirroring.
> >>> The current behavior from my patch is:
> >>>
> >>> portMirroring is None or is not set -> No action taken.
> >>> portMirroring = [] -> No action taken.
> >>> portMirroring = [a,b,z] -> Set port mirroring for nets a,b and z to the 
> >>> specified vnic.
> >>>
> >>> His proposal is:
> >>> portMirroring is None or is not set -> No action taken.
> >>> portMirroring = [] -> Unset port mirroring to the vnic that is currently 
> >>> set.
> >>> portMirroring = [a,b,z] -> Set port mirroring for nets a,b and z to the 
> >>> specified vnic.
> >>>
> >>> I would really welcome comments on this to have finally an agreement to 
> >>> the api for this
> >>> feature.
> > 
> > ack for portMirroring semantics. [] must mean "no networks are mirrored
> > to this nic".
> > 
> 
> +1, It makes sense.
> 
> > I am less sure about allowing a "No action taken" semantics, as it
> > add the current state to the API and complicates it. I'd also like to
> > keep the updateDevice API as similar as possible to vmCreate one.
> > 
> > However, we already have defaults in vmCreate (for pci address
> > allocation, portmirroring off) so I suppose I can join Adam here.
> > 
> > But this does not mean we should allow a default everywhere. I find it
> > unintuitive to have a linkState=disconnected, as setting this attribute
> > should clear the network attribute - they become non-orthogonal. So in
> > my opinion, a missing network should mean "nic is connected to no
> > network" rather than "keep current connection".
> 
> I find the 'disconnect' state a bit problematic, a 'test' I do to see if
> things make sense is a comparison to the 'physical' world.
> In the physical world a link can have up or down, any other state that
> we add is less intuitive and IMO should be avoided.
> The 'problematic' status we are trying to express in the API is a
> network card that has no wire connected to it, how about using an empty
> network name to express that no network is connected to this card?
> 
> Then we can keep the semantics of not sending a network-name means no
> change in the network.

Usually I dislike attaching special semantics to the empty string, but
it sounds like a reasonable way out this time.

+1 to network='' meaning "connect this nic to nothing".

> 
> And I also agree with danken that we should keep the API consistent,
> createVM hotplug and updateDevice should behave similarly.
> 
> Livnat
> 
> > 
> > Dan.
> > _______________________________________________
> > vdsm-devel mailing list
> > vdsm-devel@lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
> > 
> 
_______________________________________________
vdsm-devel mailing list
vdsm-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel

Reply via email to