Nitin, You are right For failed recurring snapshots, no chance providing a pop up error message, users can only track them from events. I will re-use existing thread in StorageManagerImpl for cleaning up volumes and 'storage.cleanup.interval' for this purpose.
Thanks & Regards Mice -----Original Message----- From: Nitin Mehta [mailto:nitin.me...@citrix.com] Sent: Monday, August 06, 2012 12:54 PM To: Mice Xia; cloudstack-dev@incubator.apache.org Cc: Edison Su; Koushik Das Subject: RE: Review Request: CS-15823 [VMware] failed snapshot entry should be removed automatically when multiple snopshots are taken on one volume simultaneously But, then how would the users track what happened to their snapshots - they would see no record of them at all ? And is this going to be done for recurring snapshots as well ? I agree with Edison. Keeping a row of snapshot with Error state seems fine with me. -----Original Message----- From: Mice Xia [mailto:mice_...@tcloudcomputing.com] Sent: Monday, August 06, 2012 8:47 AM To: cloudstack-dev@incubator.apache.org Cc: Edison Su; Koushik Das; 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 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...@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_...@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
