incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marcus Sorensen <shadow...@gmail.com>
Subject Re: re-implement clvm
Date Tue, 07 Aug 2012 22:36:08 GMT
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 <shadowsor@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.su@citrix.com> wrote:
>>
>>
>>> -----Original Message-----
>>> From: Marcus Sorensen [mailto:shadowsor@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.Kluge@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:shadowsor@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.su@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:shadowsor@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 <david@gnsa.us>
>>> wrote:
>>> > > >> > On Tue, Jul 31, 2012 at 3:56 PM, Wido den Hollander
>>> > > >> > <wido@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
>>> >

Mime
View raw message