Bug 856512

Summary: ovirt-engine-restapi: rename display.allow_reconnect property
Product: Red Hat Enterprise Virtualization Manager Reporter: Oded Ramraz <oramraz>
Component: ovirt-engine-restapiAssignee: Juan Hernández <juan.hernandez>
Status: CLOSED CURRENTRELEASE QA Contact: Oded Ramraz <oramraz>
Severity: medium Docs Contact:
Priority: unspecified    
Version: 3.1.0CC: dyasny, ecohen, iheim, juan.hernandez, michal.skrivanek, mpastern, pstehlik, Rhev-m-bugs, sgrinber, ykaul
Target Milestone: ---Keywords: Reopened
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard: virt
Fixed In Version: si20 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-12-04 20:01:18 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Oded Ramraz 2012-09-12 07:56:42 UTC
Description of problem:

RSDL does not document Disable strict user checking. 
Need to be added to add VM / update VM.

Version-Release number of selected component (if applicable):


How reproducible:


Steps to Reproduce:
1.
2.
3.
  
Actual results:


Expected results:


Additional info:

Comment 1 Juan Hernández 2012-09-12 11:32:13 UTC
The name is not "disable_strict_user_checking" but "allow_reconnect":

<parameter required="false" type="xs:boolean">
  <name>vm.display.allow_reconnect</name>
</parameter>

Also for templates:

<parameter required="false" type="xs:boolean">
  <name>template.display.allow_reconnect</name>
</parameter>

Don't you get these parameters in the RDSL document?

Comment 2 Michael Pasternak 2012-09-12 12:42:36 UTC
according to Oded:

odedr> this feature added to prevent a situation when 2 people are logged in to the same VM

this is does not sounds as allow_reconnect, is it same feature?

Comment 3 Juan Hernández 2012-09-12 13:00:07 UTC
Yes, it is the same feature. The flag is named "allow_reconnect" because when it is true it allows any user to connect to the console of the VM regardless of who was connected before and without requiring a reboot of the VM. Then it was renamed in the user interface to "Disable strict ...".

Comment 4 Oded Ramraz 2012-09-12 13:06:57 UTC
I find this option finally from CLI : "display-allow_reconnect" . 
I found it quite confusing that this feature has 2 names: "disable-strict-user checking" in UI and "display-allow_reconnect" in API. 
Closing this bug for now , but I still recommend to change this field name in API before people will start to use it .

Comment 5 Michael Pasternak 2012-09-12 13:15:45 UTC
(In reply to comment #4)
> I find this option finally from CLI : "display-allow_reconnect" . 
> I found it quite confusing that this feature has 2 names:
> "disable-strict-user checking" in UI and "display-allow_reconnect" in API. 
> Closing this bug for now , but I still recommend to change this field name
> in API before people will start to use it .

+1 i think allow_reconnect not enough informative, - 'shared' (or similar)
would do the trick,

<vm>
 <display>
   <shared>true|false</shared>

Comment 6 Juan Hernández 2012-09-12 13:37:16 UTC
Note that this is already in upstream 3.1, so changing the name could break backwards compatibility there. How do we solve that? Maybe there is no need to solve it as this flag is not that popular.

Also I don't think that "allow_reconnect" is less informative than "shared", but I prefer to abstain from this voting.

Einav, any suggestion or preference for naming?

Comment 7 Oded Ramraz 2012-09-12 13:39:51 UTC
I'm not sure that shared is accurate.  

When this feature is disabled ( strict-user checking disabled ) two people can try to login to a VM and the latter will override  the connection .

When this feature is enabled one must reboot the machine before other one can login to the VM .  


I would module it as follows , ( or advise with PM regarding shorter term ). 

<vm>
 <display>
   <strict_user_checking><true|false></strict_user_checking>

Comment 8 Michael Pasternak 2012-09-12 13:45:38 UTC
(In reply to comment #6)
> Note that this is already in upstream 3.1, so changing the name could break
> backwards compatibility there. How do we solve that? 

announcing the change on ML will do the job 

> Maybe there is no need to solve it as this flag is not that popular.

if we won't do it now, then we will be obligated to this name

> 
> Also I don't think that "allow_reconnect" is less informative than "shared",
> but I prefer to abstain from this voting.

in general "allow_reconnect" means that you can connect back after X,
i'm open for other names

> 
> Einav, any suggestion or preference for naming?

Comment 9 Einav Cohen 2012-09-12 13:48:47 UTC
(In reply to comment #6)
> ...
> Einav, any suggestion or preference for naming?

I don't see any problem with "allow_reconnect"; maybe "allow_connection_override"?
setting needinfo on PM for final decision.

Comment 10 Simon Grinberg 2012-09-20 11:17:04 UTC
(In reply to comment #9)
> (In reply to comment #6)
> > ...
> > Einav, any suggestion or preference for naming?
> 
> I don't see any problem with "allow_reconnect"; maybe
> "allow_connection_override"?
> setting needinfo on PM for final decision.

According to this thread "allow_connection_override" is more accurate, since same user reconnect is allowed in any case, right?

Shared is not the right term in any case and it's a different feature still waiting to be implemented, so let's reserve this term.

Comment 11 Michael Pasternak 2012-09-20 11:26:43 UTC
(In reply to comment #10)
> According to this thread "allow_connection_override" is more accurate

we avoid adding more than two period based properties in api

Comment 12 Simon Grinberg 2012-09-20 12:36:56 UTC
(In reply to comment #11)
> (In reply to comment #10)
> > According to this thread "allow_connection_override" is more accurate
> 
> we avoid adding more than two period based properties in api
But isn't that going to translate to 


<parameter required="false" type="xs:boolean">
  <name>vm.display.allow_connection_override</name>
</parameter>

Am I missing something?

Comment 13 Michael Pasternak 2012-09-20 12:54:48 UTC
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > According to this thread "allow_connection_override" is more accurate
> > 
> > we avoid adding more than two period based properties in api
> But isn't that going to translate to 
> 
> 
> <parameter required="false" type="xs:boolean">
>   <name>vm.display.allow_connection_override</name>
> </parameter>

correct, but we do not have in api, any property longer than two periods,
e.g. x_y, what about .allow_override?

> 
> Am I missing something?

Comment 14 Simon Grinberg 2012-09-20 16:19:22 UTC
(In reply to comment #13)

> correct, but we do not have in api, any property longer than two periods,
> e.g. x_y, what about .allow_override?
> 

Ha, my bad, I call those underscores :).

My problem is that allow_override, is not that meaningful, override what? 
It's more like snatching, but snatching does not sound right. 

Andy, as a native speaker can you help with this?

Comment 18 Juan Hernández 2012-10-03 11:58:05 UTC
The change proposed to rename allow_reconnect to allow_override is available here:

http://gerrit.ovirt.org/8325

Comment 19 Juan Hernández 2012-10-03 13:19:14 UTC
The change has been merged upstream:

http://gerrit.ovirt.org/gitweb?p=ovirt-engine.git;a=commit;h=0065da540311212b8a772cb9993ec01a37229d49

Comment 21 Oded Ramraz 2012-10-09 14:17:42 UTC
[RHEVM shell (connected)]# add vm display- 
display-allow_override  display-monitors        display-type  

Verified si20