[Engine-devel] Fwd: Problem in REST API handling/displaying of logical networks
Livnat Peer
lpeer at redhat.com
Thu Jul 5 07:51:28 UTC 2012
On 05/07/12 10:24, Miki Kenneth wrote:
>
>
> ----- 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...
Mike i see there a general consensus that this is a bug and the top
level entity should be a DC network.
Can you please open a bug / update the existing bug to reflect that.
Thanks, Livnat
> 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
>>
> _______________________________________________
> 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