Yes, we'd have to leave the current one intact, but allow the other as well.
On Mon, Mar 10, 2014 at 11:36 AM, Bharat Kumar <bharat.ku...@citrix.com> wrote: > Hi, > i don't know if this is still valid, but there was one more parameter > disksize which which specifies the disk size for data disks. > I would prefer adding both the root disk size and disk size to the details > map. ideally we should also change the name disksize to > dataDisksize to remove confusion but this might break backward compatibility. > > Adding them to a map will be intuitive as we already use a map to specify any > custom parameters related to VM. > > Thanks > Bharat. > > On 10-Mar-2014, at 10:57 pm, Marcus <shadow...@gmail.com> wrote: > >> 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 >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>> >
