[Engine-devel] Fwd: Problem in REST API handling/displaying of logical networks

[Itamar Heim]

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?