Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/#review79043 --- Thank you for submitting your CloudStack contribution through review board. After discussion on the dev@cloudstack.apache.org the community decided to close down review board and start accepting contributiong through GitHub pull requests. We have been using GH PR for several months now and the process is better than review board. We will keep Review Board open for another week to give you time to migrate your patch to a github PR if you wish. After that time, your patch will no longer be viewable (even though it will not be deleted). Please consider submitting a pull request. Great instructions are available at: https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md Thank you very much for your time and your contribution to Apache CloudStack, we hope that using this new process will encourage you to do more. - Sebastien Goasguen On June 27, 2014, 5:02 p.m., Alex Ough wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/ --- (Updated June 27, 2014, 5:02 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 5b9ea5c 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 c73d3d9 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 ed48d0b 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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.com Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes) 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.prokharc...@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.o...@sungardas.com Date: Thursday, June 26, 2014 at 11:32 AM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.prokharc...@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.o...@sungardas.com Date: Wednesday, June 25, 2014 at 9:03 PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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
RE: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
Alex, @Encrypt will both encrypt and decrypt. In dev setup, encryption is disabled by default. db.cloud.encryption.type is set to none in db.properties. For encryption to work, you need to do the following: 1. Set db.cloud.encryption.type=file in db.properties 2. Set db.cloud.encrypt.secret=db_secret_key in db.properties 3. Set MS secret key. $cat ms_secret_key /etc/cloudstack/management/key 4. Deploy DB From: Alex Ough [mailto:alex.o...@sungardas.com] Sent: Friday, 27 June 2014 5:12 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) Kishan, 1. Why Long instead of Integer : You replied that it should be Integer 2. @Encrypt : Does it both encrypt decrypt? Is there anything necessary to make it work because it doesn't seem to work when I trace the persist. Thanks Alex Ough On Fri, Jun 27, 2014 at 7:39 AM, Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com wrote: Alex, Are the questions on review board? From: Alex Ough [mailto:alex.o...@sungardas.commailto:alex.o...@sungardas.com] Sent: Friday, 27 June 2014 12:03 AM To: Alena Prokharchyk Cc: Kishan Kavala; dev@cloudstack.apache.orgmailto: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) 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.prokharc...@citrix.commailto:alena.prokharc...@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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Wednesday, June 25, 2014 at 9:03 PM To: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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
RE: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
Alex, Are the questions on review board? From: Alex Ough [mailto:alex.o...@sungardas.com] Sent: Friday, 27 June 2014 12:03 AM To: Alena Prokharchyk Cc: Kishan Kavala; 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) 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.prokharc...@citrix.commailto:alena.prokharc...@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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Wednesday, June 25, 2014 at 9:03 PM To: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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.prokharc...@citrix.commailto:alena.prokharc...@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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
Kishan, 1. Why Long instead of Integer : You replied that it should be Integer 2. @Encrypt : Does it both encrypt decrypt? Is there anything necessary to make it work because it doesn't seem to work when I trace the persist. Thanks Alex Ough On Fri, Jun 27, 2014 at 7:39 AM, Kishan Kavala kishan.kav...@citrix.com wrote: Alex, Are the questions on review board? *From:* Alex Ough [mailto:alex.o...@sungardas.com] *Sent:* Friday, 27 June 2014 12:03 AM *To:* Alena Prokharchyk *Cc:* Kishan Kavala; 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) 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.prokharc...@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.o...@sungardas.com *Date: *Wednesday, June 25, 2014 at 9:03 PM *To: *Alena Prokharchyk alena.prokharc...@citrix.com *Cc: *Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.prokharc...@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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
Jonn, 1. out of spec. we can add this later if necessary. 2. out of spec. we can add this later if necessary. 3. out of spec. we can add this later if necessary. 4. Whenever there are changes in the records, the time stamps are logged and the later change wins. 5. It relies on the order of events, so if the order is reversed with some reason, the creations will fail, but they will be covered by FullScan. 6. It sounds like not related with this project. 7. The interval for FullScan processing is configurable in the global setting, 'region.full.scan.interval'. Thanks Alex Ough On Fri, Jun 27, 2014 at 12:02 AM, John Burwell jburw...@basho.com wrote: All, I apologize for joining this conversation late. I understand that this patch was submitted back in February. Around this time, my family had a significant medical event, and I was disengaged from all work activities — missing the original conversation. Reading through the specification, and briefly reviewing the code, I would like to understand the following assumptions/design decisions: 1. Why aren’t projects being sync’ed? It seems very likely that users would want to have projects span data centers for redundancy/DR purposes. 2. Why aren’t events being sync’ed? I can imagine a number of scenarios where I would want to examine the operation of an logical application or system across both regions. Without the sync of event data, I would be forced to either perform that interleave visually with two browser tabs or dump the data into another datastore to be merged. 3. Why isn’t template metadata being sync’ed? When spanning an application/system across regions, it would seem to follow that I would want to use the same templates. 4. How does this design deal with modifications to a record in two or more regions during a network partition? 5. Given that messages can/will be processed out of order, how is referential integrity maintained when a parent and a set of children are created (e.g. creation of a new account and a set of users rapidly through the API)? 6. Is RabbitMQ being relied upon to provide partition tolerance? 7. Is there a back pressure mechanism to throttle the full sync operation when the database/management server is under heavy load? Finally, I would like to understand why we are taking on multi-datacenter data replication in CloudStack, and not deferring to underlying datastore. Speaking as someone whose $dayjob involves delivering such a system (at Basho for Riak), it is a very hard thing to get right (there literally thousands of corner cases). The design document does not speak to this decision, and I would like understand how CloudStack could not leverage existing, mature mechanisms at the datastore-level. I apologize if some of these questions have been answered already. I attempt to look back in the archives, but given the span of this conversation, it was difficult to piece together retroactively. Thanks, -John On June 26, 2014 at 5:34:31 PM, Alex Ough (alex.o...@sungardas.com) wrote: Sounds like it goes back to what I said I wish they have been involved more actively from the start. Thanks but really making me tired. Alex Ough On Thu, Jun 26, 2014 at 5:17 PM, Alena Prokharchyk alena.prokharc...@citrix.com wrote: I did logic review according to the FS assuming that the FS (and the design described there) was approved on the [PROPOSAL] stage, BEFORE the code was put it to the review board. Was it approved at that stage? Alex, the feature is not small, and considering that it raised so many questions and arguing, I would really like to get a final design/logic review + “ship it” from people having expertise on the topic, and/or who originally participated in review/discussion: Chiradeep, Kishan, Murail. Thank you, Alena. From: Alex Ough alex.o...@sungardas.com Date: Thursday, June 26, 2014 at 1:53 PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.com Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes) 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.prokharc...@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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
review according to the FS assuming that the FS (and the design described there) was approved on the [PROPOSAL] stage, BEFORE the code was put it to the review board. Was it approved at that stage? Alex, the feature is not small, and considering that it raised so many questions and arguing, I would really like to get a final design/logic review + “ship it” from people having expertise on the topic, and/or who originally participated in review/discussion: Chiradeep, Kishan, Murail. Thank you, Alena. From: Alex Ough alex.o...@sungardas.com Date: Thursday, June 26, 2014 at 1:53 PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.com Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes) 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.prokharc...@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.o...@sungardas.com Date: Thursday, June 26, 2014 at 11:32 AM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.prokharc...@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.o...@sungardas.com Date: Wednesday, June 25, 2014 at 9:03 PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
of this conversation, it was difficult to piece together retroactively. Thanks, -John On June 26, 2014 at 5:34:31 PM, Alex Ough (alex.o...@sungardas.com) wrote: Sounds like it goes back to what I said I wish they have been involved more actively from the start. Thanks but really making me tired. Alex Ough On Thu, Jun 26, 2014 at 5:17 PM, Alena Prokharchyk alena.prokharc...@citrix.com wrote: I did logic review according to the FS assuming that the FS (and the design described there) was approved on the [PROPOSAL] stage, BEFORE the code was put it to the review board. Was it approved at that stage? Alex, the feature is not small, and considering that it raised so many questions and arguing, I would really like to get a final design/logic review + “ship it” from people having expertise on the topic, and/or who originally participated in review/discussion: Chiradeep, Kishan, Murail. Thank you, Alena. From: Alex Ough alex.o...@sungardas.com Date: Thursday, June 26, 2014 at 1:53 PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.com Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes) 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.prokharc...@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.o...@sungardas.com Date: Thursday, June 26, 2014 at 11:32 AM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.prokharc...@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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/ --- (Updated June 27, 2014, 5:02 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 (updated) - api/src/com/cloud/event/EventTypes.java 5b9ea5c 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 c73d3d9 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 ed48d0b 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 77445a9 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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
being sync’ed? When spanning an application/system across regions, it would seem to follow that I would want to use the same templates. 4. How does this design deal with modifications to a record in two or more regions during a network partition? 5. Given that messages can/will be processed out of order, how is referential integrity maintained when a parent and a set of children are created (e.g. creation of a new account and a set of users rapidly through the API)? 6. Is RabbitMQ being relied upon to provide partition tolerance? 7. Is there a back pressure mechanism to throttle the full sync operation when the database/management server is under heavy load? Finally, I would like to understand why we are taking on multi-datacenter data replication in CloudStack, and not deferring to underlying datastore. Speaking as someone whose $dayjob involves delivering such a system (at Basho for Riak), it is a very hard thing to get right (there literally thousands of corner cases). The design document does not speak to this decision, and I would like understand how CloudStack could not leverage existing, mature mechanisms at the datastore-level. I apologize if some of these questions have been answered already. I attempt to look back in the archives, but given the span of this conversation, it was difficult to piece together retroactively. Thanks, -John On June 26, 2014 at 5:34:31 PM, Alex Ough (alex.o...@sungardas.com) wrote: Sounds like it goes back to what I said I wish they have been involved more actively from the start. Thanks but really making me tired. Alex Ough On Thu, Jun 26, 2014 at 5:17 PM, Alena Prokharchyk alena.prokharc...@citrix.com wrote: I did logic review according to the FS assuming that the FS (and the design described there) was approved on the [PROPOSAL] stage, BEFORE the code was put it to the review board. Was it approved at that stage? Alex, the feature is not small, and considering that it raised so many questions and arguing, I would really like to get a final design/logic review + “ship it” from people having expertise on the topic, and/or who originally participated in review/discussion: Chiradeep, Kishan, Murail. Thank you, Alena. From: Alex Ough alex.o...@sungardas.com Date: Thursday, June 26, 2014 at 1:53 PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.com Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes) 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.prokharc...@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.o...@sungardas.com Date: Thursday, June 26, 2014 at 11:32 AM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/#review46718 --- Alex, As discussed on the mailing list, ORIGINATEDREGIONUUID should be the regionId which is Long. So all the ORIGINATEDREGIONUUID references should just be ORIGINATEDREGIONID and of datatype Long. - Kishan Kavala On June 24, 2014, 9:24 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, 9:24 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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/#review46719 --- Alex, As discussed on the mailing list, ORIGINATEDREGIONUUID should be the regionId which is Long. So all the ORIGINATEDREGIONUUID references should just be ORIGINATEDREGIONID and of datatype Long. - Kishan Kavala On June 24, 2014, 9:24 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, 9:24 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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/#review46720 --- engine/schema/src/org/apache/cloudstack/region/RegionVO.java https://reviews.apache.org/r/20099/#comment82279 You can use @Encrypt annotation to encrypt and decrypt password. Framework will do the encryption instead of doing md5hash while persisting. You can refer to VpnUserVO.java to see an example. - Kishan Kavala On June 24, 2014, 9:24 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, 9:24 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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
Kishan, The type of region id is Integer, not Long, so I'm wondering why it should be Long. Alex Ough On Thu, Jun 26, 2014 at 2:08 AM, Kishan Kavala kishan.kav...@citrix.com wrote: This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/ Alex, As discussed on the mailing list, ORIGINATEDREGIONUUID should be the regionId which is Long. So all the ORIGINATEDREGIONUUID references should just be ORIGINATEDREGIONID and of datatype Long. - Kishan Kavala On June 24th, 2014, 9:24 p.m. IST, Alex Ough wrote: Review request for cloudstack. By Alex Ough. *Updated June 24, 2014, 9:24 p.m.* *Repository: * cloudstack-git Description This is the review request for the core changes related with #17790 that has only the new plugin codes. 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. 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) -
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Wednesday, June 25, 2014 at 9:03 PM To: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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.prokharc...@citrix.commailto:alena.prokharc...@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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Wednesday, June 25, 2014 at 7:12 PM To: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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.prokharc...@citrix.commailto:alena.prokharc...@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
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.prokharc...@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.o...@sungardas.com Date: Wednesday, June 25, 2014 at 9:03 PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.prokharc...@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.o...@sungardas.com Date: Wednesday, June 25, 2014 at 7:12 PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Thursday, June 26, 2014 at 11:32 AM To: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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.prokharc...@citrix.commailto:alena.prokharc...@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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Wednesday, June 25, 2014 at 9:03 PM To: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
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.prokharc...@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.o...@sungardas.com Date: Thursday, June 26, 2014 at 11:32 AM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.prokharc...@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.o...@sungardas.com Date: Wednesday, June 25, 2014 at 9:03 PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
I did logic review according to the FS assuming that the FS (and the design described there) was approved on the [PROPOSAL] stage, BEFORE the code was put it to the review board. Was it approved at that stage? Alex, the feature is not small, and considering that it raised so many questions and arguing, I would really like to get a final design/logic review + “ship it” from people having expertise on the topic, and/or who originally participated in review/discussion: Chiradeep, Kishan, Murail. Thank you, Alena. From: Alex Ough alex.o...@sungardas.commailto:alex.o...@sungardas.com Date: Thursday, June 26, 2014 at 1:53 PM To: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@citrix.com Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes) 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.prokharc...@citrix.commailto:alena.prokharc...@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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Thursday, June 26, 2014 at 11:32 AM To: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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.prokharc...@citrix.commailto:alena.prokharc...@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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
Sounds like it goes back to what I said I wish they have been involved more actively from the start. Thanks but really making me tired. Alex Ough On Thu, Jun 26, 2014 at 5:17 PM, Alena Prokharchyk alena.prokharc...@citrix.com wrote: I did logic review according to the FS assuming that the FS (and the design described there) was approved on the [PROPOSAL] stage, BEFORE the code was put it to the review board. Was it approved at that stage? Alex, the feature is not small, and considering that it raised so many questions and arguing, I would really like to get a final design/logic review + “ship it” from people having expertise on the topic, and/or who originally participated in review/discussion: Chiradeep, Kishan, Murail. Thank you, Alena. From: Alex Ough alex.o...@sungardas.com Date: Thursday, June 26, 2014 at 1:53 PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.com Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes) 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.prokharc...@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.o...@sungardas.com Date: Thursday, June 26, 2014 at 11:32 AM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.prokharc...@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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
All, I apologize for joining this conversation late. I understand that this patch was submitted back in February. Around this time, my family had a significant medical event, and I was disengaged from all work activities — missing the original conversation. Reading through the specification, and briefly reviewing the code, I would like to understand the following assumptions/design decisions: 1. Why aren’t projects being sync’ed? It seems very likely that users would want to have projects span data centers for redundancy/DR purposes. 2. Why aren’t events being sync’ed? I can imagine a number of scenarios where I would want to examine the operation of an logical application or system across both regions. Without the sync of event data, I would be forced to either perform that interleave visually with two browser tabs or dump the data into another datastore to be merged. 3. Why isn’t template metadata being sync’ed? When spanning an application/system across regions, it would seem to follow that I would want to use the same templates. 4. How does this design deal with modifications to a record in two or more regions during a network partition? 5. Given that messages can/will be processed out of order, how is referential integrity maintained when a parent and a set of children are created (e.g. creation of a new account and a set of users rapidly through the API)? 6. Is RabbitMQ being relied upon to provide partition tolerance? 7. Is there a back pressure mechanism to throttle the full sync operation when the database/management server is under heavy load? Finally, I would like to understand why we are taking on multi-datacenter data replication in CloudStack, and not deferring to underlying datastore. Speaking as someone whose $dayjob involves delivering such a system (at Basho for Riak), it is a very hard thing to get right (there literally thousands of corner cases). The design document does not speak to this decision, and I would like understand how CloudStack could not leverage existing, mature mechanisms at the datastore-level. I apologize if some of these questions have been answered already. I attempt to look back in the archives, but given the span of this conversation, it was difficult to piece together retroactively. Thanks, -John On June 26, 2014 at 5:34:31 PM, Alex Ough (alex.o...@sungardas.com) wrote: Sounds like it goes back to what I said I wish they have been involved more actively from the start. Thanks but really making me tired. Alex Ough On Thu, Jun 26, 2014 at 5:17 PM, Alena Prokharchyk alena.prokharc...@citrix.com wrote: I did logic review according to the FS assuming that the FS (and the design described there) was approved on the [PROPOSAL] stage, BEFORE the code was put it to the review board. Was it approved at that stage? Alex, the feature is not small, and considering that it raised so many questions and arguing, I would really like to get a final design/logic review + “ship it” from people having expertise on the topic, and/or who originally participated in review/discussion: Chiradeep, Kishan, Murail. Thank you, Alena. From: Alex Ough alex.o...@sungardas.com Date: Thursday, June 26, 2014 at 1:53 PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.com Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes) 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.prokharc...@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
RE: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
Alex, You are correct. It should be Integer and not Long. -Original Message- From: Alex Ough [mailto:alex.o...@sungardas.com] Sent: Thursday, 26 June 2014 8:09 PM To: Kishan Kavala Cc: cloudstack Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes) Kishan, The type of region id is Integer, not Long, so I'm wondering why it should be Long. Alex Ough On Thu, Jun 26, 2014 at 2:08 AM, Kishan Kavala kishan.kav...@citrix.com wrote: This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/ Alex, As discussed on the mailing list, ORIGINATEDREGIONUUID should be the regionId which is Long. So all the ORIGINATEDREGIONUUID references should just be ORIGINATEDREGIONID and of datatype Long. - Kishan Kavala On June 24th, 2014, 9:24 p.m. IST, Alex Ough wrote: Review request for cloudstack. By Alex Ough. *Updated June 24, 2014, 9:24 p.m.* *Repository: * cloudstack-git Description This is the review request for the core changes related with #17790 that has only the new plugin codes. 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. 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/CreateAccount Cmd.java (50d67d9) - api/src/org/apache/cloudstack/api/command/admin/account/DeleteAccount Cmd.java (5754ec5) - api/src/org/apache/cloudstack/api/command/admin/account/DisableAccount Cmd.java (3e5e1d3) - api/src/org/apache/cloudstack/api/command/admin/account/EnableAccount Cmd.java (f30c985) - api/src/org/apache/cloudstack/api/command/admin/account/LockAccountCm d.java (3c185e4) - api/src/org/apache/cloudstack/api/command/admin/account/UpdateAccount Cmd.java (a7ce74a) - api/src/org/apache/cloudstack/api/command/admin/domain/CreateDomainC md.java (312c9ee) - api/src/org/apache/cloudstack/api/command/admin/domain/DeleteDomainC md.java (a6d2b0b) - api/src/org/apache/cloudstack/api/command/admin/domain/UpdateDomain Cmd.java (409a84d) - api/src/org/apache/cloudstack/api/command/admin/region/AddRegionCmd.j ava (f6743ba) - api/src/org/apache/cloudstack/api/command/admin/region/UpdateRegionC md.java (b08cbbb) - api/src/org/apache/cloudstack/api/command/admin/user/CreateUserCmd.jav a (8f223ac) - api/src/org/apache/cloudstack/api/command/admin/user/DeleteUserCmd.jav a (08ba521) - api/src/org/apache/cloudstack/api/command/admin/user/DisableUserCmd.ja va (c6e09ef) - api/src/org/apache/cloudstack/api/command/admin/user/EnableUserCmd.jav a (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.ja va (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/MockAcc ountManager.java (4136b5c) - plugins/pom.xml (b5e6a61) - plugins/user- authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAcc ountCmd.java (b753952) - plugins/user- authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUs ersCmd.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
RE: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
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.o...@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.o...@sungardas.commailto:alex.o...@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.prokharc...@citrix.commailto:alena.prokharc...@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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Tuesday, June 24, 2014 at 5:41 PM To: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com Cc: dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org, Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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.prokharc...@citrix.commailto:alena.prokharc...@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
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.kav...@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.o...@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.o...@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.prokharc...@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.o...@sungardas.com *Date: *Tuesday, June 24, 2014 at 5:41 PM *To: *Alena Prokharchyk alena.prokharc...@citrix.com *Cc: *dev@cloudstack.apache.org dev@cloudstack.apache.org, Kishan Kavala kishan.kav...@citrix.com, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.prokharc...@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
RE: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
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.o...@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.kav...@citrix.commailto:kishan.kav...@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.idhttp://region.id in db.properties 2. Add a new column in region table From: Alex Ough [mailto:alex.o...@sungardas.commailto:alex.o...@sungardas.com] Sent: Wednesday, 25 June 2014 8:18 AM To: Alena Prokharchyk Cc: dev@cloudstack.apache.orgmailto: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.o...@sungardas.commailto:alex.o...@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.prokharc...@citrix.commailto:alena.prokharc...@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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Tuesday, June 24, 2014 at 5:41 PM To: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com Cc: dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org, Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Wednesday, June 25, 2014 at 4:02 PM To: Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com Cc: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com, dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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.kav...@citrix.commailto:kishan.kav...@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.o...@sungardas.commailto:alex.o...@sungardas.com] Sent: Wednesday, 25 June 2014 4:31 PM To: Kishan Kavala Cc: Alena Prokharchyk; dev@cloudstack.apache.orgmailto: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.kav...@citrix.commailto:kishan.kav...@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.idhttp://region.id in db.properties 2. Add a new column in region table From: Alex Ough [mailto:alex.o...@sungardas.commailto:alex.o...@sungardas.com] Sent: Wednesday, 25 June 2014 8:18 AM To: Alena Prokharchyk Cc: dev@cloudstack.apache.orgmailto: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.o...@sungardas.commailto:alex.o...@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.prokharc...@citrix.commailto:alena.prokharc...@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
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.prokharc...@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.o...@sungardas.com Date: Wednesday, June 25, 2014 at 4:02 PM To: Kishan Kavala kishan.kav...@citrix.com Cc: Alena Prokharchyk alena.prokharc...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.kav...@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.o...@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.kav...@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.o...@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.o...@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.prokharc...@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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Wednesday, June 25, 2014 at 6:33 PM To: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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.prokharc...@citrix.commailto:alena.prokharc...@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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Wednesday, June 25, 2014 at 4:02 PM To: Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com Cc: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com, dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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.kav...@citrix.commailto:kishan.kav...@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.o...@sungardas.commailto:alex.o...@sungardas.com] Sent: Wednesday, 25 June 2014 4:31 PM To: Kishan Kavala Cc: Alena Prokharchyk; dev@cloudstack.apache.orgmailto: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.kav...@citrix.commailto:kishan.kav...@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.idhttp://region.id in db.properties 2. Add a new column in region
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.prokharc...@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.o...@sungardas.com Date: Wednesday, June 25, 2014 at 6:33 PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.prokharc...@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.o...@sungardas.com Date: Wednesday, June 25, 2014 at 4:02 PM To: Kishan Kavala kishan.kav...@citrix.com Cc: Alena Prokharchyk alena.prokharc...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.kav...@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.o...@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.kav...@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.o...@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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Wednesday, June 25, 2014 at 7:12 PM To: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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.prokharc...@citrix.commailto:alena.prokharc...@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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Wednesday, June 25, 2014 at 6:33 PM To: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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.prokharc...@citrix.commailto:alena.prokharc...@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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Wednesday, June 25, 2014 at 4:02 PM To: Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com Cc: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com, dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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
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.prokharc...@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.o...@sungardas.com Date: Wednesday, June 25, 2014 at 7:12 PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.prokharc...@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.o...@sungardas.com Date: Wednesday, June 25, 2014 at 6:33 PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: Kishan Kavala kishan.kav...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.prokharc...@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.o...@sungardas.com Date: Wednesday, June 25, 2014 at 4:02 PM To: Kishan Kavala kishan.kav...@citrix.com Cc: Alena Prokharchyk alena.prokharc...@citrix.com, dev@cloudstack.apache.org dev@cloudstack.apache.org, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.com Subject: Re: Review Request 20099: Domain-Account-User Sync Up
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
--- 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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
Moving the discussion to the mailing list as it doesn’t have to be private.
Kishan/Murali, can you please follow up on the remaining review for
https://reviews.apache.org/r/20099/ (see my last comment and the discussion
below)
Basically what Alex wants to do is – pass the originated region to
create/update/deleteAccount commands. And the question is – what type this
parameter should have (see details below)
Thanks,
Alena.
From: Alena Prokharchyk
alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com
Date: Tuesday, June 24, 2014 at 3:06 PM
To: Alex Ough alex.o...@sungardas.commailto:alex.o...@sungardas.com, Kishan
Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, Murali
Reddy murali.re...@citrix.commailto:murali.re...@citrix.com
Cc: Animesh Chaturvedi
animesh.chaturv...@citrix.commailto:animesh.chaturv...@citrix.com, Ram
Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com
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.prokharc...@citrix.commailto:alena.prokharc...@citrix.com
Date: Tuesday, June 24, 2014 at 2:54 PM
To: Alex Ough alex.o...@sungardas.commailto:alex.o...@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
id1/id
nameLocal/name
endpointhttp://localhost:8080/client//endpoint
gslbserviceenabledtrue/gslbserviceenabled
portableipserviceenabledfalse/portableipserviceenabled
/region
Please correct if I miss something.
-Alena.
From: Alex Ough alex.o...@sungardas.commailto:alex.o...@sungardas.com
Date: Tuesday, June 24, 2014 at 2:33 PM
To: Alena Prokharchyk
alena.prokharc...@citrix.commailto:alena.prokharc...@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.prokharc...@citrix.commailto:alena.prokharc...@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
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.chaturv...@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.prokharc...@citrix.com
*Date: *Tuesday, June 24, 2014 at 2:54 PM
*To: *Alex Ough alex.o...@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
id1/id
nameLocal/name
endpointhttp://localhost:8080/client//endpoint
gslbserviceenabledtrue/gslbserviceenabled
portableipserviceenabledfalse/portableipserviceenabled
/region
Please correct if I miss something.
-Alena.
*From: *Alex Ough alex.o...@sungardas.com
*Date: *Tuesday, June 24, 2014 at 2:33 PM
*To: *Alena Prokharchyk alena.prokharc...@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.prokharc...@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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Tuesday, June 24, 2014 at 4:22 PM To: dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org Cc: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com, Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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.chaturv...@citrix.commailto:animesh.chaturv...@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.prokharc...@citrix.commailto:alena.prokharc...@citrix.com Date: Tuesday, June 24, 2014 at 2:54 PM To: Alex Ough alex.o...@sungardas.commailto:alex.o...@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 id1/id nameLocal/name endpointhttp://localhost:8080/client//endpoint gslbserviceenabledtrue/gslbserviceenabled portableipserviceenabledfalse/portableipserviceenabled /region Please correct if I miss something. -Alena. From: Alex Ough alex.o...@sungardas.commailto:alex.o...@sungardas.com Date: Tuesday, June 24, 2014 at 2:33 PM To: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@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
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.prokharc...@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.o...@sungardas.com Date: Tuesday, June 24, 2014 at 4:22 PM To: dev@cloudstack.apache.org dev@cloudstack.apache.org Cc: Alena Prokharchyk alena.prokharc...@citrix.com, Kishan Kavala kishan.kav...@citrix.com, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.chaturv...@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.prokharc...@citrix.com *Date: *Tuesday, June 24, 2014 at 2:54 PM *To: *Alex Ough alex.o...@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 id1/id nameLocal/name endpointhttp://localhost:8080/client//endpoint gslbserviceenabledtrue/gslbserviceenabled portableipserviceenabledfalse/portableipserviceenabled /region Please correct if I miss something. -Alena. *From: *Alex Ough alex.o...@sungardas.com *Date: *Tuesday, June 24, 2014 at 2:33 PM *To: *Alena Prokharchyk alena.prokharc...@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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Tuesday, June 24, 2014 at 5:17 PM To: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com Cc: dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org, Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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.prokharc...@citrix.commailto:alena.prokharc...@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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Tuesday, June 24, 2014 at 4:22 PM To: dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org Cc: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com, Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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.chaturv...@citrix.commailto:animesh.chaturv...@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
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.prokharc...@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.o...@sungardas.com Date: Tuesday, June 24, 2014 at 5:17 PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: dev@cloudstack.apache.org dev@cloudstack.apache.org, Kishan Kavala kishan.kav...@citrix.com, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.prokharc...@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.o...@sungardas.com Date: Tuesday, June 24, 2014 at 4:22 PM To: dev@cloudstack.apache.org dev@cloudstack.apache.org Cc: Alena Prokharchyk alena.prokharc...@citrix.com, Kishan Kavala kishan.kav...@citrix.com, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.chaturv...@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.prokharc
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Tuesday, June 24, 2014 at 5:41 PM To: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com Cc: dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org, Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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.prokharc...@citrix.commailto:alena.prokharc...@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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Tuesday, June 24, 2014 at 5:17 PM To: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com Cc: dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org, Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.commailto:ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@citrix.commailto:animesh.chaturv...@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.prokharc...@citrix.commailto:alena.prokharc...@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.o...@sungardas.commailto:alex.o...@sungardas.com Date: Tuesday, June 24, 2014 at 4:22 PM To: dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org dev@cloudstack.apache.orgmailto:dev@cloudstack.apache.org Cc: Alena Prokharchyk alena.prokharc...@citrix.commailto:alena.prokharc...@citrix.com, Kishan Kavala kishan.kav...@citrix.commailto:kishan.kav...@citrix.com, Murali Reddy murali.re...@citrix.commailto:murali.re...@citrix.com, Ram Ganesh
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
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.prokharc...@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.o...@sungardas.com Date: Tuesday, June 24, 2014 at 5:41 PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: dev@cloudstack.apache.org dev@cloudstack.apache.org, Kishan Kavala kishan.kav...@citrix.com, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.prokharc...@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.o...@sungardas.com Date: Tuesday, June 24, 2014 at 5:17 PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: dev@cloudstack.apache.org dev@cloudstack.apache.org, Kishan Kavala kishan.kav...@citrix.com, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.prokharc...@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.o...@sungardas.com Date: Tuesday, June 24, 2014 at 4:22 PM To: dev@cloudstack.apache.org dev@cloudstack.apache.org Cc: Alena Prokharchyk alena.prokharc...@citrix.com, Kishan Kavala kishan.kav...@citrix.com, Murali Reddy murali.re
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.o...@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.prokharc...@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.o...@sungardas.com Date: Tuesday, June 24, 2014 at 5:41 PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: dev@cloudstack.apache.org dev@cloudstack.apache.org, Kishan Kavala kishan.kav...@citrix.com, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.prokharc...@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.o...@sungardas.com Date: Tuesday, June 24, 2014 at 5:17 PM To: Alena Prokharchyk alena.prokharc...@citrix.com Cc: dev@cloudstack.apache.org dev@cloudstack.apache.org, Kishan Kavala kishan.kav...@citrix.com, Murali Reddy murali.re...@citrix.com, Ram Ganesh ram.gan...@citrix.com, Animesh Chaturvedi animesh.chaturv...@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.prokharc...@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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/ --- (Updated June 20, 2014, 3:06 p.m.) Review request for cloudstack. Changes --- No change in 'spring-server-core-managers-context.xml' because of an issue during starting the management server. Repository: cloudstack-git Description --- This is the review request for the core changes related with #17790 that has only the new plugin codes. Diffs (updated) - api/src/com/cloud/event/EventTypes.java 471b3f6 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/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml fc1c7e2 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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
On June 17, 2014, 5:06 p.m., Alena Prokharchyk wrote:
One more comment regarding spring-server-core-managers-context.xml. As your
code comes as a CS plugin, all the managers of your plugin should be
defined in your plugin's *-context.xml file, not
spring-server-core-managers-context.xml. These are the classes of your
plugin:
bean id=fullSyncer
class=org.apache.cloudstack.mom.multiregion.FullSyncer /
bean id=fullScanner
class=org.apache.cloudstack.mom.multiregion.service.FullScanner /
bean id=autoGenerator
class=org.apache.cloudstack.mom.multiregion.simulator.SimulatorAutoGenerator
/
bean id=domainSubscriber
class=org.apache.cloudstack.mom.multiregion.subscriber.DomainSubscriber /
bean id=accountSubscriber
class=org.apache.cloudstack.mom.multiregion.subscriber.AccountSubscriber
/
bean id=userSubscriber
class=org.apache.cloudstack.mom.multiregion.subscriber.UserSubscriber /
bean id=injectedCollection
class=org.apache.cloudstack.mom.multiregion.InjectedCollection /
bean id=syncQueryManagerImpl
class=org.apache.cloudstack.mom.multiregion.api.response.SyncQueryManagerImpl/
bean id=apiSyncResponseHelper
class=org.apache.cloudstack.mom.multiregion.api.response.ApiSyncResponseHelper/
bean id=syncAccountJoinDaoImpl
class=org.apache.cloudstack.mom.multiregion.api.dao.SyncAccountJoinDaoImpl
/
bean id=syncUserAccountJoinDaoImpl
class=org.apache.cloudstack.mom.multiregion.api.dao.SyncUserAccountJoinDaoImpl
/
If you want to look at the example, look at any CS plugin, for example
cloud-plugin-hypervisor-vmware (refer to spring-vmware-core-context.xml
file to see how the managers are defined there)
I moved to
plugins/event-bus/multiregion/resources/META-INF/cloudstack/spring-plugin-multiregion-system-context.xml
and have this error during starting the mgmt server.
org.springframework.beans.factory.BeanCreationException: Error creating bean
with name 'eventNotificationBus': Injection of autowired dependencies failed;
nested exception is org.springframework.beans.factory.BeanCreationException:
Could not autowire field: protected
org.apache.cloudstack.mom.multiregion.subscriber.DomainSubscriber
org.apache.cloudstack.mom.multiregion.MultiRegionEventBus.domainSubscriber;
nested exception is
org.springframework.beans.factory.NoSuchBeanDefinitionException: No qualifying
bean of type
[org.apache.cloudstack.mom.multiregion.subscriber.DomainSubscriber] found for
dependency: expected at least 1 bean which qualifies as autowire candidate for
this dependency. Dependency annotations: {@javax.inject.Inject()}
Can you give me how to resolve this?
- Alex
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20099/#review45955
---
On June 15, 2014, 9:40 p.m., Alex Ough wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20099/
---
(Updated June 15, 2014, 9:40 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 39ef710
api/src/com/cloud/user/AccountService.java 7e37b38
api/src/com/cloud/user/DomainService.java 4c1f93d
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/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
51e218d
api/src/org/apache/cloudstack/api/command/admin/user/DeleteUserCmd.java
08ba521
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/#review45955 --- One more comment regarding spring-server-core-managers-context.xml. As your code comes as a CS plugin, all the managers of your plugin should be defined in your plugin's *-context.xml file, not spring-server-core-managers-context.xml. These are the classes of your plugin: bean id=fullSyncer class=org.apache.cloudstack.mom.multiregion.FullSyncer / bean id=fullScanner class=org.apache.cloudstack.mom.multiregion.service.FullScanner / bean id=autoGenerator class=org.apache.cloudstack.mom.multiregion.simulator.SimulatorAutoGenerator / bean id=domainSubscriber class=org.apache.cloudstack.mom.multiregion.subscriber.DomainSubscriber / bean id=accountSubscriber class=org.apache.cloudstack.mom.multiregion.subscriber.AccountSubscriber / bean id=userSubscriber class=org.apache.cloudstack.mom.multiregion.subscriber.UserSubscriber / bean id=injectedCollection class=org.apache.cloudstack.mom.multiregion.InjectedCollection / bean id=syncQueryManagerImpl class=org.apache.cloudstack.mom.multiregion.api.response.SyncQueryManagerImpl/ bean id=apiSyncResponseHelper class=org.apache.cloudstack.mom.multiregion.api.response.ApiSyncResponseHelper/ bean id=syncAccountJoinDaoImpl class=org.apache.cloudstack.mom.multiregion.api.dao.SyncAccountJoinDaoImpl / bean id=syncUserAccountJoinDaoImpl class=org.apache.cloudstack.mom.multiregion.api.dao.SyncUserAccountJoinDaoImpl / If you want to look at the example, look at any CS plugin, for example cloud-plugin-hypervisor-vmware (refer to spring-vmware-core-context.xml file to see how the managers are defined there) - Alena Prokharchyk On June 15, 2014, 9:40 p.m., Alex Ough wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/ --- (Updated June 15, 2014, 9:40 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 39ef710 api/src/com/cloud/user/AccountService.java 7e37b38 api/src/com/cloud/user/DomainService.java 4c1f93d 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/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 51e218d 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 cf5d355 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 d8dbde7 client/tomcatconf/commands.properties.in 45debe4 engine/schema/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml 489b37d engine/schema/src/com/cloud/user/AccountVO.java 0f5a044 engine/schema/src/org/apache/cloudstack/region/RegionVO.java 608bd2b
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/#review45853 --- Alex, * add since=version attribute to all new parameters you've added to the existing API calls * remove listSyncAccounts/ listSyncUsers/listSyncDomains/listSyncDomainChildren * The important one - while on the call with Alex Huang, and later in all the countless emails, we've agreed not to modify the event itself, in order not to overload it with the details just specific to your plugin or the region. Now I see this in ActionEventUtils: eventDescription.put(oldentityname, oldEntityName); eventDescription.put(originatedregionuuid, originatedRegionUuid); You said the parameter would be saved and read to/from CallContext only? Without touching anything else in the CS code like account/domain/user/event objects. Please remove it from the event. Or come up with a different solution that doesn't touch the event. - Alena Prokharchyk On June 15, 2014, 9:40 p.m., Alex Ough wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/ --- (Updated June 15, 2014, 9:40 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 39ef710 api/src/com/cloud/user/AccountService.java 7e37b38 api/src/com/cloud/user/DomainService.java 4c1f93d 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/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 51e218d 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 cf5d355 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 d8dbde7 client/tomcatconf/commands.properties.in 45debe4 engine/schema/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml 489b37d 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 957f708 plugins/pom.xml 9b391b8 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java 626bb8f plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java 887ad00 server/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml fc1c7e2 server/src/com/cloud/api/ApiResponseHelper.java 38f2f0b server/src/com/cloud/api/dispatch/ParamProcessWorker.java e9bdd8b 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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
On Mon, Jun 16, 2014 at 7:07 PM, Alena Prokharchyk alena.prokharc...@citrix.com wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/#review45853 --- Alex, * add since=version attribute to all new parameters you've added to the existing API calls * remove listSyncAccounts/ listSyncUsers/listSyncDomains/listSyncDomainChildren WHY?? * The important one - while on the call with Alex Huang, and later in all the countless emails, we've agreed not to modify the event itself, in order not to overload it with the details just specific to your plugin or the region. Now I see this in ActionEventUtils: eventDescription.put(oldentityname, oldEntityName); eventDescription.put(originatedregionuuid, originatedRegionUuid); WHY??? You said the parameter would be saved and read to/from CallContext only? Without touching anything else in the CS code like account/domain/user/event objects. Please remove it from the event. Or come up with a different solution that doesn't touch the event. WHAT DO YOU MEAN?? - Alena Prokharchyk On June 15, 2014, 9:40 p.m., Alex Ough wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/ --- (Updated June 15, 2014, 9:40 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 39ef710 api/src/com/cloud/user/AccountService.java 7e37b38 api/src/com/cloud/user/DomainService.java 4c1f93d 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/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 51e218d 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 cf5d355 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 d8dbde7 client/tomcatconf/commands.properties.in 45debe4 engine/schema/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml 489b37d 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 957f708 plugins/pom.xml 9b391b8 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java 626bb8f plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java 887ad00 server/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml fc1c7e2 server/src/com/cloud/api/ApiResponseHelper.java 38f2f0b server/src/com/cloud/api/dispatch/ParamProcessWorker.java e9bdd8b server/src/com/cloud/event/ActionEventUtils.java 28e5680 server/src/com/cloud/projects/ProjectManagerImpl.java d10c059
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
On 6/16/14, 4:13 PM, Alex Ough alex.o...@sungardas.com wrote:
On Mon, Jun 16, 2014 at 7:07 PM, Alena Prokharchyk
alena.prokharc...@citrix.com wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20099/#review45853
---
Alex,
* add since=version attribute to all new parameters you've added to
the
existing API calls
* remove listSyncAccounts/
listSyncUsers/listSyncDomains/listSyncDomainChildren
WHY??
Because when the API commands come as a part of your plugin, they don¹t
have to be registered in the commands.properties file. This file defines
the permissions only, and you can define the permissions in your commands
itself. Use ³authorized² parameter in the @ApiCommand annotation. Example:
authorized = {RoleType.Admin}
If the command is available to Root admin only (equivalent to =1 in
commands.properties)
* The important one - while on the call with Alex Huang, and later in
all
the countless emails, we've agreed not to modify the event itself, in
order
not to overload it with the details just specific to your plugin or the
region. Now I see this in ActionEventUtils:
eventDescription.put(oldentityname, oldEntityName);
eventDescription.put(originatedregionuuid, originatedRegionUuid);
WHY???
Alex, because in a numerous number of email threads below we¹ve agreed on
the fact that the event object itself won¹t be overloaded and we will keep
it simple. If every plugin overloads the event with the details only
related to this plugin, there will be a mess. We¹ve discussed the way when
your plugin modifies account/domain/user objects instead, and then reads
from them. Then you said it wouldn¹t work and you would read from the
CallContext (memory). You want me to pull out this thread? There was no
notion about modifying/overloading the event object in the DB itself. If
you can find it and put it here, I would appreciate that.
You said the parameter would be saved and read to/from CallContext only?
Without touching anything else in the CS code like
account/domain/user/event objects. Please remove it from the event. Or
come
up with a different solution that doesn't touch the event.
WHAT DO YOU MEAN??
- Alena Prokharchyk
On June 15, 2014, 9:40 p.m., Alex Ough wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20099/
---
(Updated June 15, 2014, 9:40 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 39ef710
api/src/com/cloud/user/AccountService.java 7e37b38
api/src/com/cloud/user/DomainService.java 4c1f93d
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/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.ja
va
3c185e4
api/src/org/apache/cloudstack/api/command/admin/account/UpdateAccountCmd.
java
a7ce74a
api/src/org/apache/cloudstack/api/command/admin/domain/CreateDomainCmd.ja
va
312c9ee
api/src/org/apache/cloudstack/api/command/admin/domain/DeleteDomainCmd.ja
va
a6d2b0b
api/src/org/apache/cloudstack/api/command/admin/domain/UpdateDomainCmd.ja
va
409a84d
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/command/admin/user/CreateUserCmd.java
51e218d
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
cf5d355
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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/ --- (Updated June 15, 2014, 9:40 p.m.) Review request for cloudstack. Changes --- Please review Repository: cloudstack-git Description --- This is the review request for the core changes related with #17790 that has only the new plugin codes. Diffs (updated) - api/src/com/cloud/event/EventTypes.java 39ef710 api/src/com/cloud/user/AccountService.java 7e37b38 api/src/com/cloud/user/DomainService.java 4c1f93d 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/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 51e218d 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 cf5d355 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 d8dbde7 client/tomcatconf/commands.properties.in 45debe4 engine/schema/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml 489b37d 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 957f708 plugins/pom.xml 9b391b8 plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java 626bb8f plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java 887ad00 server/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml fc1c7e2 server/src/com/cloud/api/ApiResponseHelper.java 38f2f0b server/src/com/cloud/api/dispatch/ParamProcessWorker.java e9bdd8b 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/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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/ --- (Updated May 7, 2014, 10:47 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 (updated) - api/src/com/cloud/event/EventTypes.java 39ef710 api/src/com/cloud/user/AccountService.java 7e37b38 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/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 d8dbde7 client/tomcatconf/commands.properties.in 45debe4 engine/schema/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml 489b37d engine/schema/src/org/apache/cloudstack/region/RegionVO.java 608bd2b 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/ApiResponseHelper.java 38f2f0b server/src/com/cloud/api/dispatch/ParamProcessWorker.java e9bdd8b server/src/com/cloud/event/ActionEventUtils.java 28e5680 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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
On May 5, 2014, 6:27 p.m., Alena Prokharchyk wrote: Alex, generic logic looks good to me. But still some things need to be fixed. 1) RegionVO @Column(name = active) private boolean active; Explicitly set it to be active by default. Just setting it in the DB might not be enough because DAO would always default boolean to false if not set in the constructor/intance variable declaration. It should look like: @Column(name = active) private boolean active = true; 2) Please make the description for the new parameters in the addRegion API , more descriptive @Parameter(name = ApiConstants.MODE, type = CommandType.STRING, required = true, description = Region service active) 55 private String mode; 3) AccountManager * boolean updateUser(final Long id, String userName, String firstName, String lastName, String password, String email, String apiKey, String secretKey, String timeZone, Account.State state); Why do we pass account state to the call? there should be only user info passed in. If the account needs to be updated, do it in the separate command; if you need to retrive the account state - get the account object from the user object. Please remove this parameter * Why did you change the method signature for the UserAccount disableUser(long userId) to disableUser(User user)? The only one info that you have to pass to the manager - is the userId. Please change it back. The same for boolean enableUser(User user), boolean lockAccount(Account account); These changes don't seem to be critical or used for/by your plugin, so please change them back to the original way of doing things. Its not a good practice to change methods defined in the interfaces - AccountManager/AccountService - unless there is a real need for it. * boolean updateAccount(AccountVO account, String newAccountName, String newNetworkDomain, final MapString, String details, Account.State state); As updateAccount doesn't allow account state update, please remove Account.State from the method signature 4) ApiResponseHelper.java Why did you change createDomainResponse method? public DomainResponse createDomainResponse(Domain domain)? To summarize it all, I would suggest to eliminate (revert) all the changes done to account/domain managers that your code doesn't actually need. It would make the changes list much shorter and easier for review. for 3) AccountManager * updateUser in 'state' : it is necessary during the Full Sync when an object's information is not same with one in other regions. * new signatures : they are necessary during the Full Sync not to generate events while changing them. They are just another methods to manage objects, so can you show me why they are not permitted? - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/#review42175 --- On May 4, 2014, 9:12 p.m., Alex Ough wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/ --- (Updated May 4, 2014, 9:12 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 39ef710 api/src/com/cloud/user/AccountService.java 7e37b38 api/src/org/apache/cloudstack/api/ApiConstants.java fdb4558 api/src/org/apache/cloudstack/api/BaseCmd.java f6f21ae api/src/org/apache/cloudstack/api/ResponseGenerator.java 10fb6df 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/RegionResponse.java 6c74fa6 api/src/org/apache/cloudstack/query/QueryService.java 01af631 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 client/tomcatconf/commands.properties.in 45debe4 engine/schema/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml 489b37d engine/schema/src/org/apache/cloudstack/region/RegionVO.java 608bd2b 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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
On May 5, 2014, 6:27 p.m., Alena Prokharchyk wrote:
Alex, generic logic looks good to me. But still some things need to be
fixed.
1) RegionVO
@Column(name = active)
private boolean active;
Explicitly set it to be active by default. Just setting it in the DB might
not be enough because DAO would always default boolean to false if not set
in the constructor/intance variable declaration. It should look like:
@Column(name = active)
private boolean active = true;
2) Please make the description for the new parameters in the addRegion API
, more descriptive
@Parameter(name = ApiConstants.MODE, type = CommandType.STRING, required =
true, description = Region service active)
55
private String mode;
3) AccountManager
* boolean updateUser(final Long id, String userName, String firstName,
String lastName, String password, String email, String apiKey, String
secretKey, String timeZone, Account.State state);
Why do we pass account state to the call? there should be only user info
passed in. If the account needs to be updated, do it in the separate
command; if you need to retrive the account state - get the account object
from the user object. Please remove this parameter
* Why did you change the method signature for the
UserAccount disableUser(long userId) to disableUser(User user)? The only
one info that you have to pass to the manager - is the userId. Please
change it back.
The same for boolean enableUser(User user), boolean lockAccount(Account
account);
These changes don't seem to be critical or used for/by your plugin, so
please change them back to the original way of doing things. Its not a good
practice to change methods defined in the interfaces -
AccountManager/AccountService - unless there is a real need for it.
* boolean updateAccount(AccountVO account, String newAccountName, String
newNetworkDomain, final MapString, String details, Account.State state);
As updateAccount doesn't allow account state update, please remove
Account.State from the method signature
4) ApiResponseHelper.java
Why did you change createDomainResponse method?
public DomainResponse createDomainResponse(Domain domain)?
To summarize it all, I would suggest to eliminate (revert) all the changes
done to account/domain managers that your code doesn't actually need. It
would make the changes list much shorter and easier for review.
Alex Ough wrote:
for 3) AccountManager
* updateUser in 'state' : it is necessary during the Full Sync when an
object's information is not same with one in other regions.
* new signatures : they are necessary during the Full Sync not to
generate events while changing them. They are just another methods to manage
objects, so can you show me why they are not permitted?
Alex, correct me if I'm wrong. In most of your code you use CS APIs to
retrieve/update/delete/disable(disable changes the state) the CS objects
(user/domain/account - updateAccount/updateDomain/deleteAccount etc). And you
do it in each reason, and that generates the events for all the regions when
these APIs are processed. Like for example, I see these calls:
public JSONObject updateAccount(String accountId, String currentName, String
newName, String details, String domainId, String networkDomain) throws
APIFailureException {
139
140
StringBuilder param = buildCmd(updateAccount);
141
param.append(append(ApiConstants.ID, accountId));
142
if (currentName != null) param.append(append(ApiConstants.ACCOUNT,
currentName));
143
if (newName != null) param.append(append(ApiConstants.NEW_NAME,
newName));
144
if (details != null) param.append(append(ApiConstants.ACCOUNT_DETAILS,
details));
145
if (domainId != null) param.append(append(ApiConstants.DOMAIN_ID,
domainId));
146
if (networkDomain != null)
param.append(append(ApiConstants.NETWORK_DOMAIN, networkDomain));
147
param.append(appendResType(json));
148
param.append(appendSessionKey(encodeSessionKey()));
149
150
return sendApacheGet(param.toString());
151
}
152
153
public JSONObject disableAccount(String accountId) throws
APIFailureException {
164
165
StringBuilder param = buildCmd(disableAccount);
166
param.append(append(ApiConstants.ID, accountId));
167
param.append(append(ApiConstants.LOCK, false));
168
param.append(appendResType(json));
169
param.append(appendSessionKey(encodeSessionKey()));
170
171
return sendApacheGet(param.toString());
172
}
But in some parts of your code you call to the AccountManager/domainManager
interfaces directly, omitting APIs, to eliminate the events generation - like
for the state
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
On May 5, 2014, 6:27 p.m., Alena Prokharchyk wrote:
Alex, generic logic looks good to me. But still some things need to be
fixed.
1) RegionVO
@Column(name = active)
private boolean active;
Explicitly set it to be active by default. Just setting it in the DB might
not be enough because DAO would always default boolean to false if not set
in the constructor/intance variable declaration. It should look like:
@Column(name = active)
private boolean active = true;
2) Please make the description for the new parameters in the addRegion API
, more descriptive
@Parameter(name = ApiConstants.MODE, type = CommandType.STRING, required =
true, description = Region service active)
55
private String mode;
3) AccountManager
* boolean updateUser(final Long id, String userName, String firstName,
String lastName, String password, String email, String apiKey, String
secretKey, String timeZone, Account.State state);
Why do we pass account state to the call? there should be only user info
passed in. If the account needs to be updated, do it in the separate
command; if you need to retrive the account state - get the account object
from the user object. Please remove this parameter
* Why did you change the method signature for the
UserAccount disableUser(long userId) to disableUser(User user)? The only
one info that you have to pass to the manager - is the userId. Please
change it back.
The same for boolean enableUser(User user), boolean lockAccount(Account
account);
These changes don't seem to be critical or used for/by your plugin, so
please change them back to the original way of doing things. Its not a good
practice to change methods defined in the interfaces -
AccountManager/AccountService - unless there is a real need for it.
* boolean updateAccount(AccountVO account, String newAccountName, String
newNetworkDomain, final MapString, String details, Account.State state);
As updateAccount doesn't allow account state update, please remove
Account.State from the method signature
4) ApiResponseHelper.java
Why did you change createDomainResponse method?
public DomainResponse createDomainResponse(Domain domain)?
To summarize it all, I would suggest to eliminate (revert) all the changes
done to account/domain managers that your code doesn't actually need. It
would make the changes list much shorter and easier for review.
Alex Ough wrote:
for 3) AccountManager
* updateUser in 'state' : it is necessary during the Full Sync when an
object's information is not same with one in other regions.
* new signatures : they are necessary during the Full Sync not to
generate events while changing them. They are just another methods to manage
objects, so can you show me why they are not permitted?
Alena Prokharchyk wrote:
Alex, correct me if I'm wrong. In most of your code you use CS APIs to
retrieve/update/delete/disable(disable changes the state) the CS objects
(user/domain/account - updateAccount/updateDomain/deleteAccount etc). And you
do it in each reason, and that generates the events for all the regions when
these APIs are processed. Like for example, I see these calls:
public JSONObject updateAccount(String accountId, String currentName,
String newName, String details, String domainId, String networkDomain) throws
APIFailureException {
139
140
StringBuilder param = buildCmd(updateAccount);
141
param.append(append(ApiConstants.ID, accountId));
142
if (currentName != null)
param.append(append(ApiConstants.ACCOUNT, currentName));
143
if (newName != null) param.append(append(ApiConstants.NEW_NAME,
newName));
144
if (details != null)
param.append(append(ApiConstants.ACCOUNT_DETAILS, details));
145
if (domainId != null) param.append(append(ApiConstants.DOMAIN_ID,
domainId));
146
if (networkDomain != null)
param.append(append(ApiConstants.NETWORK_DOMAIN, networkDomain));
147
param.append(appendResType(json));
148
param.append(appendSessionKey(encodeSessionKey()));
149
150
return sendApacheGet(param.toString());
151
}
152
153
public JSONObject disableAccount(String accountId) throws
APIFailureException {
164
165
StringBuilder param = buildCmd(disableAccount);
166
param.append(append(ApiConstants.ID, accountId));
167
param.append(append(ApiConstants.LOCK, false));
168
param.append(appendResType(json));
169
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
On May 5, 2014, 6:27 p.m., Alena Prokharchyk wrote:
Alex, generic logic looks good to me. But still some things need to be
fixed.
1) RegionVO
@Column(name = active)
private boolean active;
Explicitly set it to be active by default. Just setting it in the DB might
not be enough because DAO would always default boolean to false if not set
in the constructor/intance variable declaration. It should look like:
@Column(name = active)
private boolean active = true;
2) Please make the description for the new parameters in the addRegion API
, more descriptive
@Parameter(name = ApiConstants.MODE, type = CommandType.STRING, required =
true, description = Region service active)
55
private String mode;
3) AccountManager
* boolean updateUser(final Long id, String userName, String firstName,
String lastName, String password, String email, String apiKey, String
secretKey, String timeZone, Account.State state);
Why do we pass account state to the call? there should be only user info
passed in. If the account needs to be updated, do it in the separate
command; if you need to retrive the account state - get the account object
from the user object. Please remove this parameter
* Why did you change the method signature for the
UserAccount disableUser(long userId) to disableUser(User user)? The only
one info that you have to pass to the manager - is the userId. Please
change it back.
The same for boolean enableUser(User user), boolean lockAccount(Account
account);
These changes don't seem to be critical or used for/by your plugin, so
please change them back to the original way of doing things. Its not a good
practice to change methods defined in the interfaces -
AccountManager/AccountService - unless there is a real need for it.
* boolean updateAccount(AccountVO account, String newAccountName, String
newNetworkDomain, final MapString, String details, Account.State state);
As updateAccount doesn't allow account state update, please remove
Account.State from the method signature
4) ApiResponseHelper.java
Why did you change createDomainResponse method?
public DomainResponse createDomainResponse(Domain domain)?
To summarize it all, I would suggest to eliminate (revert) all the changes
done to account/domain managers that your code doesn't actually need. It
would make the changes list much shorter and easier for review.
Alex Ough wrote:
for 3) AccountManager
* updateUser in 'state' : it is necessary during the Full Sync when an
object's information is not same with one in other regions.
* new signatures : they are necessary during the Full Sync not to
generate events while changing them. They are just another methods to manage
objects, so can you show me why they are not permitted?
Alena Prokharchyk wrote:
Alex, correct me if I'm wrong. In most of your code you use CS APIs to
retrieve/update/delete/disable(disable changes the state) the CS objects
(user/domain/account - updateAccount/updateDomain/deleteAccount etc). And you
do it in each reason, and that generates the events for all the regions when
these APIs are processed. Like for example, I see these calls:
public JSONObject updateAccount(String accountId, String currentName,
String newName, String details, String domainId, String networkDomain) throws
APIFailureException {
139
140
StringBuilder param = buildCmd(updateAccount);
141
param.append(append(ApiConstants.ID, accountId));
142
if (currentName != null)
param.append(append(ApiConstants.ACCOUNT, currentName));
143
if (newName != null) param.append(append(ApiConstants.NEW_NAME,
newName));
144
if (details != null)
param.append(append(ApiConstants.ACCOUNT_DETAILS, details));
145
if (domainId != null) param.append(append(ApiConstants.DOMAIN_ID,
domainId));
146
if (networkDomain != null)
param.append(append(ApiConstants.NETWORK_DOMAIN, networkDomain));
147
param.append(appendResType(json));
148
param.append(appendSessionKey(encodeSessionKey()));
149
150
return sendApacheGet(param.toString());
151
}
152
153
public JSONObject disableAccount(String accountId) throws
APIFailureException {
164
165
StringBuilder param = buildCmd(disableAccount);
166
param.append(append(ApiConstants.ID, accountId));
167
param.append(append(ApiConstants.LOCK, false));
168
param.append(appendResType(json));
169
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/ --- (Updated May 4, 2014, 9:12 p.m.) Review request for cloudstack. Changes --- core changes Repository: cloudstack-git Description --- This is the review request for the core changes related with #17790 that has only the new plugin codes. Diffs (updated) - api/src/com/cloud/event/EventTypes.java 39ef710 api/src/com/cloud/user/AccountService.java 7e37b38 api/src/org/apache/cloudstack/api/ApiConstants.java fdb4558 api/src/org/apache/cloudstack/api/BaseCmd.java f6f21ae api/src/org/apache/cloudstack/api/ResponseGenerator.java 10fb6df 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/RegionResponse.java 6c74fa6 api/src/org/apache/cloudstack/query/QueryService.java 01af631 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 client/tomcatconf/commands.properties.in 45debe4 engine/schema/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml 489b37d engine/schema/src/org/apache/cloudstack/region/RegionVO.java 608bd2b 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/ApiDBUtils.java 67e47f7 server/src/com/cloud/api/ApiResponseHelper.java 38f2f0b server/src/com/cloud/api/dispatch/ParamProcessWorker.java e9bdd8b server/src/com/cloud/api/query/QueryManagerImpl.java 3abb944 server/src/com/cloud/api/query/ViewResponseHelper.java d06e1d1 server/src/com/cloud/event/ActionEventUtils.java 28e5680 server/src/com/cloud/server/ManagementServerImpl.java bce2930 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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
Alex, please see the answers inline. May be it would be a good idea if we discuss the high level architecture - what DB changes you are going to make, how CS apis would or wouldn¹t get affected - before you do the coding? It would definitely save us time, and won¹t lead to further frustration. I¹m more than willing to help. On 4/28/14, 6:59 AM, Alex Ough alex.o...@sungard.com wrote: On April 17, 2014, 8:10 p.m., Alena Prokharchyk wrote: 1) Alex, I don't quite approve the fact that the responses were modified just to support your feature. User/account of region1 has absolutely no idea of syncing that your plugin is doing as well as he has 0 idea that it exists in multiple databases. I would suggest to keep these parameters in the DB of your component, and reflect all the sync updates there. And in the API responses account should only see actual created/removed/modified parameters reflecting the time when the account was created/modified/removed from the very first DB of your region. Just think about your component as of a plugin running on top of CS (and that plugin can be enabled/disabled at any time), and do the syncing w/o CS code knowing about it. CS just has to return you all the information that originally was set through the CS (w/o the help of your component). All extra work your component does, should be stored in your component DB. You can add a helper table/API where you reflect the sync time between the accounts/domains/users, I'm fine with that. 2) #2 is lined with #1. Please remove all the occurances of _rsyncMgr.update from AccountManagerImpl. As I said, your component should act as a plugin, and you shouldn't inject it to the CS core managers. Make all updates directly from RSyncManager after account/domain/user is created/modified/removed in the AccountManager; code without interfering with AccountManager. 3)RegionResponse.java * Please add more details to the new parameter description, its not clear what it returns now: the active of the region * add since annotation 1) 2) I'm a little frustrated because I think you confirmed that the updates can be inside the Account/Domain managers to guarantee the atomicity of the transaction when we spoke during the conference. Perhaps we misunderstood each other. When you/me/Alex Huang talked about it at the collab, we concluded that all the changes related to synced entries, should be done in Account/domain managers of YOUR component that does account/domain sync, not the CS itself. While the current fix just does the trick of not updating the initial created/updated/modified fields, but by introducing the fields that do duplicated functionality - sync_created/sync_updated/synced_removed - but across the regions. And the reason why the API responses need the synced datetime is because the synchronization needs to compare which changes are later. As I said multiple times before, your component should act as any third party component doing CS DB sync, and run all the comparisons on the layer above Account/Domain layer of the CS. No modifications should be done to AccountVO/DomainVO as well as the CS account/domain managers. The synced data should be stored in the helper VO objects of your components, and the manager of your components should perform the comparison w/o impacting the CS code. When user looks at the API response, he should see no difference whether the CS runs with multi region setup, or not. You can also utilize *DetailsVO tables of the CS (user_details for user, account_details for account, domain_details for domain) to store the Synced time, instead of creating your helper VO object. These tables are meant for storing CS object details, that can be used by third party components/services. Refer to the following FS: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Ability+to+have+bett er+control+over+first+class+objects+in+CS Account/Domain API responses shouldn¹t get affected either. If you utilize resourceDetails tables, the sync time will be returned when call listResourceDetails API for user/account/domain. Or your component can access those tables directly. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/#review40690 --- On April 16, 2014, 7:06 p.m., Alex Ough wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/ --- (Updated April 16, 2014, 7:06 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 39ef710
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
when how do you want to have it? On Mon, Apr 28, 2014 at 1:18 PM, Alena Prokharchyk alena.prokharc...@citrix.com wrote: Alex, please see the answers inline. May be it would be a good idea if we discuss the high level architecture - what DB changes you are going to make, how CS apis would or wouldn¹t get affected - before you do the coding? It would definitely save us time, and won¹t lead to further frustration. I¹m more than willing to help. On 4/28/14, 6:59 AM, Alex Ough alex.o...@sungard.com wrote: On April 17, 2014, 8:10 p.m., Alena Prokharchyk wrote: 1) Alex, I don't quite approve the fact that the responses were modified just to support your feature. User/account of region1 has absolutely no idea of syncing that your plugin is doing as well as he has 0 idea that it exists in multiple databases. I would suggest to keep these parameters in the DB of your component, and reflect all the sync updates there. And in the API responses account should only see actual created/removed/modified parameters reflecting the time when the account was created/modified/removed from the very first DB of your region. Just think about your component as of a plugin running on top of CS (and that plugin can be enabled/disabled at any time), and do the syncing w/o CS code knowing about it. CS just has to return you all the information that originally was set through the CS (w/o the help of your component). All extra work your component does, should be stored in your component DB. You can add a helper table/API where you reflect the sync time between the accounts/domains/users, I'm fine with that. 2) #2 is lined with #1. Please remove all the occurances of _rsyncMgr.update from AccountManagerImpl. As I said, your component should act as a plugin, and you shouldn't inject it to the CS core managers. Make all updates directly from RSyncManager after account/domain/user is created/modified/removed in the AccountManager; code without interfering with AccountManager. 3)RegionResponse.java * Please add more details to the new parameter description, its not clear what it returns now: the active of the region * add since annotation 1) 2) I'm a little frustrated because I think you confirmed that the updates can be inside the Account/Domain managers to guarantee the atomicity of the transaction when we spoke during the conference. Perhaps we misunderstood each other. When you/me/Alex Huang talked about it at the collab, we concluded that all the changes related to synced entries, should be done in Account/domain managers of YOUR component that does account/domain sync, not the CS itself. While the current fix just does the trick of not updating the initial created/updated/modified fields, but by introducing the fields that do duplicated functionality - sync_created/sync_updated/synced_removed - but across the regions. And the reason why the API responses need the synced datetime is because the synchronization needs to compare which changes are later. As I said multiple times before, your component should act as any third party component doing CS DB sync, and run all the comparisons on the layer above Account/Domain layer of the CS. No modifications should be done to AccountVO/DomainVO as well as the CS account/domain managers. The synced data should be stored in the helper VO objects of your components, and the manager of your components should perform the comparison w/o impacting the CS code. When user looks at the API response, he should see no difference whether the CS runs with multi region setup, or not. You can also utilize *DetailsVO tables of the CS (user_details for user, account_details for account, domain_details for domain) to store the Synced time, instead of creating your helper VO object. These tables are meant for storing CS object details, that can be used by third party components/services. Refer to the following FS: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Ability+to+have+bett er+control+over+first+class+objects+in+CS Account/Domain API responses shouldn¹t get affected either. If you utilize resourceDetails tables, the sync time will be returned when call listResourceDetails API for user/account/domain. Or your component can access those tables directly. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/#review40690 --- On April 16, 2014, 7:06 p.m., Alex Ough wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/ --- (Updated April 16, 2014, 7:06 p.m.) Review request for cloudstack. Repository:
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
Alex, that is totally up to you. I’m ready to review/talk any time. -alena. On 4/28/14, 10:35 AM, Alex Ough alex.o...@sungardas.com wrote: when how do you want to have it? On Mon, Apr 28, 2014 at 1:18 PM, Alena Prokharchyk alena.prokharc...@citrix.com wrote: Alex, please see the answers inline. May be it would be a good idea if we discuss the high level architecture - what DB changes you are going to make, how CS apis would or wouldn¹t get affected - before you do the coding? It would definitely save us time, and won¹t lead to further frustration. I¹m more than willing to help. On 4/28/14, 6:59 AM, Alex Ough alex.o...@sungard.com wrote: On April 17, 2014, 8:10 p.m., Alena Prokharchyk wrote: 1) Alex, I don't quite approve the fact that the responses were modified just to support your feature. User/account of region1 has absolutely no idea of syncing that your plugin is doing as well as he has 0 idea that it exists in multiple databases. I would suggest to keep these parameters in the DB of your component, and reflect all the sync updates there. And in the API responses account should only see actual created/removed/modified parameters reflecting the time when the account was created/modified/removed from the very first DB of your region. Just think about your component as of a plugin running on top of CS (and that plugin can be enabled/disabled at any time), and do the syncing w/o CS code knowing about it. CS just has to return you all the information that originally was set through the CS (w/o the help of your component). All extra work your component does, should be stored in your component DB. You can add a helper table/API where you reflect the sync time between the accounts/domains/users, I'm fine with that. 2) #2 is lined with #1. Please remove all the occurances of _rsyncMgr.update from AccountManagerImpl. As I said, your component should act as a plugin, and you shouldn't inject it to the CS core managers. Make all updates directly from RSyncManager after account/domain/user is created/modified/removed in the AccountManager; code without interfering with AccountManager. 3)RegionResponse.java * Please add more details to the new parameter description, its not clear what it returns now: the active of the region * add since annotation 1) 2) I'm a little frustrated because I think you confirmed that the updates can be inside the Account/Domain managers to guarantee the atomicity of the transaction when we spoke during the conference. Perhaps we misunderstood each other. When you/me/Alex Huang talked about it at the collab, we concluded that all the changes related to synced entries, should be done in Account/domain managers of YOUR component that does account/domain sync, not the CS itself. While the current fix just does the trick of not updating the initial created/updated/modified fields, but by introducing the fields that do duplicated functionality - sync_created/sync_updated/synced_removed - but across the regions. And the reason why the API responses need the synced datetime is because the synchronization needs to compare which changes are later. As I said multiple times before, your component should act as any third party component doing CS DB sync, and run all the comparisons on the layer above Account/Domain layer of the CS. No modifications should be done to AccountVO/DomainVO as well as the CS account/domain managers. The synced data should be stored in the helper VO objects of your components, and the manager of your components should perform the comparison w/o impacting the CS code. When user looks at the API response, he should see no difference whether the CS runs with multi region setup, or not. You can also utilize *DetailsVO tables of the CS (user_details for user, account_details for account, domain_details for domain) to store the Synced time, instead of creating your helper VO object. These tables are meant for storing CS object details, that can be used by third party components/services. Refer to the following FS: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Ability+to+have+be tt er+control+over+first+class+objects+in+CS Account/Domain API responses shouldn¹t get affected either. If you utilize resourceDetails tables, the sync time will be returned when call listResourceDetails API for user/account/domain. Or your component can access those tables directly. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/#review40690 --- On April 16, 2014, 7:06 p.m., Alex Ough wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
what do have any web conferencing app? If not, we can use the webex. Let me know what time is convenient for you. On Mon, Apr 28, 2014 at 1:46 PM, Alena Prokharchyk alena.prokharc...@citrix.com wrote: Alex, that is totally up to you. I'm ready to review/talk any time. -alena. On 4/28/14, 10:35 AM, Alex Ough alex.o...@sungardas.com wrote: when how do you want to have it? On Mon, Apr 28, 2014 at 1:18 PM, Alena Prokharchyk alena.prokharc...@citrix.com wrote: Alex, please see the answers inline. May be it would be a good idea if we discuss the high level architecture - what DB changes you are going to make, how CS apis would or wouldn¹t get affected - before you do the coding? It would definitely save us time, and won¹t lead to further frustration. I¹m more than willing to help. On 4/28/14, 6:59 AM, Alex Ough alex.o...@sungard.com wrote: On April 17, 2014, 8:10 p.m., Alena Prokharchyk wrote: 1) Alex, I don't quite approve the fact that the responses were modified just to support your feature. User/account of region1 has absolutely no idea of syncing that your plugin is doing as well as he has 0 idea that it exists in multiple databases. I would suggest to keep these parameters in the DB of your component, and reflect all the sync updates there. And in the API responses account should only see actual created/removed/modified parameters reflecting the time when the account was created/modified/removed from the very first DB of your region. Just think about your component as of a plugin running on top of CS (and that plugin can be enabled/disabled at any time), and do the syncing w/o CS code knowing about it. CS just has to return you all the information that originally was set through the CS (w/o the help of your component). All extra work your component does, should be stored in your component DB. You can add a helper table/API where you reflect the sync time between the accounts/domains/users, I'm fine with that. 2) #2 is lined with #1. Please remove all the occurances of _rsyncMgr.update from AccountManagerImpl. As I said, your component should act as a plugin, and you shouldn't inject it to the CS core managers. Make all updates directly from RSyncManager after account/domain/user is created/modified/removed in the AccountManager; code without interfering with AccountManager. 3)RegionResponse.java * Please add more details to the new parameter description, its not clear what it returns now: the active of the region * add since annotation 1) 2) I'm a little frustrated because I think you confirmed that the updates can be inside the Account/Domain managers to guarantee the atomicity of the transaction when we spoke during the conference. Perhaps we misunderstood each other. When you/me/Alex Huang talked about it at the collab, we concluded that all the changes related to synced entries, should be done in Account/domain managers of YOUR component that does account/domain sync, not the CS itself. While the current fix just does the trick of not updating the initial created/updated/modified fields, but by introducing the fields that do duplicated functionality - sync_created/sync_updated/synced_removed - but across the regions. And the reason why the API responses need the synced datetime is because the synchronization needs to compare which changes are later. As I said multiple times before, your component should act as any third party component doing CS DB sync, and run all the comparisons on the layer above Account/Domain layer of the CS. No modifications should be done to AccountVO/DomainVO as well as the CS account/domain managers. The synced data should be stored in the helper VO objects of your components, and the manager of your components should perform the comparison w/o impacting the CS code. When user looks at the API response, he should see no difference whether the CS runs with multi region setup, or not. You can also utilize *DetailsVO tables of the CS (user_details for user, account_details for account, domain_details for domain) to store the Synced time, instead of creating your helper VO object. These tables are meant for storing CS object details, that can be used by third party components/services. Refer to the following FS: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Ability+to+have+be tt er+control+over+first+class+objects+in+CS Account/Domain API responses shouldn¹t get affected either. If you utilize resourceDetails tables, the sync time will be returned when call listResourceDetails API for user/account/domain. Or your component can access those tables directly. - Alex --- This is an automatically generated e-mail. To reply, visit:
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
We can discuss it today at 3pm, Alex, if the time works for you. WebEx is fine, but we should make sure to record the session + share follow up notes with the community. The best approach would be - to update the FS with the design changes for this particular part of the code. Will be waiting for the webex invitation from you. -Alena. On 4/28/14, 11:23 AM, Alex Ough alex.o...@sungardas.com wrote: what do have any web conferencing app? If not, we can use the webex. Let me know what time is convenient for you. On Mon, Apr 28, 2014 at 1:46 PM, Alena Prokharchyk alena.prokharc...@citrix.com wrote: Alex, that is totally up to you. I'm ready to review/talk any time. -alena. On 4/28/14, 10:35 AM, Alex Ough alex.o...@sungardas.com wrote: when how do you want to have it? On Mon, Apr 28, 2014 at 1:18 PM, Alena Prokharchyk alena.prokharc...@citrix.com wrote: Alex, please see the answers inline. May be it would be a good idea if we discuss the high level architecture - what DB changes you are going to make, how CS apis would or wouldn¹t get affected - before you do the coding? It would definitely save us time, and won¹t lead to further frustration. I¹m more than willing to help. On 4/28/14, 6:59 AM, Alex Ough alex.o...@sungard.com wrote: On April 17, 2014, 8:10 p.m., Alena Prokharchyk wrote: 1) Alex, I don't quite approve the fact that the responses were modified just to support your feature. User/account of region1 has absolutely no idea of syncing that your plugin is doing as well as he has 0 idea that it exists in multiple databases. I would suggest to keep these parameters in the DB of your component, and reflect all the sync updates there. And in the API responses account should only see actual created/removed/modified parameters reflecting the time when the account was created/modified/removed from the very first DB of your region. Just think about your component as of a plugin running on top of CS (and that plugin can be enabled/disabled at any time), and do the syncing w/o CS code knowing about it. CS just has to return you all the information that originally was set through the CS (w/o the help of your component). All extra work your component does, should be stored in your component DB. You can add a helper table/API where you reflect the sync time between the accounts/domains/users, I'm fine with that. 2) #2 is lined with #1. Please remove all the occurances of _rsyncMgr.update from AccountManagerImpl. As I said, your component should act as a plugin, and you shouldn't inject it to the CS core managers. Make all updates directly from RSyncManager after account/domain/user is created/modified/removed in the AccountManager; code without interfering with AccountManager. 3)RegionResponse.java * Please add more details to the new parameter description, its not clear what it returns now: the active of the region * add since annotation 1) 2) I'm a little frustrated because I think you confirmed that the updates can be inside the Account/Domain managers to guarantee the atomicity of the transaction when we spoke during the conference. Perhaps we misunderstood each other. When you/me/Alex Huang talked about it at the collab, we concluded that all the changes related to synced entries, should be done in Account/domain managers of YOUR component that does account/domain sync, not the CS itself. While the current fix just does the trick of not updating the initial created/updated/modified fields, but by introducing the fields that do duplicated functionality - sync_created/sync_updated/synced_removed - but across the regions. And the reason why the API responses need the synced datetime is because the synchronization needs to compare which changes are later. As I said multiple times before, your component should act as any third party component doing CS DB sync, and run all the comparisons on the layer above Account/Domain layer of the CS. No modifications should be done to AccountVO/DomainVO as well as the CS account/domain managers. The synced data should be stored in the helper VO objects of your components, and the manager of your components should perform the comparison w/o impacting the CS code. When user looks at the API response, he should see no difference whether the CS runs with multi region setup, or not. You can also utilize *DetailsVO tables of the CS (user_details for user, account_details for account, domain_details for domain) to store the Synced time, instead of creating your helper VO object. These tables are meant for storing CS object details, that can be used by third party components/services. Refer to the following FS: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Ability+to+have+be tt er+control+over+first+class+objects+in+CS
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
What is your timezone? Can we have it tomorrow morning? On Mon, Apr 28, 2014 at 3:19 PM, Alena Prokharchyk alena.prokharc...@citrix.com wrote: We can discuss it today at 3pm, Alex, if the time works for you. WebEx is fine, but we should make sure to record the session + share follow up notes with the community. The best approach would be - to update the FS with the design changes for this particular part of the code. Will be waiting for the webex invitation from you. -Alena. On 4/28/14, 11:23 AM, Alex Ough alex.o...@sungardas.com wrote: what do have any web conferencing app? If not, we can use the webex. Let me know what time is convenient for you. On Mon, Apr 28, 2014 at 1:46 PM, Alena Prokharchyk alena.prokharc...@citrix.com wrote: Alex, that is totally up to you. I'm ready to review/talk any time. -alena. On 4/28/14, 10:35 AM, Alex Ough alex.o...@sungardas.com wrote: when how do you want to have it? On Mon, Apr 28, 2014 at 1:18 PM, Alena Prokharchyk alena.prokharc...@citrix.com wrote: Alex, please see the answers inline. May be it would be a good idea if we discuss the high level architecture - what DB changes you are going to make, how CS apis would or wouldn¹t get affected - before you do the coding? It would definitely save us time, and won¹t lead to further frustration. I¹m more than willing to help. On 4/28/14, 6:59 AM, Alex Ough alex.o...@sungard.com wrote: On April 17, 2014, 8:10 p.m., Alena Prokharchyk wrote: 1) Alex, I don't quite approve the fact that the responses were modified just to support your feature. User/account of region1 has absolutely no idea of syncing that your plugin is doing as well as he has 0 idea that it exists in multiple databases. I would suggest to keep these parameters in the DB of your component, and reflect all the sync updates there. And in the API responses account should only see actual created/removed/modified parameters reflecting the time when the account was created/modified/removed from the very first DB of your region. Just think about your component as of a plugin running on top of CS (and that plugin can be enabled/disabled at any time), and do the syncing w/o CS code knowing about it. CS just has to return you all the information that originally was set through the CS (w/o the help of your component). All extra work your component does, should be stored in your component DB. You can add a helper table/API where you reflect the sync time between the accounts/domains/users, I'm fine with that. 2) #2 is lined with #1. Please remove all the occurances of _rsyncMgr.update from AccountManagerImpl. As I said, your component should act as a plugin, and you shouldn't inject it to the CS core managers. Make all updates directly from RSyncManager after account/domain/user is created/modified/removed in the AccountManager; code without interfering with AccountManager. 3)RegionResponse.java * Please add more details to the new parameter description, its not clear what it returns now: the active of the region * add since annotation 1) 2) I'm a little frustrated because I think you confirmed that the updates can be inside the Account/Domain managers to guarantee the atomicity of the transaction when we spoke during the conference. Perhaps we misunderstood each other. When you/me/Alex Huang talked about it at the collab, we concluded that all the changes related to synced entries, should be done in Account/domain managers of YOUR component that does account/domain sync, not the CS itself. While the current fix just does the trick of not updating the initial created/updated/modified fields, but by introducing the fields that do duplicated functionality - sync_created/sync_updated/synced_removed - but across the regions. And the reason why the API responses need the synced datetime is because the synchronization needs to compare which changes are later. As I said multiple times before, your component should act as any third party component doing CS DB sync, and run all the comparisons on the layer above Account/Domain layer of the CS. No modifications should be done to AccountVO/DomainVO as well as the CS account/domain managers. The synced data should be stored in the helper VO objects of your components, and the manager of your components should perform the comparison w/o impacting the CS code. When user looks at the API response, he should see no difference whether the CS runs with multi region setup, or not. You can also utilize *DetailsVO tables of the CS (user_details for user, account_details for account, domain_details for domain) to store the Synced time, instead of creating your helper VO object.
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/ --- (Updated April 16, 2014, 7:06 p.m.) Review request for cloudstack. Changes --- This is another update for core. Repository: cloudstack-git Description --- This is the review request for the core changes related with #17790 that has only the new plugin codes. Diffs (updated) - api/src/com/cloud/event/EventTypes.java 39ef710 api/src/com/cloud/user/AccountService.java 7e37b38 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/org/apache/cloudstack/multiregion/RmapVO.java PRE-CREATION engine/schema/src/org/apache/cloudstack/multiregion/RsyncVO.java PRE-CREATION engine/schema/src/org/apache/cloudstack/multiregion/dao/RmapDao.java PRE-CREATION engine/schema/src/org/apache/cloudstack/multiregion/dao/RmapDaoImpl.java PRE-CREATION engine/schema/src/org/apache/cloudstack/multiregion/dao/RsyncDao.java PRE-CREATION engine/schema/src/org/apache/cloudstack/multiregion/dao/RsyncDaoImpl.java PRE-CREATION engine/schema/src/org/apache/cloudstack/region/RegionVO.java 608bd2b 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/event/ActionEventUtils.java 28e5680 server/src/com/cloud/multiregion/RsyncManager.java PRE-CREATION server/src/com/cloud/multiregion/RsyncManagerImpl.java PRE-CREATION 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/networkoffering/ChildTestConfiguration.java 22516c0 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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
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. 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. 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.comwrote: Alena/Alex, I think I need to give some explanation how this works. There are 2 ways of sync. 1
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
#2 : I will, and YES, you need to change the modes of all regions, which also means that you can sync the resources only in the regions you select. #1: Well... it looks like I still need more information why they can't be added to their attributes. They are also managed through the their dao interfaces, and the values are assigned only during the full scan to preserve the original value when they are missed. Thanks Alex Ough On Tue, Apr 8, 2014 at 2:29 PM, Alena Prokharchyk alena.prokharc...@citrix.com wrote: 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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
Alex, Take for example the Removed field. Its not being managed through none of the CS interfaces, and you can’t set this value neither from the API, nor from the Orchestration layer. Its being set automatically in GenericDao once the object is set for removal by the very same Dao. The same applies to the Created/Updated fields. -Alena. On 4/8/14, 11:41 AM, Alex Ough alex.o...@sungardas.com wrote: #2 : I will, and YES, you need to change the modes of all regions, which also means that you can sync the resources only in the regions you select. #1: Well... it looks like I still need more information why they can't be added to their attributes. They are also managed through the their dao interfaces, and the values are assigned only during the full scan to preserve the original value when they are missed. Thanks Alex Ough On Tue, Apr 8, 2014 at 2:29 PM, Alena Prokharchyk alena.prokharc...@citrix.com wrote: 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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
--- 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. Changes --- The is the patch for the core changes. Repository: cloudstack-git Description --- This is the review request for the core changes related with #17790 that has only the new plugin codes. Diffs (updated) - 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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
Alex, one more question. Is this patch meant to be a part of master branch only (4.5)? I guess so as your changes affect 4.4-4.5 db upgrade path. Please confirm, Alena. On 4/7/14, 12:13 PM, Alex Ough alex.o...@sungard.com 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. Changes --- The is the patch for the core changes. Repository: cloudstack-git Description --- This is the review request for the core changes related with #17790 that has only the new plugin codes. Diffs (updated) - 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.jav a 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/netwo rk/contrail/management/MockAccountManager.java 957f708 plugins/pom.xml 9b391b8 server/resources/META-INF/cloudstack/core/spring-server-core-managers-cont ext.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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
yes, that is correct. On Mon, Apr 7, 2014 at 3:27 PM, Alena Prokharchyk alena.prokharc...@citrix.com wrote: Alex, one more question. Is this patch meant to be a part of master branch only (4.5)? I guess so as your changes affect 4.4-4.5 db upgrade path. Please confirm, Alena. On 4/7/14, 12:13 PM, Alex Ough alex.o...@sungard.com 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. Changes --- The is the patch for the core changes. Repository: cloudstack-git Description --- This is the review request for the core changes related with #17790 that has only the new plugin codes. Diffs (updated) - 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.jav a 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/netwo rk/contrail/management/MockAccountManager.java 957f708 plugins/pom.xml 9b391b8 server/resources/META-INF/cloudstack/core/spring-server-core-managers-cont ext.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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
--- 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
Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
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
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
