Re: [vdsm] link state semantics

2012-12-05 Thread Adam Litke
On Wed, Dec 05, 2012 at 04:25:48AM -0500, Antoni Segura Puimedon wrote:
> 
> 
> - Original Message -
> > From: "Igor Lvovsky" 
> > To: "Antoni Segura Puimedon" 
> > Cc: "Alona Kaplan" , vdsm-devel@lists.fedorahosted.org
> > Sent: Wednesday, December 5, 2012 10:17:50 AM
> > Subject: Re: [vdsm] link state semantics
> > 
> > 
> > 
> > - Original Message -
> > > From: "Antoni Segura Puimedon" 
> > > To: vdsm-devel@lists.fedorahosted.org
> > > Cc: "Alona Kaplan" 
> > > Sent: Tuesday, December 4, 2012 7:32:34 PM
> > > Subject: [vdsm] link state semantics
> > > 
> > > 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']}
> > > 
> > 
> > If you are going for this use 'linkState'
> > 
> > > 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'.
> > 
> > In general +1 for new one, with a little doubt.
> > It looks a bit inconsistent that we leave the network as is if it
> > omitted from input,
> > but if linkState is 'disconnected' we will move it to dummy bridge.
> > But I can live with it.
> 
> Yes, the 'disconnected' overrules the network and that, as you point
> out, can be a source of confusion. I propose to add a warning to the
> return dictionary that tells the user that setting disconnected overrules
> any network setting.
> 
> > 
> > > 
> > > 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.
> > > 
> > 
> > +1 for Adam's approach, just don't forget to unset portMirroring from
> > all nets setted before
> > if they not in new portMirroring = [a,b,z]
> 
> So you're saying:
> 
> 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 AND unset any other mirroring.
> 
> I'm fine with it, I think it is even more complete and correct.

Yes, +1.

> > 
> > > I would really welcome comments on this to have finally an
> > > agreement
> > > to the api for this
> > > feature.
> > > 
> > > Best,
> > > 
> > > Toni
> > > ___
> > > 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

-- 
Adam Litke 
IBM Linux Technology Center

___
vdsm-devel mailing list
vdsm-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel


Re: [vdsm] link state semantics

2012-12-05 Thread Antoni Segura Puimedon


- Original Message -
> From: "Dan Kenigsberg" 
> To: "Livnat Peer" 
> Cc: "Adam Litke" , "Alona Kaplan" , 
> vdsm-devel@lists.fedorahosted.org, "Antoni
> Segura Puimedon" 
> 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 me

Re: [vdsm] link state semantics

2012-12-05 Thread Dan Kenigsberg
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


Re: [vdsm] link state semantics

2012-12-05 Thread Livnat Peer
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.

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


Re: [vdsm] link state semantics

2012-12-05 Thread Dan Kenigsberg
On Wed, Dec 05, 2012 at 02:21:16PM +0200, 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".
> 
> 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".

Hmm, a split second after sending this, I've realized that my suggestion
has a similar non-orthogonality problem: when you remove the network
attribute, you implictly change the linkState to "down".

So both options are bad :-(
Still I feel that linkState is a "lesser" attribute, relative to
network, and it would be better to have network imply linkState and not
vice versa.

Dan.

___
vdsm-devel mailing list
vdsm-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel


Re: [vdsm] link state semantics

2012-12-05 Thread Dan Kenigsberg
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".

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

Dan.
___
vdsm-devel mailing list
vdsm-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel


Re: [vdsm] link state semantics

2012-12-05 Thread Antoni Segura Puimedon


- Original Message -
> From: "Igor Lvovsky" 
> To: "Antoni Segura Puimedon" 
> Cc: "Alona Kaplan" , vdsm-devel@lists.fedorahosted.org
> Sent: Wednesday, December 5, 2012 10:17:50 AM
> Subject: Re: [vdsm] link state semantics
> 
> 
> 
> - Original Message -
> > From: "Antoni Segura Puimedon" 
> > To: vdsm-devel@lists.fedorahosted.org
> > Cc: "Alona Kaplan" 
> > Sent: Tuesday, December 4, 2012 7:32:34 PM
> > Subject: [vdsm] link state semantics
> > 
> > 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']}
> > 
> 
> If you are going for this use 'linkState'
> 
> > 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'.
> 
> In general +1 for new one, with a little doubt.
> It looks a bit inconsistent that we leave the network as is if it
> omitted from input,
> but if linkState is 'disconnected' we will move it to dummy bridge.
> But I can live with it.

Yes, the 'disconnected' overrules the network and that, as you point
out, can be a source of confusion. I propose to add a warning to the
return dictionary that tells the user that setting disconnected overrules
any network setting.

> 
> > 
> > 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.
> > 
> 
> +1 for Adam's approach, just don't forget to unset portMirroring from
> all nets setted before
> if they not in new portMirroring = [a,b,z]

So you're saying:

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 AND unset any other mirroring.

I'm fine with it, I think it is even more complete and correct.
> 
> > I would really welcome comments on this to have finally an
> > agreement
> > to the api for this
> > feature.
> > 
> > Best,
> > 
> > Toni
> > ___
> > 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


Re: [vdsm] link state semantics

2012-12-05 Thread Igor Lvovsky


- Original Message -
> From: "Antoni Segura Puimedon" 
> To: vdsm-devel@lists.fedorahosted.org
> Cc: "Alona Kaplan" 
> Sent: Tuesday, December 4, 2012 7:32:34 PM
> Subject: [vdsm] link state semantics
> 
> 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']}
> 

If you are going for this use 'linkState'

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

In general +1 for new one, with a little doubt.
It looks a bit inconsistent that we leave the network as is if it omitted from 
input,
but if linkState is 'disconnected' we will move it to dummy bridge.
But I can live with it.

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

+1 for Adam's approach, just don't forget to unset portMirroring from all nets 
setted before
if they not in new portMirroring = [a,b,z]

> I would really welcome comments on this to have finally an agreement
> to the api for this
> feature.
> 
> Best,
> 
> Toni
> ___
> 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


Re: [vdsm] link state semantics

2012-12-04 Thread Adam Litke
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.

+1 to the updated proposal.  Is there any better way to do it?

-- 
Adam Litke 
IBM Linux Technology Center

___
vdsm-devel mailing list
vdsm-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel