I think it's better if we make one patch per bug..
On Apr 15, 2014, at 5:59 PM, Tanner Danzey <arkan...@gmail.com> wrote: > > >> On April 15, 2014, 2:52 p.m., daan Hoogland wrote: >>> server/src/com/cloud/api/ApiDBUtils.java, line 1088 >>> <https://reviews.apache.org/r/20357/diff/1/?file=557802#file557802line1088> >>> >>> I don't fully understand the logic of querying .next() and then >>> .previous() >>> >>> It seems like guessing or is there some hard restrained that makes this >>> the way to determine this is KVM. >>> >>> Is this really the place to decide on hypervisor type? >> >> Tanner Danzey wrote: >> I had nearly finished a somewhat wordy reply when my tablet decided to >> reboot itself, so please forgive me if I seem brief in reply this time >> around. >> >> 1) From what I understand of Iterators, using .next() and .previous() in >> succession allow iterating over the same element twice. If this is not so, >> it's an easy enough fix. >> >> 2&3) This is a guess, yes. There is a similar guess above for deciding >> whether VHD should be associated with HyperV or Xenserver because of a >> similar situation (2 hypervisors, 1 format vs. 1 hypervisor, two formats) so >> this seemed like a logical place to put a similar fix. As illustrated in >> CS-6396, for the function getHypervisorTypeFromFormat() there is only one >> possible return for each format, which is not true for KVM as it can support >> RAW images (in the case of RBD or CLVM) as well as qcow2 images. The only >> thing I can think of that would remedy this is implementing a way to return >> multiple supported types of formats for hypervisors OR a separate format for >> RBD raw images & CLVM raw images, but that's a fair bit more legwork than I >> feel comfortable doing being as I am relatively new to the codebase. >> >> Another way to patch this that I thought of while writing this reply >> would be to check the datacenter for KVM and OVM clusters, and for example >> the presence of OVM clusters but not KVM clusters would indicate that RAW >> images are for OVM, whereas in the presence of KVM clusters without OVM >> cluststers RAW images could be assigned to KVM for RBD / CLVM. >> >> Let me know what you think, I hope I answered all your questions. > > I just did some additional reading and there is a better way to do that loop, > I will have to wait until I'm at my home computer to submit a revision. > > > - Tanner > > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20357/#review40394 > ----------------------------------------------------------- > > > On April 15, 2014, 3:43 a.m., Tanner Danzey wrote: >> >> ----------------------------------------------------------- >> This is an automatically generated e-mail. To reply, visit: >> https://reviews.apache.org/r/20357/ >> ----------------------------------------------------------- >> >> (Updated April 15, 2014, 3:43 a.m.) >> >> >> Review request for cloudstack. >> >> >> Bugs: CLOUDSTACK-5907 and CLOUDSTACK-6396 >> https://issues.apache.org/jira/browse/CLOUDSTACK-5907 >> https://issues.apache.org/jira/browse/CLOUDSTACK-6396 >> >> >> Repository: cloudstack-git >> >> >> Description >> ------- >> >> Fix for RBD (and potentially CLVM) volumes w/ KVM incorrectly identified as >> OVM volumes (not able to snapshot) >> >> >> Diffs >> ----- >> >> server/src/com/cloud/api/ApiDBUtils.java 67e47f7 >> >> Diff: https://reviews.apache.org/r/20357/diff/ >> >> >> Testing >> ------- >> >> Applied on otherwise clean 4.4 branch and tested in a live testing >> environment with KVM hypervisors and RBD primary storage pool that would >> otherwise identify as OVM. No errors and no weird behavior. Just the >> expected result. >> >> >> Thanks, >> >> Tanner Danzey >> >> >
