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 Wed, 25 Jun 2014 00:41:36 GMT
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