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

From: "Eli Mesika" <emesika(a)redhat.com&gt; To: "Vered Volansky" <vered(a)redhat.com&gt; Cc: engine-devel(a)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(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? 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(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 > _______________________________________________ Engine-devel mailing list Engine-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel