You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Chris Suich <ch...@netapp.com> on 2013/10/04 02:57:58 UTC
Review Request 14477: Refactor Storage Related Resource Code
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14477/
-----------------------------------------------------------
Review request for cloudstack.
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
Diff: https://reviews.apache.org/r/14477/diff/
Testing
-------
Thanks,
Chris Suich
Re: Review Request 14477: Refactor Storage Related Resource Code
Posted by edison su <ed...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14477/#review26693
-----------------------------------------------------------
Ship it!
Ship It!
- edison su
On Oct. 4, 2013, 12:57 a.m., Chris Suich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14477/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2013, 12:57 a.m.)
>
>
> Review request for cloudstack.
>
>
> 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
>
> Diff: https://reviews.apache.org/r/14477/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chris Suich
>
>
Re: Review Request 14477: Refactor Storage Related Resource Code
Posted by Chip Childers <ch...@sungard.com>.
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.Suich@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.suich@netapp.com
> NetApp Software Engineer
> Data Center Platforms – Cloud Solutions
> Citrix, Cisco & Red Hat
>
> On Oct 4, 2013, at 5:28 PM, edison su <ed...@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/>
>
>
>
Re: Review Request 14477: Refactor Storage Related Resource Code
Posted by "SuichII, Christopher" <Ch...@netapp.com>.
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.suich@netapp.com<ma...@netapp.com>
NetApp Software Engineer
Data Center Platforms – Cloud Solutions
Citrix, Cisco & Red Hat
On Oct 4, 2013, at 5:28 PM, edison su <ed...@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/>
Re: Review Request 14477: Refactor Storage Related Resource Code
Posted by Chip Childers <ch...@sungard.com>.
thanks for the comments, seems fair!
Re: Review Request 14477: Refactor Storage Related Resource Code
Posted by edison su <ed...@citrix.com>.
> On Oct. 4, 2013, 8:23 p.m., Chip Childers wrote:
> > api/src/org/apache/cloudstack/api/command/user/snapshot/RevertSnapshotCmd.java, line 36
> > <https://reviews.apache.org/r/14477/diff/1/?file=361268#file361268line36>
> >
> > 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 Oct. 4, 2013, 8:23 p.m., Chip Childers wrote:
> > core/src/org/apache/cloudstack/storage/command/ForgetObjectCmd.java, line 24
> > <https://reviews.apache.org/r/14477/diff/1/?file=361271#file361271line24>
> >
> > 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 Oct. 4, 2013, 8:23 p.m., Chip Childers wrote:
> > engine/storage/src/org/apache/cloudstack/storage/helper/HypervisorHelperImpl.java, lines 35-76
> > <https://reviews.apache.org/r/14477/diff/1/?file=361287#file361287line35>
> >
> > 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 Oct. 4, 2013, 8:23 p.m., Chip Childers wrote:
> > plugins/hypervisors/simulator/src/com/cloud/resource/SimulatorStorageProcessor.java, lines 226-236
> > <https://reviews.apache.org/r/14477/diff/1/?file=361289#file361289line226>
> >
> > Can we get rid of these TODOs?
Yes, I can remove the TODO
> On Oct. 4, 2013, 8:23 p.m., Chip Childers wrote:
> > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java, lines 264-283
> > <https://reviews.apache.org/r/14477/diff/1/?file=361292#file361292line264>
> >
> > Unit tests for this if logic?
Again, straight forward code, not sure need add unit test here.
- edison
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14477/#review26694
-----------------------------------------------------------
On Oct. 4, 2013, 12:57 a.m., Chris Suich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14477/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2013, 12:57 a.m.)
>
>
> Review request for cloudstack.
>
>
> 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
>
> Diff: https://reviews.apache.org/r/14477/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chris Suich
>
>
Re: Review Request 14477: Refactor Storage Related Resource Code
Posted by Chip Childers <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14477/#review26694
-----------------------------------------------------------
Some questions and comments that probably need to be answered before this is committed. Thanks Chris!
api/src/org/apache/cloudstack/api/command/user/snapshot/RevertSnapshotCmd.java
<https://reviews.apache.org/r/14477/#comment51994>
Are there any unit or integration tests for this new API call? I can't find any in this diff.
core/src/org/apache/cloudstack/storage/command/ForgetObjectCmd.java
<https://reviews.apache.org/r/14477/#comment51995>
Are there any unit or integration tests for this new API call? I can't find any in this diff.
core/src/org/apache/cloudstack/storage/command/IntroduceObjectCmd.java
<https://reviews.apache.org/r/14477/#comment51996>
Are there any unit or integration tests for this new API call? I can't find any in this diff.
engine/storage/src/org/apache/cloudstack/storage/helper/HypervisorHelperImpl.java
<https://reviews.apache.org/r/14477/#comment51997>
I feel like this new code should have unit test for the if logic trees.
plugins/hypervisors/simulator/src/com/cloud/resource/SimulatorStorageProcessor.java
<https://reviews.apache.org/r/14477/#comment51998>
Can we get rid of these TODOs?
server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
<https://reviews.apache.org/r/14477/#comment52000>
Unit tests for this if logic?
- Chip Childers
On Oct. 4, 2013, 12:57 a.m., Chris Suich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14477/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2013, 12:57 a.m.)
>
>
> Review request for cloudstack.
>
>
> 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
>
> Diff: https://reviews.apache.org/r/14477/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chris Suich
>
>