Re: [Engine-devel] Got some troubles when I want to modify oVirt GUI

2013-03-12 Thread Chen, Wei D
Thanks Doron & Vojetch & Itamar, we will update our design wiki page ASAP, and 
also, REST API is in pipeline.

Best Regards,
Dave Chen


> -Original Message-
> From: Doron Fediuck [mailto:dfedi...@redhat.com]
> Sent: Tuesday, March 12, 2013 10:13 PM
> To: Chen, Wei D
> Cc: engine-devel@ovirt.org; Vojtech Szocs; Itamar Heim
> Subject: Re: [Engine-devel] Got some troubles when I want to modify oVirt GUI
> 
> 
> 
> - Original Message -
> > From: "Itamar Heim" 
> > To: "Vojtech Szocs" 
> > Cc: engine-devel@ovirt.org
> > Sent: Tuesday, March 12, 2013 3:30:57 PM
> > Subject: Re: [Engine-devel] Got some troubles when I want to modify
> > oVirt GUI
> >
> > On 03/12/2013 12:57 PM, Vojtech Szocs wrote:
> > > Hi,
> > >
> > > first of all, did you consider submitting this as RFE in oVirt
> > > bugzilla? Maybe it could be useful to have it in oVirt.
> > > (Implementing this via UI plugin would be far too complicated, as UI
> > > code is tightly coupled with UiCommon code in case of VM dialog.)>
> >
> > my understanding this is a full blown feature, not a plugin.
> >
> > Wei D - please note UI is to help the user to not make mistakes, but
> > validations must also happen at engine side to cover rest api, etc.
> >
> 
> +1.
> Also, a detailed wiki page may help both developer and readers to get a better
> understanding of how it should look and work.
> For example, how do you plan to implement the REST API changes?
> 
> Wei D, I know that Gang Wei started:
> http://www.ovirt.org/Trusted_compute_pools But as you can see, it does not
> mention your original intention to implement the UI as a plugin, while as you
> can see it would be better to have it as an internal UI addition. Also, as
> mentioned REST parts are missing and a more detailed design for the engine is
> missing here.
> 
> Wei D, is there a reason why not top have a detailed page similar
> to: http://www.ovirt.org/Features/Watchdog_engine_support ?
> 
> >
> > > Regarding UI code changes, the general idea is to implement business
> > > logic in UiCommon models (VmListModel, UnitVmModel, etc.) and have
> > > UI code bind to these models. It would be best if you just send a
> > > patch (diff) instead of specific files, it's really hard to see what
> > > changes you made, but based on the files you sent, here are my
> > > comments:
> > >
> > > * Changes in UnitVmModel look good, you basically added two new
> > > fields [privateRunVMOnSpecificHost, privateRunVMOnTrustedHost],
> > > hooked up their *_EntityChanged methods, and implemented logic for
> > > handling field value changes in these methods
> > >
> > > * Changes in VmListModel look good, you used newly added UnitVmModel
> > > fields in onSave [I assume setTrustedHostFlag/setDedicatedVmForVds
> > > are new fields for VM entity?], note - you might also want to update
> > > UpdateActionAvailability disable migrating VM when
> > > RunVMOnTrustedHost=true, etc.
> > >
> > > * AbstractVmPopupWidget already has specificHost radio button &
> > > drop-down on Host dialog tab, and I assume you want to reuse the
> > > drop-down (host list) for trustedHost, so just add new radio button
> > > there:
> > >
> > >AbstractVmPopupWidget.ui.xml line 331
> > >
> > >
> > >   > >  ui:field="specificHost"
> > >  addStyleNames="{style.radioButtonSpecificHost}" />
> > >   > >  ui:field="trustedHost"
> > >  addStyleNames="{style.radioButtonSpecificHost}" />
> > >   > >  text="{constants.specificVmPopup}" />
> > >  
> > >
> > >
> > > * In AbstractVmPopupWidget you to bind newly added RadioButton:
> > >
> > >@UiField(provided = true)
> > >@Ignore
> > >@WithElementId("trustedHost")
> > >public RadioButton trustedHost;
> > >
> > >You create trustedHost widget in constructor, and in
> > >initTabAvailabilityListeners you just add
> > >trustedHost.addValueChangeHandler(...) to have logic when
> > >trustedHost gets selected.
> > >
> > > Regards,
> > > Vojtech
> > >
> > >
> > > - Original Message -
> > > From: "Wei D Chen" 
> > > To: engine-devel@ovirt.org
> > > Sent: Tuesday, March 12, 2013 9:48:34 AM
> > > Subject: [Engine-devel] Got some troubles when I want to modify
> > > oVirt GUI
> > >
> > > Hi,
> > > In order to add new feature to Ovirt, that is user can choose
> > > virtual machine whether on trusted machine or not when it runs
> > > up, we modified the relative files.
> > > Our goal is when the user click the trusted button, Run/Migration
> > > options are disabled. But unfortunately, we haven’t succeeded in
> > > graphic interface.
> > > I modified these files, I can’t see Host Tab, can you give me some
> > > help? Maybe we need modify more files. We did the following
> > > efforts:
> > > (1) add a trusted radio button.
> > > (2) Modify AbstractVmPopupWidget.ui.xml  > > verticalAlignment='ALIGN_MIDDLE'>
> > >   
> > >> >   ui:field="runVMOnTrustedHostEditor"
> > >addStyleNames="{st

