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 >> >> >> >> >> >> >> >> >>
