Feel free to close it out, it's in the repo now. I'd love if you were able to think about the test-ability of the changes... and yes, we are missing useful tests and approaches all over the place.
On Mon, Oct 7, 2013 at 4:15 PM, SuichII, Christopher <chris.su...@netapp.com > wrote: > 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 > 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> 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/> > > >