Re: [Engine-devel] VmDynamic kvm_enable

2013-03-12 Thread Itamar Heim

On 03/12/2013 05:07 PM, Dan Kenigsberg wrote:

On Tue, Mar 12, 2013 at 10:37:41AM -0400, Laszlo Hornyak wrote:

Hi,

I came across a VmDynamic property 'kvm_enable'. It sounds strange for me, 
because ovirt is very colsely integrated with kvm. So a short dig into this 
flag...
There is a similar thing for the VDS, it is set to true by vdsm is the host CPU 
has a VT flag. It is actually used to check if the host is OK to run VMS.

But the one for Vm it looks like a distributed logical loop in vdsm: it is set 
when constructing a VM object (vdsm/vm.py:~343) from the data sent by the 
client (engine) and then reported back in vm stats, so it is just a roundtrip 
between vdsm and it's client.
In the engine side, it is just keeps sending it between the frontend DB and 
vdsm, never part of a decision.

Is this still needed here? Can I remove?
VDSM guys?


I have a vague memeory that once upon at time, qemu occasionally failed
to enable kvm support - even though it was asked to. I then silently
switched to emulated mode, which was grindingly slow. Engine wanted to
know about such occasions.

I believe that a better technical approach would have been to kill the
violating process, and not let it run at all (unless qemu emulation was
strictly requested by management).

Anyway, as you have noted, this has rotten away throuh the years, and
unless older Engine versions are expecting this value in any way, I am
all for dropping it.


I think this was about something else - the kvmEnable flag was used to 
launch installations of guests not correctly supported by kvm that 
required real mode or something like that.
hopefully not relevant any more, but need to validate won't break older 
engines indeed.


___
Engine-devel mailing list
Engine-devel@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-devel


[Engine-devel] Some notes on UI composition

2013-03-12 Thread Vojtech Szocs
Hi guys,

I've encountered an interesting pattern while working on a recent patch: some 
View classes actually embed other View classes, and add those embedded Views to 
the (top-level) View.

For example, ImportVmPopupView has a field "ImportVmGeneralSubTabView 
generalView", which is created as part of constructor and added to 
ImportVmPopupView's layout.

The bottom line is, please don't use this pattern. Views are architectural 
components in Model-View-Presenter which are living in a particular scope (e.g. 
singleton/non-singleton), so UI composition should be always done on Widget 
level, instead of View level.

In other words, ImportVmPopupView shouldn't embed ImportVmGeneralSubTabView, 
but instead embed UI (Widgets) similar to ImportVmGeneralSubTabView. In case 
embedded View's UI is complex, we can just create a reusable widget that would 
be used in both ImportVmPopupView and ImportVmGeneralSubTabView.

Otherwise, we could run into problems, especially since ImportVmPopupView is 
dialog-specific (non-singleton) and ImportVmGeneralSubTabView is 
sub-tab-specific (singleton). In Model-View-Presenter, View != immediate 
Widget, View == MVP component wrapping UI (Widget) and bound to a specific 
scope, so that's the reason why UI composition should be always done on Widget 
level (on the other hand, subclassing Views is perfectly OK).

Sorry for being police man here .. it's just a note to keep in mind for future 
:)

Regards,
Vojtech
___
Engine-devel mailing list
Engine-devel@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-devel


Re: [Engine-devel] VmDynamic kvm_enable

2013-03-12 Thread Dan Kenigsberg
On Tue, Mar 12, 2013 at 10:37:41AM -0400, Laszlo Hornyak wrote:
> Hi,
> 
> I came across a VmDynamic property 'kvm_enable'. It sounds strange for me, 
> because ovirt is very colsely integrated with kvm. So a short dig into this 
> flag...
> There is a similar thing for the VDS, it is set to true by vdsm is the host 
> CPU has a VT flag. It is actually used to check if the host is OK to run VMS.
> 
> But the one for Vm it looks like a distributed logical loop in vdsm: it is 
> set when constructing a VM object (vdsm/vm.py:~343) from the data sent by the 
> client (engine) and then reported back in vm stats, so it is just a roundtrip 
> between vdsm and it's client.
> In the engine side, it is just keeps sending it between the frontend DB and 
> vdsm, never part of a decision.
> 
> Is this still needed here? Can I remove?
> VDSM guys?

