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