It is valid, as I've implemented it. So we need to decide if we're using 'details' or rootdisksize as an api param. That's why I'm asking.
On Mon, Mar 10, 2014 at 1:43 AM, Bharat Kumar <bharat.ku...@citrix.com> wrote: > Hi All, > the roodiskresize is no longer valid. as there is no code which is using > rootdiskresize currently. > > As a part of the custom service offering we had to enable specifying custom > values to parameters cpu, memory and cpuCores. > instead of adding a parameter for each of these values we changed it use a > details map. This will also not require any further > changes in the API if we need to add some more custom values in future. > > On 08-Mar-2014, at 1:42 am, Marcus <shadow...@gmail.com> wrote: > >> Any suggestion? Do we go forward assuming that the correct parameter >> for resize on deploy is: >> >> deployVirtualMachine&details[0].rootdisksize=3 >> >> or do we change it to >> >> deployVirtualMachine&rootdisksize=3 >> >> On Tue, Mar 4, 2014 at 4:14 PM, Marcus <shadow...@gmail.com> wrote: >>> Ok, sounds like there needs to be some work done to make these more >>> consistent, perhaps. Can you comment on why rootdisksize was made from >>> a parameter into a part of the details map? >>> >>> On Tue, Mar 4, 2014 at 3:12 AM, Bharat Kumar <bharat.ku...@citrix.com> >>> wrote: >>>> Hi ALL, >>>> There are many other APIs that use Map like createNetworkOffering, >>>> updateZone, createTemplate, in most of the cases we do not >>>> say how to use maps, one way would be to write this in the description or >>>> to >>>> use the same way to access maps of all APIs. >>>> >>>> BTW the way to use details in deploy vm API is >>>> details[0].foo=bar&details[1].baz=12 where foo and baz are keys. >>>> >>>> Also if we want to use the regix protected static final String >>>> MAP_KEY_PATTERN_EXPRESSION = "^([^\\[^\\]]+)\\[(\\d+)\\]\\.key$"; >>>> protected static final >>>> String MAP_VALUE_PATTERN_EXPRESSION = "^[^\\[^\\]]+\\[\\d+\\]\\.value$"; >>>> >>>> wil this work in the following case. I believe service is the key here >>>> which >>>> repeats. >>>> http://10.147.59.119:8080/client/api?command=createNetworkOffering&response=json&sessionkey=/kGFJDXFmMQU8JZnnC7QFfj3tV8=&name=bharat&displayText=bharat&guestIpType=Isolated&lbType=publicLb&; >>>> servicecapabilitylist[0].service=SourceNat&servicecapabilitylist[0].capabilitytype=SupportedSourceNatTypes& >>>> servicecapabilitylist[0].capabilityvalue=peraccount& >>>> servicecapabilitylist[1].service=lb&servicecapabilitylist[1].capabilitytype=SupportedLbIsolation& >>>> servicecapabilitylist[1].capabilityvalue=dedicated&availability=Optional&egresspolicy=ALLOW&state=Creating&status=Creating&allocationstate=Creating&supportedServices=Vpn,Dhcp,Dns,Firewall,Lb,UserData,SourceNat,StaticNat,PortForwarding&specifyIpRanges=false&specifyVlan=false&isPersistent=false&conservemode=false&serviceProviderList[0].service=Vpn&serviceProviderList[0].provider=VirtualRouter&serviceProviderList[1].service=Dhcp&serviceProviderList[1].provider=VirtualRouter&serviceProviderList[2].service=Dns&serviceProviderList[2].provider=VirtualRouter&serviceProviderList[3].service=Firewall&serviceProviderList[3].provider=VirtualRouter&serviceProviderList[4].service=Lb&serviceProviderList[4].provider=VirtualRouter&serviceProviderList[5].service=UserData&serviceProviderList[5].provider=VirtualRouter&serviceProviderList[6].service=SourceNat&serviceProviderList[6].provider=JuniperSRX&serviceProviderList[7].service=StaticNat&serviceProviderList[7].provider=JuniperSRX&serviceProviderList[8].service=PortForwarding&serviceProviderList[8].provider=JuniperSRX&egressdefaultpolicy=true&traffictype=GUEST&_=1393925230248 >>>> >>>> >>>> >>>> On 04-Mar-2014, at 2:30 am, Marcus <shadow...@gmail.com> wrote: >>>> >>>> Along these lines, the details parameter in deployVirtualMachine seems >>>> broken. If I call "details[0].key=foo,details[0].value=bar", it stores >>>> entries in the database like this: >>>> >>>> id | vmid | name | value | display >>>> >>>> 12 | 25 | value | bar | 1 >>>> 13 | 25 | key | foo | 1 >>>> >>>> It seems as though this might be correct per Alena's email, but I >>>> don't understand how this can be reconstructed into foo=bar when >>>> there's no binding between the two rows. Perhaps details are supposed >>>> to be passed differently than the resource tags, because if I do >>>> "details[0].foo=bar&details[1].baz=12", I get: >>>> >>>> id | vmid | name | value | display >>>> >>>> 12 | 25 | foo | bar | 1 >>>> 13 | 25 | baz | 12 | 1 >>>> >>>> And indeed there is code utilizing these details already committed >>>> that expects this format, as deployVirtualMachines getDetails() only >>>> seems to pass a correct Map<String, String> with Key, Value if I use >>>> this format. >>>> >>>> On Mon, Mar 3, 2014 at 11:48 AM, Antonio Fornié Casarrubios >>>> <antonio.for...@gmail.com> wrote: >>>> >>>> Hi Alena, >>>> >>>> Of course, the API will not have any changes. This is not a functional >>>> change, just some refactoring. The problem is there are many things in CS >>>> that really need some refactoring otherwise the problem will continue >>>> growing more and more, but doing the change and above all making sure it >>>> all works afterwards is not simple. >>>> >>>> I will make sure that everything works exactly the same way and that the >>>> data returned is also the same. >>>> >>>> Thanks. Cheers >>>> Antonio >>>> >>>> >>>> 2014-03-03 18:48 GMT+01:00 Alena Prokharchyk >>>> <alena.prokharc...@citrix.com>: >>>> >>>> Antonio, sure I will review the patch. But please make sure that API >>>> backwards compatibly is intact, otherwise the fix won¹t be accepted. >>>> >>>> -Alena. >>>> >>>> >>>> On 3/2/14, 4:31 PM, "Antonio Fornié Casarrubios" >>>> <antonio.for...@gmail.com> wrote: >>>> >>>> Hi Alena, >>>> >>>> The reasons for this strange format? I don't know. There doesn't seem to >>>> be >>>> one. After asking on my team and in the dev list I thought perhaps you >>>> could know. It seems we all see it strange and nobody knows why. But of >>>> course, if it is for reasons I will stop the change. >>>> >>>> >>>> >>>> And about the DB, you are right, in the DB is not like I said. But you can >>>> have this in a table row field: >>>> {0={value=Toronto,key=City}} >>>> for some tables. I think there are two cases: >>>> >>>> 1- params in wich the get method fixes the params on the fly. In these of >>>> course the strange format is not propagated anymore. But this is still >>>> wrong: the format itself before the get is invoked, the time spent on >>>> fixing something that should be a normal Map from the begining (each time >>>> the get method is invoked) and mainly the fact that these get methods that >>>> fix the map on the fly are copies of each other: instead of fixing the >>>> structure in one method, the are plenty of methods almost identical >>>> copying >>>> and pasting the same lines. Some times the same method twice in the same >>>> cmd class for two Map params (look CreateNetworkOfferingCmd >>>> #getServiceCapabilities and #getServiceProviders). >>>> >>>> 2- params in which the get method returns the map as it is. With the >>>> strange format. For example, >>>> Cloudmonkey command >>>> create networkoffering ... tags[0].key="City" tags[0].value="Toronto" >>>> >>>> You store in the table network_offeringstags, field tags, the String: >>>> {0={value=Toronto,key=City}} >>>> (including brackets and all) >>>> >>>> So knowing all this I guess you agree this should be refactored... unless >>>> at some point the strange format is needed. But after looking for it >>>> everywhere I didn't find any place where it was. I already did the change >>>> and tested most of the cases and it all seems to work. >>>> >>>> >>>> It would be great if once I upload the patch somebody could help me double >>>> check that it doesn't brake anything, not only reviewing to code. I did >>>> plenty of tests of many kinds, but I cannot be sure that I am covering >>>> enough. Further, there seem to be several places where the code expects >>>> the >>>> strange format. >>>> ->ConfigurationManagerImpl line 1545 >>>> >>>> >>>> Thanks. Cheers >>>> Antonio >>>> >>>> >>>> 2014-02-28 18:44 GMT+01:00 Alena Prokharchyk >>>> <alena.prokharc...@citrix.com>: >>>> >>>> >>>> >>>> From: Antonio Fornié Casarrubios <antonio.for...@gmail.com> >>>> Date: Friday, February 28, 2014 at 2:09 AM >>>> To: Rohit Yadav <rohityada...@gmail.com>, cloudstack < >>>> dev@cloudstack.apache.org>, Alena Prokharchyk < >>>> alena.prokharc...@citrix.com> >>>> Subject: Re: [PROPOSAL][QUESTION] Map parameters in API Commands >>>> >>>> Hi Alena, >>>> >>>> I would like to know your opinion on this change. Mainly consists on: >>>> 1- Change the way we store the Map params after unpackParams in order to >>>> have, for each Map param, a Map<String, String> instead of Map<String, >>>> Map<String, Object>>. >>>> >>>> >>>> -Antonio, what was the reason for storing the parameter in the old >>>> format to begin with? Where there any case where we actually needed a >>>> map >>>> of map parameters? >>>> >>>> >>>> >>>> >>>> 2- There are many commands that fix this strange format on demand on >>>> their getters, so they do the conversion there. Since I already have the >>>> final format I replace these getters with just >>>> getTags(){ return this.tags;} >>>> >>>> 3- Persistence of these Map params. This last change is more tricky and >>>> error-prone but the previous two would brake the functionality without >>>> it. >>>> Actually it doesn't seem that I should change this for all the cases, >>>> given >>>> that for some commands the current behavior is storing in the DB the >>>> Map as >>>> it comes, so after the change it will just do the same and thus >>>> retrieve it >>>> with the right format. So, although in the tables we move from >>>> ------ >>>> key | City >>>> ------ >>>> value | The Hague >>>> ------ >>>> >>>> to >>>> ------ >>>> City | The Hague >>>> ------ >>>> >>>> then in memory, after DB read, we will just have the proper format >>>> again >>>> (Map<String, String>). Is that right? >>>> >>>> >>>> >>>> - in what table do you see key name being a field name? I've looked >>>> at >>>> various *_details tables, as well as resource_tag table, everywhere >>>> we have >>>> key/value fields where we store key and the value respectfully: >>>> >>>> mysql> desc user_Vm_details; >>>> >>>> +---------+---------------------+------+-----+---------+----------------+ >>>> | Field | Type | Null | Key | Default | Extra >>>> | >>>> >>>> +---------+---------------------+------+-----+---------+----------------+ >>>> | id | bigint(20) unsigned | NO | PRI | NULL | auto_increment >>>> | >>>> | vm_id | bigint(20) unsigned | NO | MUL | NULL | >>>> | >>>> | name | varchar(255) | NO | | NULL | >>>> | >>>> | value | varchar(1024) | NO | | NULL | >>>> | >>>> | display | tinyint(1) | NO | | 1 | >>>> | >>>> >>>> +---------+---------------------+------+-----+---------+----------------+ >>>> 5 rows in set (0.01 sec) >>>> >>>> mysql> desc resource_tags; >>>> >>>> >>>> +---------------+---------------------+------+-----+---------+----------- >>>> -----+ >>>> | Field | Type | Null | Key | Default | Extra >>>> | >>>> >>>> >>>> +---------------+---------------------+------+-----+---------+----------- >>>> -----+ >>>> | id | bigint(20) unsigned | NO | PRI | NULL | >>>> auto_increment | >>>> | uuid | varchar(40) | YES | UNI | NULL | >>>> | >>>> | key | varchar(255) | YES | | NULL | >>>> | >>>> | value | varchar(255) | YES | | NULL | >>>> | >>>> | resource_id | bigint(20) unsigned | NO | MUL | NULL | >>>> | >>>> | resource_uuid | varchar(40) | YES | | NULL | >>>> | >>>> | resource_type | varchar(255) | YES | | NULL | >>>> | >>>> | customer | varchar(255) | YES | | NULL | >>>> | >>>> | domain_id | bigint(20) unsigned | NO | MUL | NULL | >>>> | >>>> | account_id | bigint(20) unsigned | NO | MUL | NULL | >>>> | >>>> >>>> >>>> +---------------+---------------------+------+-----+---------+----------- >>>> -----+ >>>> >>>> >>>> 4- The last change should be related to any code expecting the old >>>> format, that will fail with the new one. I guess UI will be an example >>>> of >>>> that, but I didn't check yet. If the JS code receives the new Map >>>> serialized, then chances are this will break it, right? Can you tell >>>> your >>>> thoughts on this? Can you tell me places I should check first to confirm >>>> this guess? >>>> >>>> >>>> - Its not just about the uI> You should never break the API backwards >>>> compatibility. Remember that lots of third party vendors use our APIs, >>>> not >>>> the UI. As long as we support the old format, introducing the new one >>>> shouldn't be a problem. >>>> >>>> >>>> >>>> Considering it all, do you think this change is worth it? For me this >>>> seems to be something that was wrong from the beginning and it should >>>> have >>>> been changed before the mess got spread. But know, although I want to >>>> fix >>>> it, I'm afraid this involves touching too many things in order to fix >>>> something that looks horrible but seems to be actually working and I >>>> don't >>>> want to break. >>>> >>>> Thanks. Cheers >>>> Antonio >>>> >>>> >>>> >>>> 2014-02-12 23:32 GMT+01:00 Rohit Yadav <rohityada...@gmail.com>: >>>> >>>> On Wed, Feb 12, 2014 at 9:52 PM, Antonio Fornié Casarrubios >>>> <antonio.for...@gmail.com> wrote: >>>> >>>> Hi Rohit, >>>> >>>> I didn't mean changing the format of the HTTP request, but only >>>> >>>> changing the >>>> >>>> intermediate format in which we keep it in the property of the >>>> >>>> Command >>>> >>>> class. I mentioned the format in the request just to explain what I >>>> >>>> meant. >>>> >>>> >>>> My proposal is to leave the request format as it is, but then when >>>> >>>> the >>>> >>>> method "apiDispatcher#setFieldValue" parses the map and assign it to >>>> >>>> the >>>> >>>> property, do it in a normal way: which is a Map<String, String> >>>> >>>> instead >>>> of a >>>> >>>> Map<String, Map<String, Object>> as it is now. And then, our getter >>>> >>>> methods >>>> >>>> (like CreateTagsCommand#GetTag) will be just a normal getter that >>>> >>>> doesn't >>>> >>>> need to transform the structure on the fly. >>>> >>>> >>>> Cool, let's request the present API layer maintainer(s) and other >>>> folks in the community to comment. >>>> >>>> Regards. >>>> >>>> >>>> Thanks, cheers >>>> antonio >>>> >>>> >>>> 2014-02-11 17:38 GMT+01:00 Rohit Yadav <rohityada...@gmail.com>: >>>> >>>> Hi Antonio, >>>> >>>> On Tue, Feb 11, 2014 at 9:57 PM, Antonio Fornié Casarrubios >>>> <antonio.for...@gmail.com> wrote: >>>> >>>> Hi all, >>>> >>>> When invoking a CS API command that has parameters of type Map, >>>> >>>> the >>>> >>>> request >>>> will be something like this: >>>> >>>> >>>> >>>> URL/api?command=createTags&tags[0].key=region&tags[0].value=canada&tags[ >>>> 1].key=name&tags[1].value=bob >>>> >>>> >>>> in order to send a Map with the pairs: >>>> >>>> tags{ >>>> region : "canada", >>>> name : "bob" >>>> } >>>> >>>> Then in the server side the parameters go through several stages >>>> >>>> (IMHO >>>> >>>> too >>>> many), and have different formats. At some point >>>> >>>> apiDispatcher#setFieldValue >>>> >>>> will assign the value to the command property (CreateTagsCmd#tag >>>> >>>> in >>>> the >>>> >>>> example) in a VERY strange way: >>>> >>>> CreateTagsCmd#tag = { >>>> 0 : { >>>> "key" : "region", >>>> "value" : "canada" >>>> }, >>>> 1 : { >>>> "key" : "name", >>>> "value" : "bob" >>>> } >>>> } >>>> >>>> This is true for several Cmd classes. And the funny thing is they >>>> usually >>>> provide a public getter method to get the Map in an already >>>> >>>> "normalized" >>>> >>>> structure. The problem is we have this method again a again in >>>> >>>> each >>>> of >>>> >>>> these commands, only with different name depending on what >>>> >>>> property >>>> the >>>> >>>> get, and the body is almost copy and pasted. so my next >>>> >>>> refactoring >>>> >>>> would >>>> be to have a generic method only once in BaseCmd so that all >>>> >>>> subclasses >>>> >>>> can >>>> reuse it for their Map getters. Pretty obvious, but... >>>> >>>> >>>> This is a well know issue and is such a pain, both for the users of >>>> the API to create this API and the programmer who have to put hack >>>> >>>> at >>>> >>>> the backend to extract the map. >>>> >>>> Is it really necessary to have this strange format? Wouldn't it be >>>> >>>> much >>>> >>>> better to just store it in a more normal way from the beginning, >>>> >>>> and >>>> >>>> have >>>> the getters just standard getters? Does it have any use to have >>>> >>>> those >>>> >>>> Maps >>>> of Maps? >>>> >>>> >>>> Changing the API will break many client so no one attempted it for >>>> keeping backward-compatibility I think. >>>> >>>> The HTTP RFC states that if same keys are sent in param they must be >>>> received as an array. For example, /api?q=1&q=2&q=3 should received >>>> >>>> q >>>> >>>> = [1,2,3] which is what we're not doing. >>>> >>>> We should do that and this way we can capture maps using keys and >>>> values in order, so for example, >>>> /api?q.key1=value1&q.key2=value2&q.key1=value3&q.key2=value4 should >>>> >>>> be >>>> >>>> received as as array of maps: [{key1: value1, key2: value2}, >>>> {key3:value3, key4: value4}] etc. >>>> >>>> I think it does not have to be maps of maps, but since our API is >>>> query based these ugly hacks were invented. We should definitely get >>>> rid of them, and perhaps work on the RESTful API layer, cloud-engine >>>> and other good stuff we were talking about more than a year ago and >>>> deprecate the present query API over next few years. Thoughts, >>>> >>>> flames? >>>> >>>> >>>> Regards. >>>> >>>> >>>> Thanks. Cheers >>>> Antonio Fornie >>>> Schuberg Philis - MCE >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> >
