[Engine-devel] Disk BE very small refactoring

Thu May 30 08:29:40 UTC 2013

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

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

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