cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jayapal Reddy Uradi <jayapalreddy.ur...@citrix.com>
Subject Re: [PROPOSAL][QUESTION] Map parameters in API Commands
Date Mon, 10 Mar 2014 09:30:18 GMT
Hi Antonio,


I am working on the below feature for 4.4 as part it adding an extra MAP param vmidipmap to
assignToLoadBalancerRule API.
https://cwiki.apache.org/confluence/display/CLOUDSTACK/Configuring+load+balancing+rules+for+VM+nic+secondary+ips

I am following the existing way for map access and in my case vm can have two ips, one is
primay and other secondary
vmidipmap[0].vmid=10 vmidipmap[0].vmip=10.1.1.17 also we can have vmidipmap[1].vmid=10 vmidipmap[0].vmip=10.1.1.18

Will your changes effect for my changes ?

My opinion is wait 4.4 code freeze to commit patch. This way the patch can handle newly coming
changes.

Thanks,
Jayapal



On 10-Mar-2014, at 1:13 PM, Bharat Kumar <bharat.kumar@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 <shadowsor@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 <shadowsor@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.kumar@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 <shadowsor@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.fornie@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.Prokharchyk@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.fornie@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.Prokharchyk@citrix.com>:
>>>> 
>>>> 
>>>> 
>>>> From: Antonio Fornié Casarrubios <antonio.fornie@gmail.com>
>>>> Date: Friday, February 28, 2014 at 2:09 AM
>>>> To: Rohit Yadav <rohityadav89@gmail.com>, cloudstack <
>>>> dev@cloudstack.apache.org>, Alena Prokharchyk <
>>>> alena.prokharchyk@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 <rohityadav89@gmail.com>:
>>>> 
>>>> On Wed, Feb 12, 2014 at 9:52 PM, Antonio Fornié Casarrubios
>>>> <antonio.fornie@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 <rohityadav89@gmail.com>:
>>>> 
>>>> Hi Antonio,
>>>> 
>>>> On Tue, Feb 11, 2014 at 9:57 PM, Antonio Fornié Casarrubios
>>>> <antonio.fornie@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
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
> 


Mime
View raw message