[Engine-devel] Disk BE very small refactoring

Thu May 30 09:57:26 UTC 2013


----- 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 ....)

> 
> > 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 

> 
> > 
> > ----- 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
>