Personally, I feel like the rootdisksize itself should be a
fully-fledged api parameter, especially where we use it to create the
instance but don't actually persist it in the user_vm_details
database. I began doing that and then noticed that such a parameter
was actually removed with Bharat's details patch.
I'd like to get this ironed out to wrap up the root-resize branch.
On Mon, Mar 3, 2014 at 2:06 PM, Alena Prokharchyk
<alena.prokharc...@citrix.com> wrote:
> Adding Bharat to the thread as he was the one who introduced the details
> param to the deployVm call.
>
> Bharat, why did you pick this format for storing the vm details? Were you
> following the rules from some other calls? If so, what those calls are?
>
> -Alena.
>
> On 3/3/14, 1:00 PM, "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&ta
>>>>>>>gs[
>>>> >>>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
>>>> >>> >
>>>> >>> >
>>>> >>>
>>>> >>
>>>> >>
>>>>
>>>>
>
