cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Ough <alex.o...@sungardas.com>
Subject Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
Date Thu, 26 Jun 2014 20:53:10 GMT
Alena,
Didn't you say that you guys already "did logic review" in the previous
email?

Thanks
Alex Ough


On Thu, Jun 26, 2014 at 2:59 PM, Alena Prokharchyk <
Alena.Prokharchyk@citrix.com> wrote:

>  Alex, sorry to hear that it took so long to get on the review process.
> The question still remains – before you started working on implementation,
> and posted your plugin’s code, was the FS approved/reviewed as a part of
> [PROPOSAL] discussion? We should never start the development until you get
> the input from the community on the FS and confirm that the design is valid
> and the feature can contribute to CS. Only after the proposal is accepted,
> you can request the Reviewboard ticket review. So I did assume that the
> [PROPOSAL] phase was finished, and the FS was validated as a part of it,
> when I was asked by Daan to review the Reviewboard ticket.
>
>  I’ve also looked at the history. I can see that Chiradeep contributed to
> the design/plugin logic discussion as well as pointed to the changes that
> need to be done to the code structure. I helped to review the second.
>
>  Lets wait for the update from Kishan. Kishan, in addition to answering
> Alex’s questions, please go over the plugin design once again.
>
>  Thank you,
> Alena.
>
>   From: Alex Ough <alex.ough@sungardas.com>
> Date: Thursday, June 26, 2014 at 11:32 AM
>
> To: Alena Prokharchyk <alena.prokharchyk@citrix.com>
> Cc: Kishan Kavala <Kishan.Kavala@citrix.com>, "dev@cloudstack.apache.org"
> <dev@cloudstack.apache.org>, Murali Reddy <Murali.Reddy@citrix.com>, Ram
> Ganesh <Ram.Ganesh@citrix.com>, Animesh Chaturvedi <
> animesh.chaturvedi@citrix.com>
> Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among
> Multiple Regions (Core Changes)
>
>   Alena,
>
>  It has been reduced almost twice because a lot has been separated from
> the CS and moved to the plug-in not because they are 'unnecessary'. Please
> remember that my initial implementation was inside the CS not as a plug-in
> as I said in the previous email.
>
>  Of course, I asked and urged the review repeatedly and you'll see the
> all the histories of them if you find emails using this subject, which
> started 10/17/13.
> [DISCUSS] Domain-Account-User Sync Up Among Multiple Regions
>  Even if I asked so many times, unfortunately, I couldn't get an actual
> feedback until Daan finally asked Chiradeep and you to review them, which
> is 3/10/14.
>
>  Kishan,
> I posted 2 questions, so please guide me for the questions.
>
>  Thanks
> Alex Ough
>
>
>
> On Thu, Jun 26, 2014 at 12:57 PM, Alena Prokharchyk <
> Alena.Prokharchyk@citrix.com> wrote:
>
>>  Alex,
>>
>>  By “huge” I’ve meant that there was a lot of repetitive hardcoded
>> things, lot of unnecessary changes to the CS orchestration layer. If you
>> compare a number of changes now and originally, you can see that it reduced
>> almost twice.
>>
>>  But lets discuss the complains about lack of initial review as its more
>> important question.
>>
>>  Review of the design spec should happen before you start
>> designing/coding. As I jumped on review much later, after you’ve submitted
>> the entire plugin code, so I I didn’t participate in “Feature Request”
>> discussion review that might have happened earlier. And I do assume that
>> the reviews/emails exchanges were done at that initial phase? You should
>> have contacted the people participating in the initial phase, and ask them
>> for the review as well.
>>
>>  As a part of my review, I’ve made sure to cover the things I’m certain
>> should have been changed. I’ve reviewed the feature logic as well,
>> consulting the FS you’ve written. I’m not saying that there is anything
>> wrong with your initial design, but asking for a second opinion from the
>> guys who have more expertise in Regions.
>>
>>  Kishan, please help to do the final review the Alex’s plugin design
>> https://reviews.apache.org/r/17790
>>
>>  Thank you,
>> Alena.
>>  From: Alex Ough <alex.ough@sungardas.com>
>> Date: Wednesday, June 25, 2014 at 9:03 PM
>>
>> To: Alena Prokharchyk <alena.prokharchyk@citrix.com>
>> Cc: Kishan Kavala <Kishan.Kavala@citrix.com>, "dev@cloudstack.apache.org"
>> <dev@cloudstack.apache.org>, Murali Reddy <Murali.Reddy@citrix.com>, Ram
>> Ganesh <Ram.Ganesh@citrix.com>, Animesh Chaturvedi <
>> animesh.chaturvedi@citrix.com>
>> Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among
>> Multiple Regions (Core Changes)
>>
>>   Alena,
>>
>>  I understand that you have been helping a lot to make my codes to match
>> the coding standards, but I'm not sure what you mean by "the code base was
>> unnecessary huge".
>> The initial implementation was to support the synchronization inside the
>> CS because this feature is missing in the current multiple region support,
>> and most of jobs were  to separate the implementation from the CS because
>> you guys wanted me to provide it as a plugin.
>>
>>  And I kept asking reviews for the design spec from when I published the
>> documents with initial prototype, it took a while for you to start to
>> review my implementation and they have been mostly about the coding
>> standards instead of the logic itself. So I'm saying that it would have
>> been better if there has been someone to review the design spec and the
>> prototype from the initial phase.
>>
>>  Again, I really appreciate your help to come this far, but it was also
>> very painful for me.
>> Thanks
>> Alex Ough
>>
>>
>> On Wed, Jun 25, 2014 at 10:41 PM, Alena Prokharchyk <
>> Alena.Prokharchyk@citrix.com> wrote:
>>
>>>  Alex,
>>>
>>>  In the beginning the code was not very well organazied, didn't match
>>> coding standarts (no use of spring, misleading names, not segregated to its
>>> own plugin), and the code base was unneccessary huge.
>>> All of the above it very hard to review and understand the code logic
>>> from the beginning and engage more people to the review. Therefore
>>> Chiradeep pointed it in his original review that the code needs to match CS
>>> standarts first, and be better organized. I helped to review the fixes, and
>>> did logic review as well after the code came into “reviewable” shape.
>>>
>>>  I'm asking Kishan/Murali to look at it to see if anything is missing
>>> or incorrect in the final review, not to make you override or change
>>> everything you've already put in.
>>>
>>>  Thank you,
>>> Alena.
>>>
>>>   From: Alex Ough <alex.ough@sungardas.com>
>>> Date: Wednesday, June 25, 2014 at 7:12 PM
>>>
>>> To: Alena Prokharchyk <alena.prokharchyk@citrix.com>
>>> Cc: Kishan Kavala <Kishan.Kavala@citrix.com>, "dev@cloudstack.apache.org"
>>> <dev@cloudstack.apache.org>, Murali Reddy <Murali.Reddy@citrix.com>,
>>> Ram Ganesh <Ram.Ganesh@citrix.com>, Animesh Chaturvedi <
>>> animesh.chaturvedi@citrix.com>
>>> Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among
>>> Multiple Regions (Core Changes)
>>>
>>>   Alena,
>>>
>>>  Don't get me wrong. What I'm saying is that it would have been better
>>> if you asked the review to whomever you thought was important when you
>>> started the review.
>>>
>>>  Thanks
>>> Alex Ough
>>>
>>>
>>> On Wed, Jun 25, 2014 at 9:45 PM, Alena Prokharchyk <
>>> Alena.Prokharchyk@citrix.com> wrote:
>>>
>>>>  Alex,
>>>>
>>>>  I did my best to review the code, made sure it came in shape with the
>>>> CS guidelines and java code style There was no way to anticipate all the
>>>> things to fix originally, as every subsequent review update added more
>>>> things to fix as the review code was new/refactored.
>>>>
>>>>  And I don’t see anything wrong about asking for a FINAL opinion from
>>>> other people on the mailing list, considering some of them participated in
>>>> the review process along the way already (Kishan). Anybody can review the
>>>> review ticket till its closed, and point to the items that other reviewers
>>>> might have missed.
>>>>
>>>>  Thank you,
>>>> Alena.
>>>>
>>>>   From: Alex Ough <alex.ough@sungardas.com>
>>>> Date: Wednesday, June 25, 2014 at 6:33 PM
>>>> To: Alena Prokharchyk <alena.prokharchyk@citrix.com>
>>>> Cc: Kishan Kavala <Kishan.Kavala@citrix.com>, "
>>>> dev@cloudstack.apache.org" <dev@cloudstack.apache.org>, Murali Reddy <
>>>> Murali.Reddy@citrix.com>, Ram Ganesh <Ram.Ganesh@citrix.com>, Animesh
>>>> Chaturvedi <animesh.chaturvedi@citrix.com>
>>>>
>>>> Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among
>>>> Multiple Regions (Core Changes)
>>>>
>>>>   Thanks Alena, and I'm glad if they spend time for the review, but
>>>> could it be a little earlier for you to ask them to review instead of at
>>>> the last moment?
>>>> I'm really exhausted with repeatedly added items whenever I post a
>>>> review.
>>>>
>>>>  Thanks
>>>> Alex Ough
>>>>
>>>>
>>>> On Wed, Jun 25, 2014 at 7:44 PM, Alena Prokharchyk <
>>>> Alena.Prokharchyk@citrix.com> wrote:
>>>>
>>>>>  Alex, looks fine to me. Make sure that you put the regionId
>>>>> validation as our in-built API validation won’t work in this case because
>>>>> there is no UUID field support for the Region object. You can check how
>>>>> validation is begin done in updateRegion/deleteRegion scenarios.
>>>>>
>>>>>  Kishan/Murali, can you please spend some time doing the final review
>>>>> for Alex’s tickets? As you are the original developers for Region, and
>>>>> probably have the most expertise on the topic. I don’t want to commit the
>>>>> fixes before I hear “ship it” from both of you, guys.
>>>>>
>>>>>  Thanks,
>>>>> Alena.
>>>>>  From: Alex Ough <alex.ough@sungardas.com>
>>>>> Date: Wednesday, June 25, 2014 at 4:02 PM
>>>>> To: Kishan Kavala <Kishan.Kavala@citrix.com>
>>>>> Cc: Alena Prokharchyk <alena.prokharchyk@citrix.com>, "
>>>>> dev@cloudstack.apache.org" <dev@cloudstack.apache.org>, Murali Reddy <
>>>>> Murali.Reddy@citrix.com>, Ram Ganesh <Ram.Ganesh@citrix.com>, Animesh
>>>>> Chaturvedi <animesh.chaturvedi@citrix.com>
>>>>>
>>>>> Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among
>>>>> Multiple Regions (Core Changes)
>>>>>
>>>>>   Hi Alena,
>>>>>
>>>>>  Can you confirm if this fix is correct?
>>>>>
>>>>>      @Parameter(name = ApiConstants.ORIGINATED_REGION_ID, type =
>>>>> CommandType.INTEGER, description = "Region where this account is created.",
>>>>> since = "4.5")
>>>>>     private Integer originatedRegionId;
>>>>>
>>>>>  Thanks
>>>>> Alex Ough
>>>>>
>>>>>
>>>>> On Wed, Jun 25, 2014 at 11:03 AM, Kishan Kavala <
>>>>> Kishan.Kavala@citrix.com> wrote:
>>>>>
>>>>>>  Alex,
>>>>>>
>>>>>> You can refer to the code from initDataSource  method in
>>>>>> Transaction.java.
>>>>>>
>>>>>> Properties file can be loaded using the following:
>>>>>>
>>>>>>
>>>>>>
>>>>>> *File dbPropsFile = PropertiesUtil.findConfigFile(propsFileName);*
>>>>>>
>>>>>>
>>>>>>
>>>>>> *From:* Alex Ough [mailto:alex.ough@sungardas.com]
>>>>>> *Sent:* Wednesday, 25 June 2014 4:31 PM
>>>>>> *To:* Kishan Kavala
>>>>>> *Cc:* Alena Prokharchyk; dev@cloudstack.apache.org; Murali Reddy;
>>>>>> Ram Ganesh; Animesh Chaturvedi
>>>>>>
>>>>>> *Subject:* Re: Review Request 20099: Domain-Account-User Sync Up
>>>>>> Among Multiple Regions (Core Changes)
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks Kishan, but there seems to be lots of 'db.properties' files,
>>>>>> so which one should be referenced?
>>>>>>
>>>>>>
>>>>>>
>>>>>> Alex Ough
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Jun 25, 2014 at 2:25 AM, Kishan Kavala <
>>>>>> Kishan.Kavala@citrix.com> wrote:
>>>>>>
>>>>>> Alex,
>>>>>>
>>>>>> As Alena mentioned, it is admin’s responsibility to keep ids same
>>>>>> across Regions. Ids should be used as unique identifier. Region name is
>>>>>> merely descriptive name and its mostly associated with geographic location.
>>>>>>
>>>>>> Also note that region name can be updated anytime using updateRegion
>>>>>> API.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Unlike, other internal Ids in CS, region Ids are assigned by admin.
>>>>>> So exposing region Id to admin should not be an issue.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Id of the local region cannot be guaranteed to be “1” always. Region
>>>>>> Id has to be unique across all regions. While creating new region admin
>>>>>> will provide unique region id to *cloud-setup-databases* script. Id
>>>>>> of the local region is stored in db.properties. To identify a Local region
>>>>>> you can use one of the following options:
>>>>>>
>>>>>> 1.       Look up region.id in db.properties
>>>>>>
>>>>>> 2.       Add a new column in region table
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> *From:* Alex Ough [mailto:alex.ough@sungardas.com]
>>>>>> *Sent:* Wednesday, 25 June 2014 8:18 AM
>>>>>> *To:* Alena Prokharchyk
>>>>>> *Cc:* dev@cloudstack.apache.org; Kishan Kavala; Murali Reddy; Ram
>>>>>> Ganesh; Animesh Chaturvedi
>>>>>>
>>>>>>
>>>>>> *Subject:* Re: Review Request 20099: Domain-Account-User Sync Up
>>>>>> Among Multiple Regions (Core Changes)
>>>>>>
>>>>>>
>>>>>>
>>>>>> There is one thing that was not mentioned, which is that currently
>>>>>> the id of 'Local' region is always 1 and if we do not guarantee that, there
>>>>>> is no way to find out which is the local region unless we add one more
>>>>>> field to tells which is the local region.
>>>>>>
>>>>>> I'm wondering if we have a solution for this now.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Alex Ough
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, Jun 24, 2014 at 9:59 PM, Alex Ough <alex.ough@sungardas.com>
>>>>>> wrote:
>>>>>>
>>>>>> I agree with that the ids are unique identifier, but they are usually
>>>>>> internal purpose not exposed to the users. So it is a little strange to ask
>>>>>> users to assign ids when they add new regions. And if we do not allow
>>>>>> duplicated names, I'm not sure why it is not good to use names as a unique
>>>>>> identifier.
>>>>>>
>>>>>>
>>>>>>
>>>>>> It's been a long way to come this far with several reasons, so I
>>>>>> really want to wrap this up as soon as possible, and this doesn't seem to
>>>>>> be a major obstacle, so let me just use 'id' as a parameter if there is no
>>>>>> one with a different thought until tomorrow morning.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Alex Ough
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, Jun 24, 2014 at 8:52 PM, Alena Prokharchyk <
>>>>>> Alena.Prokharchyk@citrix.com> wrote:
>>>>>>
>>>>>> Alex, id is used as a unique identifier for CS objects. And it is the
>>>>>> CS requirement to refer to the object by id if the id is present. Look at
>>>>>> all the other APIs. We nowhere refer to the network/vpc/vm by name just
>>>>>> because its more human readable. The id is used by Api layer when parameter
>>>>>> validation is done, by lots of Dao methods (findById is one of them), etc.
>>>>>>  Even look at updateRegion/deleteRegion – we don’t refer to them by name,
>>>>>> but by the id.
>>>>>>
>>>>>>
>>>>>>
>>>>>> The reason why Kishan added the support for controlling the id by
>>>>>> adding it to the createRegion call (and making it unique) is exactly that –
>>>>>> region administrator can decide what id to set on the region, and to
>>>>>> introduce the region with the same id to the other regions’ db.
>>>>>>
>>>>>>
>>>>>>
>>>>>> So I would still suggest on using the id of the region in the API
>>>>>> calls you are modifying. Unless developers who worked on regions feature –
>>>>>> Kishan/Murali – come up with the valid objection.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Alena.
>>>>>>
>>>>>>
>>>>>>
>>>>>> *From: *Alex Ough <alex.ough@sungardas.com>
>>>>>> *Date: *Tuesday, June 24, 2014 at 5:41 PM
>>>>>>
>>>>>>
>>>>>> *To: *Alena Prokharchyk <alena.prokharchyk@citrix.com>
>>>>>> *Cc: *"dev@cloudstack.apache.org" <dev@cloudstack.apache.org>,
>>>>>> Kishan Kavala <Kishan.Kavala@citrix.com>, Murali Reddy <
>>>>>> Murali.Reddy@citrix.com>, Ram Ganesh <Ram.Ganesh@citrix.com>,
>>>>>> Animesh Chaturvedi <animesh.chaturvedi@citrix.com>
>>>>>> *Subject: *Re: Review Request 20099: Domain-Account-User Sync Up
>>>>>> Among Multiple Regions (Core Changes)
>>>>>>
>>>>>>
>>>>>>
>>>>>> We can use the same ids & names, but we don't have to use the same
>>>>>> ids if we use names, which is a little easier because names are user
>>>>>> readable but ids are not, so we don't need to memorize/check all the ids
>>>>>> when we add new regions in multiple regions, which can be confusing.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Alex Ough
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, Jun 24, 2014 at 8:33 PM, Alena Prokharchyk <
>>>>>> Alena.Prokharchyk@citrix.com> wrote:
>>>>>>
>>>>>> Aren’t we supposed to sync the regions across the multiple regions
>>>>>> Dbs? Because that’s what region FS states:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> https://cwiki.apache.org/confluence/display/CLOUDSTACK/AWS-Style+Regions+Functional+Spec,
>>>>>> “Adding 2nd region” paragraph, bullet #4:
>>>>>>
>>>>>>
>>>>>>
>>>>>> 1. Install a 2nd CS instance.
>>>>>>
>>>>>> 2. While installing database set region_id using -r option in
>>>>>> cloud-setup-databases script (Make sure *database_key* is same
>>>>>> across all regions).
>>>>>>
>>>>>> *cloud-setup-databases cloud:**<**dbpassword**>**@localhost
>>>>>> --deploy-as=root:**<**password**>** -e **<**encryption_type**>** -m *
>>>>>> *<**management_server_key**>** -k **<**database_key**> -r
>>>>>> <region_id>*
>>>>>>
>>>>>> 3. Start mgmt server
>>>>>>
>>>>>> 4. *Using addRegion API, add region 1 to region 2 and also region 2
>>>>>> to region 1.*
>>>>>>
>>>>>>
>>>>>>
>>>>>> I assume that we expect the admin to add the region with the same
>>>>>> name and the same id to ALL regions Dbs (both id and name should be passed
>>>>>> to createRegion call). So they are all in sync. Isn’t it the requirement?
>>>>>> If so, we should rely on the fact that all regions Dbs will have the same
>>>>>> set of regions having the same ids and names cross regions.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Alena.
>>>>>>
>>>>>> *From: *Alex Ough <alex.ough@sungardas.com>
>>>>>> *Date: *Tuesday, June 24, 2014 at 5:17 PM
>>>>>> *To: *Alena Prokharchyk <alena.prokharchyk@citrix.com>
>>>>>> *Cc: *"dev@cloudstack.apache.org" <dev@cloudstack.apache.org>,
>>>>>> Kishan Kavala <Kishan.Kavala@citrix.com>, Murali Reddy <
>>>>>> Murali.Reddy@citrix.com>, Ram Ganesh <Ram.Ganesh@citrix.com>,
>>>>>> Animesh Chaturvedi <animesh.chaturvedi@citrix.com>
>>>>>>
>>>>>>
>>>>>> *Subject: *Re: Review Request 20099: Domain-Account-User Sync Up
>>>>>> Among Multiple Regions (Core Changes)
>>>>>>
>>>>>>
>>>>>>
>>>>>> What I'm trying to say is that when we pass the ids of regions, the
>>>>>> receivers do not know what the originated region is and the id of each
>>>>>> region is not same across all the regions.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Alex Ough
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, Jun 24, 2014 at 7:35 PM, Alena Prokharchyk <
>>>>>> Alena.Prokharchyk@citrix.com> wrote:
>>>>>>
>>>>>> Alex, thank you for summarizing.
>>>>>>
>>>>>>
>>>>>>
>>>>>>  I still don’t see why id can’t be unique across regions as you can
>>>>>> control the id assignment – id is required when createRegion call is made.
>>>>>> And that’s how the region should be represented in other region’s Dbs – by
>>>>>> its id that is unique across the regions. Kishan/Murali, please confirm.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thank you,
>>>>>>
>>>>>> Alena.
>>>>>>
>>>>>>
>>>>>>
>>>>>> *From: *Alex Ough <alex.ough@sungardas.com>
>>>>>> *Date: *Tuesday, June 24, 2014 at 4:22 PM
>>>>>> *To: *"dev@cloudstack.apache.org" <dev@cloudstack.apache.org>
>>>>>> *Cc: *Alena Prokharchyk <alena.prokharchyk@citrix.com>, Kishan
>>>>>> Kavala <Kishan.Kavala@citrix.com>, Murali Reddy <
>>>>>> Murali.Reddy@citrix.com>, Ram Ganesh <Ram.Ganesh@citrix.com>,
>>>>>> Animesh Chaturvedi <animesh.chaturvedi@citrix.com>
>>>>>>
>>>>>>
>>>>>> *Subject: *Re: Review Request 20099: Domain-Account-User Sync Up
>>>>>> Among Multiple Regions (Core Changes)
>>>>>>
>>>>>>
>>>>>>
>>>>>> All,
>>>>>>
>>>>>>
>>>>>>
>>>>>> There is one open question in this topic, which is to figure out
>>>>>> which value is appropriate to pass the region object, id or name?
>>>>>>
>>>>>> During this implementation, we decided to add the information of
>>>>>> regions where user/account/domain objects have been originally
>>>>>> created/modified/removed.
>>>>>>
>>>>>> But the ids of regions are not same across the regions and currently
>>>>>> the regions do not have uuids(they will not be same either if we add them
>>>>>> to regions), so I'd like to use names.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Please let me know what you think.
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Alex Ough
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, Jun 24, 2014 at 7:05 PM, Animesh Chaturvedi <
>>>>>> animesh.chaturvedi@citrix.com> wrote:
>>>>>>
>>>>>> Let’s have the discussion on dev mailing list
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Animesh
>>>>>>
>>>>>>
>>>>>>
>>>>>> *From:* Alena Prokharchyk
>>>>>> *Sent:* Tuesday, June 24, 2014 3:06 PM
>>>>>> *To:* Alex Ough; Kishan Kavala; Murali Reddy
>>>>>> *Cc:* Animesh Chaturvedi; Ram Ganesh
>>>>>>
>>>>>>
>>>>>> *Subject:* Re: Review Request 20099: Domain-Account-User Sync Up
>>>>>> Among Multiple Regions (Core Changes)
>>>>>>
>>>>>>
>>>>>>
>>>>>> Adding Kishan to the thread as he was the one who implemented the
>>>>>> region feature originally.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Kishan, in a situation when there are 2 regions in the system, we
>>>>>> expect “region” table to be populated with the same id/name in both Dbs for
>>>>>> both regions, right? So my question is – what uniquely identifies the
>>>>>> region in CS system in cross region setup – id/name?
>>>>>>
>>>>>>
>>>>>>
>>>>>> That unique identifier should be the value that is passed to the
>>>>>> calls you modify, Alex. WE can’t just pass some random name to the call
>>>>>> without making any further verification.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Kishan/Murali, please help to verify this part of Alex’s fix as it
>>>>>> should really be someone with an expertise in Regions. I’ve reviewed the
>>>>>> rest of the feature, just this one item is open. See my latest comment to
>>>>>> the https://reviews.apache.org/r/17790/diff/?page=1#0 as well as
>>>>>> refer to this email thread for the context.
>>>>>>
>>>>>>
>>>>>>
>>>>>> -Alena.
>>>>>>
>>>>>>
>>>>>>
>>>>>> *From: *Alena Prokharchyk <alena.prokharchyk@citrix.com>
>>>>>> *Date: *Tuesday, June 24, 2014 at 2:54 PM
>>>>>> *To: *Alex Ough <alex.ough@sungardas.com>
>>>>>> *Subject: *Re: Review Request 20099: Domain-Account-User Sync Up
>>>>>> Among Multiple Regions (Core Changes)
>>>>>>
>>>>>>
>>>>>>
>>>>>> That what would everybody assume 100% just by looking at the
>>>>>> parameter description and parameter – that you refer to region UUID :
>>>>>> "Region where this account is created.”/ORIGINATEDREGIONUUID
>>>>>>
>>>>>> In CS the UUID has a special meaning. It has to have the UUID format,
>>>>>> and its randomly generated value that is stored in the DB along with the
>>>>>> actual db id. I can see that regionVO lacks UUID field. Looks like existing
>>>>>> RegionVO object lacks this filed unlike other CS objects (uservm, etc). I
>>>>>> will follow up with Murali on that.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Alex, so originatedRegionUUID refers to the region name, correct?.
>>>>>> Why don’t use the region id instead? That’s what we do when refer to CS
>>>>>> objects – we always refer to them by id which is unique. Which is true even
>>>>>> for the region:
>>>>>>
>>>>>>
>>>>>>
>>>>>> mysql> show create table region;
>>>>>>
>>>>>>
>>>>>>
>>>>>>  UNIQUE KEY `id` (`id`),
>>>>>>
>>>>>>   UNIQUE KEY `name` (`name`)
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> That’s what you do when you manipulate the region itself
>>>>>> (delete/updateRegion) - refer to the region by its id. And this field is
>>>>>> returned to you when you call listRegions API:
>>>>>>
>>>>>>
>>>>>>
>>>>>> http://localhost:8096/?command=listRegions
>>>>>>
>>>>>> <region>
>>>>>>
>>>>>> <id>1</id>
>>>>>>
>>>>>> <name>Local</name>
>>>>>>
>>>>>> <endpoint>http://localhost:8080/client/</endpoint>
>>>>>>
>>>>>> <gslbserviceenabled>true</gslbserviceenabled>
>>>>>>
>>>>>> <portableipserviceenabled>false</portableipserviceenabled>
>>>>>>
>>>>>> </region>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Please correct if I miss something.
>>>>>>
>>>>>> -Alena.
>>>>>>
>>>>>>
>>>>>>
>>>>>> *From: *Alex Ough <alex.ough@sungardas.com>
>>>>>> *Date: *Tuesday, June 24, 2014 at 2:33 PM
>>>>>> *To: *Alena Prokharchyk <alena.prokharchyk@citrix.com>
>>>>>> *Subject: *Re: Review Request 20099: Domain-Account-User Sync Up
>>>>>> Among Multiple Regions (Core Changes)
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks for the clarification, but here is a thing.
>>>>>>
>>>>>>
>>>>>>
>>>>>> I'm passing names as the values of originatedRegionUuids because the
>>>>>> uuids are randomly generated and the same regions do NOT have the same
>>>>>> uuidss.
>>>>>>
>>>>>> So I'd like to change the parameter types into String.
>>>>>>
>>>>>> Let me know if you think otherwise.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Alex Ough
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, Jun 24, 2014 at 5:17 PM, Alena Prokharchyk <
>>>>>> Alena.Prokharchyk@citrix.com> wrote:
>>>>>>
>>>>>>   Alex,
>>>>>>
>>>>>>
>>>>>>
>>>>>> take a look at ParamProcessWorker class, and how API parameters are
>>>>>> being dispatched/verified.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> 1)  public void processParameters(final BaseCmd cmd, final Map
>>>>>> params) method
>>>>>>
>>>>>>
>>>>>>
>>>>>> First of all, EntityType parameter should be defined in the
>>>>>> @Parameter annotation for the originatedRegionID field. This parameter is
>>>>>> used by paramProcessWorker to make "if entity exists" validation
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> 2) Check another method in the same class:
>>>>>>
>>>>>>
>>>>>>
>>>>>> private void setFieldValue(final Field field, final BaseCmd cmdObj,
>>>>>> final Object paramObj, final Parameter annotation) throws
>>>>>> IllegalArgumentException, ParseException {
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thats the method responsible for dispatching/setting the field
>>>>>> values. Here is the snippet of the code for the case when UUID is defined:
>>>>>>
>>>>>>
>>>>>>
>>>>>>  case UUID:
>>>>>>
>>>>>>                     if (paramObj.toString().isEmpty())
>>>>>>
>>>>>>                         break;
>>>>>>
>>>>>>                     final Long internalId =
>>>>>> translateUuidToInternalId(paramObj.toString(), annotation);
>>>>>>
>>>>>>                     field.set(cmdObj, internalId);
>>>>>>
>>>>>>                     break;
>>>>>>
>>>>>>
>>>>>>
>>>>>> it always transforms the UUID to Long id, not string. And at the end,
>>>>>> it will be internal DB UUID, not the UUID. If you need the UUID, you have
>>>>>> to get it by a) retrieving the object from the DB by id b) Getting its UUID
>>>>>> property.
>>>>>>
>>>>>>
>>>>>>
>>>>>> If you leave it as a String, you will hit IllegalArgumentException at
>>>>>> "field.set(cmdObj, internalId);" line.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hope it answers your questions, and let me know if anything else
>>>>>> needs to be clarified.
>>>>>>
>>>>>>
>>>>>>
>>>>>> -alena.
>>>>>>
>>>>>>
>>>>>>
>>>>>> *From: *Alex Ough <alex.ough@sungardas.com>
>>>>>> *Date: *Tuesday, June 24, 2014 at 1:57 PM
>>>>>>
>>>>>>
>>>>>> *To: *Alena Prokharchyk <alena.prokharchyk@citrix.com>
>>>>>> *Subject: *Re: Review Request 20099: Domain-Account-User Sync Up
>>>>>> Among Multiple Regions (Core Changes)
>>>>>>
>>>>>>
>>>>>>
>>>>>> Why do you want to change UUID to 'Long'?
>>>>>>
>>>>>> Can you just correct what I fixed?
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, Jun 24, 2014 at 4:21 PM, Alena Prokharchyk <
>>>>>> Alena.Prokharchyk@citrix.com> wrote:
>>>>>>
>>>>>>  You need to put:
>>>>>>
>>>>>>
>>>>>>
>>>>>> *  the entityType parameter to the annotation.
>>>>>>
>>>>>>    - Change the type to Long as I’ve already mentioned. Check how
>>>>>>    other commands handle the parameters (networkId, vpcId, etc)
>>>>>>
>>>>>>  —Alena.
>>>>>>
>>>>>>
>>>>>>
>>>>>> *From: *Alex Ough <alex.ough@sungardas.com>
>>>>>> *Date: *Tuesday, June 24, 2014 at 12:47 PM
>>>>>>
>>>>>>
>>>>>> *To: *Alena Prokharchyk <alena.prokharchyk@citrix.com>
>>>>>> *Subject: *Re: Review Request 20099: Domain-Account-User Sync Up
>>>>>> Among Multiple Regions (Core Changes)
>>>>>>
>>>>>>
>>>>>>
>>>>>> Will this change work?
>>>>>>
>>>>>>
>>>>>>
>>>>>>     @Parameter(name = ApiConstants.ORIGINATED_REGION_ID, type =
>>>>>> CommandType.UUID, description = "Region UUID where this account is
>>>>>> created.", since = "4.5")
>>>>>>
>>>>>>     private String originatedRegionUUID;
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, Jun 24, 2014 at 3:25 PM, Alex Ough <alex.ough@sungardas.com>
>>>>>> wrote:
>>>>>>
>>>>>>  Alena,
>>>>>>
>>>>>>
>>>>>>
>>>>>> This is what really frustrates me, but can you give the final items
>>>>>> instead of keeping adding more items whenever I post a review, please?
>>>>>>
>>>>>> Can you gurantee that this is the only item you want me to fix?
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, Jun 24, 2014 at 3:04 PM, Alena Prokharchyk <
>>>>>> Alena.Prokharchyk@citrix.com> wrote:
>>>>>>
>>>>>> Alex, as a part of the fix, also change the param name to be regionId
>>>>>> (there should be a value in apiconstants already) as the parameter really
>>>>>> reflects CS region object, and we usually refer to those as networkID,
>>>>>> vpcID (not uuid) although uuid are passed in. Check if the rest of the api
>>>>>> changes you've done, respect this rule. Sorry, out of the office now and
>>>>>> cant check myself if there are any.
>>>>>>
>>>>>> -alena
>>>>>>
>>>>>>
>>>>>> > On Jun 24, 2014, at 11:12 AM, "Alena Prokharchyk" <
>>>>>> alena.prokharchyk@citrix.com> wrote:
>>>>>> >
>>>>>> >
>>>>>> > -----------------------------------------------------------
>>>>>>
>>>>>> > This is an automatically generated e-mail. To reply, visit:
>>>>>>
>>>>>> > https://reviews.apache.org/r/20099/#review46557
>>>>>> > -----------------------------------------------------------
>>>>>>
>>>>>> >
>>>>>> >
>>>>>> > Alex, one small thing.
>>>>>> >
>>>>>> > Just noticed that in the API commands you pass regionUUID as a
>>>>>> string. You should pass it as a type of UUID and specify the entityType
>>>>>> parameter in @Parameter so the entity validation is done correctly. Example:
>>>>>> >
>>>>>> > @Parameter(name=ApiConstants.ZONE_ID, type=CommandType.UUID,
>>>>>> entityType = ZoneResponse.class,
>>>>>> >            required=true, description="the Zone ID for the network")
>>>>>> >    private Long zoneId;
>>>>>> >
>>>>>> > That is the rule when passing id/uuid of the first class CS object
>>>>>> to the API call
>>>>>> >
>>>>>> > Then be aware of the fact that the APIDispatcher will transform
>>>>>> UUID to the actual DB id, and that would be the Id that you pass to the
>>>>>> services call. If what you need is UUID, not the actual id, to be saved in
>>>>>> the callContext, you have to transform it explicitly.
>>>>>> >
>>>>>> > - Alena Prokharchyk
>>>>>> >
>>>>>> >
>>>>>>
>>>>>> >> On June 24, 2014, 3:54 p.m., Alex Ough wrote:
>>>>>> >>
>>>>>> >> -----------------------------------------------------------
>>>>>>
>>>>>> >> This is an automatically generated e-mail. To reply, visit:
>>>>>> >> https://reviews.apache.org/r/20099/
>>>>>>
>>>>>> >> -----------------------------------------------------------
>>>>>> >>
>>>>>> >> (Updated June 24, 2014, 3:54 p.m.)
>>>>>> >>
>>>>>> >>
>>>>>> >> Review request for cloudstack.
>>>>>> >>
>>>>>> >>
>>>>>> >> Repository: cloudstack-git
>>>>>> >>
>>>>>> >>
>>>>>> >> Description
>>>>>> >> -------
>>>>>>
>>>>>> >>
>>>>>> >> This is the review request for the core changes related with
>>>>>> #17790 that has only the new plugin codes.
>>>>>> >>
>>>>>> >>
>>>>>>
>>>>>> >> Diffs
>>>>>> >> -----
>>>>>> >>
>>>>>> >>  api/src/com/cloud/event/EventTypes.java 0fa3cd5
>>>>>>
>>>>>> >>  api/src/com/cloud/user/AccountService.java eac8a76
>>>>>> >>  api/src/com/cloud/user/DomainService.java 4c1f93d
>>>>>> >>  api/src/org/apache/cloudstack/api/ApiConstants.java adda5f4
>>>>>> >>  api/src/org/apache/cloudstack/api/BaseCmd.java ac9a208
>>>>>> >>
>>>>>>  api/src/org/apache/cloudstack/api/command/admin/account/CreateAccountCmd.java
>>>>>> 50d67d9
>>>>>> >>
>>>>>>  api/src/org/apache/cloudstack/api/command/admin/account/DeleteAccountCmd.java
>>>>>> 5754ec5
>>>>>> >>
>>>>>>  api/src/org/apache/cloudstack/api/command/admin/account/DisableAccountCmd.java
>>>>>> 3e5e1d3
>>>>>> >>
>>>>>>  api/src/org/apache/cloudstack/api/command/admin/account/EnableAccountCmd.java
>>>>>> f30c985
>>>>>> >>
>>>>>>  api/src/org/apache/cloudstack/api/command/admin/account/LockAccountCmd.java
>>>>>> 3c185e4
>>>>>> >>
>>>>>>  api/src/org/apache/cloudstack/api/command/admin/account/UpdateAccountCmd.java
>>>>>> a7ce74a
>>>>>> >>
>>>>>>  api/src/org/apache/cloudstack/api/command/admin/domain/CreateDomainCmd.java
>>>>>> 312c9ee
>>>>>> >>
>>>>>>  api/src/org/apache/cloudstack/api/command/admin/domain/DeleteDomainCmd.java
>>>>>> a6d2b0b
>>>>>> >>
>>>>>>  api/src/org/apache/cloudstack/api/command/admin/domain/UpdateDomainCmd.java
>>>>>> 409a84d
>>>>>> >>
>>>>>>  api/src/org/apache/cloudstack/api/command/admin/region/AddRegionCmd.java
>>>>>> f6743ba
>>>>>> >>
>>>>>>  api/src/org/apache/cloudstack/api/command/admin/region/UpdateRegionCmd.java
>>>>>> b08cbbb
>>>>>> >>
>>>>>>  api/src/org/apache/cloudstack/api/command/admin/user/CreateUserCmd.java
>>>>>> 8f223ac
>>>>>> >>
>>>>>>  api/src/org/apache/cloudstack/api/command/admin/user/DeleteUserCmd.java
>>>>>> 08ba521
>>>>>> >>
>>>>>>  api/src/org/apache/cloudstack/api/command/admin/user/DisableUserCmd.java
>>>>>> c6e09ef
>>>>>> >>
>>>>>>  api/src/org/apache/cloudstack/api/command/admin/user/EnableUserCmd.java
>>>>>> d69eccf
>>>>>> >>
>>>>>>  api/src/org/apache/cloudstack/api/command/admin/user/LockUserCmd.java
>>>>>> 69623d0
>>>>>> >>
>>>>>>  api/src/org/apache/cloudstack/api/command/admin/user/RegisterCmd.java
>>>>>> 2090d21
>>>>>> >>
>>>>>>  api/src/org/apache/cloudstack/api/command/admin/user/UpdateUserCmd.java
>>>>>> f21e264
>>>>>> >>  api/src/org/apache/cloudstack/api/response/RegionResponse.java
>>>>>> 6c74fa6
>>>>>> >>  api/src/org/apache/cloudstack/region/Region.java df64e44
>>>>>> >>  api/src/org/apache/cloudstack/region/RegionService.java afefcc7
>>>>>> >>
>>>>>>  api/test/org/apache/cloudstack/api/command/test/RegionCmdTest.java 10c3d85
>>>>>> >>  client/pom.xml 29fef4f
>>>>>> >>
>>>>>>  engine/schema/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml
>>>>>> 2ef0d20
>>>>>> >>  engine/schema/src/com/cloud/user/AccountVO.java 0f5a044
>>>>>> >>  engine/schema/src/org/apache/cloudstack/region/RegionVO.java
>>>>>> 608bd2b
>>>>>> >>
>>>>>>  plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/MockAccountManager.java
>>>>>> 4136b5c
>>>>>> >>  plugins/pom.xml b5e6a61
>>>>>> >>
>>>>>>  plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java
>>>>>> b753952
>>>>>> >>
>>>>>>  plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java
>>>>>> 6f7be90
>>>>>>
>>>>>> >>  server/src/com/cloud/api/ApiResponseHelper.java f1f0d2c
>>>>>> >>  server/src/com/cloud/api/dispatch/ParamProcessWorker.java 1592b93
>>>>>> >>  server/src/com/cloud/event/ActionEventUtils.java 2b3cfea
>>>>>> >>  server/src/com/cloud/projects/ProjectManagerImpl.java d10c059
>>>>>> >>  server/src/com/cloud/user/AccountManager.java 194c5d2
>>>>>> >>  server/src/com/cloud/user/AccountManagerImpl.java 7a889f1
>>>>>> >>  server/src/com/cloud/user/DomainManager.java f72b18a
>>>>>> >>  server/src/com/cloud/user/DomainManagerImpl.java fbbe0c2
>>>>>> >>  server/src/org/apache/cloudstack/region/RegionManager.java 6f25481
>>>>>> >>  server/src/org/apache/cloudstack/region/RegionManagerImpl.java
>>>>>> 8910714
>>>>>> >>  server/src/org/apache/cloudstack/region/RegionServiceImpl.java
>>>>>> 98cf500
>>>>>> >>  server/test/com/cloud/user/AccountManagerImplTest.java 176cf1d
>>>>>> >>  server/test/com/cloud/user/MockAccountManagerImpl.java 746fa1b
>>>>>> >>  server/test/com/cloud/user/MockDomainManagerImpl.java 7dddefb
>>>>>> >>  server/test/org/apache/cloudstack/region/RegionManagerTest.java
>>>>>> d7bc537
>>>>>> >>  setup/db/db/schema-440to450.sql ee419a2
>>>>>> >>  ui/scripts/regions.js 368c1bf
>>>>>> >>
>>>>>> >> Diff: https://reviews.apache.org/r/20099/diff/
>>>>>> >>
>>>>>> >>
>>>>>> >> Testing
>>>>>> >> -------
>>>>>> >>
>>>>>>
>>>>>> >> 1. Successfully tested real time synchronization as soon as
>>>>>> resources are created/deleted/modified in one region.
>>>>>> >> 2. Successfully tested full scans to synchronize resources that
>>>>>> were missed during real time synchronization because of any reasons like
>>>>>> network connection issues.
>>>>>> >> 3. The tests were done manually and also automatically by randomly
>>>>>> generating changes each region.
>>>>>> >>
>>>>>> >>
>>>>>>
>>>>>> >> Thanks,
>>>>>> >>
>>>>>> >> Alex Ough
>>>>>> >
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message