Re: [Engine-devel] Disk BE very small refactoring

From: "Omer Frenkel" <ofrenkel(a)redhat.com&gt; To: "Vered Volansky" <vered(a)redhat.com&gt; Cc: engine-devel(a)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(a)redhat.com&gt; > To: "Maor Lipchuk" <mlipchuk(a)redhat.com&gt;, "Yair Zaslavsky" > <yzaslavs(a)redhat.com&gt;, "Omer Frenkel" > <ofrenkel(a)redhat.com&gt;, "Mike Kolesnik" <mkolesni(a)redhat.com&gt;, "Allon > Mureinik" <amureini(a)redhat.com&gt;, "Daniel Erez" > <derez(a)redhat.com&gt; > Cc: engine-devel(a)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(a)redhat.com&gt; > > To: "Vered Volansky" <vered(a)redhat.com&gt; > > Cc: engine-devel(a)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(a)redhat.com&gt; > > >> To: "Vered Volansky" <vered(a)redhat.com&gt; > > >> Cc: engine-devel(a)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(a)redhat.com&gt; > > >>> To: engine-devel(a)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(a)ovirt.org > > >>> http://lists.ovirt.org/mailman/listinfo/engine-devel > > >>> > > >> _______________________________________________ > > >> Engine-devel mailing list > > >> Engine-devel(a)ovirt.org > > >> http://lists.ovirt.org/mailman/listinfo/engine-devel > > >> > > > _______________________________________________ > > > Engine-devel mailing list > > > Engine-devel(a)ovirt.org > > > http://lists.ovirt.org/mailman/listinfo/engine-devel > > > > > > > > > _______________________________________________ > > Engine-devel mailing list > > Engine-devel(a)ovirt.org > > http://lists.ovirt.org/mailman/listinfo/engine-devel > > > _______________________________________________ Engine-devel mailing list Engine-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel