You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Donal Lafferty <do...@citrix.com> on 2013/08/28 02:38:45 UTC
Review Request 13877: CIFS Secondary Storage
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13877/
-----------------------------------------------------------
Review request for cloudstack, Chiradeep Vittal and Devdeep Singh.
Repository: cloudstack-git
Description
-------
CIFS support for secondary storage is documented here: https://cwiki.apache.org/confluence/display/CLOUDSTACK/CIFS+Support
The feature was implemented by extending the NFS provider. Its validation was updated so that you can pass it a URL containing the details of a CIFS share. The code that mounts NFS shares was extended to allow it do the same for CIFS shares. Otherwise, the secondary storage code is left unchanged.
The source is in the cifs_support branch of https://github.com/lafferty/cloudstack
NB: not able to submit using git format-patch due to issue described here: http://stackoverflow.com/questions/14594051/upload-from-git-format-patch-to-reviewboard-fails-on-file-not-found-in-the-repo
Diffs
-----
engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java d5e8a84516f4f37f23610a3d0315aa0a84991d46
engine/storage/src/org/apache/cloudstack/storage/datastore/protocol/DataStoreProtocol.java b27c96edbb72751c72c3963d6945eaeaeb42c74b
plugins/storage/image/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackImageStoreLifeCycleImpl.java 21a5e0a7194449d8b72f4537e14586c2b65b0bb5
plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java b9b74243eddd2008227f8ea2b88832e6e276811d
server/src/com/cloud/resource/ResourceManagerImpl.java 44ee8e3bd51be38acd6a90fd2fdd2383dad0fd15
server/src/com/cloud/storage/StorageManagerImpl.java b5d6ff953ef5d099fbe4dc56866e5948a46f1c8a
server/src/com/cloud/test/DatabaseConfig.java 63f77b66520d5f27736d62a67eeb16af808c7dd5
services/secondary-storage/conf/agent.properties aab82b6337474b52ae26c472b321f2c93ac57e81
services/secondary-storage/conf/log4j.xml PRE-CREATION
services/secondary-storage/pom.xml ea4bfcaf477af6be6a01113ac99ef262ae789071
services/secondary-storage/src/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResource.java c55c2361bd5551eb51ca1b14340ab4c2e1563865
services/secondary-storage/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java 8b9de39b70981b7fb4458b5a337b1b577c45aa3b
services/secondary-storage/src/org/apache/cloudstack/storage/resource/SecondaryStorageDiscoverer.java 5d6d61f740ac69acc228b0712633190b654b8f27
services/secondary-storage/src/org/apache/cloudstack/storage/template/DownloadManagerImpl.java bf68b299c5c233f420ed1f78667f0f9522cf64de
services/secondary-storage/test/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResourceTest.java 0c355ec884d50c530d2c77bec85e14550cc57352
services/secondary-storage/test/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResourceTest.java PRE-CREATION
utils/src/com/cloud/utils/UriUtils.java 1ff4d729cbf50154688bcea79bb8f48d1a972674
Diff: https://reviews.apache.org/r/13877/diff/
Testing
-------
Testing involved running the CIFS-specific and existing NFS tests for the secondary-storage service. E.g.
cd ./services/secondary-storage
mvn clean install -P developer,systemvm, -DskipTests=false
Notice that the pom.xml was updated to allow tests to be turned on using a conditional. I ran the tests on the Citrix network, which allowed the existing NFS tests to run without modification.
In addition, an integration test was done exercise the code in the SystemVM environment and to exercise updated validation code. The NFS secondary storage integration test was done using a XenServer deployment in which a template was registered and a VM created from the template. The CIFS secondary storage test was done on a Hyper-V hypervisor a template was registered.
Thanks,
Donal Lafferty
Re: Review Request 13877: CIFS Secondary Storage
Posted by Devdeep Singh <de...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13877/#review26038
-----------------------------------------------------------
Ship it!
pushed in commit e958f22ff34e089b4214ee5be68688540db5291a
- Devdeep Singh
On Aug. 28, 2013, 8:58 p.m., Donal Lafferty wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13877/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2013, 8:58 p.m.)
>
>
> Review request for cloudstack, Chiradeep Vittal, Devdeep Singh, and edison su.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> CIFS support for secondary storage is documented here: https://cwiki.apache.org/confluence/display/CLOUDSTACK/CIFS+Support
>
> The feature was implemented by extending the NFS provider. Its validation was updated so that you can pass it a URL containing the details of a CIFS share. The code that mounts NFS shares was extended to allow it do the same for CIFS shares. Otherwise, the secondary storage code is left unchanged.
>
> The source is in the cifs_support branch of https://github.com/lafferty/cloudstack
>
> NB: not able to submit using git format-patch due to issue described here: http://stackoverflow.com/questions/14594051/upload-from-git-format-patch-to-reviewboard-fails-on-file-not-found-in-the-repo
>
>
> Diffs
> -----
>
> engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java d5e8a84516f4f37f23610a3d0315aa0a84991d46
> engine/storage/src/org/apache/cloudstack/storage/datastore/protocol/DataStoreProtocol.java b27c96edbb72751c72c3963d6945eaeaeb42c74b
> plugins/storage/image/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackImageStoreLifeCycleImpl.java 21a5e0a7194449d8b72f4537e14586c2b65b0bb5
> plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java b9b74243eddd2008227f8ea2b88832e6e276811d
> server/src/com/cloud/resource/ResourceManagerImpl.java 44ee8e3bd51be38acd6a90fd2fdd2383dad0fd15
> server/src/com/cloud/storage/StorageManagerImpl.java b5d6ff953ef5d099fbe4dc56866e5948a46f1c8a
> server/src/com/cloud/test/DatabaseConfig.java 63f77b66520d5f27736d62a67eeb16af808c7dd5
> services/secondary-storage/conf/agent.properties aab82b6337474b52ae26c472b321f2c93ac57e81
> services/secondary-storage/conf/log4j.xml PRE-CREATION
> services/secondary-storage/pom.xml ea4bfcaf477af6be6a01113ac99ef262ae789071
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResource.java c55c2361bd5551eb51ca1b14340ab4c2e1563865
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java 8b9de39b70981b7fb4458b5a337b1b577c45aa3b
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/SecondaryStorageDiscoverer.java 5d6d61f740ac69acc228b0712633190b654b8f27
> services/secondary-storage/src/org/apache/cloudstack/storage/template/DownloadManagerImpl.java bf68b299c5c233f420ed1f78667f0f9522cf64de
> services/secondary-storage/test/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResourceTest.java 0c355ec884d50c530d2c77bec85e14550cc57352
> services/secondary-storage/test/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResourceTest.java PRE-CREATION
> utils/src/com/cloud/utils/UriUtils.java 1ff4d729cbf50154688bcea79bb8f48d1a972674
>
> Diff: https://reviews.apache.org/r/13877/diff/
>
>
> Testing
> -------
>
> Testing involved running the CIFS-specific and existing NFS tests for the secondary-storage service. E.g.
>
> cd ./services/secondary-storage
> mvn clean install -P developer,systemvm, -DskipTests=false
>
> Notice that the pom.xml was updated to allow tests to be turned on using a conditional. I ran the tests on the Citrix network, which allowed the existing NFS tests to run without modification.
>
> In addition, an integration test was done exercise the code in the SystemVM environment and to exercise updated validation code. The NFS secondary storage integration test was done using a XenServer deployment in which a template was registered and a VM created from the template. The CIFS secondary storage test was done on a Hyper-V hypervisor a template was registered.
>
>
> Thanks,
>
> Donal Lafferty
>
>
Re: Review Request 13877: CIFS Secondary Storage
Posted by Rajesh Battala <ra...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13877/#review26034
-----------------------------------------------------------
Ship it!
Ship It!
- Rajesh Battala
On Aug. 28, 2013, 8:58 p.m., Donal Lafferty wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13877/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2013, 8:58 p.m.)
>
>
> Review request for cloudstack, Chiradeep Vittal, Devdeep Singh, and edison su.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> CIFS support for secondary storage is documented here: https://cwiki.apache.org/confluence/display/CLOUDSTACK/CIFS+Support
>
> The feature was implemented by extending the NFS provider. Its validation was updated so that you can pass it a URL containing the details of a CIFS share. The code that mounts NFS shares was extended to allow it do the same for CIFS shares. Otherwise, the secondary storage code is left unchanged.
>
> The source is in the cifs_support branch of https://github.com/lafferty/cloudstack
>
> NB: not able to submit using git format-patch due to issue described here: http://stackoverflow.com/questions/14594051/upload-from-git-format-patch-to-reviewboard-fails-on-file-not-found-in-the-repo
>
>
> Diffs
> -----
>
> engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java d5e8a84516f4f37f23610a3d0315aa0a84991d46
> engine/storage/src/org/apache/cloudstack/storage/datastore/protocol/DataStoreProtocol.java b27c96edbb72751c72c3963d6945eaeaeb42c74b
> plugins/storage/image/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackImageStoreLifeCycleImpl.java 21a5e0a7194449d8b72f4537e14586c2b65b0bb5
> plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java b9b74243eddd2008227f8ea2b88832e6e276811d
> server/src/com/cloud/resource/ResourceManagerImpl.java 44ee8e3bd51be38acd6a90fd2fdd2383dad0fd15
> server/src/com/cloud/storage/StorageManagerImpl.java b5d6ff953ef5d099fbe4dc56866e5948a46f1c8a
> server/src/com/cloud/test/DatabaseConfig.java 63f77b66520d5f27736d62a67eeb16af808c7dd5
> services/secondary-storage/conf/agent.properties aab82b6337474b52ae26c472b321f2c93ac57e81
> services/secondary-storage/conf/log4j.xml PRE-CREATION
> services/secondary-storage/pom.xml ea4bfcaf477af6be6a01113ac99ef262ae789071
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResource.java c55c2361bd5551eb51ca1b14340ab4c2e1563865
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java 8b9de39b70981b7fb4458b5a337b1b577c45aa3b
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/SecondaryStorageDiscoverer.java 5d6d61f740ac69acc228b0712633190b654b8f27
> services/secondary-storage/src/org/apache/cloudstack/storage/template/DownloadManagerImpl.java bf68b299c5c233f420ed1f78667f0f9522cf64de
> services/secondary-storage/test/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResourceTest.java 0c355ec884d50c530d2c77bec85e14550cc57352
> services/secondary-storage/test/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResourceTest.java PRE-CREATION
> utils/src/com/cloud/utils/UriUtils.java 1ff4d729cbf50154688bcea79bb8f48d1a972674
>
> Diff: https://reviews.apache.org/r/13877/diff/
>
>
> Testing
> -------
>
> Testing involved running the CIFS-specific and existing NFS tests for the secondary-storage service. E.g.
>
> cd ./services/secondary-storage
> mvn clean install -P developer,systemvm, -DskipTests=false
>
> Notice that the pom.xml was updated to allow tests to be turned on using a conditional. I ran the tests on the Citrix network, which allowed the existing NFS tests to run without modification.
>
> In addition, an integration test was done exercise the code in the SystemVM environment and to exercise updated validation code. The NFS secondary storage integration test was done using a XenServer deployment in which a template was registered and a VM created from the template. The CIFS secondary storage test was done on a Hyper-V hypervisor a template was registered.
>
>
> Thanks,
>
> Donal Lafferty
>
>
Re: Review Request 13877: CIFS Secondary Storage
Posted by Devdeep Singh <de...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13877/#review26036
-----------------------------------------------------------
Can you resubmit the patch? I am not able to apply the patch on the master branch.
- Devdeep Singh
On Aug. 28, 2013, 8:58 p.m., Donal Lafferty wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13877/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2013, 8:58 p.m.)
>
>
> Review request for cloudstack, Chiradeep Vittal, Devdeep Singh, and edison su.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> CIFS support for secondary storage is documented here: https://cwiki.apache.org/confluence/display/CLOUDSTACK/CIFS+Support
>
> The feature was implemented by extending the NFS provider. Its validation was updated so that you can pass it a URL containing the details of a CIFS share. The code that mounts NFS shares was extended to allow it do the same for CIFS shares. Otherwise, the secondary storage code is left unchanged.
>
> The source is in the cifs_support branch of https://github.com/lafferty/cloudstack
>
> NB: not able to submit using git format-patch due to issue described here: http://stackoverflow.com/questions/14594051/upload-from-git-format-patch-to-reviewboard-fails-on-file-not-found-in-the-repo
>
>
> Diffs
> -----
>
> engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java d5e8a84516f4f37f23610a3d0315aa0a84991d46
> engine/storage/src/org/apache/cloudstack/storage/datastore/protocol/DataStoreProtocol.java b27c96edbb72751c72c3963d6945eaeaeb42c74b
> plugins/storage/image/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackImageStoreLifeCycleImpl.java 21a5e0a7194449d8b72f4537e14586c2b65b0bb5
> plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java b9b74243eddd2008227f8ea2b88832e6e276811d
> server/src/com/cloud/resource/ResourceManagerImpl.java 44ee8e3bd51be38acd6a90fd2fdd2383dad0fd15
> server/src/com/cloud/storage/StorageManagerImpl.java b5d6ff953ef5d099fbe4dc56866e5948a46f1c8a
> server/src/com/cloud/test/DatabaseConfig.java 63f77b66520d5f27736d62a67eeb16af808c7dd5
> services/secondary-storage/conf/agent.properties aab82b6337474b52ae26c472b321f2c93ac57e81
> services/secondary-storage/conf/log4j.xml PRE-CREATION
> services/secondary-storage/pom.xml ea4bfcaf477af6be6a01113ac99ef262ae789071
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResource.java c55c2361bd5551eb51ca1b14340ab4c2e1563865
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java 8b9de39b70981b7fb4458b5a337b1b577c45aa3b
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/SecondaryStorageDiscoverer.java 5d6d61f740ac69acc228b0712633190b654b8f27
> services/secondary-storage/src/org/apache/cloudstack/storage/template/DownloadManagerImpl.java bf68b299c5c233f420ed1f78667f0f9522cf64de
> services/secondary-storage/test/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResourceTest.java 0c355ec884d50c530d2c77bec85e14550cc57352
> services/secondary-storage/test/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResourceTest.java PRE-CREATION
> utils/src/com/cloud/utils/UriUtils.java 1ff4d729cbf50154688bcea79bb8f48d1a972674
>
> Diff: https://reviews.apache.org/r/13877/diff/
>
>
> Testing
> -------
>
> Testing involved running the CIFS-specific and existing NFS tests for the secondary-storage service. E.g.
>
> cd ./services/secondary-storage
> mvn clean install -P developer,systemvm, -DskipTests=false
>
> Notice that the pom.xml was updated to allow tests to be turned on using a conditional. I ran the tests on the Citrix network, which allowed the existing NFS tests to run without modification.
>
> In addition, an integration test was done exercise the code in the SystemVM environment and to exercise updated validation code. The NFS secondary storage integration test was done using a XenServer deployment in which a template was registered and a VM created from the template. The CIFS secondary storage test was done on a Hyper-V hypervisor a template was registered.
>
>
> Thanks,
>
> Donal Lafferty
>
>
Re: Review Request 13877: CIFS Secondary Storage
Posted by Rajesh Battala <ra...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13877/#review26035
-----------------------------------------------------------
Ship it!
Looks good for me.
- Rajesh Battala
On Aug. 28, 2013, 8:58 p.m., Donal Lafferty wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13877/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2013, 8:58 p.m.)
>
>
> Review request for cloudstack, Chiradeep Vittal, Devdeep Singh, and edison su.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> CIFS support for secondary storage is documented here: https://cwiki.apache.org/confluence/display/CLOUDSTACK/CIFS+Support
>
> The feature was implemented by extending the NFS provider. Its validation was updated so that you can pass it a URL containing the details of a CIFS share. The code that mounts NFS shares was extended to allow it do the same for CIFS shares. Otherwise, the secondary storage code is left unchanged.
>
> The source is in the cifs_support branch of https://github.com/lafferty/cloudstack
>
> NB: not able to submit using git format-patch due to issue described here: http://stackoverflow.com/questions/14594051/upload-from-git-format-patch-to-reviewboard-fails-on-file-not-found-in-the-repo
>
>
> Diffs
> -----
>
> engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java d5e8a84516f4f37f23610a3d0315aa0a84991d46
> engine/storage/src/org/apache/cloudstack/storage/datastore/protocol/DataStoreProtocol.java b27c96edbb72751c72c3963d6945eaeaeb42c74b
> plugins/storage/image/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackImageStoreLifeCycleImpl.java 21a5e0a7194449d8b72f4537e14586c2b65b0bb5
> plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java b9b74243eddd2008227f8ea2b88832e6e276811d
> server/src/com/cloud/resource/ResourceManagerImpl.java 44ee8e3bd51be38acd6a90fd2fdd2383dad0fd15
> server/src/com/cloud/storage/StorageManagerImpl.java b5d6ff953ef5d099fbe4dc56866e5948a46f1c8a
> server/src/com/cloud/test/DatabaseConfig.java 63f77b66520d5f27736d62a67eeb16af808c7dd5
> services/secondary-storage/conf/agent.properties aab82b6337474b52ae26c472b321f2c93ac57e81
> services/secondary-storage/conf/log4j.xml PRE-CREATION
> services/secondary-storage/pom.xml ea4bfcaf477af6be6a01113ac99ef262ae789071
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResource.java c55c2361bd5551eb51ca1b14340ab4c2e1563865
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java 8b9de39b70981b7fb4458b5a337b1b577c45aa3b
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/SecondaryStorageDiscoverer.java 5d6d61f740ac69acc228b0712633190b654b8f27
> services/secondary-storage/src/org/apache/cloudstack/storage/template/DownloadManagerImpl.java bf68b299c5c233f420ed1f78667f0f9522cf64de
> services/secondary-storage/test/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResourceTest.java 0c355ec884d50c530d2c77bec85e14550cc57352
> services/secondary-storage/test/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResourceTest.java PRE-CREATION
> utils/src/com/cloud/utils/UriUtils.java 1ff4d729cbf50154688bcea79bb8f48d1a972674
>
> Diff: https://reviews.apache.org/r/13877/diff/
>
>
> Testing
> -------
>
> Testing involved running the CIFS-specific and existing NFS tests for the secondary-storage service. E.g.
>
> cd ./services/secondary-storage
> mvn clean install -P developer,systemvm, -DskipTests=false
>
> Notice that the pom.xml was updated to allow tests to be turned on using a conditional. I ran the tests on the Citrix network, which allowed the existing NFS tests to run without modification.
>
> In addition, an integration test was done exercise the code in the SystemVM environment and to exercise updated validation code. The NFS secondary storage integration test was done using a XenServer deployment in which a template was registered and a VM created from the template. The CIFS secondary storage test was done on a Hyper-V hypervisor a template was registered.
>
>
> Thanks,
>
> Donal Lafferty
>
>
Re: Review Request 13877: CIFS Secondary Storage
Posted by edison su <ed...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13877/#review25940
-----------------------------------------------------------
Ok for me.
- edison su
On Aug. 28, 2013, 8:58 p.m., Donal Lafferty wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13877/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2013, 8:58 p.m.)
>
>
> Review request for cloudstack, Chiradeep Vittal, Devdeep Singh, and edison su.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> CIFS support for secondary storage is documented here: https://cwiki.apache.org/confluence/display/CLOUDSTACK/CIFS+Support
>
> The feature was implemented by extending the NFS provider. Its validation was updated so that you can pass it a URL containing the details of a CIFS share. The code that mounts NFS shares was extended to allow it do the same for CIFS shares. Otherwise, the secondary storage code is left unchanged.
>
> The source is in the cifs_support branch of https://github.com/lafferty/cloudstack
>
> NB: not able to submit using git format-patch due to issue described here: http://stackoverflow.com/questions/14594051/upload-from-git-format-patch-to-reviewboard-fails-on-file-not-found-in-the-repo
>
>
> Diffs
> -----
>
> engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java d5e8a84516f4f37f23610a3d0315aa0a84991d46
> engine/storage/src/org/apache/cloudstack/storage/datastore/protocol/DataStoreProtocol.java b27c96edbb72751c72c3963d6945eaeaeb42c74b
> plugins/storage/image/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackImageStoreLifeCycleImpl.java 21a5e0a7194449d8b72f4537e14586c2b65b0bb5
> plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java b9b74243eddd2008227f8ea2b88832e6e276811d
> server/src/com/cloud/resource/ResourceManagerImpl.java 44ee8e3bd51be38acd6a90fd2fdd2383dad0fd15
> server/src/com/cloud/storage/StorageManagerImpl.java b5d6ff953ef5d099fbe4dc56866e5948a46f1c8a
> server/src/com/cloud/test/DatabaseConfig.java 63f77b66520d5f27736d62a67eeb16af808c7dd5
> services/secondary-storage/conf/agent.properties aab82b6337474b52ae26c472b321f2c93ac57e81
> services/secondary-storage/conf/log4j.xml PRE-CREATION
> services/secondary-storage/pom.xml ea4bfcaf477af6be6a01113ac99ef262ae789071
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResource.java c55c2361bd5551eb51ca1b14340ab4c2e1563865
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java 8b9de39b70981b7fb4458b5a337b1b577c45aa3b
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/SecondaryStorageDiscoverer.java 5d6d61f740ac69acc228b0712633190b654b8f27
> services/secondary-storage/src/org/apache/cloudstack/storage/template/DownloadManagerImpl.java bf68b299c5c233f420ed1f78667f0f9522cf64de
> services/secondary-storage/test/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResourceTest.java 0c355ec884d50c530d2c77bec85e14550cc57352
> services/secondary-storage/test/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResourceTest.java PRE-CREATION
> utils/src/com/cloud/utils/UriUtils.java 1ff4d729cbf50154688bcea79bb8f48d1a972674
>
> Diff: https://reviews.apache.org/r/13877/diff/
>
>
> Testing
> -------
>
> Testing involved running the CIFS-specific and existing NFS tests for the secondary-storage service. E.g.
>
> cd ./services/secondary-storage
> mvn clean install -P developer,systemvm, -DskipTests=false
>
> Notice that the pom.xml was updated to allow tests to be turned on using a conditional. I ran the tests on the Citrix network, which allowed the existing NFS tests to run without modification.
>
> In addition, an integration test was done exercise the code in the SystemVM environment and to exercise updated validation code. The NFS secondary storage integration test was done using a XenServer deployment in which a template was registered and a VM created from the template. The CIFS secondary storage test was done on a Hyper-V hypervisor a template was registered.
>
>
> Thanks,
>
> Donal Lafferty
>
>
Re: Review Request 13877: CIFS Secondary Storage
Posted by Donal Lafferty <do...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13877/
-----------------------------------------------------------
(Updated Aug. 28, 2013, 8:58 p.m.)
Review request for cloudstack, Chiradeep Vittal, Devdeep Singh, and edison su.
Changes
-------
Respond to reviewer's comments.
Repository: cloudstack-git
Description
-------
CIFS support for secondary storage is documented here: https://cwiki.apache.org/confluence/display/CLOUDSTACK/CIFS+Support
The feature was implemented by extending the NFS provider. Its validation was updated so that you can pass it a URL containing the details of a CIFS share. The code that mounts NFS shares was extended to allow it do the same for CIFS shares. Otherwise, the secondary storage code is left unchanged.
The source is in the cifs_support branch of https://github.com/lafferty/cloudstack
NB: not able to submit using git format-patch due to issue described here: http://stackoverflow.com/questions/14594051/upload-from-git-format-patch-to-reviewboard-fails-on-file-not-found-in-the-repo
Diffs
-----
engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java d5e8a84516f4f37f23610a3d0315aa0a84991d46
engine/storage/src/org/apache/cloudstack/storage/datastore/protocol/DataStoreProtocol.java b27c96edbb72751c72c3963d6945eaeaeb42c74b
plugins/storage/image/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackImageStoreLifeCycleImpl.java 21a5e0a7194449d8b72f4537e14586c2b65b0bb5
plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java b9b74243eddd2008227f8ea2b88832e6e276811d
server/src/com/cloud/resource/ResourceManagerImpl.java 44ee8e3bd51be38acd6a90fd2fdd2383dad0fd15
server/src/com/cloud/storage/StorageManagerImpl.java b5d6ff953ef5d099fbe4dc56866e5948a46f1c8a
server/src/com/cloud/test/DatabaseConfig.java 63f77b66520d5f27736d62a67eeb16af808c7dd5
services/secondary-storage/conf/agent.properties aab82b6337474b52ae26c472b321f2c93ac57e81
services/secondary-storage/conf/log4j.xml PRE-CREATION
services/secondary-storage/pom.xml ea4bfcaf477af6be6a01113ac99ef262ae789071
services/secondary-storage/src/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResource.java c55c2361bd5551eb51ca1b14340ab4c2e1563865
services/secondary-storage/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java 8b9de39b70981b7fb4458b5a337b1b577c45aa3b
services/secondary-storage/src/org/apache/cloudstack/storage/resource/SecondaryStorageDiscoverer.java 5d6d61f740ac69acc228b0712633190b654b8f27
services/secondary-storage/src/org/apache/cloudstack/storage/template/DownloadManagerImpl.java bf68b299c5c233f420ed1f78667f0f9522cf64de
services/secondary-storage/test/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResourceTest.java 0c355ec884d50c530d2c77bec85e14550cc57352
services/secondary-storage/test/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResourceTest.java PRE-CREATION
utils/src/com/cloud/utils/UriUtils.java 1ff4d729cbf50154688bcea79bb8f48d1a972674
Diff: https://reviews.apache.org/r/13877/diff/
Testing
-------
Testing involved running the CIFS-specific and existing NFS tests for the secondary-storage service. E.g.
cd ./services/secondary-storage
mvn clean install -P developer,systemvm, -DskipTests=false
Notice that the pom.xml was updated to allow tests to be turned on using a conditional. I ran the tests on the Citrix network, which allowed the existing NFS tests to run without modification.
In addition, an integration test was done exercise the code in the SystemVM environment and to exercise updated validation code. The NFS secondary storage integration test was done using a XenServer deployment in which a template was registered and a VM created from the template. The CIFS secondary storage test was done on a Hyper-V hypervisor a template was registered.
Thanks,
Donal Lafferty
Re: Review Request 13877: CIFS Secondary Storage
Posted by Donal Lafferty <do...@citrix.com>.
> On Aug. 28, 2013, 6:52 p.m., Chiradeep Vittal wrote:
> > engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java, line 152
> > <https://reviews.apache.org/r/13877/diff/1/?file=345386#file345386line152>
> >
> > No idea why this is here. Can you add Edison as a reviewer
The deleted code used to bypass checking for a staging area for templates downloaded from object store. However, with CIFS a staging area can be created for Hyper-V zones.
> On Aug. 28, 2013, 6:52 p.m., Chiradeep Vittal wrote:
> > plugins/storage/image/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackImageStoreLifeCycleImpl.java, line 105
> > <https://reviews.apache.org/r/13877/diff/1/?file=345388#file345388line105>
> >
> > Coding convention is if (foo) {
Yes, but only when substantial changes are made should the convention diverge from that of the existing, surrounding code. The method in question uses if (x != null) all over :(
> On Aug. 28, 2013, 6:52 p.m., Chiradeep Vittal wrote:
> > services/secondary-storage/conf/agent.properties, line 3
> > <https://reviews.apache.org/r/13877/diff/1/?file=345393#file345393line3>
> >
> > test in agent.properties?
ATM, agent.properties cannot be used as-is. Since the settings have to be rewritten, it makes sense to provide guidance as to what properties exist and what their values look like. I think Microsoft environments take this approach, because it makes the developer's life a lot easier.
In this case, I have provided an example of a property required by the test portion of the build.
> On Aug. 28, 2013, 6:52 p.m., Chiradeep Vittal wrote:
> > services/secondary-storage/src/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResource.java, line 86
> > <https://reviews.apache.org/r/13877/diff/1/?file=345396#file345396line86>
> >
> > Instead of modifying the meaning of Nfs to include Cifs, how about a 'base' class with an embedded configurer that configures the mount differently based on protocol
Good idea, but doing so was explicitly excluded from the scope of this feature. I suggest we look at refactoring when adding the next file sharing protocol that is mounted to the local file system.
> On Aug. 28, 2013, 6:52 p.m., Chiradeep Vittal wrote:
> > services/secondary-storage/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java, line 27
> > <https://reviews.apache.org/r/13877/diff/1/?file=345397#file345397line27>
> >
> > Instead of modifying the meaning of Nfs to include Cifs, how about a 'base' class with an embedded configurer that configures the mount differently based on protocol
Good idea, but doing so was explicitly excluded from the scope of this feature. I suggest we look at refactoring when adding the next file sharing protocol that is mounted to the local file system.
> On Aug. 28, 2013, 6:52 p.m., Chiradeep Vittal wrote:
> > utils/src/com/cloud/utils/UriUtils.java, line 124
> > <https://reviews.apache.org/r/13877/diff/1/?file=345402#file345402line124>
> >
> > public or private?
General functionality, no sense in hiding it.
- Donal
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13877/#review25675
-----------------------------------------------------------
On Aug. 28, 2013, 12:38 a.m., Donal Lafferty wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13877/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2013, 12:38 a.m.)
>
>
> Review request for cloudstack, Chiradeep Vittal and Devdeep Singh.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> CIFS support for secondary storage is documented here: https://cwiki.apache.org/confluence/display/CLOUDSTACK/CIFS+Support
>
> The feature was implemented by extending the NFS provider. Its validation was updated so that you can pass it a URL containing the details of a CIFS share. The code that mounts NFS shares was extended to allow it do the same for CIFS shares. Otherwise, the secondary storage code is left unchanged.
>
> The source is in the cifs_support branch of https://github.com/lafferty/cloudstack
>
> NB: not able to submit using git format-patch due to issue described here: http://stackoverflow.com/questions/14594051/upload-from-git-format-patch-to-reviewboard-fails-on-file-not-found-in-the-repo
>
>
> Diffs
> -----
>
> engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java d5e8a84516f4f37f23610a3d0315aa0a84991d46
> engine/storage/src/org/apache/cloudstack/storage/datastore/protocol/DataStoreProtocol.java b27c96edbb72751c72c3963d6945eaeaeb42c74b
> plugins/storage/image/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackImageStoreLifeCycleImpl.java 21a5e0a7194449d8b72f4537e14586c2b65b0bb5
> plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java b9b74243eddd2008227f8ea2b88832e6e276811d
> server/src/com/cloud/resource/ResourceManagerImpl.java 44ee8e3bd51be38acd6a90fd2fdd2383dad0fd15
> server/src/com/cloud/storage/StorageManagerImpl.java b5d6ff953ef5d099fbe4dc56866e5948a46f1c8a
> server/src/com/cloud/test/DatabaseConfig.java 63f77b66520d5f27736d62a67eeb16af808c7dd5
> services/secondary-storage/conf/agent.properties aab82b6337474b52ae26c472b321f2c93ac57e81
> services/secondary-storage/conf/log4j.xml PRE-CREATION
> services/secondary-storage/pom.xml ea4bfcaf477af6be6a01113ac99ef262ae789071
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResource.java c55c2361bd5551eb51ca1b14340ab4c2e1563865
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java 8b9de39b70981b7fb4458b5a337b1b577c45aa3b
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/SecondaryStorageDiscoverer.java 5d6d61f740ac69acc228b0712633190b654b8f27
> services/secondary-storage/src/org/apache/cloudstack/storage/template/DownloadManagerImpl.java bf68b299c5c233f420ed1f78667f0f9522cf64de
> services/secondary-storage/test/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResourceTest.java 0c355ec884d50c530d2c77bec85e14550cc57352
> services/secondary-storage/test/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResourceTest.java PRE-CREATION
> utils/src/com/cloud/utils/UriUtils.java 1ff4d729cbf50154688bcea79bb8f48d1a972674
>
> Diff: https://reviews.apache.org/r/13877/diff/
>
>
> Testing
> -------
>
> Testing involved running the CIFS-specific and existing NFS tests for the secondary-storage service. E.g.
>
> cd ./services/secondary-storage
> mvn clean install -P developer,systemvm, -DskipTests=false
>
> Notice that the pom.xml was updated to allow tests to be turned on using a conditional. I ran the tests on the Citrix network, which allowed the existing NFS tests to run without modification.
>
> In addition, an integration test was done exercise the code in the SystemVM environment and to exercise updated validation code. The NFS secondary storage integration test was done using a XenServer deployment in which a template was registered and a VM created from the template. The CIFS secondary storage test was done on a Hyper-V hypervisor a template was registered.
>
>
> Thanks,
>
> Donal Lafferty
>
>
Re: Review Request 13877: CIFS Secondary Storage
Posted by Chiradeep Vittal <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13877/#review25675
-----------------------------------------------------------
engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java
<https://reviews.apache.org/r/13877/#comment50170>
No idea why this is here. Can you add Edison as a reviewer
plugins/storage/image/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackImageStoreLifeCycleImpl.java
<https://reviews.apache.org/r/13877/#comment50171>
Coding convention is if (foo) {
services/secondary-storage/conf/agent.properties
<https://reviews.apache.org/r/13877/#comment50172>
test in agent.properties?
services/secondary-storage/src/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResource.java
<https://reviews.apache.org/r/13877/#comment50173>
Instead of modifying the meaning of Nfs to include Cifs, how about a 'base' class with an embedded configurer that configures the mount differently based on protocol
services/secondary-storage/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java
<https://reviews.apache.org/r/13877/#comment50174>
Instead of modifying the meaning of Nfs to include Cifs, how about a 'base' class with an embedded configurer that configures the mount differently based on protocol
utils/src/com/cloud/utils/UriUtils.java
<https://reviews.apache.org/r/13877/#comment50175>
public or private?
- Chiradeep Vittal
On Aug. 28, 2013, 12:38 a.m., Donal Lafferty wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13877/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2013, 12:38 a.m.)
>
>
> Review request for cloudstack, Chiradeep Vittal and Devdeep Singh.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> CIFS support for secondary storage is documented here: https://cwiki.apache.org/confluence/display/CLOUDSTACK/CIFS+Support
>
> The feature was implemented by extending the NFS provider. Its validation was updated so that you can pass it a URL containing the details of a CIFS share. The code that mounts NFS shares was extended to allow it do the same for CIFS shares. Otherwise, the secondary storage code is left unchanged.
>
> The source is in the cifs_support branch of https://github.com/lafferty/cloudstack
>
> NB: not able to submit using git format-patch due to issue described here: http://stackoverflow.com/questions/14594051/upload-from-git-format-patch-to-reviewboard-fails-on-file-not-found-in-the-repo
>
>
> Diffs
> -----
>
> engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java d5e8a84516f4f37f23610a3d0315aa0a84991d46
> engine/storage/src/org/apache/cloudstack/storage/datastore/protocol/DataStoreProtocol.java b27c96edbb72751c72c3963d6945eaeaeb42c74b
> plugins/storage/image/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackImageStoreLifeCycleImpl.java 21a5e0a7194449d8b72f4537e14586c2b65b0bb5
> plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java b9b74243eddd2008227f8ea2b88832e6e276811d
> server/src/com/cloud/resource/ResourceManagerImpl.java 44ee8e3bd51be38acd6a90fd2fdd2383dad0fd15
> server/src/com/cloud/storage/StorageManagerImpl.java b5d6ff953ef5d099fbe4dc56866e5948a46f1c8a
> server/src/com/cloud/test/DatabaseConfig.java 63f77b66520d5f27736d62a67eeb16af808c7dd5
> services/secondary-storage/conf/agent.properties aab82b6337474b52ae26c472b321f2c93ac57e81
> services/secondary-storage/conf/log4j.xml PRE-CREATION
> services/secondary-storage/pom.xml ea4bfcaf477af6be6a01113ac99ef262ae789071
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResource.java c55c2361bd5551eb51ca1b14340ab4c2e1563865
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java 8b9de39b70981b7fb4458b5a337b1b577c45aa3b
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/SecondaryStorageDiscoverer.java 5d6d61f740ac69acc228b0712633190b654b8f27
> services/secondary-storage/src/org/apache/cloudstack/storage/template/DownloadManagerImpl.java bf68b299c5c233f420ed1f78667f0f9522cf64de
> services/secondary-storage/test/org/apache/cloudstack/storage/resource/LocalNfsSecondaryStorageResourceTest.java 0c355ec884d50c530d2c77bec85e14550cc57352
> services/secondary-storage/test/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResourceTest.java PRE-CREATION
> utils/src/com/cloud/utils/UriUtils.java 1ff4d729cbf50154688bcea79bb8f48d1a972674
>
> Diff: https://reviews.apache.org/r/13877/diff/
>
>
> Testing
> -------
>
> Testing involved running the CIFS-specific and existing NFS tests for the secondary-storage service. E.g.
>
> cd ./services/secondary-storage
> mvn clean install -P developer,systemvm, -DskipTests=false
>
> Notice that the pom.xml was updated to allow tests to be turned on using a conditional. I ran the tests on the Citrix network, which allowed the existing NFS tests to run without modification.
>
> In addition, an integration test was done exercise the code in the SystemVM environment and to exercise updated validation code. The NFS secondary storage integration test was done using a XenServer deployment in which a template was registered and a VM created from the template. The CIFS secondary storage test was done on a Hyper-V hypervisor a template was registered.
>
>
> Thanks,
>
> Donal Lafferty
>
>