I have a vague memeory that once upon at time, qemu occasionally failed
to enable kvm support - even though it was asked to. I then silently
switched to emulated mode, which was grindingly slow. Engine wanted to
know about such occasions.

I believe that a better technical approach would have been to kill the
violating process, and not let it run at all (unless qemu emulation was
strictly requested by management).

Anyway, as you have noted, this has rotten away throuh the years, and
unless older Engine versions are expecting this value in any way, I am
all for dropping it.

Dan.
___
Engine-devel mailing list
Engine-devel@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-devel


[Engine-devel] VmDynamic kvm_enable

2013-03-12 Thread Laszlo Hornyak
Hi,

I came across a VmDynamic property 'kvm_enable'. It sounds strange for me, 
because ovirt is very colsely integrated with kvm. So a short dig into this 
flag...
There is a similar thing for the VDS, it is set to true by vdsm is the host CPU 
has a VT flag. It is actually used to check if the host is OK to run VMS.

But the one for Vm it looks like a distributed logical loop in vdsm: it is set 
when constructing a VM object (vdsm/vm.py:~343) from the data sent by the 
client (engine) and then reported back in vm stats, so it is just a roundtrip 
between vdsm and it's client.
In the engine side, it is just keeps sending it between the frontend DB and 
vdsm, never part of a decision.

Is this still needed here? Can I remove?
VDSM guys?

Thx,
Laszlo
___
Engine-devel mailing list
Engine-devel@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-devel


Re: [Engine-devel] Got some troubles when I want to modify oVirt GUI

2013-03-12 Thread Doron Fediuck


- Original Message -
> From: "Itamar Heim" 
> To: "Vojtech Szocs" 
> Cc: engine-devel@ovirt.org
> Sent: Tuesday, March 12, 2013 3:30:57 PM
> Subject: Re: [Engine-devel] Got some troubles when I want to modify oVirt GUI
> 
> On 03/12/2013 12:57 PM, Vojtech Szocs wrote:
> > Hi,
> >
> > first of all, did you consider submitting this as RFE in oVirt
> > bugzilla? Maybe it could be useful to have it in oVirt.
> > (Implementing this via UI plugin would be far too complicated, as
> > UI code is tightly coupled with UiCommon code in case of VM
> > dialog.)>
> 
> my understanding this is a full blown feature, not a plugin.
> 
> Wei D - please note UI is to help the user to not make mistakes, but
> validations must also happen at engine side to cover rest api, etc.
> 

+1.
Also, a detailed wiki page may help both developer and readers
to get a better understanding of how it should look and work.
For example, how do you plan to implement the REST API changes?

Wei D, I know that Gang Wei started:
http://www.ovirt.org/Trusted_compute_pools But as you can see, 
it does not mention your original intention to implement the UI
as a plugin, while as you can see it would be better to have it
as an internal UI addition. Also, as mentioned REST parts are
missing and a more detailed design for the engine is missing here.

Wei D, is there a reason why not top have a detailed page similar
to: http://www.ovirt.org/Features/Watchdog_engine_support ?

