On 4/8/14, 11:26 AM, "Alex Ough" <alex.o...@sungardas.com> wrote:
>Yes, I really hope so from now on :-) > > >For #2, the current setting is NOT syncing the resources and these are >the steps to enable this feature > - Set the "mode" value of the remote regions to sync into 'sync on' > - Set the value of global parameter, "region.full.scan.interval" (the >default value is '0"), into value of milliseconds to enable the Full Scan > Let me know if you want a different way. Sounds good to me. We have to document it though very clearly under ³how to disable the feature² section. When you set the mode, you have to set it for all the regions to completely disable the feature, right? > > >For #1, after another thought, I'm not still clear why those datetime >values can't be added to their attributes. As you recommended, we can use >another table to track them, but there can be an issue of partial commits >when only the resource are successfully > created/updated/removed and their datetime values fail to be stored >because they are NOT in the same transaction. > > >And we also need to track the 'removed' datetime because when a resource >exists in only either the local or the remote region, we don't know >whether it was created but it failed to be created in another region or >it was removed but it failed to be removed > in another region. That's why I track the "removed" datetime and >preserve the original datetime. I¹m fine with keeping the removed field, as long as you don¹t update this field in CS DB. > > >Let me think about the #1 more closely and let you know what I find. >So please let me know if you have any other suggestion related with this. >Thanks >Alex Ough > > > >On Tue, Apr 8, 2014 at 2:09 PM, Alena Prokharchyk ><alena.prokharc...@citrix.com> wrote: > >Sorry, Alex, for not providing the input earlier, I understand that >requests like this shouldn¹t come the last minute. But unfortunately your >FS didn¹t have enough information to caught these design problems the >fact that the GenericDaoFields modified/removed > get modified directly - from the very start, and only by reviewing the >review board ticket you could see it. Hopefully the next time the >experience will be more smooth for both parties. Meanwhile I¹m ready to >help with whatever questions you have. > > >Yes, only 2 things need to be fixed. #2 should be pretty straightforward. >Make sure your component injects the global config variable if you decide >to make a switch via global config. For that, your class has to implement >Configurable interface. You can > find examples of how this interface is utilized by existing CS >components. Here is the FS: > > >https://cwiki.apache.org/confluence/display/CLOUDSTACK/Configuration > > >For #1 I don¹t think you need to keep track of Removed date times. Once >the removed field is set in Region1, its safe to just go ahead and remove >the entry in all the rest of the regions. You can¹t (and don¹t need to) >perform any other operation on the > resource if its removed in one of the regions. >You just have to fix it in created/updated(modified) fields. > > >-Alena. > > >From: Alex Ough <alex.o...@sungardas.com> >Date: Tuesday, April 8, 2014 at 10:39 AM >To: Alena Prokharchyk <alena.prokharc...@citrix.com> >Cc: "dev@cloudstack.apache.org" <dev@cloudstack.apache.org>, Alex Huang ><alex.hu...@citrix.com> > >Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among >Multiple Regions (Core Changes) > > > > > >Alena, > > >It would be really nice if I had this feedback when I presented this at >the beginning. >Anyway, so are only these 2 things necessary to be merged into the master? > > >1. find another way to store the created/updated/removed datetimes of the >resources >2. provide a way to disable this feature. > > >Let me try to resolve those 2 and let you know once done, >so please let me know now if there is anything else needed. > > >Thanks >Alex Ough > > > >On Tue, Apr 8, 2014 at 1:02 PM, Alena Prokharchyk ><alena.prokharc...@citrix.com> wrote: > >Alex, thank you for the explanation. But I think there should be another >way of saving all the ³updated²/³created² info rather than updating the >UserVO/AccountVO/DomainVO fields directly. > > >The one way I can think of you can create a helper table where you >store the ref UUID->Region->UpdatedTime stamps for all Local/Remote >regions, and you update/compare those fields. Your component is a >business logic component, and it shouldn¹t modify > the fields originally set by GenericDao (Removed/Created/Modified). You >should operate only with parameters that are exposed to changes coming >through the APIs. > > >-Alena. > > >From: Alex Ough <alex.o...@sungardas.com> >Date: Monday, April 7, 2014 at 9:16 PM >To: "dev@cloudstack.apache.org" <dev@cloudstack.apache.org>, Alena >Prokharchyk <alena.prokharc...@citrix.com>, > Alex Huang <alex.hu...@citrix.com> >Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among >Multiple Regions (Core Changes) > > > >And all 4 of the recommendations Alex Hwang gave were already implemented >to support the real time synchronization (#1), >but like I said in the previous email, we need to support the full scan >(#2) to cover any failures during the synchronization. > > >Thanks >Alex Ough > > > > > >On Tue, Apr 8, 2014 at 12:10 AM, Alex Ough ><alex.o...@sungardas.com> wrote: > >Alena/Alex, > > >I think I need to give some explanation how this works. > > >There are 2 ways of sync. > > >1. The real time sync : Whenever a resource is create/updated/removed, >each region gets the event of that and requests the same job to all >remote regions using API interfaces, which will create/update/remove the >same resource in each remote region in the > real time. > > >2. Full scan : There can be some interruption (whatever the reason is) >that makes fail the real time sync, so each region scans its own >resources with an interval and compares them with those of remote >regions. This is a little tricky because we need to > find out whose change is later when there is any discrepancy between >resources in the local & remote regions and this is why we need maintain >the created/updated/removed times. > > > 1) When a resource exists in both local (L) and remote regions (R1) > - compare the 'updated' times of both > - Update the local's resource using remote resource attributes >only if the updated time of the remote region is later > - Store the "updated" time of the remote resource (not the >current time) as the local's updated time > - If we store the current time as the updated time, it will cause >an mis-sync > because if the resource was updated in another remote region >(R2) a little after it was updated in the remote region(R1), > update in R2 region will be overwritten by the update in R1 >because the current time is later than the updated time of R2 > > > 2) When a resource exists only in the local region > - Find if there was a removal event of this resource in the remote >region > - and if so, compare the time when the event occurred with the >created time in the local region > - Remove the resource from the local only if the event time is >later > - Like #1, store the time the event was occurred as the 'removed' >time of the removed resource in the local region > > > 3) When a resource exists only in the remote region > - Find if the resource in the local has been removed > - If so, compare the removal time in the local and the created time >in the remote > - Create the resource in the local only if either the local was not >removed or the created time of the remote is later than the removal in >local > - LIke #1 & #2, store the time of the created of the remote region >as the 'created' time of the newly created in the local region. > > >I hope this will help you understand how the create/updated/removed times >are managed. >If there is no failure in the real time sync (#1), we'll not need the >full scan (#2) and we don't need to worry about the times, >but there is no way to guarantee #1 not to be failed, so #2 is very >important to be supported. > > >Please let me know what you think and we can have a conference call if >you want. >Thanks >Alex Ough > > > >On Mon, Apr 7, 2014 at 8:39 PM, Alena Prokharchyk ><alena.prokharc...@citrix.com> wrote: > > >----------------------------------------------------------- >This is an automatically generated e-mail. To reply, visit: > >https://reviews.apache.org/r/20099/#review39753 >----------------------------------------------------------- > > > >Alex, just discussed your changes to existing CS code >(Account/Domain/UserManager, VOs, Daos, GenericDaoBase), with Alex Huang. >Here are the concerns: > > >1) there is no need to update the Removed field. Once the entry is >removed, you should purely rely on the removed not to be null, in order >to update it in the second region database. > >2) The big issue - you can't trigger generic dao fields modification - >Modified/Removed - through the Management/Services layer. Once the field >is defined in Generic Dao Attributes, it should be populated by generic >dao only. It should never be populated by > the business logic component what your service is. > >You should remove the field updates from all the methods I've mentioned >in my previous review (for example, > > AccountManager > > public boolean disableAccount(long accountId, Date modified) throws >ConcurrentOperationException, ResourceUnavailableException;) > >and find another way of keeping track of modified attribute update for >the CS objects. > >Alex suggested a following solution: >========================================================= >* Every time user/domain/account is created/updated/removed in CS, >certain Action event is generated. There is a way to pubish this event to >the event bus, either rabbitMQ or in-memory bus. >* Instead of scanning user/domain/account tables, your plugin should >listen to the create/update/remove user/domain/account events published >to the event bus, and update the user/domain/account in all regions in >the system accordingly >* Of course, you have to skip the event processing for the event >generated by your plugin - this part can be tricky >* The events processing should be synchronized on the resource type >(user/account/domain) + event timestamp in your plugin, so all the events >for the same resource are processed in the order based on the timeStamp >they were generated > >When to use in-memory bus vs RabbitMQ? It depends on how your service >runs. If it runs on each management server, then it would be responsible >only for events generated by this management server, and should use >events published to in-memory bus. >If your server runs only on one of the Management servers, and there are >multiple servers in the cluster, you should listen to all the events from >all the management servers in cluster. In this case, you should use >RabbitMQ bus. > >Here is the link to the in-memory bus FS: >https://cwiki.apache.org/confluence/display/CLOUDSTACK/In-memory+event+bus >But I'm sure you are familiar how this stuff works as you utilize the >event bus in your code already. > >Or you can find another solution; but this solution should never involve >direct modification for "Modified" field by your plugin. > >3) You should add a way to disable your plugin through Global Config/API. >This should be done to support the case when user/domain/account entries >are generated by some app running on top of CS, for all the regions in >the system. In this case admin controls > the user/domain/account entries outside of the CS, and synchronize them >outside as well - so might need to disable your plugin. > >- Alena Prokharchyk > > >On April 7, 2014, 7:13 p.m., Alex Ough wrote: >> >> ----------------------------------------------------------- >> This is an automatically generated e-mail. To reply, visit: >> https://reviews.apache.org/r/20099/ > >> ----------------------------------------------------------- >> >> (Updated April 7, 2014, 7:13 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/domain/Domain.java 365a705 >> api/src/com/cloud/event/EventTypes.java 39ef710 >> api/src/com/cloud/user/Account.java b912e51 >> api/src/com/cloud/user/AccountService.java 7e37b38 >> api/src/com/cloud/user/User.java 36e9028 >> api/src/com/cloud/user/UserAccount.java c5a0637 >> api/src/org/apache/cloudstack/api/ApiConstants.java fdb4558 >> api/src/org/apache/cloudstack/api/BaseCmd.java f6f21ae >> >>api/src/org/apache/cloudstack/api/command/admin/region/AddRegionCmd.java >>f6743ba > >> >>api/src/org/apache/cloudstack/api/command/admin/region/UpdateRegionCmd.ja >>va b08cbbb >> api/src/org/apache/cloudstack/api/response/AccountResponse.java >>2e50c51 >> api/src/org/apache/cloudstack/api/response/DomainResponse.java 0c0281e >> api/src/org/apache/cloudstack/api/response/RegionResponse.java 6c74fa6 >> api/src/org/apache/cloudstack/api/response/UserResponse.java 40e1561 >> 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 d8dbde7 > >> >>engine/schema/resources/META-INF/cloudstack/core/spring-engine-schema-cor >>e-daos-context.xml 489b37d >> engine/schema/src/com/cloud/domain/DomainVO.java f6494b3 >> engine/schema/src/com/cloud/user/AccountVO.java 0f5a044 >> engine/schema/src/com/cloud/user/UserAccountVO.java cef9239 >> engine/schema/src/com/cloud/user/UserVO.java 68879f6 >> engine/schema/src/org/apache/cloudstack/region/RegionVO.java 608bd2b >> framework/db/src/com/cloud/utils/db/Attribute.java 82c2bdb >> framework/db/src/com/cloud/utils/db/GenericDao.java cb401cd >> framework/db/src/com/cloud/utils/db/GenericDaoBase.java 2052aad >> framework/db/src/com/cloud/utils/db/SqlGenerator.java befe34b >> framework/db/test/com/cloud/utils/db/GenericDaoBaseTest.java aef0c69 >> framework/db/test/com/cloud/utils/db/SqlGeneratorTest.java >>PRE-CREATION >> >>plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/netw >>ork/contrail/management/MockAccountManager.java 957f708 >> plugins/pom.xml 9b391b8 > >> >>server/resources/META-INF/cloudstack/core/spring-server-core-managers-con >>text.xml fc1c7e2 >> server/src/com/cloud/api/ApiDispatcher.java 95074e2 >> server/src/com/cloud/api/ApiResponseHelper.java 38f2f0b >> server/src/com/cloud/api/dispatch/ParamProcessWorker.java e9bdd8b >> server/src/com/cloud/api/query/dao/AccountJoinDaoImpl.java ecd97c7 >> server/src/com/cloud/api/query/dao/UserAccountJoinDaoImpl.java 923a238 >> server/src/com/cloud/api/query/vo/AccountJoinVO.java 8d642ed >> server/src/com/cloud/api/query/vo/UserAccountJoinVO.java ed29284 >> server/src/com/cloud/event/ActionEventUtils.java 28e5680 >> server/src/com/cloud/projects/ProjectManagerImpl.java d10c059 >> server/src/com/cloud/user/AccountManager.java 03bf842 >> server/src/com/cloud/user/AccountManagerImpl.java 2070ee6 >> 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/api/dispatch/ParamProcessWorkerTest.java 12051a6 >> server/test/com/cloud/user/MockAccountManagerImpl.java f373cba >> server/test/com/cloud/user/MockDomainManagerImpl.java 7dddefb >> server/test/org/apache/cloudstack/region/RegionManagerTest.java >>d7bc537 >> setup/db/db/schema-440to450.sql 2bd5386 >> ui/scripts/regions.js 66dae8c >> >> 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 >> >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
