Thanks for the notes, Edison. Chip - Edison hit all the important points, but I'm not sure what the proper etiquette is here. I'd be more than happy to add some tests, but after looking through the code, I can't find any tests to model after. While I don't mind coming up with a model for testing this stuff, I think there should be a larger discussion about what that would look like.
I wanted to make sure I followed up on this to see what your thoughts were before closing the review request. -Chris -- Chris Suich chris.su...@netapp.com<mailto:chris.su...@netapp.com> NetApp Software Engineer Data Center Platforms – Cloud Solutions Citrix, Cisco & Red Hat On Oct 4, 2013, at 5:28 PM, edison su <edison...@citrix.com<mailto:edison...@citrix.com>> wrote: This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14477/ On October 4th, 2013, 8:23 p.m. UTC, Chip Childers wrote: api/src/org/apache/cloudstack/api/command/user/snapshot/RevertSnapshotCmd.java<https://reviews.apache.org/r/14477/diff/1/?file=361268#file361268line36> (Diff revision 1) 36 @APICommand(name = "RevertSnapshot", description = "revert a volume snapshot.", responseObject = SnapshotResponse.class) Are there any unit or integration tests for this new API call? I can't find any in this diff. There is no implementation for revertsnapshot yet in the current storage drivers. If any storage vendor wants to implement this feature, they can add their implementation in their storage driver. Also, if anybody is interested in implementing this feature in the default storage driver, for example, for Ceph, then it's the place to start with. On October 4th, 2013, 8:23 p.m. UTC, Chip Childers wrote: core/src/org/apache/cloudstack/storage/command/ForgetObjectCmd.java<https://reviews.apache.org/r/14477/diff/1/?file=361271#file361271line24> (Diff revision 1) 24 public class ForgetObjectCmd extends Command implements StorageSubSystemCommand { Are there any unit or integration tests for this new API call? I can't find any in this diff. It's not an api, it's just a simple command will send to hypervisor host, will be used by HypervisorHelper to introduce/forget an object on hypervisor host. On October 4th, 2013, 8:23 p.m. UTC, Chip Childers wrote: engine/storage/src/org/apache/cloudstack/storage/helper/HypervisorHelperImpl.java<https://reviews.apache.org/r/14477/diff/1/?file=361287#file361287line35> (Diff revision 1) 35 public class HypervisorHelperImpl implements HypervisorHelper { 36 private static final Logger s_logger = Logger.getLogger(HypervisorHelperImpl.class); 37 @Inject 38 EndPointSelector selector; 39 40 @Override 41 public DataTO introduceObject(DataTO object, Scope scope, Long storeId) { 42 EndPoint ep = selector.select(scope, storeId); 43 IntroduceObjectCmd cmd = new IntroduceObjectCmd(object); 44 Answer answer = ep.sendMessage(cmd); 45 if (answer == null || !answer.getResult()) { 46 String errMsg = answer == null ? null : answer.getDetails(); 47 throw new CloudRuntimeException("Failed to introduce object, due to " + errMsg); 48 } 49 IntroduceObjectAnswer introduceObjectAnswer = (IntroduceObjectAnswer)answer; 50 return introduceObjectAnswer.getDataTO(); 51 } 52 53 @Override 54 public boolean forgetObject(DataTO object, Scope scope, Long storeId) { 55 EndPoint ep = selector.select(scope, storeId); 56 ForgetObjectCmd cmd = new ForgetObjectCmd(object); 57 Answer answer = ep.sendMessage(cmd); 58 if (answer == null || !answer.getResult()) { 59 String errMsg = answer == null ? null : answer.getDetails(); 60 if (errMsg != null) { 61 s_logger.debug("Failed to forget object: " + errMsg); 62 } 63 return false; 64 } 65 return true; 66 } 67 68 @Override 69 public SnapshotObjectTO takeSnapshot(SnapshotObjectTO snapshotObjectTO, Scope scope) { 70 return null; //To change body of implemented methods use File | Settings | File Templates. 71 } 72 73 @Override 74 public boolean revertSnapshot(SnapshotObjectTO snapshotObjectTO, Scope scope) { 75 return false; //To change body of implemented methods use File | Settings | File Templates. 76 } I feel like this new code should have unit test for the if logic trees. The logic here is very simple: just send a command to hypervisor host. Not sure need unit test here or not. On October 4th, 2013, 8:23 p.m. UTC, Chip Childers wrote: plugins/hypervisors/simulator/src/com/cloud/resource/SimulatorStorageProcessor.java<https://reviews.apache.org/r/14477/diff/1/?file=361289#file361289line226> (Diff revision 1) 221 @Override 222 public Answer introduceObject(IntroduceObjectCmd cmd) { 223 // TODO Auto-generated method stub 224 return null; 225 } 226 227 @Override 228 public Answer forgetObject(ForgetObjectCmd cmd) { 229 // TODO Auto-generated method stub 230 return null; 231 } Can we get rid of these TODOs? Yes, I can remove the TODO On October 4th, 2013, 8:23 p.m. UTC, Chip Childers wrote: server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java<https://reviews.apache.org/r/14477/diff/1/?file=361292#file361292line264> (Diff revision 1) 261 public boolean revertSnapshot(Long snapshotId) { 262 Snapshot snapshot = _snapshotDao.findById(snapshotId); 263 if (snapshot == null) { 264 throw new InvalidParameterValueException("No such snapshot"); 265 } 266 267 SnapshotStrategy snapshotStrategy = null; 268 for (SnapshotStrategy strategy : snapshotStrategies) { 269 if (strategy.canHandle(snapshot)) { 270 snapshotStrategy = strategy; 271 break; 272 } 273 } 274 275 if (snapshotStrategy == null) { 276 return false; 277 } 278 279 return snapshotStrategy.revertSnapshot(snapshotId); 280 } Unit tests for this if logic? Again, straight forward code, not sure need add unit test here. - edison On October 4th, 2013, 12:57 a.m. UTC, Chris Suich wrote: Review request for cloudstack. By Chris Suich. Updated Oct. 4, 2013, 12:57 a.m. Repository: cloudstack-git Description These changes are a joint effort between Edison and I to refactor some of the code around snapshotting VM volumes and creating templates/volumes from VM volume snapshots. In general, we were working towards allowing PrimaryDataStoreDrivers to create snapshots on primary storage and not requiring the snapshots to be transferred to secondary storage. High level changes: -Added uuid to NfsTO, SwiftTO & S3TO to cut down on the requirement of PrimaryDataStoreTO and ImageStoreTO which don't really serve much of a purpose -Initial work towards enable reverting VM volume from snapshots -Added hypervisor commands for introducing and forgetting new hypervisor objects (snapshots, templates & volumes) Diffs * api/src/com/cloud/agent/api/to/DataStoreTO.java (9014f8e) * api/src/com/cloud/agent/api/to/NfsTO.java (415c95c) * api/src/com/cloud/agent/api/to/SwiftTO.java (7349d77) * api/src/com/cloud/event/EventTypes.java (ec9604e) * api/src/com/cloud/storage/snapshot/SnapshotApiService.java (23e6522) * api/src/org/apache/cloudstack/api/command/admin/storage/ListStoragePoolsCmd.java (26351bb) * api/src/org/apache/cloudstack/api/command/user/snapshot/RevertSnapshotCmd.java (PRE-CREATION) * core/src/com/cloud/storage/resource/StorageProcessor.java (5fa9f8a) * core/src/com/cloud/storage/resource/StorageSubsystemCommandHandlerBase.java (ab9aa2a) * core/src/org/apache/cloudstack/storage/command/ForgetObjectCmd.java (PRE-CREATION) * core/src/org/apache/cloudstack/storage/command/IntroduceObjectAnswer.java (PRE-CREATION) * core/src/org/apache/cloudstack/storage/command/IntroduceObjectCmd.java (PRE-CREATION) * core/src/org/apache/cloudstack/storage/to/ImageStoreTO.java (0037ea5) * core/src/org/apache/cloudstack/storage/to/PrimaryDataStoreTO.java (5e870df) * engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/EndPointSelector.java (ca0cc2c) * engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotService.java (d594a07) * engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotStrategy.java (86ae532) * engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java (96d1f5a) * engine/storage/image/src/org/apache/cloudstack/storage/image/store/ImageStoreImpl.java (855d8cb) * engine/storage/integration-test/test/org/apache/cloudstack/storage/test/SnapshotTestWithFakeData.java (2aaabda) * engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java (3ead93f) * engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotStrategyBase.java (1b57922) * engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/XenserverSnapshotStrategy.java (60d9407) * engine/storage/src/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java (fdc12bf) * engine/storage/src/org/apache/cloudstack/storage/helper/HypervisorHelper.java (PRE-CREATION) * engine/storage/src/org/apache/cloudstack/storage/helper/HypervisorHelperImpl.java (PRE-CREATION) * plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java (82fd2ce) * plugins/hypervisors/simulator/src/com/cloud/resource/SimulatorStorageProcessor.java (c7768aa) * plugins/hypervisors/vmware/src/com/cloud/storage/resource/VmwareStorageProcessor.java (4982d87) * plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/XenServerStorageProcessor.java (739b974) * server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java (2297e6a) * services/secondary-storage/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java (3ef950b) View Diff<https://reviews.apache.org/r/14477/diff/>
