[Engine-devel] Disk BE very small refactoring

Sun Jun 2 09:02:55 UTC 2013


----- Original Message -----
> From: "Eli Mesika" <emesika at redhat.com>
> To: "Vered Volansky" <vered at redhat.com>
> Cc: engine-devel at ovirt.org
> Sent: Thursday, May 30, 2013 12:57:26 PM
> Subject: Re: [Engine-devel] Disk BE very small refactoring
> 
> 
> 
> ----- Original Message -----
> > From: "Omer Frenkel" <ofrenkel at redhat.com>
> > To: "Vered Volansky" <vered at redhat.com>
> > Cc: engine-devel at ovirt.org
> > Sent: Thursday, May 30, 2013 11:29:40 AM
> > Subject: Re: [Engine-devel] Disk BE very small refactoring
> > 
> > 
> > 
> > ----- Original Message -----
> > > From: "Vered Volansky" <vered at redhat.com>
> > > To: "Maor Lipchuk" <mlipchuk at redhat.com>, "Yair Zaslavsky"
> > > <yzaslavs at redhat.com>, "Omer Frenkel"
> > > <ofrenkel at redhat.com>, "Mike Kolesnik" <mkolesni at redhat.com>, "Allon
> > > Mureinik" <amureini at redhat.com>, "Daniel Erez"
> > > <derez at redhat.com>
> > > Cc: engine-devel at ovirt.org
> > > Sent: Tuesday, May 28, 2013 8:40:38 PM
> > > Subject: Re: [Engine-devel] Disk BE very small refactoring
> > > 
> > > I had a problem, didn't see all the replies till now.
> > > 
> > > I'll add some more info as to why we want to do this like we do.
> > > It all started from adding the readOnly property to disks. It should have
> > > been handled like plugged is handled, yet plugged is a hack, and if we
> > > don't
> > > change that now, we'll only keep on adding unreasonable hacks.
> > > 
> > 
> > please explain why you think plugged is a hack? what is wrong with it?
> 
> You have is_readonly and is_plugged in the device level , this is far from
> being a hack and managed in the correct place since both may apply to
> multiple device types (disk, nics ....)
There's no problem with device level, only I can't use VmDevice (which is where this naturally belongs) in UI since it will cause a major change we don't want right now.
Not only that, VmDevice does hold this info, but not other relevant info used in UI and available in Disk, so using VmDevice on it's own is not only to extensive a change, but also by far will not end there.

> 
> > 
> > > So from the start -
> > > Disk currently:
> > > - Sometimes represents a disk in a vm context and sometimes not.
> > > - Holds plugged property, which is only relevant when a disk is in a vm
> > > context, which already suggests this is not the natural place for it.
> > > - Also holds bootable and interface, which cause limitations of use, but
> > > are
> > > not so obviously related to the relationship between Disk and Vm as
> > > plugged.
> > > - Can be shared between several vm's, to some plugged and to some not
> > > plugged.
> > > - Will soon be optionally RO in one VM and RW in another, which is
> > > exactly
> > > the same as plugged, and therefore plugged issue should be fixed first.
> > > 
> > > Every column in that shows a disk in the UI receives a Disk entity, and
> > > show
> > > its contents, while plugged/unplugged is ignored when not in a VM
> > > context.
> > > The way things are now, using a VmDevice in the where we need it to show
> > > plug
> > > status, we'll also have to use it in all other columns, which is
> > > irrelevant
> > > and just totally not related.
> > > So using VmDevice in UI is a no go.
> > > 
> > > The UI is the main limitation forcing us to use something that extends
> > > Disk,
> > > and what I described below is the easiest thing to implement in the time
> > > restrictions we have without changing the entire system.
> > > 
> > > I think this answers all the questions not already answered by others.
> > > Regarding Maor suggestion - might be a good idea, but not in this scope
> > > or
> > > time-frame.
> > > 
> > > If there is any other/unanswered issue or objection to the design change
> > > please share.
> > > 
> > > I appreciate your inputs,
> > > Vered
> > > 
> > 
> > sorry i didnt understand why the current disk object isnt good enough,
> > you get a disk, some of its properties are valid only in some situations.
> > i think its easier to use instead of couple of different objects
> > representing
> > the same entity in different situations.
> 
> I totally agree with Omer, please specify what is impossible or hard to
> achieve with the current schema
No problem with DB schema, please see my answer in engine-devel, I'm not sure you're directly cc'ed.

> 
> > 
> > > 
> > > ----- Original Message -----
> > > > From: "Maor Lipchuk" <mlipchuk at redhat.com>
> > > > To: "Vered Volansky" <vered at redhat.com>
> > > > Cc: engine-devel at ovirt.org
> > > > Sent: Tuesday, May 28, 2013 1:49:46 PM
> > > > Subject: Re: [Engine-devel] Disk BE very small refactoring
> > > > 
> > > > I think the main problem is that we abuse the business entity to act as
> > > > an O/R mapping class to the DB and also to be used as a business entity
> > > > for presentation purposes.
> > > > 
> > > > I understand how Yair thought isPlugged could be fetched from vmDevice,
> > > > this is a confusing design, and it is just one example and more to
> > > > come.
> > > > 
> > > > I suggest that if we already thinking of changing the class hierarchy,
> > > > we can start by implementing a package for presentation classes for
> > > > transient classes such as this instead enforcing complex hierarchy.
> > > > 
> > > > The query class will fetch all the data from the DB and initialized the
> > > > transient class and send it to the client.
> > > > I think it could be a good start and will solve many issues we might
> > > > encounter in the future.
> > > > 
> > > > Regards,
> > > > Maor
> > > > 
> > > > On 05/28/2013 11:24 AM, Omer Frenkel wrote:
> > > > > 
> > > > > 
> > > > > ----- Original Message -----
> > > > >> From: "Yair Zaslavsky" <yzaslavs at redhat.com>
> > > > >> To: "Vered Volansky" <vered at redhat.com>
> > > > >> Cc: engine-devel at ovirt.org
> > > > >> Sent: Monday, May 27, 2013 6:22:58 PM
> > > > >> Subject: Re: [Engine-devel] Disk BE very small refactoring
> > > > >>
> > > > >> Vered,
> > > > >> VmDevice has "isPlugged" field,
> > > > >> Why not have somehow in your inheritence (either Disk or a subclass)
> > > > >> a
> > > > >> field
> > > > >> : "VmDevice device"
> > > > > 
> > > > > disk id is the device id, no need for field to represent the
> > > > > relation.
> > > > > the combination of disk-id and a specific entity (vm/template) will
> > > > > get
> > > > > you
> > > > > the other info
> > > > > 
> > > > >> and have isPlugged method called "device.isPlugged()" ?
> > > > >>
> > > > >> Then you can also add the readOnly property which is not represented
> > > > >> at
> > > > >> VmDevice.
> > > > >>
> > > > >>
> > > > >> Does this sound logical to you?
> > > > >>
> > > > >> ----- Original Message -----
> > > > >>> From: "Vered Volansky" <vered at redhat.com>
> > > > >>> To: engine-devel at ovirt.org
> > > > >>> Sent: Monday, May 27, 2013 6:18:58 PM
> > > > >>> Subject: [Engine-devel] Disk BE very small refactoring
> > > > >>>
> > > > >>> Hi All,
> > > > >>>
> > > > >>> Please express your opinion if you have any -
> > > > >>>
> > > > >>> Currently Disk BE has a plugged property, which should be a
> > > > >>> property
> > > > >>> of
> > > > >>> the
> > > > >>> relationship between vm(or template) and a disk.
> > > > >>> I plan to remove this property from the Disk entity, and add new
> > > > >>> entity,
> > > > >>> called DeviceDisk.
> > > > >>> This should inherit from Disk and contain the vm/template guid and
> > > > >>> the
> > > > >>> plugged property at first round.
> > > > >>> At second round it'll also contain the readOnly property, for RO
> > > > >>> disks,
> > > > >>> TBD
> > > > >>> right after.
> > > > >>>
> > > > >>> Appreciate any input,
> > > > >>> Vered
> > > > >>> _______________________________________________
> > > > >>> Engine-devel mailing list
> > > > >>> Engine-devel at ovirt.org
> > > > >>> http://lists.ovirt.org/mailman/listinfo/engine-devel
> > > > >>>
> > > > >> _______________________________________________
> > > > >> Engine-devel mailing list
> > > > >> Engine-devel at ovirt.org
> > > > >> http://lists.ovirt.org/mailman/listinfo/engine-devel
> > > > >>
> > > > > _______________________________________________
> > > > > Engine-devel mailing list
> > > > > Engine-devel at ovirt.org
> > > > > http://lists.ovirt.org/mailman/listinfo/engine-devel
> > > > > 
> > > > 
> > > > 
> > > > _______________________________________________
> > > > Engine-devel mailing list
> > > > Engine-devel at ovirt.org
> > > > http://lists.ovirt.org/mailman/listinfo/engine-devel
> > > > 
> > > 
> > _______________________________________________
> > Engine-devel mailing list
> > Engine-devel at ovirt.org
> > http://lists.ovirt.org/mailman/listinfo/engine-devel
> > 
> _______________________________________________
> Engine-devel mailing list
> Engine-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/engine-devel
>