cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Syed Mushtaq <syed1.mush...@gmail.com>
Subject Re: Refactoring CitrixResourceBase
Date Thu, 26 May 2016 01:45:51 GMT
Nope. I think they should all be XenServer (they were Citrix earlier) I
plan to clean that up too.

-Syed

On Wed, May 25, 2016 at 9:43 PM, Will Stevens <williamstevens@gmail.com>
wrote:

> Is there a reason some are prefixed with 'Xs' and others are prefixed with
> 'XenServer'?
> On May 25, 2016 9:41 PM, "Will Stevens" <williamstevens@gmail.com> wrote:
>
> > Ya. I didn't know either and made the same mistake. :)
> > On May 25, 2016 9:40 PM, "Syed Mushtaq" <syed1.mushtaq@gmail.com> wrote:
> >
> >> Aah I did not know that. Thanks Will .. here is the link
> >> http://i.imgur.com/5B55XMB.png
> >>
> >> On Wed, May 25, 2016 at 6:37 PM, Will Stevens <williamstevens@gmail.com
> >
> >> wrote:
> >>
> >> > Attachments don't work. You will have to host it publicly and then
> post
> >> a
> >> > link to it.
> >> > On May 25, 2016 4:28 PM, "Syed Ahmed" <sahmed@cloudops.com> wrote:
> >> >
> >> > > Forgot to attach screenshot
> >> > >
> >> > > On Wed, May 25, 2016 at 4:09 PM, Syed Ahmed <sahmed@cloudops.com>
> >> wrote:
> >> > >
> >> > >> Thanks Rafael,
> >> > >>
> >> > >> Here is how I am doing. You can see the tree in the screenshot
> >> attached.
> >> > >> I have started with storage and extracted the storage commands
out
> of
> >> > >> CitrixResourceBase (I've renamed it to XenServerResourceBase).
This
> >> is
> >> > the
> >> > >> file for refrence. Let me know what you think.
> >> > >>
> >> > >>
> >> > >>
> >> >
> >>
> https://github.com/syed/cloudstack/blob/ca2d1469f991727515d9b71a790411d36eb60fdc/plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/storage/XenServerStorageResource.java
> >> > >>
> >> > >> It is a non-trivial refactor so may take some time but I think
the
> >> end
> >> > >> result would be very good for everyone.
> >> > >>
> >> > >>
> >> > >>
> >> > >> On Wed, May 25, 2016 at 11:22 AM, Rafael Weingärtner <
> >> > >> rafaelweingartner@gmail.com> wrote:
> >> > >>
> >> > >>> Hi Syed,
> >> > >>> That is a great job.
> >> > >>> I would only suggest breaking the commons a little bit more
> between
> >> > >>> “monitoring” and “common”. On the monitoring side,
we could have
> >> host
> >> > >>> monitoring, VMs monitoring, VMs' status checks (running, stopped,
> >> and
> >> > >>> others) and maybe other tasks that aim to monitor/check a
resource
> >> > >>> state/information.
> >> > >>>
> >> > >>> About the singletons you extracted, I do not see a need for
an
> >> > interface
> >> > >>> right now (maybe I am overlooking the need); in the future
if the
> >> need
> >> > >>> for
> >> > >>> interfaces appears, we can create some interfaces and use
them
> into
> >> the
> >> > >>> singletons you created.
> >> > >>>
> >> > >>>
> >> > >>> On Wed, May 25, 2016 at 12:10 PM, Syed Mushtaq <
> >> > syed1.mushtaq@gmail.com>
> >> > >>> wrote:
> >> > >>>
> >> > >>> > Hey Guys,
> >> > >>> >
> >> > >>> > To give you an update, I've identified and categorized
the
> >> functions
> >> > in
> >> > >>> > CitrixResourceBase into 4 categories
> >> > >>> >
> >> > >>> > 1) common: These deal with the host as a whole (example
> >> getHostInfo,
> >> > >>> > callPlugin, connection pool  etc)
> >> > >>> > 2) compute: Dealing with operations on VMs (start,stop,
reboot,
> >> > update
> >> > >>> etc)
> >> > >>> > 3) storage: Dealing with storage operations (creating
VDI,
> >> attaching
> >> > >>> to VM,
> >> > >>> > SR operations)
> >> > >>> > 4) network: OVS / PIF / VIF operations
> >> > >>> >
> >> > >>> > I've created singleton classes for each and slowly moving
the
> >> > >>> functionality
> >> > >>> > there. These singletons don't have a base class nor do
they
> >> implement
> >> > >>> an
> >> > >>> > interface. I don't know what the best practice is. One
question
> >> that
> >> > I
> >> > >>> have
> >> > >>> > is, should I create an interface for them or use some
existing
> >> > >>> interface?
> >> > >>> >
> >> > >>> > Thanks,
> >> > >>> > -Syed
> >> > >>> >
> >> > >>> >
> >> > >>> > On Fri, May 20, 2016 at 9:58 AM, Syed Mushtaq <
> >> > syed1.mushtaq@gmail.com
> >> > >>> >
> >> > >>> > wrote:
> >> > >>> >
> >> > >>> > > Thanks guys for the Ideas. I will open a JIRA ticket
and start
> >> > >>> working on
> >> > >>> > > it.
> >> > >>> > >
> >> > >>> > > -Syed
> >> > >>> > >
> >> > >>> > >
> >> > >>> > > On Thu, May 19, 2016 at 7:57 PM, Rafael Weingärtner
<
> >> > >>> > > rafaelweingartner@gmail.com> wrote:
> >> > >>> > >
> >> > >>> > >> Hi Syed,
> >> > >>> > >> That is a great idea; however, it is a very
hard task.
> >> > >>> > >> The idea of Tim is great; actually, we already
have some sort
> >> of
> >> > >>> > hierarchy
> >> > >>> > >> that is used in “CitrixResourceBase.java”.
> >> > >>> > >> I would suggest you first removing the unused
code, unused
> >> > >>> variable, and
> >> > >>> > >> duplicate methods; that would be one PR. You
can use a tool
> >> called
> >> > >>> > >> UCdetector to find unused code. To find duplicated
code you
> can
> >> > use
> >> > >>> PMD.
> >> > >>> > >>
> >> > >>> > >> One very good example of code duplicity are
the methods
> called
> >> > >>> > >>
> >> > >>> > >>
> >> > >>> >
> >> > >>>
> >> >
> >>
> “com.cloud.hypervisor.xenserver.resource.CitrixResourceBase.callHostPluginAsync(…)”.
> >> > >>> > >>
> >> > >>> > >> After you have cleaned the class, I suggest
you analyzing
> where
> >> > each
> >> > >>> > >> remaining method is used and then look for the
proper place
> to
> >> put
> >> > >>> them.
> >> > >>> > >> It might be a good idea on separating between
singletons that
> >> are
> >> > >>> > >> responsible for well-defined tasks such as managing
storage,
> >> > >>> networking,
> >> > >>> > >> VMs creating and deletion, VMs monitoring and
others.
> >> > >>> > >>
> >> > >>> > >> If you need any help, please do not hesitate
on asking for
> our
> >> > help.
> >> > >>> > >>
> >> > >>> > >>
> >> > >>> > >> On Thu, May 19, 2016 at 4:50 PM, Daan Hoogland
<
> >> > >>> daan.hoogland@gmail.com
> >> > >>> > >
> >> > >>> > >> wrote:
> >> > >>> > >>
> >> > >>> > >> > Syed,
> >> > >>> > >> >
> >> > >>> > >> > gogogo. actually it has shrunk to 5k lines
since 2012 ;)
> >> > >>> > >> >
> >> > >>> > >> > I like your initiative and initial direction.
A lot of
> small
> >> > >>> steps to
> >> > >>> > >> > improve the blob have been taken and I
would sugest to keep
> >> > going
> >> > >>> in
> >> > >>> > >> small
> >> > >>> > >> > steps.
> >> > >>> > >> >
> >> > >>> > >> > On Thu, May 19, 2016 at 9:44 PM, Tim Mackey
<
> >> tmackey@gmail.com>
> >> > >>> > wrote:
> >> > >>> > >> >
> >> > >>> > >> > > +1
> >> > >>> > >> > >
> >> > >>> > >> > > When I went through this last time,
not only was it hard
> to
> >> > >>> > understand
> >> > >>> > >> > the
> >> > >>> > >> > > flows, but the XenServer version management
was a pain.
> >> Would
> >> > >>> > suggest
> >> > >>> > >> > > creating a base class which always
works (i.e. is
> >> independent
> >> > of
> >> > >>> > >> > XenServer
> >> > >>> > >> > > version) for core functions. Then
add in that which
> exists
> >> > for a
> >> > >>> > >> specific
> >> > >>> > >> > > version. Should help greatly with
testing IMO.
> >> > >>> > >> > >
> >> > >>> > >> > > -tim
> >> > >>> > >> > >
> >> > >>> > >> > > On Thu, May 19, 2016 at 2:37 PM, Syed
Mushtaq <
> >> > >>> > >> syed1.mushtaq@gmail.com>
> >> > >>> > >> > > wrote:
> >> > >>> > >> > >
> >> > >>> > >> > > > Hi All,
> >> > >>> > >> > > >
> >> > >>> > >> > > > I would like to refactor CitrixResourceBase
class which
> >> is
> >> > >>> > >> responsible
> >> > >>> > >> > > for
> >> > >>> > >> > > > communicating with Xenserver.
It has grown too long
> (>5K
> >> > >>> lines)
> >> > >>> > and
> >> > >>> > >> has
> >> > >>> > >> > > > absolutely no testing.
> >> > >>> > >> > > >
> >> > >>> > >> > > > In my first pass I want to separate
out the
> functionality
> >> > buy
> >> > >>> the
> >> > >>> > >> > > subsystem
> >> > >>> > >> > > > it targets (compute, storage,
network etc) and will go
> on
> >> > from
> >> > >>> > >> there.
> >> > >>> > >> > > What
> >> > >>> > >> > > > do you think? Is anyone working
on this currently?
> >> > >>> > >> > > >
> >> > >>> > >> > > > Thanks,
> >> > >>> > >> > > > -Syed
> >> > >>> > >> > > >
> >> > >>> > >> > >
> >> > >>> > >> >
> >> > >>> > >> >
> >> > >>> > >> >
> >> > >>> > >> > --
> >> > >>> > >> > Daan
> >> > >>> > >> >
> >> > >>> > >>
> >> > >>> > >>
> >> > >>> > >>
> >> > >>> > >> --
> >> > >>> > >> Rafael Weingärtner
> >> > >>> > >>
> >> > >>> > >
> >> > >>> > >
> >> > >>> >
> >> > >>>
> >> > >>>
> >> > >>>
> >> > >>> --
> >> > >>> Rafael Weingärtner
> >> > >>>
> >> > >>
> >> > >>
> >> > >
> >> >
> >>
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message