Bug 1113478

Eli - I discussed it with Martin, and it seems like we shouldn't save anything from the PM details entered in the dialog, in case PM is disabled.
Any reason we're doing it so far?

(In reply to Oved Ourfali from comment #1)
> Eli - I discussed it with Martin, and it seems like we shouldn't save
> anything from the PM details entered in the dialog, in case PM is disabled.
> Any reason we're doing it so far?

Why not?
If I am configuring a Host PM and want only to disable PM for a period of time from my reasons, why I will be required to insert all the PM information again.
The operation is disable PM not delete so I expect all data to remain in DB except of the change of the PM enabled flag

Eli, you are right, but my idea was a bit different. Let me show you this example:

1) You have valid PM options and PM enabled
2) You enter invalid options (like removing username, setting port number
   to XXX), disable PM options and click on the Save button -> when PM is 
   disabled, there's no validation, so there will be saved invalid values into DB


My idea:

When PM is disabled, don't update PM options in the database (for example by  
not propagating user input from UI to saved entity or loading current DB values 
PM options values prior to update query or using customized query or ...).
So we will  always have valid PM options in db no matter if PM is enabled/disabled.
I know it's a bit problematic because PM options are part of vds_static, but if we decide to move PM options into its own entity, it will be much easier to achieve this.

(In reply to Martin Perina from comment #3)
> Eli, you are right, but my idea was a bit different. Let me show you this
> example:
> 
> 1) You have valid PM options and PM enabled
> 2) You enter invalid options (like removing username, setting port number
>    to XXX), disable PM options and click on the Save button -> when PM is 
>    disabled, there's no validation, so there will be saved invalid values
> into DB
> 
> 
> My idea:
> 
> When PM is disabled, don't update PM options in the database (for example by
> 
> not propagating user input from UI to saved entity or loading current DB
> values 
> PM options values prior to update query or using customized query or ...).
> So we will  always have valid PM options in db no matter if PM is
> enabled/disabled.
> I know it's a bit problematic because PM options are part of vds_static, but
> if we decide to move PM options into its own entity, it will be much easier
> to achieve this.

You may have a use-case in which someone sets the values when adding the host, but doesn't want to enable it for now. So I suggest leaving the current behavior, and just saving the missing field (port).

What we can do is remove the periodic PM status check, not to test PM details when PM is disabled, however it doesn't relate to this bug.

Scott - thoughts?

(In reply to Oved Ourfali from comment #5)
> What we can do is remove the periodic PM status check, not to test PM
> details when PM is disabled, however it doesn't relate to this bug.
> 
> Scott - thoughts?

It is not periodic, it is done when the user press OK on the dialog.
The periodic check is he PM Health feature and it is not done for host with pm_enabled = false

(In reply to Oved Ourfali from comment #5)
> What we can do is remove the periodic PM status check, not to test PM
> details when PM is disabled, however it doesn't relate to this bug.
> 
> Scott - thoughts?

Oved - It appears as if the use case is simply how we deal with the database values on PM Disable.  From what I gather in this BZ, when a user disables PM and saves, we are explicitly wiping all DB data related to PM settings.  Is there any reason we can't simply disable PM while still maintaining the specific PM configuration settings?  This will make re-enabling PM a simple single checkbox, not a checkbox and re-filling in 3-5 options every time.

(In reply to Scott Herold from comment #7)
> (In reply to Oved Ourfali from comment #5)
> > What we can do is remove the periodic PM status check, not to test PM
> > details when PM is disabled, however it doesn't relate to this bug.
> > 
> > Scott - thoughts?
> 
> Oved - It appears as if the use case is simply how we deal with the database
> values on PM Disable.  From what I gather in this BZ, when a user disables
> PM and saves, we are explicitly wiping all DB data related to PM settings. 
> Is there any reason we can't simply disable PM while still maintaining the
> specific PM configuration settings?  This will make re-enabling PM a simple
> single checkbox, not a checkbox and re-filling in 3-5 options every time.

We are saving the values.
However, we still do the PM status check, even if disabled.
It is harmless, but might trigger some unwanted alerts.
I'd leave this as-is, unless someone complains about it.

Verified.

All the values in PM tab are saved.

oVirt Engine Version: 3.5.0-0.0.master.20140821064931.gitb794d66.el6

rhev 3.5.0 was released. closing.