On 05/07/12 10:24, Miki Kenneth wrote: > > > ----- Original Message ----- >> >> >> ----- Original Message ----- >>> From: "Itamar Heim" <ih...@redhat.com> >>> To: "Simon Grinberg" <si...@redhat.com> >>> Cc: "engine-devel" <engine-devel@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" <lp...@redhat.com> >>>>> To: "Itamar Heim" <ih...@redhat.com> >>>>> Cc: "engine-devel" <engine-devel@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@ovirt.org >>> http://lists.ovirt.org/mailman/listinfo/engine-devel >>> >> _______________________________________________ >> Engine-devel mailing list >> Engine-devel@ovirt.org >> http://lists.ovirt.org/mailman/listinfo/engine-devel >> > _______________________________________________ > Engine-devel mailing list > Engine-devel@ovirt.org > http://lists.ovirt.org/mailman/listinfo/engine-devel > _______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
