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<mailto:alex.o...@sungardas.com>> Date: Tuesday, April 8, 2014 at 10:39 AM To: Alena Prokharchyk <alena.prokharc...@citrix.com<mailto:alena.prokharc...@citrix.com>> Cc: "dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org>" <dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org>>, Alex Huang <alex.hu...@citrix.com<mailto: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<mailto: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<mailto:alex.o...@sungardas.com>> Date: Monday, April 7, 2014 at 9:16 PM To: "dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org>" <dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org>>, Alena Prokharchyk <alena.prokharc...@citrix.com<mailto:alena.prokharc...@citrix.com>>, Alex Huang <alex.hu...@citrix.com<mailto: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<mailto: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<mailto: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.java > 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-core-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/network/contrail/management/MockAccountManager.java > 957f708 > plugins/pom.xml 9b391b8 > > server/resources/META-INF/cloudstack/core/spring-server-core-managers-context.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 > >
