cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Donal Lafferty" <donal.laffe...@citrix.com>
Subject Re: Review Request 13877: CIFS Secondary Storage
Date Wed, 28 Aug 2013 20:52:25 GMT


> 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
> 
>


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