I've done some testing on our plugin and that appears to work fine for me.

-- 
Chris Suich
chris.su...@netapp.com
NetApp Software Engineer
Data Center Platforms – Cloud Solutions
Citrix, Cisco & Red Hat

On Oct 17, 2013, at 6:05 PM, Marcus Sorensen <shadow...@gmail.com> wrote:

> The following seems to fix the issue, by the way, but again since I
> didn't initially change the code I'd like someone else to
> review/handle it.
> 
> diff --git 
> a/core/src/com/cloud/storage/resource/StorageSubsystemCommandHandlerBase.java
> b/core/src/com/cloud/storage/resource/StorageSubsystemCommandHandlerBase.java
> index 002143f..3ac82e3 100644
> --- 
> a/core/src/com/cloud/storage/resource/StorageSubsystemCommandHandlerBase.java
> +++ 
> b/core/src/com/cloud/storage/resource/StorageSubsystemCommandHandlerBase.java
> @@ -68,7 +68,7 @@ public class StorageSubsystemCommandHandlerBase
> implements StorageSubsystemComma
>         DataStoreTO srcDataStore = srcData.getDataStore();
>         DataStoreTO destDataStore = destData.getDataStore();
> 
> -        if ((srcData.getObjectType() == DataObjectType.TEMPLATE) &&
> (destData.getObjectType() == DataObjectType.TEMPLATE &&
> destData.getDataStore().getRole() == DataStoreRole.Primary)) {
> +        if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
> srcData.getDataStore().getRole() == DataStoreRole.Image &&
> destData.getDataStore().getRole() == DataStoreRole.Primary) {
>             //copy template to primary storage
>             return processor.copyTemplateToPrimaryStorage(cmd);
>         } else if (srcData.getObjectType() == DataObjectType.TEMPLATE
> && srcDataStore.getRole() == DataStoreRole.Primary &&
> destDataStore.getRole() == DataStoreRole.Primary) {
> 
> On Thu, Oct 17, 2013 at 4:03 PM, Marcus Sorensen <shadow...@gmail.com> wrote:
>> All of the above mentioned is in
>> core/src/com/cloud/storage/resource/StorageSubsystemCommandHandlerBase.java
>> , by the way.
>> 
>> On Thu, Oct 17, 2013 at 4:02 PM, Marcus Sorensen <shadow...@gmail.com> wrote:
>>> Sure, but CopyCommand is being triggered in this code. I've tested
>>> several variations to this one line, some work, some don't.
>>> 
>>> On Thu, Oct 17, 2013 at 3:38 PM, Edison Su <edison...@citrix.com> wrote:
>>>> For CLVM, the copy template from secondary to primary and create volume 
>>>> from template logic is handled by 
>>>> CloudStackPrimaryDataStoreDriverImpl->copyAsync, not in 
>>>> AncientDataMotionStrategy
>>>> You can check the code: 4fb459355337c874a10f47c0224af72d6fef1ff2.
>>>> 
>>>>> -----Original Message-----
>>>>> From: Marcus Sorensen [mailto:shadow...@gmail.com]
>>>>> Sent: Thursday, October 17, 2013 2:07 PM
>>>>> To: dev@cloudstack.apache.org
>>>>> Subject: Re: CLVM broken on master
>>>>> 
>>>>> I think if we can change this line:
>>>>> 
>>>>> if ((srcData.getObjectType() == DataObjectType.TEMPLATE) &&
>>>>> (destData.getObjectType() == DataObjectType.TEMPLATE &&
>>>>> destData.getDataStore().getRole() == DataStoreRole.Primary)) {
>>>>> 
>>>>> to something like:
>>>>> 
>>>>> if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
>>>>> srcData.getDataStore().getRole() == DataStoreRole.Image &&
>>>>> destData.getDataStore().getRole() == DataStoreRole.Primary) {
>>>>> 
>>>>> Maybe that will work? That way it's strictly secondary -> primary 
>>>>> templates,
>>>>> not primary->primary templates.
>>>>> 
>>>>> Alternatively we could put it back to where it was:
>>>>> 
>>>>> if (srcData.getObjectType() == DataObjectType.TEMPLATE && srcDataStore
>>>>> instanceof NfsTO && destData.getDataStore().getRole() ==
>>>>> DataStoreRole.Primary) {
>>>>> 
>>>>> But your patch on the reviewboard removes NfsTO, and I'm assuming the
>>>>> idea was to work towards getting away from NFS-specific secondary storage.
>>>>> 
>>>>> On Thu, Oct 17, 2013 at 2:57 PM, Marcus Sorensen <shadow...@gmail.com>
>>>>> wrote:
>>>>>> I ran that through my tester, it didn't like it.  That actually kept
>>>>>> the system vms from starting. Since CopyCommand is used for both
>>>>>> template to template and template to primary, it seems that the
>>>>>> original template copy is fine but now this catches the case where the
>>>>>> source template is on primary and we are making a root disk.
>>>>>> copyTemplateToPrimaryStorage has:
>>>>>> 
>>>>>>        if (!(imageStore instanceof NfsTO)) {
>>>>>>            return new CopyCmdAnswer("unsupported protocol");
>>>>>>        }
>>>>>> 
>>>>>> we should be calling 'cloneVolumeFromBaseTemplate', but the original
>>>>>> if statement is now too loose.  I'll play with it a bit and see if I
>>>>>> can suggest a solution that works.
>>>>>> 
>>>>>> 2013-09-17 17:58:07,178 DEBUG [cloud.agent.Agent]
>>>>>> (agentRequest-Handler-2:null) Request:Seq 1-829816935:  { Cmd ,
>>>>>> MgmtId: 52241639751, via: 1, Ver: v1, Flags: 100011,
>>>>>> 
>>>>> [{"org.apache.cloudstack.storage.command.CopyCommand":{"srcTO":{"org.
>>>>> a
>>>>>> pache.cloudstack.storage.to.TemplateObjectTO":{"path":"bf53a7c6-1fed-1
>>>>>> 1e3-a1ff-000c29d82947","origUrl":"http://download.cloud.com/templates/
>>>>>> 4.2/systemvmtemplate-2013-06-12-master-
>>>>> kvm.qcow2.bz2","uuid":"bf53a7c6
>>>>>> -1fed-11e3-a1ff-000c29d82947","id":3,"format":"QCOW2","accountId":1,"c
>>>>>> 
>>>>> hecksum":"6cea42b2633841648040becb588bd8f0","hvm":false,"displayText":
>>>>>> "SystemVM Template
>>>>>> (KVM)","imageDataStore":{"org.apache.cloudstack.storage.to.PrimaryData
>>>>>> StoreTO":{"uuid":"8932daaf-272c-45c9-a078-d601dfc5ca56","id":1,"poolTy
>>>>>> pe":"Filesystem","host":"172.17.10.10","path":"/var/lib/libvirt/images
>>>>>> ","port":0}},"name":"routing-3","hypervisorType":"KVM"}},"destTO":{"or
>>>>>> g.apache.cloudstack.storage.to.VolumeObjectTO":{"uuid":"0c15b340-228b-
>>>>>> 48f1-88c4-
>>>>> b717ad08d4e3","volumeType":"ROOT","dataStore":{"org.apache.c
>>>>>> loudstack.storage.to.PrimaryDataStoreTO":{"uuid":"8932daaf-272c-45c9-a
>>>>>> 078-d601dfc5ca56","id":1,"poolType":"Filesystem","host":"172.17.10.10"
>>>>>> ,"path":"/var/lib/libvirt/images","port":0}},"name":"ROOT-1","size":0,
>>>>>> "volumeId":2,"vmName":"s-1-
>>>>> VM","accountId":1,"format":"QCOW2","id":2,"
>>>>>> hypervisorType":"KVM"}},"executeInSequence":false,"wait":0}}]
>>>>>> }
>>>>>> 
>>>>>> 2013-09-17 17:58:07,179 DEBUG [cloud.agent.Agent]
>>>>>> (agentRequest-Handler-2:null) Processing command:
>>>>>> org.apache.cloudstack.storage.command.CopyCommand
>>>>>> 2013-09-17 17:58:07,179 DEBUG [cloud.agent.Agent]
>>>>>> (agentRequest-Handler-2:null) Seq 1-829816935:  { Ans: , MgmtId:
>>>>>> 52241639751, via: 1, Ver: v1, Flags: 10,
>>>>>> 
>>>>> [{"org.apache.cloudstack.storage.command.CopyCmdAnswer":{"result":fals
>>>>>> e,"details":"unsupported
>>>>>> protocol","wait":0}}] }
>>>>>> 
>>>>>> On Thu, Oct 17, 2013 at 1:45 PM, SuichII, Christopher
>>>>>> <chris.su...@netapp.com> wrote:
>>>>>>> Hm, interesting.
>>>>>>> 
>>>>>>> Since nothing else in the if/else if series there depends on the src 
>>>>>>> being a
>>>>> template, I'd imagine it would be safe to just have the check be:
>>>>>>> 
>>>>>>> } else if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
>>>>>>> destDataStore.getRole() == DataStoreRole.Primary) {
>>>>>>> 
>>>>>>> In hindsight, adding the check for the destination being a template was
>>>>> just overkill and shouldn't have been added. So, if that fixes your 
>>>>> problem, I
>>>>> believe it is in line with that Edison and I were doing with the storage
>>>>> subsystem, however, we should check with him as well.
>>>>>>> 
>>>>>>> --
>>>>>>> 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 17, 2013, at 3:29 PM, Marcus Sorensen
>>>>> <shadow...@gmail.com<mailto:shadow...@gmail.com>> wrote:
>>>>>>> 
>>>>>>> Actually, I don't think that will fix this (though it probably fixes
>>>>>>> something :-)
>>>>>>> 
>>>>>>> The issue I'm having is that we went from 'if source is a template on
>>>>>>> nfs and destination is primary storage' to 'if source is a template
>>>>>>> and destination is a template on primary storage'. We aren't copying
>>>>>>> 'template on secondary' -> 'template on primary', with CLVM we copy
>>>>>>> 'template on secondary' -> 'root disk on primary', since it's
>>>>>>> wasteful and slow to copy a thin template (say a 50G img of size
>>>>>>> 500MB) to a template on primary that's 50G, and then copy that 50G
>>>>>>> primary template to another 50G primary root disk, since the primary
>>>>>>> storage is neither thin nor clone-able.
>>>>>>> 
>>>>>>> On Thu, Oct 17, 2013 at 1:16 PM, Marcus Sorensen
>>>>> <shadow...@gmail.com<mailto:shadow...@gmail.com>> wrote:
>>>>>>> Ok, let me test it.
>>>>>>> 
>>>>>>> On Thu, Oct 17, 2013 at 12:56 PM, SuichII, Christopher
>>>>>>> <chris.su...@netapp.com<mailto:chris.su...@netapp.com>> wrote:
>>>>>>> Oh, I noticed this and created a fix, which I thought I already had
>>>>> submitted since it was a part of the storage refactoring a couple weeks 
>>>>> back.
>>>>> I'll post the patch for review now.
>>>>>>> 
>>>>>>> --
>>>>>>> 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 17, 2013, at 2:51 PM, Marcus Sorensen <shadow...@gmail.com>
>>>>> wrote:
>>>>>>> 
>>>>>>> Just posting this to dev for visibility.
>>>>>>> 
>>>>>>> I think commit 180cfa19 broke CLVM primary storage for KVM. I'm
>>>>>>> failing VM deploy from template. I've been building a 'sanity check'
>>>>>>> test that focuses on the KVM specific suff (tests storage types and
>>>>>>> supported host OS for now), and this bubbled up.
>>>>>>> 
>>>>>>> Read more at:  https://issues.apache.org/jira/browse/CLOUDSTACK-4887
>>>>>>> 
>>>>>>> 

Reply via email to