> -----Original Message----- > From: John Burwell [mailto:nore...@reviews.apache.org] On Behalf Of John > Burwell > Sent: Monday, November 26, 2012 2:54 PM > To: cloudstack; Rohit Yadav; John Burwell > Subject: Re: Review Request: S3-backed Secondary Storage > > > > > On Nov. 21, 2012, 4:32 a.m., Rohit Yadav wrote: > > > Read the coding conventions and checkout the diff so you'll know. > > > > > > Watch out for the red lines and extra indents. > > > For example, fix for following files with tabwidth=4 with spaces, no > > > tabs; fix trailing spaces; > > > > > > api/src/com/cloud/agent/api/BackupSnapshotCommand.java > > > api/src/com/cloud/agent/api/DeleteSnapshotBackupCommand.java > > > api/src/com/cloud/agent/api/DeleteTemplateFromS3Command.java > > > api/src/com/cloud/agent/api/DownloadSnapshotFromS3Command.java > > > api/src/com/cloud/agent/api/to/S3TO.java > > > core/src/com/cloud/storage/S3VO.java > > > core/src/com/cloud/storage/SnapshotVO.java > > > core/src/com/cloud/storage/VMTemplateS3VO.java > > > > core/src/com/cloud/storage/resource/NfsSecondaryStorageResource.java > > > (lots of tab excess indents; 1tab>4spaces, trailing spaces) > > > server/src/com/cloud/api/ApiDBUtils.java > > > server/src/com/cloud/api/doc/ApiXmlDocWriter.java > > > server/src/com/cloud/configuration/ConfigurationManagerImpl.java > > > server/src/com/cloud/storage/StorageManagerImpl.java > > > server/src/com/cloud/storage/dao/VMTemplateDaoImpl.java > > > server/src/com/cloud/storage/dao/VMTemplateS3DaoImpl.java > > > server/src/com/cloud/storage/s3/S3Manager.java > > > server/src/com/cloud/storage/s3/S3ManagerImpl.java > > > server/src/com/cloud/storage/snapshot/SnapshotManager.java > > > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java > > > server/src/com/cloud/template/S3SyncTask.java > > > server/src/com/cloud/template/TemplateManagerImpl.java > > > utils/src/com/cloud/utils/S3Utils.java > > > utils/src/com/cloud/utils/StringUtils.java > > > > > > Not sure about js, though they look alright: > > > ui/scripts/system.js > > > > > > For python code, it should be pep8 compliant (for example all under > > > 80 char width etc.), as per coding conventions; to check that I uses > > > pep8; (for ex. tools/cli/codemonkey/*.py is pep8 compliant) pip > > > install pep8 > > > pep8 <filename> will tell you what to fix; for example: > > > pep8 scripts/vm/hypervisor/xenserver/s3xen.py > > > > > > Try to fix it until you get no error/output from pep8. > > > Unfortunately, we don't have anything like pep8 for Java. > > > > > > Code reviews: > > > > > > For tools/marvin/marvin/cloudstackConnection.py: > > > Why was the function moved: make_request_with_auth? Port 8096 is > the > > > default integration port, that is added to configuration when we do > deploydb with mvn, If we replace this; > > > if port == 8096: > > > by; > > > if self.apiKey == None and self.securityKey == None: > > > > > > If still fails, as I can feed incorrect api and secret keys, and if port > > > is 8096, I > would still want non auth api calls? May be we can do this; > > > if port == 8096 or (self.apiKey == None and self.securityKey == > None): > > > Lastly, why remove the logging code, we need that to see the response, > to see what was returned. > > > > > > Great work, lastly one suggestion on submitting patches: > > > Since, this is a big patch you can split it and submit multiple patches. > > > This > would make it easier to review, commit and revert if necessary. > > > And, use git patch mode, using git add -p. This would ask you what parts > of the changes, even withing a file to add or stage for commit. > > > To fix the current patch (there may be other better ways), you can > > > do this; assuming on your branch it was one patch and is on HEAD git > > > format-patch -o patches HEAD~1 (backup your patch) git rebase -i > > > HEAD~2; NOTE: this deletes the current commit from vim that deletes > > > the commit from history, make sure you've a backup patch > > > > > > git apply <patch file> (this won't commit, but stage the changes) > > > git add -p (add what is needed for a commit, say group changes by > > > java/python/sql/js code or by server, api etc.) git commit -s repeat > > > git add -p say you generated 10 patches; git format-patch -o patches > > > HEAD~10 (email them on ML, review board fails for multiple patches, > > > there is one way though by adding them as parent-child one after the > > > other) > > > > > > > > I have pushed a new version of the patch that corrects the tab and > whitespace issues, as well as, PEP-8 compliance for the s3xen plugin. > > Unfortunately, this patch can't reasonably be split into smaller pieces. The > S3 > functionality, like Swift, cuts a wide path through the code base. The > enhancements added to utility classes (e.g. StringUtils, GlobalLock, etc) are
I want to add s3 as an example how to integrate storage into cloudstack storage subsystem I proposed in (https://cwiki.apache.org/confluence/display/CLOUDSTACK/Storage+subsystem+2.0), so hopefully, we don't need to change so many code, all over the place, to add a new storage. > directly used by the S3 implementation. There are roughly 10-20 lines of > code (e.g. 1 line in cloudstackConnection.py and 3 lines in ApiXmlDocWriter) > across the entire patch that are independent. However, given the size of the > work and the prospect of future submissions, it seems counter-productive to > attempt to split them out from a 250k patch. > > I made the change to the unauthenticated behavior of > cloudstackConnection.py precisely because Marvin does not connect to the > management server via 8096 in our integration environment. I modified the > change to match your recommendations, and narrowed the scope of the > change to a single line in most recent revision of the patch, > > > - John > > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8123/#review13659 > ----------------------------------------------------------- > > > On Nov. 26, 2012, 9:44 p.m., John Burwell wrote: > > > > ----------------------------------------------------------- > > This is an automatically generated e-mail. To reply, visit: > > https://reviews.apache.org/r/8123/ > > ----------------------------------------------------------- > > > > (Updated Nov. 26, 2012, 9:44 p.m.) > > > > > > Review request for cloudstack. > > > > > > Description > > ------- > > > > Backs NFS-based secondary storage with an S3-compatible object store. > Periodically, a reaper thread synchronizes templates and ISOs stored on a > NFS secondary storage mount with a configured S3 object store. It also > pushes snapshots to the object store when they are created and downloads > them in other zones on-demand. In addition to permitting the use of > commodity or IaaS storage solutions for static assets, it provides a means of > automatically synchronizing template and ISO assets across multiple zones. > > > > For more information about the design of the patch, please see the design > document (https://cwiki.apache.org/confluence/display/CLOUDSTACK/S3- > backed+Secondary+Storage). > > > > > > This addresses bug CLOUDSTACK-509. > > > > > > Diffs > > ----- > > > > .gitignore 33f95c7 > > api/src/com/cloud/agent/api/BackupSnapshotCommand.java 9476d7d > > api/src/com/cloud/agent/api/DeleteSnapshotBackupCommand.java > 3fa8c2b > > api/src/com/cloud/agent/api/DeleteTemplateFromS3Command.java PRE- > CREATION > > api/src/com/cloud/agent/api/DownloadSnapshotFromS3Command.java > PRE-CREATION > > > api/src/com/cloud/agent/api/DownloadTemplateFromS3ToSecondaryStorag > eCommand.java PRE-CREATION > > > api/src/com/cloud/agent/api/UploadTemplateToS3FromSecondaryStorageC > ommand.java PRE-CREATION > > api/src/com/cloud/agent/api/to/S3TO.java PRE-CREATION > > api/src/com/cloud/api/ApiConstants.java 78a3ded > > api/src/com/cloud/api/ResponseGenerator.java 4e8fbd8 > > api/src/com/cloud/api/commands/AddS3Cmd.java PRE-CREATION > > api/src/com/cloud/api/commands/ListS3sCmd.java PRE-CREATION > > api/src/com/cloud/api/response/S3Response.java PRE-CREATION > > api/src/com/cloud/resource/ResourceService.java 1065453 > > api/src/com/cloud/storage/S3.java PRE-CREATION > > build/package.xml 09ed939 > > client/WEB-INF/classes/resources/messages.properties 626e44a > > client/tomcatconf/commands.properties.in 149547e > > core/pom.xml 15f0f7b > > core/src/com/cloud/storage/S3VO.java PRE-CREATION > > core/src/com/cloud/storage/SnapshotVO.java 08dfafa > > core/src/com/cloud/storage/VMTemplateS3VO.java PRE-CREATION > > > core/src/com/cloud/storage/resource/NfsSecondaryStorageResource.java > 155210d > > patches/systemvm/debian/config/etc/sysctl.conf 7f945b0 > > > plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixReso > urceBase.java d2db85c > > pom.xml 4a4276e > > scripts/vm/hypervisor/xenserver/s3xen PRE-CREATION > > scripts/vm/hypervisor/xenserver/xenserver56/patch d485414 > > scripts/vm/hypervisor/xenserver/xenserver56fp1/patch 9fe9740 > > scripts/vm/hypervisor/xenserver/xenserver60/patch f049109 > > server/pom.xml e3308d8 > > server/src/com/cloud/api/ApiDBUtils.java 3b5f634 > > server/src/com/cloud/api/ApiDispatcher.java dfe4a1f > > server/src/com/cloud/api/ApiResponseHelper.java ebe8415 > > server/src/com/cloud/api/doc/ApiXmlDocWriter.java d31ef5a > > server/src/com/cloud/configuration/Config.java 66ac276 > > server/src/com/cloud/configuration/ConfigurationManagerImpl.java > ef940e8 > > server/src/com/cloud/configuration/DefaultComponentLibrary.java > ef61044 > > server/src/com/cloud/resource/ResourceManagerImpl.java ced601b > > server/src/com/cloud/storage/StorageManagerImpl.java e252633 > > server/src/com/cloud/storage/dao/S3Dao.java PRE-CREATION > > server/src/com/cloud/storage/dao/S3DaoImpl.java PRE-CREATION > > server/src/com/cloud/storage/dao/VMTemplateDao.java f5b6913 > > server/src/com/cloud/storage/dao/VMTemplateDaoImpl.java 2a0dfc8 > > server/src/com/cloud/storage/dao/VMTemplateS3Dao.java PRE- > CREATION > > server/src/com/cloud/storage/dao/VMTemplateS3DaoImpl.java PRE- > CREATION > > server/src/com/cloud/storage/s3/S3Manager.java PRE-CREATION > > server/src/com/cloud/storage/s3/S3ManagerImpl.java PRE-CREATION > > server/src/com/cloud/storage/snapshot/SnapshotManager.java a10298e > > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java > 32e37e6 > > server/src/com/cloud/template/S3SyncTask.java PRE-CREATION > > server/src/com/cloud/template/TemplateManagerImpl.java 1e87de2 > > setup/db/create-schema.sql fff084e > > setup/db/db/schema-40to410.sql PRE-CREATION > > tools/apidoc/gen_toc.py eeaf2a2 > > tools/marvin/marvin/cloudstackConnection.py c805213 > > tools/marvin/marvin/deployDataCenter.py bdf08cc > > ui/dictionary.jsp b80e296 > > ui/scripts/cloudStack.js de3bd73 > > ui/scripts/sharedFunctions.js b6b3ef8 > > ui/scripts/system.js 9e3932f > > ui/scripts/templates.js 74511f0 > > utils/pom.xml 6bed67f > > utils/src/com/cloud/utils/DateUtil.java be1627d > > utils/src/com/cloud/utils/S3Utils.java PRE-CREATION > > utils/src/com/cloud/utils/StringUtils.java 0f0ef05 > > utils/src/com/cloud/utils/db/GlobalLock.java 7c1c943 > > > > Diff: https://reviews.apache.org/r/8123/diff/ > > > > > > Testing > > ------- > > > > I am submitting patch to begin the feedback process while we complete > integration testing. I have verified that it does not interfere with normal > CloudStack operations when S3-backed Secondary Storage is disabled (the > default setting) . I have successfully tested operation of single zone > template and ISO scenarios on devcloud described in the design document. I > am currently working through some issues in our multi-zone test > environment to complete all scenarios described. The following are the > known deficiencies of the current implementation which I plan to correct in a > subsequent patch: > > > > * Cross zone garbage collection: When a global asset is deleted from one > zone's secondary storage, it is not deleted from the secondary storage of > other zones which have downloaded it from the object store > > * S3 Configuration Update: The API only supports adding an object store > configuration. Users should be able to edit the access key, secret key, > connection timeout. max error retries, and socket timeout. > > * Multi-threaded Uploads: Permit the upload of multiple assets to the > object store simultaneously to decrease the propagation latency across all > zones. > > > > > > Screenshots > > ----------- > > > > S3 Configuration Form > > https://reviews.apache.org/r/8123/s/13/ > > S3 Enable Menu on the Zone Tab > > https://reviews.apache.org/r/8123/s/14/ > > > > > > Thanks, > > > > John Burwell > > > >
