incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Mice Xia" <mice_...@tcloudcomputing.com>
Subject RE: Review Request: CS-15823 [VMware] failed snapshot entry should be removed automatically when multiple snopshots are taken on one volume simultaneously
Date Mon, 06 Aug 2012 03:16:46 GMT
After reviewing the legacy code again, I suggest to expunge failed snapshot no matter what
reason. 
I think there is no need for users to see an Error snapshot, setting up a new thread for expunging
Error/Removed snapshots seem a bit heavy-weight and does not seem to improve much feasibility
for users.

I'll update the patch if everyone agrees on it.

Thanks & Regards
Mice

-----Original Message-----
From: Edison Su [mailto:Edison.su@citrix.com] 
Sent: Saturday, August 04, 2012 2:12 AM
To: Mice Xia; Koushik Das; cloudstack-dev@incubator.apache.org; Nitin Mehta
Subject: RE: Review Request: CS-15823 [VMware] failed snapshot entry should be removed automatically
when multiple snopshots are taken on one volume simultaneously

I think if creating snapshot failed, no matter whatever reason, the snapshot should be marked
as "Error". Just like the VM, if it's failed to create, them marked as "Error", then a background
reclaim thread will expunge it.

> -----Original Message-----
> From: Mice Xia [mailto:mice_xia@tcloudcomputing.com]
> Sent: Friday, August 03, 2012 1:29 AM
> To: Koushik Das; cloudstack-dev@incubator.apache.org; Nitin Mehta
> Cc: Edison Su
> Subject: RE: Review Request: CS-15823 [VMware] failed snapshot entry
> should be removed automatically when multiple snopshots are taken on
> one volume simultaneously
> 
> Remove the wrong reviewer from loop.. sorry for that, rely on auto-
> complete feature in review board but it turned out that was not Anthony
> Xu..
> 
> Yes, handling it in finally block seems more neat, but I need some
> background knowledge, i.e. in which situations failed snapshots should
> be expunged, and in which should be marked as Error?
> From legacy code I assume that if it does not to pass the check before
> createSnapshotOnPrimary, it is supposed to be expunged, otherwise means
> something wrong occurs in resource side and snapshot was kept as Error.
> 
> And by the way, does VMs that failed to create get handled by the same
> rule?
> 
> Regards
> Mice
> 
> -----Original Message-----
> From: Koushik Das [mailto:koushik.das@citrix.com]
> Sent: Friday, August 03, 2012 2:23 PM
> To: cloudstack-dev@incubator.apache.org; Mice Xia; Edison Su; Anthony
> Urso
> Subject: RE: Review Request: CS-15823 [VMware] failed snapshot entry
> should be removed automatically when multiple snopshots are taken on
> one volume simultaneously
> 
> But even in generic cases (just look at the exception above the changed
> one), the snapshot is marked as expunged. So I think it is better to do
> it in finally block.
> 
> -----Original Message-----
> From: Nitin Mehta [mailto:Nitin.Mehta@citrix.com]
> Sent: Friday, August 03, 2012 11:11 AM
> To: cloudstack-dev@incubator.apache.org; mice xia; Edison Su; Anthony
> Urso
> Subject: RE: Review Request: CS-15823 [VMware] failed snapshot entry
> should be removed automatically when multiple snopshots are taken on
> one volume simultaneously
> 
> Good point. But this is not failure per se in the snapshot creation but
> more of a limitation of these hypervisors and probably we want to keep
> that subtle difference by not putting it in Error but deleting the row
> altogether and surfacing the limitation through the exception to the
> end user.
> Satisfied :) ?
> 
> -----Original Message-----
> From: Koushik Das [mailto:koushik.das@citrix.com]
> Sent: Friday, August 03, 2012 10:38 AM
> To: cloudstack-dev@incubator.apache.org; mice xia; Edison Su; Anthony
> Urso
> Subject: RE: Review Request: CS-15823 [VMware] failed snapshot entry
> should be removed automatically when multiple snopshots are taken on
> one volume simultaneously
> 
> Any reason to handle this specific case only? I see in the code there
> are other places as well where snapshot creation may fail. Why not do
> it in the finally block so that all failure cases get handled.
> 
> -Koushik
> 
> -----Original Message-----
> From: mice xia [mailto:noreply@reviews.apache.org] On Behalf Of mice
> xia
> Sent: Friday, August 03, 2012 5:33 AM
> To: Edison Su; Anthony Urso
> Cc: cloudstack; mice xia
> Subject: Review Request: CS-15823 [VMware] failed snapshot entry should
> be removed automatically when multiple snopshots are taken on one
> volume simultaneously
> 
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6325/
> -----------------------------------------------------------
> 
> Review request for cloudstack, Anthony Urso and edison su.
> 
> 
> Description
> -------
> 
> changes:
> in SnapshotManagerImpl.java, expunge snapshot for this situation
> 
> 
> This addresses bug CS-15823.
> 
> 
> Diffs
> -----
> 
>   server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
> 6e3f9c1
> 
> Diff: https://reviews.apache.org/r/6325/diff/
> 
> 
> Testing
> -------
> 
> verified
> 
> 
> Thanks,
> 
> mice xia

Mime
View raw message