[Engine-devel] Fwd: Problem in REST API handling/displaying of logical networks
Miki Kenneth
mkenneth at redhat.com
Thu Jul 5 07:24:58 UTC 2012
----- Original Message -----
>
>
> ----- Original Message -----
> > From: "Itamar Heim" <iheim at redhat.com>
> > To: "Simon Grinberg" <simon at redhat.com>
> > Cc: "engine-devel" <engine-devel at ovirt.org>
> > Sent: Wednesday, July 4, 2012 7:38:46 PM
> > Subject: Re: [Engine-devel] Fwd: Problem in REST API
> > handling/displaying of logical networks
> >
> > On 07/04/2012 07:27 PM, Simon Grinberg wrote:
> > >
> > >
> > > ----- Original Message -----
> > >> From: "Livnat Peer" <lpeer at redhat.com>
> > >> To: "Itamar Heim" <iheim at redhat.com>
> > >> Cc: "engine-devel" <engine-devel at ovirt.org>
> > >> Sent: Tuesday, July 3, 2012 10:06:37 PM
> > >> Subject: Re: [Engine-devel] Fwd: Problem in REST API
> > >> handling/displaying of logical networks
> > >>
> > >> On 03/07/12 21:28, Itamar Heim wrote:
> > >>> On 07/03/2012 10:30 AM, Livnat Peer wrote:
> > >>>> On 02/07/12 17:35, Mike Kolesnik wrote:
> > >>>>> Hi All,
> > >>>>>
> > >>>>> I would like to hear opinions about a behaviour that I think
> > >>>>> is
> > >>>>> problematic in
> > >>>>> REST API handling of logical networks.
> > >>>>>
> > >>>>> -- Intro --
> > >>>>> Today in the REST API we are exposing two collections for
> > >>>>> "logical
> > >>>>> network" related entities.
> > >>>>>
> > >>>>> First is a top level collection which is out of any context
> > >>>>> at
> > >>>>> the
> > >>>>> address
> > >>>>> http://engine/api/networks.
> > >>>>> Second is a sub-collection in the context of a cluster:
> > >>>>> http://engine/api/cluster/xxx/networks
> > >>>>>
> > >>>>> The network itself is defined per DC level, so for each DC
> > >>>>> you
> > >>>>> would
> > >>>>> have
> > >>>>> at least one logical network for management, which has some
> > >>>>> properties such
> > >>>>> as STP, MTU, etc..
> > >>>>> The top level collection is used to create/delete such
> > >>>>> network
> > >>>>> entities.
> > >>>>>
> > >>>>> The sub-collection in the context of a Cluster is used to
> > >>>>> attach/detach a
> > >>>>> network from the DC of that cluster.
> > >>>>> The network in the context of a cluster has some additional
> > >>>>> information,
> > >>>>> let's
> > >>>>> say for example 'status' of the network:
> > >>>>> If a network is defined on all hosts in the cluster
> > >>>>> then
> > >>>>> it's
> > >>>>> status is
> > >>>>> 'Operational'.
> > >>>>> If a network is not defined on some of the hosts in the
> > >>>>> cluster
> > >>>>> then
> > >>>>> it's
> > >>>>> status is 'Not Operational'[1].
> > >>>>>
> > >>>>>
> > >>>>> -- Problem --
> > >>>>> The problem is that details which are only relevant in
> > >>>>> context
> > >>>>> of
> > >>>>> a
> > >>>>> cluster, are still displayed in the root context as well
> > >>>>> (e.g:
> > >>>>> 'status').
> > >>>>> This can, in certain cases, cause unexpected behaviour.
> > >>>>>
> > >>>>> For example, let's consider this topology:
> > >>>>> Data Center A
> > >>>>> |
> > >>>>> |\____ Network 'red'
> > >>>>> |\____ Cluster A1
> > >>>>> | \______ Network 'red' attached
> > >>>>> \____ Cluster A2
> > >>>>> \______ Network 'red' attached
> > >>>>>
> > >>>>> If the 'status' is the same on all the clusters that the
> > >>>>> network
> > >>>>> is
> > >>>>> attached to
> > >>>>> (A1, A2) then there will be one element in the top level
> > >>>>> collection,
> > >>>>> with the
> > >>>>> network details and the 'status' field representing the state
> > >>>>> (which
> > >>>>> is same
> > >>>>> for all networks in the cluster contexts of the cluster).
> > >>>>> If, however, the status is not the same (ie. on A1 the
> > >>>>> network
> > >>>>> is
> > >>>>> 'Operational' and on A2 it is 'Non Operational') then the
> > >>>>> top-level
> > >>>>> collection will show two elements for the network, where all
> > >>>>> network
> > >>>>> details are the same and only the 'status' field is
> > >>>>> different.
> > >>>>>
> > >>>>
> > >>>> That sounds like a bug to me.
> > >>>> I think that top collection should include only DC level
> > >>>> properties and
> > >>>> not cluster level properties, status should not be there (same
> > >>>> as
> > >>>> display required etc.)
> > >>>>
> > >>>>
> > >>>>> This is problematic IMHO for several reasons:
> > >>>>> 1. Showing one network in certain states, and multiple
> > >>>>> copies
> > >>>>> of this
> > >>>>> network in other states is not optimal, to say the
> > >>>>> least.
> > >>>>> 2. In the top-level collection there is no indicator of
> > >>>>> the
> > >>>>> cluster
> > >>>>> for which
> > >>>>> the network is displayed, so there is no way to
> > >>>>> differentiate
> > >>>>> between the
> > >>>>> two 'red' network elements (they will have same id,
> > >>>>> name,
> > >>>>> etc.).
> > >>>>> 3. There is a certain asymmetry between the remove
> > >>>>> action[2]
> > >>>>> and the
> > >>>>> result in that you would expect: you either remove a
> > >>>>> network but
> > >>>>> in the
> > >>>>> result you would see several elements removed.
> > >>>>>
> > >>>>>
> > >>>>> -- Proposed Solutions --
> > >>>>> Personally I can think of several solutions to this problem:
> > >>>>> 1. Declare the top-level collection as a collection of
> > >>>>> all
> > >>>>> networks
> > >>>>> that are
> > >>>>> either attached to cluster or not, and if they are
> > >>>>> indeed
> > >>>>> attached
> > >>>>> then
> > >>>>> show the details for each cluster, including a link
> > >>>>> to
> > >>>>> the
> > >>>>> cluster.
> > >>>>> 2. Declare the top-level collection as a collection of
> > >>>>> all
> > >>>>> networks
> > >>>>> that are
> > >>>>> defined in data-centers, but they will not contain
> > >>>>> any
> > >>>>> cluster
> > >>>>> specific
> > >>>>> data, and thus each entry is unique.
> > >>>>>
> > >>>>> Solution #2 is breaking the API backwards-compatibility,
> > >>>>> since
> > >>>>> it
> > >>>>> includes
> > >>>>> removing certain fields that have appeared today (namely
> > >>>>> 'status'
> > >>>>> and
> > >>>>> 'display') but IMO would give a better experience since the
> > >>>>> top-level
> > >>>>> collection is actually used for managing networks, and not
> > >>>>> their
> > >>>>> attachment
> > >>>>> to clusters which should be done in the context of each
> > >>>>> cluster.
> > >>>>>
> > >>>> I really don't think top collections should include cluster
> > >>>> networks it
> > >>>> is not user-friendly to say the least.
> > >>>
> > >>> how is that different from top collections including VMs and
> > >>> templates?
> > >>> (or logical networks becoming main tab in the UI going forward)
> > >>>
> > >>
> > >> I think you missed the point of cluster network entity vs. DC
> > >> network
> > >> entity.
> > >>
> > >> we should have in the top collection a DC network, I think we
> > >> should
> > >> not
> > >> display the network per cluster in top network collection (that
> > >> can
> > >> be
> > >> viewed under the cluster context).
> > >
> > > Actually the API has the same concept as you suggest for storage
> > > domains.
> > > At the top level you don't have a status field, but under data
> > > center level, where it's valid then you get the status property.
> > >
> > > Same should go for networks.
> > > The status property should be added only where it's valid, in
> > > this
> > > case the cluster level sub-collection
> >
> > so sounds like we want to declare these properties deprecated to be
> > able
> > to remove them in a future version?
>
> I guess so,
> The question is, are there other location where the status property
> (or any other property) exists at an irrelevant level. Unless we
> want to go into the effort of mapping them all now we probably need
> to define a concept and anything not complying to that is a bug that
> is allowed to be fixed without considering it as breaking the API.
>
> Thoughts?
>
+1
I agree that this is a bug and I DO suggest we go into the effort of reviewing the other objects as well.
This is too major to just fix this one, and wait until we bump into another one...
Any suggestions of how to do that?
>
>
>
> > _______________________________________________
> > Engine-devel mailing list
> > Engine-devel at ovirt.org
> > http://lists.ovirt.org/mailman/listinfo/engine-devel
> >
> _______________________________________________
> Engine-devel mailing list
> Engine-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/engine-devel
>
More information about the Engine-devel
mailing list