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_...@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....@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.me...@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....@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:nore...@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
