----- Original Message ----- > From: "Dan Kenigsberg" <dan...@redhat.com> > To: "Livnat Peer" <lp...@redhat.com> > Cc: "Adam Litke" <a...@us.ibm.com>, "Alona Kaplan" <alkap...@redhat.com>, > vdsm-devel@lists.fedorahosted.org, "Antoni > Segura Puimedon" <asegu...@redhat.com> > Sent: Wednesday, December 5, 2012 2:06:18 PM > Subject: Re: [vdsm] link state semantics > > 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".
I agree as well. AFAICT, I think you bought us a way out. > > > > > And I also agree with danken that we should keep the API > > consistent, > > createVM hotplug and updateDevice should behave similarly. Indeed! I'm going over them to try to make it so. > > > > 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