You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Min Chen <mi...@citrix.com> on 2013/11/02 05:34:43 UTC
S3 as secondary storage is broken in master
Hi Marcus,
Your commit a504c004bf10555e5ea67ec89fe7bf6f00fe4622 broke S3 functionality. With S3 as secondary storage, system vm cannot be started. Since for S3, copy template to primary will be from an ImageCache store. Your following line of code :
if (srcData.getObjectType() == DataObjectType.TEMPLATE && srcData.getDataStore().getRole() == DataStoreRole.Image && destData.getDataStore().getRole() == DataStoreRole.Primary) {
//copy template to primary storage
return processor.copyTemplateToPrimaryStorage(cmd);
}
will not cover this case. I saw that you mentioned about this commit in another thread about CLVM broken in master. To fix this problem, we can change the above line to:
if (srcData.getObjectType() == DataObjectType.TEMPLATE && (srcData.getDataStore().getRole() == DataStoreRole.Image || srcData.getDataStore().getRole() == DataStoreRole.ImageCache) && destData.getDataStore().getRole() == DataStoreRole.Primary) {
//copy template to primary storage
return processor.copyTemplateToPrimaryStorage(cmd);
}
Edison/Chris, do you see any issue with this fix?
Thanks
-min
Re: S3 as secondary storage is broken in master
Posted by Marcus Sorensen <sh...@gmail.com>.
nevermind, I was pointing the test at the wrong revision
On Wed, Nov 6, 2013 at 10:17 PM, Marcus Sorensen <sh...@gmail.com> wrote:
> Min,
> I ran my basic regression test for KVM, and the original bug I
> fixed in the commit you mention seems to be back. I'm not sure if
> it's related to your check-in, and I don't see any currently obvious
> reason why it would be so, but I'm investigating. Here's the log from
> latest master, you can see the CopyCommand is falling through to "not
> implemented yet"
>
> 2013-11-06 22:01:44,682 DEBUG [cloud.agent.Agent]
> (agentRequest-Handler-5:null) Request:Seq 1-384630832: { Cmd ,
> MgmtId: 52241639751, via: 1, Ver: v1, Flags: 100111,
> [{"org.apache.cloudstack.storage.command.CopyCommand":{"srcTO":{"org.apache.cloudstack.storage.to.TemplateObjectTO":{"path":"template/tmpl/1/201/d4201b3a-9fcf-35d1-986b-133cfd64779c.qcow2","origUrl":"http://marcus.mlsorensen.com/cloudstack-extras/tiny-centos-63.qcow2","uuid":"c27d031a-b695-41d7-9996-5b8e7688c489","id":201,"format":"QCOW2","accountId":1,"checksum":"44cd0e6330a59f031460bc18a40c95a2","hvm":true,"displayText":"tiny","imageDataStore":{"com.cloud.agent.api.to.NfsTO":{"_url":"nfs://172.17.10.10:/nfs/secondary","_role":"Image"}},"name":"201-1-2379913e-f51c-3e53-a0e5-f780c4782b16","hypervisorType":"KVM"}},"destTO":{"org.apache.cloudstack.storage.to.VolumeObjectTO":{"uuid":"94812f0c-c247-46d9-8689-c1600d09b9ae","volumeType":"ROOT","dataStore":{"org.apache.cloudstack.storage.to.PrimaryDataStoreTO":{"uuid":"c6669280-c5ad-4e91-8cab-5d7cb2f1455b","id":2,"poolType":"CLVM","host":"localhost","path":"vg0","port":0}},"name":"ROOT-6","size":1073741824,"volumeId":6,"vmName":"i-1-6-VM","accountId":1,"format":"QCOW2","id":6,"hypervisorType":"KVM"}},"executeInSequence":true,"wait":10800}}]
> }
>
> 2013-11-06 22:01:44,683 DEBUG [cloud.agent.Agent]
> (agentRequest-Handler-5:null) Processing command:
> org.apache.cloudstack.storage.command.CopyCommand
>
> 2013-11-06 22:01:44,684 DEBUG [cloud.agent.Agent]
> (agentRequest-Handler-5:null) Seq 1-384630832: { Ans: , MgmtId:
> 52241639751, via: 1, Ver: v1, Flags: 110,
> [{"com.cloud.agent.api.Answer":{"result":false,"details":"not
> implemented yet","wait":0}}] }
>
> On Sat, Nov 2, 2013 at 11:31 PM, Min Chen <mi...@citrix.com> wrote:
>> Checked in fix 99ead3419c80fc7135a95f34ced7650eda3572fd to master.
>>
>> Thanks
>> -min
>>
>> From: Marcus Sorensen <sh...@gmail.com>
>> Date: Saturday, November 2, 2013 9:25 AM
>> To: Min Chen <mi...@citrix.com>
>> Cc: "dev@cloudstack.apache.org" <de...@cloudstack.apache.org>, "SuichII,
>> Christopher" <Ch...@netapp.com>, Edison Su <Ed...@citrix.com>
>> Subject: Re: S3 as secondary storage is broken in master
>>
>> OK. I was thinking that S3 was acting as secondary storage now but really it
>> still requires NFS as the intermediary, hence the "image cache". In that
>> case your suggestion would work.
>>
>> On Nov 2, 2013 10:10 AM, "Min Chen" <mi...@citrix.com> wrote:
>>>
>>> No, Marcus. That piece of code worked in 4.2 for S3. Note that image ache
>>> store is also NfsTO.
>>>
>>> Thanks
>>> -min
>>>
>>> Sent from my iPhone
>>>
>>> > On Nov 2, 2013, at 7:18 AM, "Marcus Sorensen" <sh...@gmail.com>
>>> > wrote:
>>> >
>>> > My only issue is that I believe copyTemplateToPrimaryStorage fails if
>>> > the source datastore is not NfsTO, so I'm not sure if this change will
>>> > do anything, or where S3 copy is actually handled. se
>>> > copyTemplateToPrimaryStorage:
>>> >
>>> > KVM:
>>> > if (!(imageStore instanceof NfsTO)) {
>>> > return new CopyCmdAnswer("unsupported protocol");
>>> > }
>>> >
>>> > Xen (all logic wrapped in this if statement):
>>> > if ((srcStore instanceof NfsTO) && (srcData.getObjectType()
>>> > == DataObjectType.TEMPLATE)) {
>>> >
>>> > VMware:
>>> > if (!(srcStore instanceof NfsTO)) {
>>> > return new CopyCmdAnswer("unsupported protocol");
>>> > }
>>> >
>>> > This line of code has a history. In 4.2 it didn't work for S3, either.
>>> > It specifically only allowed NFS:
>>> >
>>> > if ((srcData.getObjectType() == DataObjectType.TEMPLATE) &&
>>> > (srcDataStore instanceof NfsTO) && (destData.getDataStore().getRole()
>>> > == DataStoreRole.Primary)) {
>>> >
>>> > Then for a short while it was changed during some refactoring Chris
>>> > and Edison were working on:
>>> >
>>> > if ((srcData.getObjectType() == DataObjectType.TEMPLATE) &&
>>> > (destData.getObjectType() == DataObjectType.TEMPLATE &&
>>> > destData.getDataStore().getRole() == DataStoreRole.Primary)) {
>>> >
>>> > That broke CLVM, so I changed it to:
>>> >
>>> > if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
>>> > srcData.getDataStore().getRole() == DataStoreRole.Image &&
>>> > destData.getDataStore().getRole() == DataStoreRole.Primary) {
>>> >
>>> >> On Fri, Nov 1, 2013 at 10:34 PM, Min Chen <mi...@citrix.com> wrote:
>>> >> Hi Marcus,
>>> >>
>>> >> Your commit a504c004bf10555e5ea67ec89fe7bf6f00fe4622 broke S3
>>> >> functionality.
>>> >> With S3 as secondary storage, system vm cannot be started. Since for
>>> >> S3,
>>> >> copy template to primary will be from an ImageCache store. Your
>>> >> following
>>> >> line of code :
>>> >>
>>> >> if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
>>> >> srcData.getDataStore().getRole() == DataStoreRole.Image &&
>>> >> destData.getDataStore().getRole() == DataStoreRole.Primary) {
>>> >> //copy template to primary storage
>>> >> return processor.copyTemplateToPrimaryStorage(cmd);
>>> >> }
>>> >>
>>> >> will not cover this case. I saw that you mentioned about this commit in
>>> >> another thread about CLVM broken in master. To fix this problem, we
>>> >> can
>>> >> change the above line to:
>>> >>
>>> >> if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
>>> >> (srcData.getDataStore().getRole() == DataStoreRole.Image ||
>>> >> srcData.getDataStore().getRole() == DataStoreRole.ImageCache) &&
>>> >> destData.getDataStore().getRole() == DataStoreRole.Primary) {
>>> >> //copy template to primary storage
>>> >> return processor.copyTemplateToPrimaryStorage(cmd);
>>> >> }
>>> >>
>>> >> Edison/Chris, do you see any issue with this fix?
>>> >>
>>> >> Thanks
>>> >> -min
Re: S3 as secondary storage is broken in master
Posted by Min Chen <mi...@citrix.com>.
Hi Marcus,
I am not sure that my last commit
99ead3419c80fc7135a95f34ced7650eda3572fd will cause this, since it is just
checking srcTO is an Image or an ImageCache store. In your case, it is
Image store, so the same old code will kick in. But I do see an issue in
current code compared to what we have in 4.2, in 4.2, we used to check
both srcTO object type and destTO object type, only both are
TemplateObject, it will go to the control. Now in master, not sure you or
Chris changed this by removing the check on destTO object type. Then in
this case, you are actually copying a Template on secondary to a Volume on
Primary, it will also get into the control of
processor.copyTemplateToPrimaryStorage(cmd), which seems incorrect.
Thanks
-min
On 11/6/13 9:17 PM, "Marcus Sorensen" <sh...@gmail.com> wrote:
>Min,
> I ran my basic regression test for KVM, and the original bug I
>fixed in the commit you mention seems to be back. I'm not sure if
>it's related to your check-in, and I don't see any currently obvious
>reason why it would be so, but I'm investigating. Here's the log from
>latest master, you can see the CopyCommand is falling through to "not
>implemented yet"
>
>2013-11-06 22:01:44,682 DEBUG [cloud.agent.Agent]
>(agentRequest-Handler-5:null) Request:Seq 1-384630832: { Cmd ,
>MgmtId: 52241639751, via: 1, Ver: v1, Flags: 100111,
>[{"org.apache.cloudstack.storage.command.CopyCommand":{"srcTO":{"org.apach
>e.cloudstack.storage.to.TemplateObjectTO":{"path":"template/tmpl/1/201/d42
>01b3a-9fcf-35d1-986b-133cfd64779c.qcow2","origUrl":"http://marcus.mlsorens
>en.com/cloudstack-extras/tiny-centos-63.qcow2","uuid":"c27d031a-b695-41d7-
>9996-5b8e7688c489","id":201,"format":"QCOW2","accountId":1,"checksum":"44c
>d0e6330a59f031460bc18a40c95a2","hvm":true,"displayText":"tiny","imageDataS
>tore":{"com.cloud.agent.api.to.NfsTO":{"_url":"nfs://172.17.10.10:/nfs/sec
>ondary","_role":"Image"}},"name":"201-1-2379913e-f51c-3e53-a0e5-f780c4782b
>16","hypervisorType":"KVM"}},"destTO":{"org.apache.cloudstack.storage.to.V
>olumeObjectTO":{"uuid":"94812f0c-c247-46d9-8689-c1600d09b9ae","volumeType"
>:"ROOT","dataStore":{"org.apache.cloudstack.storage.to.PrimaryDataStoreTO"
>:{"uuid":"c6669280-c5ad-4e91-8cab-5d7cb2f1455b","id":2,"poolType":"CLVM","
>host":"localhost","path":"vg0","port":0}},"name":"ROOT-6","size":107374182
>4,"volumeId":6,"vmName":"i-1-6-VM","accountId":1,"format":"QCOW2","id":6,"
>hypervisorType":"KVM"}},"executeInSequence":true,"wait":10800}}]
>}
>
>2013-11-06 22:01:44,683 DEBUG [cloud.agent.Agent]
>(agentRequest-Handler-5:null) Processing command:
>org.apache.cloudstack.storage.command.CopyCommand
>
>2013-11-06 22:01:44,684 DEBUG [cloud.agent.Agent]
>(agentRequest-Handler-5:null) Seq 1-384630832: { Ans: , MgmtId:
>52241639751, via: 1, Ver: v1, Flags: 110,
>[{"com.cloud.agent.api.Answer":{"result":false,"details":"not
>implemented yet","wait":0}}] }
>
>On Sat, Nov 2, 2013 at 11:31 PM, Min Chen <mi...@citrix.com> wrote:
>> Checked in fix 99ead3419c80fc7135a95f34ced7650eda3572fd to master.
>>
>> Thanks
>> -min
>>
>> From: Marcus Sorensen <sh...@gmail.com>
>> Date: Saturday, November 2, 2013 9:25 AM
>> To: Min Chen <mi...@citrix.com>
>> Cc: "dev@cloudstack.apache.org" <de...@cloudstack.apache.org>, "SuichII,
>> Christopher" <Ch...@netapp.com>, Edison Su <Ed...@citrix.com>
>> Subject: Re: S3 as secondary storage is broken in master
>>
>> OK. I was thinking that S3 was acting as secondary storage now but
>>really it
>> still requires NFS as the intermediary, hence the "image cache". In that
>> case your suggestion would work.
>>
>> On Nov 2, 2013 10:10 AM, "Min Chen" <mi...@citrix.com> wrote:
>>>
>>> No, Marcus. That piece of code worked in 4.2 for S3. Note that image
>>>ache
>>> store is also NfsTO.
>>>
>>> Thanks
>>> -min
>>>
>>> Sent from my iPhone
>>>
>>> > On Nov 2, 2013, at 7:18 AM, "Marcus Sorensen" <sh...@gmail.com>
>>> > wrote:
>>> >
>>> > My only issue is that I believe copyTemplateToPrimaryStorage fails
>>>if
>>> > the source datastore is not NfsTO, so I'm not sure if this change
>>>will
>>> > do anything, or where S3 copy is actually handled. se
>>> > copyTemplateToPrimaryStorage:
>>> >
>>> > KVM:
>>> > if (!(imageStore instanceof NfsTO)) {
>>> > return new CopyCmdAnswer("unsupported protocol");
>>> > }
>>> >
>>> > Xen (all logic wrapped in this if statement):
>>> > if ((srcStore instanceof NfsTO) && (srcData.getObjectType()
>>> > == DataObjectType.TEMPLATE)) {
>>> >
>>> > VMware:
>>> > if (!(srcStore instanceof NfsTO)) {
>>> > return new CopyCmdAnswer("unsupported protocol");
>>> > }
>>> >
>>> > This line of code has a history. In 4.2 it didn't work for S3,
>>>either.
>>> > It specifically only allowed NFS:
>>> >
>>> > if ((srcData.getObjectType() == DataObjectType.TEMPLATE) &&
>>> > (srcDataStore instanceof NfsTO) &&
>>>(destData.getDataStore().getRole()
>>> > == DataStoreRole.Primary)) {
>>> >
>>> > Then for a short while it was changed during some refactoring Chris
>>> > and Edison were working on:
>>> >
>>> > if ((srcData.getObjectType() == DataObjectType.TEMPLATE) &&
>>> > (destData.getObjectType() == DataObjectType.TEMPLATE &&
>>> > destData.getDataStore().getRole() == DataStoreRole.Primary)) {
>>> >
>>> > That broke CLVM, so I changed it to:
>>> >
>>> > if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
>>> > srcData.getDataStore().getRole() == DataStoreRole.Image &&
>>> > destData.getDataStore().getRole() == DataStoreRole.Primary) {
>>> >
>>> >> On Fri, Nov 1, 2013 at 10:34 PM, Min Chen <mi...@citrix.com>
>>>wrote:
>>> >> Hi Marcus,
>>> >>
>>> >> Your commit a504c004bf10555e5ea67ec89fe7bf6f00fe4622 broke S3
>>> >> functionality.
>>> >> With S3 as secondary storage, system vm cannot be started. Since for
>>> >> S3,
>>> >> copy template to primary will be from an ImageCache store. Your
>>> >> following
>>> >> line of code :
>>> >>
>>> >> if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
>>> >> srcData.getDataStore().getRole() == DataStoreRole.Image &&
>>> >> destData.getDataStore().getRole() == DataStoreRole.Primary) {
>>> >> //copy template to primary storage
>>> >> return processor.copyTemplateToPrimaryStorage(cmd);
>>> >> }
>>> >>
>>> >> will not cover this case. I saw that you mentioned about this
>>>commit in
>>> >> another thread about CLVM broken in master. To fix this problem, we
>>> >> can
>>> >> change the above line to:
>>> >>
>>> >> if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
>>> >> (srcData.getDataStore().getRole() == DataStoreRole.Image ||
>>> >> srcData.getDataStore().getRole() == DataStoreRole.ImageCache) &&
>>> >> destData.getDataStore().getRole() == DataStoreRole.Primary) {
>>> >> //copy template to primary storage
>>> >> return processor.copyTemplateToPrimaryStorage(cmd);
>>> >> }
>>> >>
>>> >> Edison/Chris, do you see any issue with this fix?
>>> >>
>>> >> Thanks
>>> >> -min
Re: S3 as secondary storage is broken in master
Posted by Marcus Sorensen <sh...@gmail.com>.
Min,
I ran my basic regression test for KVM, and the original bug I
fixed in the commit you mention seems to be back. I'm not sure if
it's related to your check-in, and I don't see any currently obvious
reason why it would be so, but I'm investigating. Here's the log from
latest master, you can see the CopyCommand is falling through to "not
implemented yet"
2013-11-06 22:01:44,682 DEBUG [cloud.agent.Agent]
(agentRequest-Handler-5:null) Request:Seq 1-384630832: { Cmd ,
MgmtId: 52241639751, via: 1, Ver: v1, Flags: 100111,
[{"org.apache.cloudstack.storage.command.CopyCommand":{"srcTO":{"org.apache.cloudstack.storage.to.TemplateObjectTO":{"path":"template/tmpl/1/201/d4201b3a-9fcf-35d1-986b-133cfd64779c.qcow2","origUrl":"http://marcus.mlsorensen.com/cloudstack-extras/tiny-centos-63.qcow2","uuid":"c27d031a-b695-41d7-9996-5b8e7688c489","id":201,"format":"QCOW2","accountId":1,"checksum":"44cd0e6330a59f031460bc18a40c95a2","hvm":true,"displayText":"tiny","imageDataStore":{"com.cloud.agent.api.to.NfsTO":{"_url":"nfs://172.17.10.10:/nfs/secondary","_role":"Image"}},"name":"201-1-2379913e-f51c-3e53-a0e5-f780c4782b16","hypervisorType":"KVM"}},"destTO":{"org.apache.cloudstack.storage.to.VolumeObjectTO":{"uuid":"94812f0c-c247-46d9-8689-c1600d09b9ae","volumeType":"ROOT","dataStore":{"org.apache.cloudstack.storage.to.PrimaryDataStoreTO":{"uuid":"c6669280-c5ad-4e91-8cab-5d7cb2f1455b","id":2,"poolType":"CLVM","host":"localhost","path":"vg0","port":0}},"name":"ROOT-6","size":1073741824,"volumeId":6,"vmName":"i-1-6-VM","accountId":1,"format":"QCOW2","id":6,"hypervisorType":"KVM"}},"executeInSequence":true,"wait":10800}}]
}
2013-11-06 22:01:44,683 DEBUG [cloud.agent.Agent]
(agentRequest-Handler-5:null) Processing command:
org.apache.cloudstack.storage.command.CopyCommand
2013-11-06 22:01:44,684 DEBUG [cloud.agent.Agent]
(agentRequest-Handler-5:null) Seq 1-384630832: { Ans: , MgmtId:
52241639751, via: 1, Ver: v1, Flags: 110,
[{"com.cloud.agent.api.Answer":{"result":false,"details":"not
implemented yet","wait":0}}] }
On Sat, Nov 2, 2013 at 11:31 PM, Min Chen <mi...@citrix.com> wrote:
> Checked in fix 99ead3419c80fc7135a95f34ced7650eda3572fd to master.
>
> Thanks
> -min
>
> From: Marcus Sorensen <sh...@gmail.com>
> Date: Saturday, November 2, 2013 9:25 AM
> To: Min Chen <mi...@citrix.com>
> Cc: "dev@cloudstack.apache.org" <de...@cloudstack.apache.org>, "SuichII,
> Christopher" <Ch...@netapp.com>, Edison Su <Ed...@citrix.com>
> Subject: Re: S3 as secondary storage is broken in master
>
> OK. I was thinking that S3 was acting as secondary storage now but really it
> still requires NFS as the intermediary, hence the "image cache". In that
> case your suggestion would work.
>
> On Nov 2, 2013 10:10 AM, "Min Chen" <mi...@citrix.com> wrote:
>>
>> No, Marcus. That piece of code worked in 4.2 for S3. Note that image ache
>> store is also NfsTO.
>>
>> Thanks
>> -min
>>
>> Sent from my iPhone
>>
>> > On Nov 2, 2013, at 7:18 AM, "Marcus Sorensen" <sh...@gmail.com>
>> > wrote:
>> >
>> > My only issue is that I believe copyTemplateToPrimaryStorage fails if
>> > the source datastore is not NfsTO, so I'm not sure if this change will
>> > do anything, or where S3 copy is actually handled. se
>> > copyTemplateToPrimaryStorage:
>> >
>> > KVM:
>> > if (!(imageStore instanceof NfsTO)) {
>> > return new CopyCmdAnswer("unsupported protocol");
>> > }
>> >
>> > Xen (all logic wrapped in this if statement):
>> > if ((srcStore instanceof NfsTO) && (srcData.getObjectType()
>> > == DataObjectType.TEMPLATE)) {
>> >
>> > VMware:
>> > if (!(srcStore instanceof NfsTO)) {
>> > return new CopyCmdAnswer("unsupported protocol");
>> > }
>> >
>> > This line of code has a history. In 4.2 it didn't work for S3, either.
>> > It specifically only allowed NFS:
>> >
>> > if ((srcData.getObjectType() == DataObjectType.TEMPLATE) &&
>> > (srcDataStore instanceof NfsTO) && (destData.getDataStore().getRole()
>> > == DataStoreRole.Primary)) {
>> >
>> > Then for a short while it was changed during some refactoring Chris
>> > and Edison were working on:
>> >
>> > if ((srcData.getObjectType() == DataObjectType.TEMPLATE) &&
>> > (destData.getObjectType() == DataObjectType.TEMPLATE &&
>> > destData.getDataStore().getRole() == DataStoreRole.Primary)) {
>> >
>> > That broke CLVM, so I changed it to:
>> >
>> > if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
>> > srcData.getDataStore().getRole() == DataStoreRole.Image &&
>> > destData.getDataStore().getRole() == DataStoreRole.Primary) {
>> >
>> >> On Fri, Nov 1, 2013 at 10:34 PM, Min Chen <mi...@citrix.com> wrote:
>> >> Hi Marcus,
>> >>
>> >> Your commit a504c004bf10555e5ea67ec89fe7bf6f00fe4622 broke S3
>> >> functionality.
>> >> With S3 as secondary storage, system vm cannot be started. Since for
>> >> S3,
>> >> copy template to primary will be from an ImageCache store. Your
>> >> following
>> >> line of code :
>> >>
>> >> if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
>> >> srcData.getDataStore().getRole() == DataStoreRole.Image &&
>> >> destData.getDataStore().getRole() == DataStoreRole.Primary) {
>> >> //copy template to primary storage
>> >> return processor.copyTemplateToPrimaryStorage(cmd);
>> >> }
>> >>
>> >> will not cover this case. I saw that you mentioned about this commit in
>> >> another thread about CLVM broken in master. To fix this problem, we
>> >> can
>> >> change the above line to:
>> >>
>> >> if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
>> >> (srcData.getDataStore().getRole() == DataStoreRole.Image ||
>> >> srcData.getDataStore().getRole() == DataStoreRole.ImageCache) &&
>> >> destData.getDataStore().getRole() == DataStoreRole.Primary) {
>> >> //copy template to primary storage
>> >> return processor.copyTemplateToPrimaryStorage(cmd);
>> >> }
>> >>
>> >> Edison/Chris, do you see any issue with this fix?
>> >>
>> >> Thanks
>> >> -min
Re: S3 as secondary storage is broken in master
Posted by Min Chen <mi...@citrix.com>.
Checked in fix 99ead3419c80fc7135a95f34ced7650eda3572fd to master.
Thanks
-min
From: Marcus Sorensen <sh...@gmail.com>>
Date: Saturday, November 2, 2013 9:25 AM
To: Min Chen <mi...@citrix.com>>
Cc: "dev@cloudstack.apache.org<ma...@cloudstack.apache.org>" <de...@cloudstack.apache.org>>, "SuichII, Christopher" <Ch...@netapp.com>>, Edison Su <Ed...@citrix.com>>
Subject: Re: S3 as secondary storage is broken in master
OK. I was thinking that S3 was acting as secondary storage now but really it still requires NFS as the intermediary, hence the "image cache". In that case your suggestion would work.
On Nov 2, 2013 10:10 AM, "Min Chen" <mi...@citrix.com>> wrote:
No, Marcus. That piece of code worked in 4.2 for S3. Note that image ache store is also NfsTO.
Thanks
-min
Sent from my iPhone
> On Nov 2, 2013, at 7:18 AM, "Marcus Sorensen" <sh...@gmail.com>> wrote:
>
> My only issue is that I believe copyTemplateToPrimaryStorage fails if
> the source datastore is not NfsTO, so I'm not sure if this change will
> do anything, or where S3 copy is actually handled. se
> copyTemplateToPrimaryStorage:
>
> KVM:
> if (!(imageStore instanceof NfsTO)) {
> return new CopyCmdAnswer("unsupported protocol");
> }
>
> Xen (all logic wrapped in this if statement):
> if ((srcStore instanceof NfsTO) && (srcData.getObjectType()
> == DataObjectType.TEMPLATE)) {
>
> VMware:
> if (!(srcStore instanceof NfsTO)) {
> return new CopyCmdAnswer("unsupported protocol");
> }
>
> This line of code has a history. In 4.2 it didn't work for S3, either.
> It specifically only allowed NFS:
>
> if ((srcData.getObjectType() == DataObjectType.TEMPLATE) &&
> (srcDataStore instanceof NfsTO) && (destData.getDataStore().getRole()
> == DataStoreRole.Primary)) {
>
> Then for a short while it was changed during some refactoring Chris
> and Edison were working on:
>
> if ((srcData.getObjectType() == DataObjectType.TEMPLATE) &&
> (destData.getObjectType() == DataObjectType.TEMPLATE &&
> destData.getDataStore().getRole() == DataStoreRole.Primary)) {
>
> That broke CLVM, so I changed it to:
>
> if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
> srcData.getDataStore().getRole() == DataStoreRole.Image &&
> destData.getDataStore().getRole() == DataStoreRole.Primary) {
>
>> On Fri, Nov 1, 2013 at 10:34 PM, Min Chen <mi...@citrix.com>> wrote:
>> Hi Marcus,
>>
>> Your commit a504c004bf10555e5ea67ec89fe7bf6f00fe4622 broke S3 functionality.
>> With S3 as secondary storage, system vm cannot be started. Since for S3,
>> copy template to primary will be from an ImageCache store. Your following
>> line of code :
>>
>> if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
>> srcData.getDataStore().getRole() == DataStoreRole.Image &&
>> destData.getDataStore().getRole() == DataStoreRole.Primary) {
>> //copy template to primary storage
>> return processor.copyTemplateToPrimaryStorage(cmd);
>> }
>>
>> will not cover this case. I saw that you mentioned about this commit in
>> another thread about CLVM broken in master. To fix this problem, we can
>> change the above line to:
>>
>> if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
>> (srcData.getDataStore().getRole() == DataStoreRole.Image ||
>> srcData.getDataStore().getRole() == DataStoreRole.ImageCache) &&
>> destData.getDataStore().getRole() == DataStoreRole.Primary) {
>> //copy template to primary storage
>> return processor.copyTemplateToPrimaryStorage(cmd);
>> }
>>
>> Edison/Chris, do you see any issue with this fix?
>>
>> Thanks
>> -min
Re: S3 as secondary storage is broken in master
Posted by Marcus Sorensen <sh...@gmail.com>.
OK. I was thinking that S3 was acting as secondary storage now but really
it still requires NFS as the intermediary, hence the "image cache". In that
case your suggestion would work.
On Nov 2, 2013 10:10 AM, "Min Chen" <mi...@citrix.com> wrote:
> No, Marcus. That piece of code worked in 4.2 for S3. Note that image ache
> store is also NfsTO.
>
> Thanks
> -min
>
> Sent from my iPhone
>
> > On Nov 2, 2013, at 7:18 AM, "Marcus Sorensen" <sh...@gmail.com>
> wrote:
> >
> > My only issue is that I believe copyTemplateToPrimaryStorage fails if
> > the source datastore is not NfsTO, so I'm not sure if this change will
> > do anything, or where S3 copy is actually handled. se
> > copyTemplateToPrimaryStorage:
> >
> > KVM:
> > if (!(imageStore instanceof NfsTO)) {
> > return new CopyCmdAnswer("unsupported protocol");
> > }
> >
> > Xen (all logic wrapped in this if statement):
> > if ((srcStore instanceof NfsTO) && (srcData.getObjectType()
> > == DataObjectType.TEMPLATE)) {
> >
> > VMware:
> > if (!(srcStore instanceof NfsTO)) {
> > return new CopyCmdAnswer("unsupported protocol");
> > }
> >
> > This line of code has a history. In 4.2 it didn't work for S3, either.
> > It specifically only allowed NFS:
> >
> > if ((srcData.getObjectType() == DataObjectType.TEMPLATE) &&
> > (srcDataStore instanceof NfsTO) && (destData.getDataStore().getRole()
> > == DataStoreRole.Primary)) {
> >
> > Then for a short while it was changed during some refactoring Chris
> > and Edison were working on:
> >
> > if ((srcData.getObjectType() == DataObjectType.TEMPLATE) &&
> > (destData.getObjectType() == DataObjectType.TEMPLATE &&
> > destData.getDataStore().getRole() == DataStoreRole.Primary)) {
> >
> > That broke CLVM, so I changed it to:
> >
> > if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
> > srcData.getDataStore().getRole() == DataStoreRole.Image &&
> > destData.getDataStore().getRole() == DataStoreRole.Primary) {
> >
> >> On Fri, Nov 1, 2013 at 10:34 PM, Min Chen <mi...@citrix.com> wrote:
> >> Hi Marcus,
> >>
> >> Your commit a504c004bf10555e5ea67ec89fe7bf6f00fe4622 broke S3
> functionality.
> >> With S3 as secondary storage, system vm cannot be started. Since for S3,
> >> copy template to primary will be from an ImageCache store. Your
> following
> >> line of code :
> >>
> >> if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
> >> srcData.getDataStore().getRole() == DataStoreRole.Image &&
> >> destData.getDataStore().getRole() == DataStoreRole.Primary) {
> >> //copy template to primary storage
> >> return processor.copyTemplateToPrimaryStorage(cmd);
> >> }
> >>
> >> will not cover this case. I saw that you mentioned about this commit in
> >> another thread about CLVM broken in master. To fix this problem, we can
> >> change the above line to:
> >>
> >> if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
> >> (srcData.getDataStore().getRole() == DataStoreRole.Image ||
> >> srcData.getDataStore().getRole() == DataStoreRole.ImageCache) &&
> >> destData.getDataStore().getRole() == DataStoreRole.Primary) {
> >> //copy template to primary storage
> >> return processor.copyTemplateToPrimaryStorage(cmd);
> >> }
> >>
> >> Edison/Chris, do you see any issue with this fix?
> >>
> >> Thanks
> >> -min
>
Re: S3 as secondary storage is broken in master
Posted by Min Chen <mi...@citrix.com>.
No, Marcus. That piece of code worked in 4.2 for S3. Note that image ache store is also NfsTO.
Thanks
-min
Sent from my iPhone
> On Nov 2, 2013, at 7:18 AM, "Marcus Sorensen" <sh...@gmail.com> wrote:
>
> My only issue is that I believe copyTemplateToPrimaryStorage fails if
> the source datastore is not NfsTO, so I'm not sure if this change will
> do anything, or where S3 copy is actually handled. se
> copyTemplateToPrimaryStorage:
>
> KVM:
> if (!(imageStore instanceof NfsTO)) {
> return new CopyCmdAnswer("unsupported protocol");
> }
>
> Xen (all logic wrapped in this if statement):
> if ((srcStore instanceof NfsTO) && (srcData.getObjectType()
> == DataObjectType.TEMPLATE)) {
>
> VMware:
> if (!(srcStore instanceof NfsTO)) {
> return new CopyCmdAnswer("unsupported protocol");
> }
>
> This line of code has a history. In 4.2 it didn't work for S3, either.
> It specifically only allowed NFS:
>
> if ((srcData.getObjectType() == DataObjectType.TEMPLATE) &&
> (srcDataStore instanceof NfsTO) && (destData.getDataStore().getRole()
> == DataStoreRole.Primary)) {
>
> Then for a short while it was changed during some refactoring Chris
> and Edison were working on:
>
> if ((srcData.getObjectType() == DataObjectType.TEMPLATE) &&
> (destData.getObjectType() == DataObjectType.TEMPLATE &&
> destData.getDataStore().getRole() == DataStoreRole.Primary)) {
>
> That broke CLVM, so I changed it to:
>
> if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
> srcData.getDataStore().getRole() == DataStoreRole.Image &&
> destData.getDataStore().getRole() == DataStoreRole.Primary) {
>
>> On Fri, Nov 1, 2013 at 10:34 PM, Min Chen <mi...@citrix.com> wrote:
>> Hi Marcus,
>>
>> Your commit a504c004bf10555e5ea67ec89fe7bf6f00fe4622 broke S3 functionality.
>> With S3 as secondary storage, system vm cannot be started. Since for S3,
>> copy template to primary will be from an ImageCache store. Your following
>> line of code :
>>
>> if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
>> srcData.getDataStore().getRole() == DataStoreRole.Image &&
>> destData.getDataStore().getRole() == DataStoreRole.Primary) {
>> //copy template to primary storage
>> return processor.copyTemplateToPrimaryStorage(cmd);
>> }
>>
>> will not cover this case. I saw that you mentioned about this commit in
>> another thread about CLVM broken in master. To fix this problem, we can
>> change the above line to:
>>
>> if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
>> (srcData.getDataStore().getRole() == DataStoreRole.Image ||
>> srcData.getDataStore().getRole() == DataStoreRole.ImageCache) &&
>> destData.getDataStore().getRole() == DataStoreRole.Primary) {
>> //copy template to primary storage
>> return processor.copyTemplateToPrimaryStorage(cmd);
>> }
>>
>> Edison/Chris, do you see any issue with this fix?
>>
>> Thanks
>> -min
Re: S3 as secondary storage is broken in master
Posted by Marcus Sorensen <sh...@gmail.com>.
My only issue is that I believe copyTemplateToPrimaryStorage fails if
the source datastore is not NfsTO, so I'm not sure if this change will
do anything, or where S3 copy is actually handled. se
copyTemplateToPrimaryStorage:
KVM:
if (!(imageStore instanceof NfsTO)) {
return new CopyCmdAnswer("unsupported protocol");
}
Xen (all logic wrapped in this if statement):
if ((srcStore instanceof NfsTO) && (srcData.getObjectType()
== DataObjectType.TEMPLATE)) {
VMware:
if (!(srcStore instanceof NfsTO)) {
return new CopyCmdAnswer("unsupported protocol");
}
This line of code has a history. In 4.2 it didn't work for S3, either.
It specifically only allowed NFS:
if ((srcData.getObjectType() == DataObjectType.TEMPLATE) &&
(srcDataStore instanceof NfsTO) && (destData.getDataStore().getRole()
== DataStoreRole.Primary)) {
Then for a short while it was changed during some refactoring Chris
and Edison were working on:
if ((srcData.getObjectType() == DataObjectType.TEMPLATE) &&
(destData.getObjectType() == DataObjectType.TEMPLATE &&
destData.getDataStore().getRole() == DataStoreRole.Primary)) {
That broke CLVM, so I changed it to:
if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
srcData.getDataStore().getRole() == DataStoreRole.Image &&
destData.getDataStore().getRole() == DataStoreRole.Primary) {
On Fri, Nov 1, 2013 at 10:34 PM, Min Chen <mi...@citrix.com> wrote:
> Hi Marcus,
>
> Your commit a504c004bf10555e5ea67ec89fe7bf6f00fe4622 broke S3 functionality.
> With S3 as secondary storage, system vm cannot be started. Since for S3,
> copy template to primary will be from an ImageCache store. Your following
> line of code :
>
> if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
> srcData.getDataStore().getRole() == DataStoreRole.Image &&
> destData.getDataStore().getRole() == DataStoreRole.Primary) {
> //copy template to primary storage
> return processor.copyTemplateToPrimaryStorage(cmd);
> }
>
> will not cover this case. I saw that you mentioned about this commit in
> another thread about CLVM broken in master. To fix this problem, we can
> change the above line to:
>
> if (srcData.getObjectType() == DataObjectType.TEMPLATE &&
> (srcData.getDataStore().getRole() == DataStoreRole.Image ||
> srcData.getDataStore().getRole() == DataStoreRole.ImageCache) &&
> destData.getDataStore().getRole() == DataStoreRole.Primary) {
> //copy template to primary storage
> return processor.copyTemplateToPrimaryStorage(cmd);
> }
>
> Edison/Chris, do you see any issue with this fix?
>
> Thanks
> -min