Edison, I'm having trouble testing my patch against incubator-cloudstack. Every indication seems to be that it's working, but the secondary storage VM seems to be messed up. I found this:
http://www.mail-archive.com/cloudstack-dev@incubator.apache.org/msg02431.html Which you commented on, I just want to ensure that the issues aren't due to my patch. Basically the ssvm doesn't launch its agent. I would like to launch an instance and test my patch further, but am unable to. 22:23:34,035 ERROR AgentShell:606 - Unable to start agent: Resource class not found: com.cloud.storage.resource.PremiumSecondaryStorageResource due to: java.lang.ClassNotFoundException: com.cloud.storage.resource.PremiumSecondaryStorageResource On Mon, Aug 6, 2012 at 2:51 PM, Marcus Sorensen <shadow...@gmail.com> wrote: > Ok, I'll send on the core java patch portion. I've reviewed the > managesnapshot.sh changes. I had someone begin to create a clean room > version implementing the same functionality, and in the process found > that we can really only change it superficially as well. The clean > room version changed the bulk of the code into a single 'lvcreate > --snapshot' command, however we found that there's an issue with > --snapshot and CLVM exclusive locking, which led the developer to > implement 'lvcreate --snapshot' using equivalent dmsetup commands. At > that point we had something that looked the same as the original > patch, with only minor changes in syntax and variable names. > > On Thu, Aug 2, 2012 at 5:11 PM, Edison Su <edison...@citrix.com> wrote: >> >> >>> -----Original Message----- >>> From: Marcus Sorensen [mailto:shadow...@gmail.com] >>> Sent: Thursday, August 02, 2012 3:09 PM >>> To: cloudstack-dev@incubator.apache.org >>> Subject: RE: re-implement clvm >>> >>> Oh, I understand, but most of the relevant code is implemented in >>> 'if/else >>> if/else' blocks, such that its simply a matter of copying the existing >>> RBD >>> code into another 'else if' block and changing a few words (which in >>> turn >>> the RBD stuff did previously with the pulled, pre apache CLVM code, or >>> so >>> it looks). In my opinion there's really no other way to do it without >>> restructuring to avoid the cascading ifs. >> >> >> That's true, in current code, that's the only way. >> >>> >>> It sounds like aside from the snapshot code the removal was probably >>> unnecessary, having been reworked by a third party from Rommer's >>> submissions prior to being merged. I know little about the application >>> of >>> licensing details though. >> >> Please send the core java code to reviewboard, we can apply it. This part of >> java code is general enough, meaning everybody wants to implement CLVM needs >> to write the same code. >> >>> >>> What I do know however is that its extremely painful to rip something >>> major >>> like this out on existing users, rendering their whole infrastructure >>> obsolete with no upgrade path. Especially given the relative trivial >>> nature >>> of the patch, you'd think that one of the project owners would take an >>> hour >>> or two and rework it. Of course it makes sense that the people who use >>> and >>> care about a component should help develop it in an open source world, >>> but >>> the cloud stack consumers don't always follow development. Maybe we are >>> the >>> only ones who use it, but I think if the next major release pulls CLVM >>> support there will be an uproar. What if it had been the code >>> implementing >>> NFS support? >> >> I removed it in May 15, due to the concern that it conflicts with Apache >> license. While at that time, I didn't send an email to dev/user list about >> this decision. That's my mistake. I'll make sure this kind of thing will not >> happen again. >> >>> >>> Sorry for the rant. Hopefully we can get it resolved. >> >> Sure, we can get it resolved. >> >>> On Aug 2, 2012 2:21 PM, "Kevin Kluge" <kevin.kl...@citrix.com> wrote: >>> >>> > Marcus, you should write the new code in compliance with the Apache >>> CLA, >>> > which will forbid directly copying code from some other source. >>> Having >>> > said that, if the problem is constrained enough by existing >>> CloudStack code >>> > and/or the solution is so obvious that your code looks like the >>> original >>> > code, that's just what it is. >>> > >>> > I'm not a lawyer so please don't take this as legal advice from >>> Citrix or >>> > me. >>> > >>> > -kevin >>> > >>> > >>> > > -----Original Message----- >>> > > From: Marcus Sorensen [mailto:shadow...@gmail.com] >>> > > Sent: Tuesday, July 31, 2012 3:09 PM >>> > > To: cloudstack-dev@incubator.apache.org >>> > > Subject: Re: re-implement clvm >>> > > >>> > > Here's the refactored patch. The CLVM stuff is basically a copy of >>> the >>> > RBD >>> > > additions; and the patch also includes the original changes to >>> > > managesnapshot.sh, which is unmodified. >>> > > >>> > > I'm personally more concerned about the core functionality at this >>> > point, I >>> > > can go through and re-implement the snapshot stuff, but if the core >>> stuff >>> > > can't be pulled in due to licensing then it's not worth the trouble >>> to >>> > redo the >>> > > snapshotting. If there's anyone in a position of authority to >>> address the >>> > > licensing stuff any input would be appreciated. >>> > > >>> > > Thanks, >>> > > Marcus >>> > > >>> > > On Tue, Jul 31, 2012 at 3:21 PM, Edison Su <edison...@citrix.com> >>> wrote: >>> > > > The most complicated part in rommer's patch is LVM snapshot. If >>> > snapshot >>> > > support is not a must, then adding CLVM is simple as RBD. >>> > > > >>> > > >> -----Original Message----- >>> > > >> From: Marcus Sorensen [mailto:shadow...@gmail.com] >>> > > >> Sent: Tuesday, July 31, 2012 1:33 PM >>> > > >> To: cloudstack-dev@incubator.apache.org >>> > > >> Subject: Re: re-implement clvm >>> > > >> >>> > > >> Ok, so I've created a refactored patch that seems to work. It >>> was >>> > > >> pretty much entirely the RBD additions that were blocking the >>> > > >> original from being rolled back in. If a developer would be >>> willing >>> > > >> to take on the whole license issue and see this functionality >>> put >>> > > >> back in I'd still be willing to pay half of the bounty ($400). >>> As >>> > > >> the code looks, the changes are fairly minor, and I'm not sure >>> how >>> > > >> novel you'd have to get avoid the license issues (or that >>> there's any >>> > > >> easy alternative way to change the code sufficiently) >>> > > >> >>> > > >> On Tue, Jul 31, 2012 at 2:12 PM, David Nalley <da...@gnsa.us> >>> wrote: >>> > > >> > On Tue, Jul 31, 2012 at 3:56 PM, Wido den Hollander >>> > > >> > <w...@widodh.nl> >>> > > >> wrote: >>> > > >> >> >>> > > >> >> >>> > > >> >> On 07/31/2012 09:48 PM, Marcus Sorensen wrote: >>> > > >> >>> >>> > > >> >>> I'd be happy to try more if I had access to any contact info. >>> As >>> > > >> it >>> > > >> >>> is, things in the surrounding code have changed enough that >>> a bit >>> > > >> of >>> > > >> >>> re-factoring would need to be done even if there were >>> permission. >>> > > >> >>> >>> > > >> >>> My hunch is that unless he's switched roles, once the new >>> version >>> > > >> is >>> > > >> >>> released he may come out of the woodwork wondering why that >>> > > thing >>> > > >> he >>> > > >> >>> has a need for and developed is gone. >>> > > >> >> >>> > > >> >> >>> > > >> >> After writing the last RBD implementation this CLVM seems >>> trivial. >>> > > >> >> >>> > > >> >> A lot of code is still in there and looking at the commit >>> where it >>> > > >> got >>> > > >> >> removed it wont be that much work. >>> > > >> >> >>> > > >> >> The problem (and I'm not a licensing expert) is that if I >>> would >>> > > >> implement >>> > > >> >> CLVM again it would look a lot like the original code, do we >>> have >>> > > >> >> to >>> > > >> refer >>> > > >> >> to the old author for that? >>> > > >> >> >>> > > >> >> I'm assuming here that we won't be able to contact the >>> original >>> > > >> author, but >>> > > >> >> we want to keep the CLVM functionality for 4.0. >>> > > >> >> >>> > > >> >> Wido >>> > > >> > >>> > > >> > >>> > > >> > Actually - you should compare the original patches, with what >>> was >>> > > >> reverted. : >>> > > >> > http://bugs.cloudstack.org/browse/CS-10317 >>> > > >> > >>> > > >> > There was already something of a rewrite when Edison changed >>> how >>> > > >> > some of the storage was handled (which is the iteration that >>> was >>> > > pulled). >>> > > >> > >>> > > >> > IANAL either, so I won't bother to even try and answer that >>> > question. >>> > > >> > >>> > > >> > --David >>> >