> 
> > Regarding UI code changes, the general idea is to implement
> > business logic in UiCommon models (VmListModel, UnitVmModel, etc.)
> > and have UI code bind to these models. It would be best if you
> > just send a patch (diff) instead of specific files, it's really
> > hard to see what changes you made, but based on the files you
> > sent, here are my comments:
> >
> > * Changes in UnitVmModel look good, you basically added two new
> > fields [privateRunVMOnSpecificHost, privateRunVMOnTrustedHost],
> > hooked up their *_EntityChanged methods, and implemented logic for
> > handling field value changes in these methods
> >
> > * Changes in VmListModel look good, you used newly added
> > UnitVmModel fields in onSave [I assume
> > setTrustedHostFlag/setDedicatedVmForVds are new fields for VM
> > entity?], note - you might also want to update
> > UpdateActionAvailability disable migrating VM when
> > RunVMOnTrustedHost=true, etc.
> >
> > * AbstractVmPopupWidget already has specificHost radio button &
> > drop-down on Host dialog tab, and I assume you want to reuse the
> > drop-down (host list) for trustedHost, so just add new radio
> > button there:
> >
> >AbstractVmPopupWidget.ui.xml line 331
> >
> >
> >   >  ui:field="specificHost"
> >  addStyleNames="{style.radioButtonSpecificHost}" />
> >   >  ui:field="trustedHost"
> >  addStyleNames="{style.radioButtonSpecificHost}" />
> >   >  text="{constants.specificVmPopup}" />
> >  
> >
> >
> > * In AbstractVmPopupWidget you to bind newly added RadioButton:
> >
> >@UiField(provided = true)
> >@Ignore
> >@WithElementId("trustedHost")
> >public RadioButton trustedHost;
> >
> >You create trustedHost widget in constructor, and in
> >initTabAvailabilityListeners you just add
> >trustedHost.addValueChangeHandler(...) to have logic when
> >trustedHost gets selected.
> >
> > Regards,
> > Vojtech
> >
> >
> > - Original Message -
> > From: "Wei D Chen" 
> > To: engine-devel@ovirt.org
> > Sent: Tuesday, March 12, 2013 9:48:34 AM
> > Subject: [Engine-devel] Got some troubles when I want to modify
> > oVirt GUI
> >
> > Hi,
> > In order to add new feature to Ovirt, that is user can choose
> > virtual machine whether on trusted machine or not when it runs
> > up, we modified the relative files.
> > Our goal is when the user click the trusted button, Run/Migration
> > options are disabled. But unfortunately, we haven’t succeeded in
> > graphic interface.
> > I modified these files, I can’t see Host Tab, can you give me some
> > help? Maybe we need modify more files. We did the following
> > efforts:
> > (1) add a trusted radio button.
> > (2) Modify AbstractVmPopupWidget.ui.xml
> > 
> >   
> >>   ui:field="runVMOnTrustedHostEditor"
> >addStyleNames="{style.radioButton}" />
> > 
> > (3) Modify AbstractVmPopupWidget.java
> > @UiField(provided = true)
> > @Path(value = "runVMOnTrustedHost.entity")
> >  @WithElementId("runVMOnTrustedHost")
> > public EntityModelRadioButtonEditor runVMOnTrustedHostEditor;
> >
> > initListeners method:
> > object.getIsAutoAssign().getPropertyChangedEvent().addListener(new
> > IEventListener() {
> >  @Override
> >  public void eventRaised(Event ev, Object sender,
> >  EventArgs args) {
> >  boolean isAutoAssign = (Boolean)
> >  object.getIsAutoAssign().getEntity();
> >  boolean runVMOnTrustedHost =

Re: [Engine-devel] new engine watchdog version

2013-03-12 Thread Laszlo Hornyak


- Original Message -
> From: "Omer Frenkel" 
> To: "Laszlo Hornyak" 
> Cc: "engine-devel" 
> Sent: Tuesday, March 12, 2013 2:41:38 PM
> Subject: Re: [Engine-devel] new engine watchdog version
> 
> 
> 
> - Original Message -
> > From: "Laszlo Hornyak" 
> > To: "Omer Frenkel" 
> > Cc: "engine-devel" 
> > Sent: Monday, March 11, 2013 6:46:06 PM
> > Subject: Re: [Engine-devel] new engine watchdog version
> > 
> > 
> > 
> > - Original Message -
> > > From: "Omer Frenkel" 
> > > To: "Laszlo Hornyak" 
> > > Cc: "engine-devel" 
> > > Sent: Monday, March 11, 2013 1:25:39 PM
> > > Subject: Re: [Engine-devel] new engine watchdog version
> > > 
> > > 
> > > 
> > > - Original Message -
> > > > From: "Laszlo Hornyak" 
> > > > To: "Omer Frenkel" 
> > > > Cc: "engine-devel" 
> > > > Sent: Monday, March 11, 2013 12:15:39 PM
> > > > Subject: Re: [Engine-devel] new engine watchdog version
> > > > 
> > > > 
> > > > 
> > > > - Original Message -
> > > > > From: "Omer Frenkel" 
> > > > > To: "Laszlo Hornyak" 
> > > > > Cc: "engine-devel" 
> > > > > Sent: Monday, March 11, 2013 11:12:48 AM
> > > > > Subject: Re: [Engine-devel] new engine watchdog version
> > > > > 
> > > > > 
> > > > > 
> > > > > - Original Message -
> > > > > > From: "Laszlo Hornyak" 
> > > > > > To: "Omer Frenkel" 
> > > > > > Cc: "engine-devel" 
> > > > > > Sent: Monday, March 11, 2013 9:59:53 AM
> > > > > > Subject: Re: [Engine-devel] new engine watchdog version
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > - Original Message -
> > > > > > > From: "Omer Frenkel" 
> > > > > > > To: "Laszlo Hornyak" 
> > > > > > > Cc: "engine-devel" 
> > > > > > > Sent: Sunday, March 10, 2013 8:36:46 AM
> > > > > > > Subject: Re: [Engine-devel] new engine watchdog version
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > - Original Message -
> > > > > > > > From: "Laszlo Hornyak" 
> > > > > > > > To: "engine-devel" 
> > > > > > > > Sent: Friday, March 8, 2013 7:18:59 PM
> > > > > > > > Subject: [Engine-devel] new engine watchdog version
> > > > > > > > 
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > I uploaded a new version of the watchdog patch. This
> > > > > > > > patch
> > > > > > > > is
> > > > > > > > still
> > > > > > > > a
> > > > > > > > work in progress, it adds audit log alerts to the
> > > > > > > > functionality.
> > > > > > > > http://gerrit.ovirt.org/12419/
> > > > > > > > 
> > > > > > > > Feature page:
> > > > > > > > http://www.ovirt.org/Features/Watchdog_engine_support
> > > > > > > > 
> > > > > > > > Laszlo
> > > > > > > > ___
> > > > > > > > Engine-devel mailing list
> > > > > > > > Engine-devel@ovirt.org
> > > > > > > > http://lists.ovirt.org/mailman/listinfo/engine-devel
> > > > > > > > 
> > > > > > > 
> > > > > > > Hi,
> > > > > > > i looked at the patch and there is something i don't
> > > > > > > understand,
> > > > > > > i see you are treating the watchdog as a vm device, which
> > > > > > > is
> > > > > > > great,
> > > > > > > so why do we need to save the device details in vm_static
> > > > > > > table
> > > > > > > in
> > > > > > > addition to the vm_devices?
> > > > > > > i think its even not used at all (only setting the device
> > > > > > > in
> > > > > > > command
> > > > > > > which could be parameters, no need to persist)
> > > > > > > 
> > > > > > 
> > > > > > Hi Omer,
> > > > > > 
> > > > > > Thanks, I hoped someone will come up with that question :)
> > > > > > The
> > > > > > answer
> > > > > > is that I followed the established design patterns in the
> > > > > > backend.
> > > > > > See smartcard and memory balloon, probably others. The
> > > > > > motivation
> > > > > > for this pattern could be that in case of these devices,
> > > > > > you
> > > > > > must
> > > > > > have the settings in the VM data, not separately in the
> > > > > > devices.
> > > > > > Also when vdsbroker builds the devices list, it just asks
> > > > > > the
> > > > > > device
> > > > > > list. The redundancy is already there, we can make it
> > > > > > differently
> > > > > > in
> > > > > > this case but that will present the readers with a puzzle:
> > > > > > why
> > > > > > this
> > > > > > pattern in feature X, why that pattern in feature Y...
> > > > > > So I would recommend to leave it like this for now and
> > > > > > schedule
> > > > > > a
> > > > > > cleanup on device handling. Devices deserve a cleanup.
> > > > > > 
> > > > > > Thx,
> > > > > > Laszlo
> > > > > > 
> > > > > 
> > > > > i agree there is a mess that requires clean-up,
> > > > > but i don't think its a good thing to keep piling up the
> > > > > mess,
> > > > > i don't like it that smartcard is there, but some other
> > > > > devices
> > > > > are
> > > > > ok (balloon and payload)
> > > > > so we already have 2 'patterns', lets go with the right one..
> > > > > and answering also @Doron's question - yes the device data
> > > > > should
> > > > > be
> > > > > kept 

Re: [Engine-devel] new engine watchdog version

2013-03-12 Thread Omer Frenkel


- Original Message -
> From: "Laszlo Hornyak" 
> To: "Omer Frenkel" 
> Cc: "engine-devel" 
> Sent: Monday, March 11, 2013 6:46:06 PM
> Subject: Re: [Engine-devel] new engine watchdog version
> 
> 
> 
> - Original Message -
> > From: "Omer Frenkel" 
> > To: "Laszlo Hornyak" 
> > Cc: "engine-devel" 
> > Sent: Monday, March 11, 2013 1:25:39 PM
> > Subject: Re: [Engine-devel] new engine watchdog version
> > 
> > 
> > 
> > - Original Message -
> > > From: "Laszlo Hornyak" 
> > > To: "Omer Frenkel" 
> > > Cc: "engine-devel" 
> > > Sent: Monday, March 11, 2013 12:15:39 PM
> > > Subject: Re: [Engine-devel] new engine watchdog version
> > > 
> > > 
> > > 
> > > - Original Message -
> > > > From: "Omer Frenkel" 
> > > > To: "Laszlo Hornyak" 
> > > > Cc: "engine-devel" 
> > > > Sent: Monday, March 11, 2013 11:12:48 AM
> > > > Subject: Re: [Engine-devel] new engine watchdog version
> > > > 
> > > > 
> > > > 
> > > > - Original Message -
> > > > > From: "Laszlo Hornyak" 
> > > > > To: "Omer Frenkel" 
> > > > > Cc: "engine-devel" 
> > > > > Sent: Monday, March 11, 2013 9:59:53 AM
> > > > > Subject: Re: [Engine-devel] new engine watchdog version
> > > > > 
> > > > > 
> > > > > 
> > > > > - Original Message -
> > > > > > From: "Omer Frenkel" 
> > > > > > To: "Laszlo Hornyak" 
> > > > > > Cc: "engine-devel" 
> > > > > > Sent: Sunday, March 10, 2013 8:36:46 AM
> > > > > > Subject: Re: [Engine-devel] new engine watchdog version
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > - Original Message -
> > > > > > > From: "Laszlo Hornyak" 
> > > > > > > To: "engine-devel" 
> > > > > > > Sent: Friday, March 8, 2013 7:18:59 PM
> > > > > > > Subject: [Engine-devel] new engine watchdog version
> > > > > > > 
> > > > > > > Hi,
> > > > > > > 
> > > > > > > I uploaded a new version of the watchdog patch. This
> > > > > > > patch
> > > > > > > is
> > > > > > > still
> > > > > > > a
> > > > > > > work in progress, it adds audit log alerts to the
> > > > > > > functionality.
> > > > > > > http://gerrit.ovirt.org/12419/
> > > > > > > 
> > > > > > > Feature page:
> > > > > > > http://www.ovirt.org/Features/Watchdog_engine_support
> > > > > > > 
> > > > > > > Laszlo
> > > > > > > ___
> > > > > > > Engine-devel mailing list
> > > > > > > Engine-devel@ovirt.org
> > > > > > > http://lists.ovirt.org/mailman/listinfo/engine-devel
> > > > > > > 
> > > > > > 
> > > > > > Hi,
> > > > > > i looked at the patch and there is something i don't
> > > > > > understand,
> > > > > > i see you are treating the watchdog as a vm device, which
> > > > > > is
> > > > > > great,
> > > > > > so why do we need to save the device details in vm_static
> > > > > > table
> > > > > > in
> > > > > > addition to the vm_devices?
> > > > > > i think its even not used at all (only setting the device
> > > > > > in
> > > > > > command
> > > > > > which could be parameters, no need to persist)
> > > > > > 
> > > > > 
> > > > > Hi Omer,
> > > > > 
> > > > > Thanks, I hoped someone will come up with that question :)
> > > > > The
> > > > > answer
> > > > > is that I followed the established design patterns in the
> > > > > backend.
> > > > > See smartcard and memory balloon, probably others. The
> > > > > motivation
> > > > > for this pattern could be that in case of these devices, you
> > > > > must
> > > > > have the settings in the VM data, not separately in the
> > > > > devices.
> > > > > Also when vdsbroker builds the devices list, it just asks the
> > > > > device
> > > > > list. The redundancy is already there, we can make it
> > > > > differently
> > > > > in
> > > > > this case but that will present the readers with a puzzle:
> > > > > why
> > > > > this
> > > > > pattern in feature X, why that pattern in feature Y...
> > > > > So I would recommend to leave it like this for now and
> > > > > schedule
> > > > > a
> > > > > cleanup on device handling. Devices deserve a cleanup.
> > > > > 
> > > > > Thx,
> > > > > Laszlo
> > > > > 
> > > > 
> > > > i agree there is a mess that requires clean-up,
> > > > but i don't think its a good thing to keep piling up the mess,
> > > > i don't like it that smartcard is there, but some other devices
> > > > are
> > > > ok (balloon and payload)
> > > > so we already have 2 'patterns', lets go with the right one..
> > > > and answering also @Doron's question - yes the device data
> > > > should
> > > > be
> > > > kept with the device
> > > > 
> > > 
> > > Ok, I may have missed the other pattern, could you explain which
> > > one
> > > do you mean?
> > > Balloon does not very different from smartcard, it is there in
> > > VM.
> > > 
> > 
> > the difference is that balloon is not in vm_static table at all
> > (the
> > only place in the db for it is in vm_devices)
> > and smartcard has 'is_smartcard_enabled' field in vm_static in
> > addition to vm_devices (which is not needed..)
> 
> Ok, so what you want is that
> - the engine should query

Re: [Engine-devel] Got some troubles when I want to modify oVirt GUI

2013-03-12 Thread Itamar Heim

On 03/12/2013 12:57 PM, Vojtech Szocs wrote:

Hi,

first of all, did you consider submitting this as RFE in oVirt bugzilla? Maybe it 
could be useful to have it in oVirt. (Implementing this via UI plugin would be far 
too complicated, as UI code is tightly coupled with UiCommon code in case of VM 
dialog.)>


my understanding this is a full blown feature, not a plugin.

Wei D - please note UI is to help the user to not make mistakes, but 
validations must also happen at engine side to cover rest api, etc.




Regarding UI code changes, the general idea is to implement business logic in 
UiCommon models (VmListModel, UnitVmModel, etc.) and have UI code bind to these 
models. It would be best if you just send a patch (diff) instead of specific 
files, it's really hard to see what changes you made, but based on the files 
you sent, here are my comments:

* Changes in UnitVmModel look good, you basically added two new fields 
[privateRunVMOnSpecificHost, privateRunVMOnTrustedHost], hooked up their 
*_EntityChanged methods, and implemented logic for handling field value changes 
in these methods

* Changes in VmListModel look good, you used newly added UnitVmModel fields in 
onSave [I assume setTrustedHostFlag/setDedicatedVmForVds are new fields for VM 
entity?], note - you might also want to update UpdateActionAvailability disable 
migrating VM when RunVMOnTrustedHost=true, etc.

* AbstractVmPopupWidget already has specificHost radio button & drop-down on 
Host dialog tab, and I assume you want to reuse the drop-down (host list) for 
trustedHost, so just add new radio button there:

   AbstractVmPopupWidget.ui.xml line 331

   
 
 
 
 
   

* In AbstractVmPopupWidget you to bind newly added RadioButton:

   @UiField(provided = true)
   @Ignore
   @WithElementId("trustedHost")
   public RadioButton trustedHost;

   You create trustedHost widget in constructor, and in 
initTabAvailabilityListeners you just add 
trustedHost.addValueChangeHandler(...) to have logic when trustedHost gets 
selected.

Regards,
Vojtech


- Original Message -
From: "Wei D Chen" 
To: engine-devel@ovirt.org
Sent: Tuesday, March 12, 2013 9:48:34 AM
Subject: [Engine-devel] Got some troubles when I want to modify oVirt GUI

Hi,
In order to add new feature to Ovirt, that is user can choose virtual 
machine whether on trusted machine or not when it runs up, we modified the 
relative files.
Our goal is when the user click the trusted button, Run/Migration options are 
disabled. But unfortunately, we haven’t succeeded in graphic interface.
I modified these files, I can’t see Host Tab, can you give me some help? Maybe 
we need modify more files. We did the following efforts:
(1) add a trusted radio button.
(2) Modify AbstractVmPopupWidget.ui.xml

  
  

(3) Modify AbstractVmPopupWidget.java
@UiField(provided = true)
@Path(value = "runVMOnTrustedHost.entity")
 @WithElementId("runVMOnTrustedHost")
public EntityModelRadioButtonEditor runVMOnTrustedHostEditor;

initListeners method:
object.getIsAutoAssign().getPropertyChangedEvent().addListener(new 
IEventListener() {
 @Override
 public void eventRaised(Event ev, Object sender, EventArgs args) {
 boolean isAutoAssign = (Boolean) 
object.getIsAutoAssign().getEntity();
 boolean runVMOnTrustedHost = (Boolean) 
object.getRunVMOnTrustedHost().getEntity();
 defaultHostEditor.setEnabled(!isAutoAssign && 
!runVMOnTrustedHost);
 //defaultHostEditor.setEnabled(!isAutoAssign);
 // only this is not bind to the model, so needs to listen to 
the change explicitly
specificHost.setValue(!isAutoAssign && !runVMOnTrustedHost);
//specificHost.setValue(!isAutoAssign);
 }
 });
isAutoAssignEditor.addDomHandler(new ClickHandler() {
 @Override
 public void onClick(ClickEvent event) {
 defaultHostEditor.setEnabled(false);
 }
 }, ClickEvent.getType());
 vm.getIsAutoAssign().getEntityChangedEvent().addListener(new 
IEventListener() {
 @Override
 public void eventRaised(Event ev, Object sender, EventArgs args) {
 if (!isAutoAssignEditor.asRadioButton().getValue() && 
!runVMOnTrustedHostEditor.asRadioButton().getValue())
 {
 specificHost.setValue(true, true);
 }
 }
 });
 runVMOnTrustedHostEditor.addDomHandler(new ClickHandler() {
 @Override
 public void onClick(ClickEvent event) {
 defaultHostEditor.setEnabled(false);
 }
 }, ClickEvent.getType());
 vm.getRunVMOnTrustedHost().getEntityChangedEvent().addListener(new 
IEventListener() {
 @Override
 public void eventRaised(Event ev, Object sender, EventArgs args) {
 if (!runVMOnTrustedHostEditor.as

Re: [Engine-devel] Got some troubles when I want to modify oVirt GUI

2013-03-12 Thread Vojtech Szocs
Hi,

first of all, did you consider submitting this as RFE in oVirt bugzilla? Maybe 
it could be useful to have it in oVirt. (Implementing this via UI plugin would 
be far too complicated, as UI code is tightly coupled with UiCommon code in 
case of VM dialog.)

Regarding UI code changes, the general idea is to implement business logic in 
UiCommon models (VmListModel, UnitVmModel, etc.) and have UI code bind to these 
models. It would be best if you just send a patch (diff) instead of specific 
files, it's really hard to see what changes you made, but based on the files 
you sent, here are my comments:

* Changes in UnitVmModel look good, you basically added two new fields 
[privateRunVMOnSpecificHost, privateRunVMOnTrustedHost], hooked up their 
*_EntityChanged methods, and implemented logic for handling field value changes 
in these methods

* Changes in VmListModel look good, you used newly added UnitVmModel fields in 
onSave [I assume setTrustedHostFlag/setDedicatedVmForVds are new fields for VM 
entity?], note - you might also want to update UpdateActionAvailability disable 
migrating VM when RunVMOnTrustedHost=true, etc.

* AbstractVmPopupWidget already has specificHost radio button & drop-down on 
Host dialog tab, and I assume you want to reuse the drop-down (host list) for 
trustedHost, so just add new radio button there:

  AbstractVmPopupWidget.ui.xml line 331

  




  

* In AbstractVmPopupWidget you to bind newly added RadioButton:

  @UiField(provided = true)
  @Ignore
  @WithElementId("trustedHost")
  public RadioButton trustedHost;

  You create trustedHost widget in constructor, and in 
initTabAvailabilityListeners you just add 
trustedHost.addValueChangeHandler(...) to have logic when trustedHost gets 
selected.

Regards,
Vojtech


- Original Message -
From: "Wei D Chen" 
To: engine-devel@ovirt.org
Sent: Tuesday, March 12, 2013 9:48:34 AM
Subject: [Engine-devel] Got some troubles when I want to modify oVirt GUI

Hi,
   In order to add new feature to Ovirt, that is user can choose virtual 
machine whether on trusted machine or not when it runs up, we modified the 
relative files.
Our goal is when the user click the trusted button, Run/Migration options are 
disabled. But unfortunately, we haven’t succeeded in graphic interface. 
I modified these files, I can’t see Host Tab, can you give me some help? Maybe 
we need modify more files. We did the following efforts:
(1) add a trusted radio button.
(2) Modify AbstractVmPopupWidget.ui.xml

 
     

(3) Modify AbstractVmPopupWidget.java
@UiField(provided = true)
@Path(value = "runVMOnTrustedHost.entity")
    @WithElementId("runVMOnTrustedHost")
public EntityModelRadioButtonEditor runVMOnTrustedHostEditor;

initListeners method:
object.getIsAutoAssign().getPropertyChangedEvent().addListener(new 
IEventListener() {
    @Override
    public void eventRaised(Event ev, Object sender, EventArgs args) {
    boolean isAutoAssign = (Boolean) 
object.getIsAutoAssign().getEntity();
    boolean runVMOnTrustedHost = (Boolean) 
object.getRunVMOnTrustedHost().getEntity();
    defaultHostEditor.setEnabled(!isAutoAssign && 
!runVMOnTrustedHost);
    //defaultHostEditor.setEnabled(!isAutoAssign);
    // only this is not bind to the model, so needs to listen to 
the change explicitly
   specificHost.setValue(!isAutoAssign && !runVMOnTrustedHost);
   //specificHost.setValue(!isAutoAssign);
    }
    });
isAutoAssignEditor.addDomHandler(new ClickHandler() {
    @Override
    public void onClick(ClickEvent event) {
    defaultHostEditor.setEnabled(false);
    }
    }, ClickEvent.getType());
    vm.getIsAutoAssign().getEntityChangedEvent().addListener(new 
IEventListener() {
    @Override
    public void eventRaised(Event ev, Object sender, EventArgs args) {
    if (!isAutoAssignEditor.asRadioButton().getValue() && 
!runVMOnTrustedHostEditor.asRadioButton().getValue())
    {
    specificHost.setValue(true, true);
    }
    }
    });
    runVMOnTrustedHostEditor.addDomHandler(new ClickHandler() {
    @Override
    public void onClick(ClickEvent event) {
    defaultHostEditor.setEnabled(false);
    }
    }, ClickEvent.getType());
    vm.getRunVMOnTrustedHost().getEntityChangedEvent().addListener(new 
IEventListener() {
    @Override
    public void eventRaised(Event ev, Object sender, EventArgs args) {
    if (!runVMOnTrustedHostEditor.asRadioButton().getValue() && 
!isAutoAssignEditor.asRadioButton().getValue())
    {
    specificHost.setValue(true, true);
    }
    }
    });
(4) Modify UnitVmModel.java
  private void RunVMOnTrustedHost_EntityChanged(Object sender, EventArgs args)