You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by bharat kumar <bh...@citrix.com> on 2014/04/10 17:00:53 UTC
Review Request 20210: List templates returns destroyed temples when copying
the same template after deleting.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20210/
-----------------------------------------------------------
Review request for cloudstack and Kishan Kavala.
Summary (updated)
-----------------
List templates returns destroyed temples when copying the same template after deleting.
Bugs: CLOUDSTACK-6377
https://issues.apache.org/jira/browse/CLOUDSTACK-6377
Repository: cloudstack-git
Description (updated)
-------
List templates returns destroyed temples when copying the same template after deleting.
https://issues.apache.org/jira/browse/CLOUDSTACK-6377
Diffs (updated)
-----
engine/api/src/org/apache/cloudstack/storage/datastore/db/TemplateDataStoreDao.java 048ce22
engine/storage/src/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java d9a5164
engine/storage/src/org/apache/cloudstack/storage/image/db/TemplateDataStoreDaoImpl.java f5140e0
server/src/com/cloud/template/HypervisorTemplateAdapter.java 963aec9
Diff: https://reviews.apache.org/r/20210/diff/
Testing (updated)
-------
Tested on 4.2
Thanks,
bharat kumar
Re: Review Request 20210: List templates returns destroyed temples when
copying the same template after deleting.
Posted by bharat kumar <bh...@citrix.com>.
> On April 10, 2014, 5:19 p.m., Nitin Mehta wrote:
> > engine/storage/src/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java, line 134
> > <https://reviews.apache.org/r/20210/diff/2/?file=554427#file554427line134>
> >
> > Why do we need to update the old entry and not create a new one ? We should handle listTemplate not returning the destroyed result for the template ?
Hi Min,
in case of template deletion and recopying we update the same entry in template zone ref, So i wanted to make the behaviour consistent even in case of entries in the template_store_ref when dealing with re copying of templates. I am not sure which one to follow as both have advantages and disadvantages. but what ever is done i think it should be consistent.
so since the idea is to create new entries always. i will create a bug for template_zone_ref case and change the current patch to do the same.
> On April 10, 2014, 5:19 p.m., Nitin Mehta wrote:
> > engine/storage/src/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java, line 141
> > <https://reviews.apache.org/r/20210/diff/2/?file=554427#file554427line141>
> >
> > Changing the state not using state machine is bad practice.
Hi Min,
I was updating a already destroyed template so had to set it manually as state machine will not come into picture for cases like these.
> On April 10, 2014, 5:19 p.m., Nitin Mehta wrote:
> > engine/storage/src/org/apache/cloudstack/storage/image/db/TemplateDataStoreDaoImpl.java, line 286
> > <https://reviews.apache.org/r/20210/diff/2/?file=554428#file554428line286>
> >
> > method name says its zone but then you are passing the image store ?
Hi Min,
will change this to markAsDestroyedByTemplateStore.
> On April 10, 2014, 5:19 p.m., Nitin Mehta wrote:
> > server/src/com/cloud/template/HypervisorTemplateAdapter.java, line 341
> > <https://reviews.apache.org/r/20210/diff/2/?file=554429#file554429line341>
> >
> > The comment doesnt match what you are doing.
The comment was not about what the function is doing, it was about the intent of that snippet of the code. will change the comment to be more explicit about this.
> On April 10, 2014, 5:19 p.m., Nitin Mehta wrote:
> > server/src/com/cloud/template/HypervisorTemplateAdapter.java, line 342
> > <https://reviews.apache.org/r/20210/diff/2/?file=554429#file554429line342>
> >
> > Why is this required ?
> > This should happen via state machine and not like this. Are you seeing the destroyed not being set ?
Hi Min,
There are two fields which can be set to destroyed int the template_store_ref. one is the state and the other is the destroyed flag. Again this is confusing as I am not sure why do we have a destroyed flag and a destroyed state. I think one should be enough.
- bharat
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20210/#review40034
-----------------------------------------------------------
On April 10, 2014, 3:04 p.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20210/
> -----------------------------------------------------------
>
> (Updated April 10, 2014, 3:04 p.m.)
>
>
> Review request for cloudstack, Kishan Kavala and Min Chen.
>
>
> Bugs: CLOUDSTACK-6377
> https://issues.apache.org/jira/browse/CLOUDSTACK-6377
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> List templates returns destroyed temples when copying the same template after deleting.
> https://issues.apache.org/jira/browse/CLOUDSTACK-6377
>
>
> Diffs
> -----
>
> engine/api/src/org/apache/cloudstack/storage/datastore/db/TemplateDataStoreDao.java 048ce22
> engine/storage/src/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java d9a5164
> engine/storage/src/org/apache/cloudstack/storage/image/db/TemplateDataStoreDaoImpl.java f5140e0
> server/src/com/cloud/template/HypervisorTemplateAdapter.java 963aec9
>
> Diff: https://reviews.apache.org/r/20210/diff/
>
>
> Testing
> -------
>
> Tested on 4.2
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 20210: List templates returns destroyed temples when
copying the same template after deleting.
Posted by Nitin Mehta <ni...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20210/#review40034
-----------------------------------------------------------
engine/storage/src/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java
<https://reviews.apache.org/r/20210/#comment72835>
Why do we need to update the old entry and not create a new one ? We should handle listTemplate not returning the destroyed result for the template ?
engine/storage/src/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java
<https://reviews.apache.org/r/20210/#comment72834>
Changing the state not using state machine is bad practice.
engine/storage/src/org/apache/cloudstack/storage/image/db/TemplateDataStoreDaoImpl.java
<https://reviews.apache.org/r/20210/#comment72832>
method name says its zone but then you are passing the image store ?
server/src/com/cloud/template/HypervisorTemplateAdapter.java
<https://reviews.apache.org/r/20210/#comment72836>
The comment doesnt match what you are doing.
server/src/com/cloud/template/HypervisorTemplateAdapter.java
<https://reviews.apache.org/r/20210/#comment72831>
Why is this required ?
This should happen via state machine and not like this. Are you seeing the destroyed not being set ?
- Nitin Mehta
On April 10, 2014, 3:04 p.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20210/
> -----------------------------------------------------------
>
> (Updated April 10, 2014, 3:04 p.m.)
>
>
> Review request for cloudstack, Kishan Kavala and Min Chen.
>
>
> Bugs: CLOUDSTACK-6377
> https://issues.apache.org/jira/browse/CLOUDSTACK-6377
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> List templates returns destroyed temples when copying the same template after deleting.
> https://issues.apache.org/jira/browse/CLOUDSTACK-6377
>
>
> Diffs
> -----
>
> engine/api/src/org/apache/cloudstack/storage/datastore/db/TemplateDataStoreDao.java 048ce22
> engine/storage/src/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java d9a5164
> engine/storage/src/org/apache/cloudstack/storage/image/db/TemplateDataStoreDaoImpl.java f5140e0
> server/src/com/cloud/template/HypervisorTemplateAdapter.java 963aec9
>
> Diff: https://reviews.apache.org/r/20210/diff/
>
>
> Testing
> -------
>
> Tested on 4.2
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 20210: List templates returns destroyed temples when
copying the same template after deleting.
Posted by bharat kumar <bh...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20210/
-----------------------------------------------------------
(Updated April 17, 2014, 4:15 a.m.)
Review request for cloudstack, Kishan Kavala and Min Chen.
Changes
-------
we don't modify the existing entries anymore. we remove all the entries in the template_store ref when a template is deleted.
Bugs: CLOUDSTACK-6377
https://issues.apache.org/jira/browse/CLOUDSTACK-6377
Repository: cloudstack-git
Description
-------
List templates returns destroyed temples when copying the same template after deleting.
https://issues.apache.org/jira/browse/CLOUDSTACK-6377
Diffs (updated)
-----
engine/api/src/org/apache/cloudstack/storage/datastore/db/TemplateDataStoreDao.java 048ce22
engine/storage/src/org/apache/cloudstack/storage/image/db/TemplateDataStoreDaoImpl.java f5140e0
server/src/com/cloud/template/HypervisorTemplateAdapter.java 963aec9
server/src/com/cloud/template/TemplateManagerImpl.java 57f7ff6
Diff: https://reviews.apache.org/r/20210/diff/
Testing
-------
Tested on 4.2
Thanks,
bharat kumar
Re: Review Request 20210: List templates returns destroyed temples when
copying the same template after deleting.
Posted by bharat kumar <bh...@citrix.com>.
> On April 10, 2014, 5:25 p.m., Min Chen wrote:
> > The root cause for this issue is that we didn't remove template_store_ref entry from db when a template is deleted from a zone and only mark them as 'Destroyed'. In addition, template_view db view is created by joining with those destroyed entries in template_store_ref as well, which causing it to only randomly pick the entry that may be 'Destroyed'. The much simpler fix to me is to remove entries from template_store_ref table at the end of deleting template flow when the deletion is successful (since they have already been deleted from secondary storage) instead of finding and then manually updating entry done in the patch.
Hi Min,
The template_view dose the correct thing here. This problem occurs because we set the template state as destroyed but we do not mark the destroyed flag to true. The template view picks all the entries where destroyed flag is false, since even the deleted templates have this set to false they are also getting populated in the template_view table.
- bharat
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20210/#review40039
-----------------------------------------------------------
On April 10, 2014, 3:04 p.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20210/
> -----------------------------------------------------------
>
> (Updated April 10, 2014, 3:04 p.m.)
>
>
> Review request for cloudstack, Kishan Kavala and Min Chen.
>
>
> Bugs: CLOUDSTACK-6377
> https://issues.apache.org/jira/browse/CLOUDSTACK-6377
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> List templates returns destroyed temples when copying the same template after deleting.
> https://issues.apache.org/jira/browse/CLOUDSTACK-6377
>
>
> Diffs
> -----
>
> engine/api/src/org/apache/cloudstack/storage/datastore/db/TemplateDataStoreDao.java 048ce22
> engine/storage/src/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java d9a5164
> engine/storage/src/org/apache/cloudstack/storage/image/db/TemplateDataStoreDaoImpl.java f5140e0
> server/src/com/cloud/template/HypervisorTemplateAdapter.java 963aec9
>
> Diff: https://reviews.apache.org/r/20210/diff/
>
>
> Testing
> -------
>
> Tested on 4.2
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 20210: List templates returns destroyed temples when
copying the same template after deleting.
Posted by Min Chen <mi...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20210/#review40039
-----------------------------------------------------------
The root cause for this issue is that we didn't remove template_store_ref entry from db when a template is deleted from a zone and only mark them as 'Destroyed'. In addition, template_view db view is created by joining with those destroyed entries in template_store_ref as well, which causing it to only randomly pick the entry that may be 'Destroyed'. The much simpler fix to me is to remove entries from template_store_ref table at the end of deleting template flow when the deletion is successful (since they have already been deleted from secondary storage) instead of finding and then manually updating entry done in the patch.
- Min Chen
On April 10, 2014, 3:04 p.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20210/
> -----------------------------------------------------------
>
> (Updated April 10, 2014, 3:04 p.m.)
>
>
> Review request for cloudstack, Kishan Kavala and Min Chen.
>
>
> Bugs: CLOUDSTACK-6377
> https://issues.apache.org/jira/browse/CLOUDSTACK-6377
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> List templates returns destroyed temples when copying the same template after deleting.
> https://issues.apache.org/jira/browse/CLOUDSTACK-6377
>
>
> Diffs
> -----
>
> engine/api/src/org/apache/cloudstack/storage/datastore/db/TemplateDataStoreDao.java 048ce22
> engine/storage/src/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java d9a5164
> engine/storage/src/org/apache/cloudstack/storage/image/db/TemplateDataStoreDaoImpl.java f5140e0
> server/src/com/cloud/template/HypervisorTemplateAdapter.java 963aec9
>
> Diff: https://reviews.apache.org/r/20210/diff/
>
>
> Testing
> -------
>
> Tested on 4.2
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 20210: List templates returns destroyed temples when
copying the same template after deleting.
Posted by bharat kumar <bh...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20210/
-----------------------------------------------------------
(Updated April 10, 2014, 3:04 p.m.)
Review request for cloudstack, Kishan Kavala and Min Chen.
Changes
-------
Adding Min to the reviewer's list.
Bugs: CLOUDSTACK-6377
https://issues.apache.org/jira/browse/CLOUDSTACK-6377
Repository: cloudstack-git
Description
-------
List templates returns destroyed temples when copying the same template after deleting.
https://issues.apache.org/jira/browse/CLOUDSTACK-6377
Diffs
-----
engine/api/src/org/apache/cloudstack/storage/datastore/db/TemplateDataStoreDao.java 048ce22
engine/storage/src/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java d9a5164
engine/storage/src/org/apache/cloudstack/storage/image/db/TemplateDataStoreDaoImpl.java f5140e0
server/src/com/cloud/template/HypervisorTemplateAdapter.java 963aec9
Diff: https://reviews.apache.org/r/20210/diff/
Testing
-------
Tested on 4.2
Thanks,
bharat kumar