incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Rohit Yadav" <>
Subject Re: Review Request: S3-backed Secondary Storage
Date Wed, 21 Nov 2012 04:32:26 GMT

This is an automatically generated e-mail. To reply, visit:

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;

core/src/com/cloud/storage/resource/ (lots of tab excess indents;
1tab>4spaces, trailing spaces)

Not sure about js, though they look alright:

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/

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/
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:
        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)

- Rohit Yadav

On Nov. 19, 2012, 5:16 p.m., John Burwell wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> -----------------------------------------------------------
> (Updated Nov. 19, 2012, 5:16 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 (
> This addresses bug CLOUDSTACK-509.
> Diffs
> -----
>   .gitignore 4661ea2 
>   api/src/com/cloud/agent/api/ 9476d7d 
>   api/src/com/cloud/agent/api/ 3fa8c2b 
>   api/src/com/cloud/agent/api/ PRE-CREATION 
>   api/src/com/cloud/agent/api/ PRE-CREATION 
>   api/src/com/cloud/agent/api/ PRE-CREATION

>   api/src/com/cloud/agent/api/ PRE-CREATION

>   api/src/com/cloud/agent/api/to/ PRE-CREATION 
>   api/src/com/cloud/api/ 78a3ded 
>   api/src/com/cloud/api/ 4e8fbd8 
>   api/src/com/cloud/api/commands/ PRE-CREATION 
>   api/src/com/cloud/api/commands/ PRE-CREATION 
>   api/src/com/cloud/api/response/ PRE-CREATION 
>   api/src/com/cloud/resource/ 1065453 
>   api/src/com/cloud/storage/ PRE-CREATION 
>   build/package.xml 09ed939 
>   client/WEB-INF/classes/resources/ 626e44a 
>   client/tomcatconf/ 149547e 
>   console-proxy/scripts/ e408378 
>   core/pom.xml 15f0f7b 
>   core/src/com/cloud/storage/ PRE-CREATION 
>   core/src/com/cloud/storage/ 08dfafa 
>   core/src/com/cloud/storage/ PRE-CREATION 
>   core/src/com/cloud/storage/resource/ 155210d 
>   patches/systemvm/debian/config/etc/sysctl.conf 7f945b0 
>   plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/
>   pom.xml 08f61f5 
>   scripts/vm/hypervisor/xenserver/ 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 cd4fbbe 
>   server/src/com/cloud/api/ 3b5f634 
>   server/src/com/cloud/api/ dfe4a1f 
>   server/src/com/cloud/api/ ebe8415 
>   server/src/com/cloud/api/doc/ d31ef5a 
>   server/src/com/cloud/configuration/ 66ac276 
>   server/src/com/cloud/configuration/ ef940e8 
>   server/src/com/cloud/configuration/ ef61044 
>   server/src/com/cloud/resource/ ced601b 
>   server/src/com/cloud/servlet/ 2638c8b 
>   server/src/com/cloud/storage/ e252633 
>   server/src/com/cloud/storage/dao/ PRE-CREATION 
>   server/src/com/cloud/storage/dao/ PRE-CREATION 
>   server/src/com/cloud/storage/dao/ f5b6913 
>   server/src/com/cloud/storage/dao/ 2a0dfc8 
>   server/src/com/cloud/storage/dao/ PRE-CREATION 
>   server/src/com/cloud/storage/dao/ PRE-CREATION 
>   server/src/com/cloud/storage/s3/ PRE-CREATION 
>   server/src/com/cloud/storage/s3/ PRE-CREATION 
>   server/src/com/cloud/storage/snapshot/ a10298e 
>   server/src/com/cloud/storage/snapshot/ 32e37e6 
>   server/src/com/cloud/template/ PRE-CREATION 
>   server/src/com/cloud/template/ 1e87de2 
>   setup/db/create-schema.sql fff084e 
>   setup/db/db/schema-40to410.sql PRE-CREATION 
>   tools/apidoc/ eeaf2a2 
>   tools/marvin/marvin/ bd8a5b2 
>   tools/marvin/marvin/ 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 2e4e74f 
>   utils/src/com/cloud/utils/ be1627d 
>   utils/src/com/cloud/utils/ PRE-CREATION 
>   utils/src/com/cloud/utils/ 0f0ef05 
>   utils/src/com/cloud/utils/db/ 7c1c943 
> 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
> S3 Enable Menu on the Zone Tab
> Thanks,
> John Burwell

  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message