CP-10204: XenCenter: guest RDP control #275

cheng-z · 2014-12-08T07:56:28Z

This code change is to add changes as bellow:

The option "Enable Remote Desktop console scanning"change to"Enable Remote Desktop console scanning (XenServer 6.5 and earlier)"
Remove connection between "Automatically switch to the Remote Desktop console when it becomes available" and "Enable Remote Desktop console scanning"
Add "Turn on Remote desktop"

One thing need to modify is that "CreedenceorGreater" funtion check should be change.

Signed-off-by: Cheng Zhang cheng.zhang@citrix.com

stephen-turner · 2014-12-10T13:47:53Z

Waiting on this until the requirements are finalised.

huizh · 2014-12-15T10:02:40Z

Based on the discussion of the requirements, guest agent would watch and update RDP status to ensure it be correct for XenCenter to display corresponding button.
My understanding is this would not impact XenCenter code with this pull request.
Stephen, please could you correct me if I misunderstand anything?
Thanks.

stephen-turner · 2014-12-15T10:09:04Z

We don't yet know whether the guest agent can do that (with sufficient performance), so I don't want to take this pull request and then find we have to revert much of it.

huizh · 2014-12-15T10:25:00Z

Yes, I agree that might be an issue.
Thanks Stephen for the instant response and the comprehensive consideration.

cheng-z · 2015-01-05T08:44:13Z

Hi @stephen-turner & @huizh
I already change the API interface to vm_call_plugin.(Why using this name can refer to xapi-project/xapi-project.github.io#12 (comment))
Also about the windows firewall and watch RDP status issue, I already made pull request to win-guestagent, and the code change already merged into master.

So, @stephen-turner do you think we can start the code review now?

stephen-turner · 2015-01-05T14:49:54Z

Thank you, Cheng. So just to be clear, with your changes to the guest agent, will XenCenter now get notified even if RDP is turned on or off directly in the guest?

If so, we will proceed with reviewing the pull request.

cheng-z · 2015-01-06T01:24:18Z

Hi @stephen-turner,
Yes, with the change of windows pv tool which I mentioned as guest agent, RDP status change can be notified.

stephen-turner · 2015-01-06T14:32:16Z

We're planning to review this on Thursday. Sorry, we don't have any free slots today or tomorrow with all the right people.

stephen-turner · 2015-01-08T11:32:29Z

XenModel/Messages.resx

@@ -11406,6 +11409,9 @@ To learn more about the XenServer Dynamic Workload Balancing feature or to start
  <data name="VNC_RDESKTOP" xml:space="preserve">
    <value>Sw&amp;itch to Remote Desktop</value>
  </data>
+  <data name="VNC_RDESKTOP_TRUN_ON" xml:space="preserve">


Spelling mistake: should be TURN not TRUN

Already change the typo. Thanks.

Signed-off-by: Cheng Zhang <cheng.zhang@citrix.com>

stephen-turner · 2015-01-09T10:18:23Z

You asked about CreamOrGreater(): feel free to write this function.

Let us know when you want us to review it again.

stephen-turner · 2015-01-09T11:46:53Z

You wrote:

Since this api call can not return correct result of RDP enable or not. I add the connect operation in
guestMetrics_PropertyChanged. Thanks.

The preferred behaviour here is different depending whether the user turned on RDP by pressing the button in XenCenter, or by turning it on in the guest.

If the user turned on RDP by pressing the button in XenCenter, we should always switch to RDP as soon as it's available
But if the user turned on RDP in-guest, we might or might not switch to RDP depending whether the setting "Automatically switch to the Remote Desktop console when it becomes available" is on or off.

If this is too difficult to implement, we should always do 2., and change the text in the dialog to read simply "Would you like to turn on Remote Desktop in this VM? [Yes] [No]"

Signed-off-by: Cheng Zhang <cheng.zhang@citrix.com>

cheng-z · 2015-01-12T09:34:38Z

Hi @stephen-turner , thank you for you reminder.
I doing following changes to meet your conditons:
When RDP status is on and currently the VM using VNC, these two condition you mentioned will be checked to make decision about try to connect RDP or not.
Those logic is also in guestMetrics_PropertyChanged

Current, all rework jobs are been done based on all your comments, and I think we can start the review process again. Thanks a lot.

stephen-turner · 2015-01-13T15:12:38Z

Sorry I didn't notice before, but this pull request accidentally introduces a new file, XenModel/XenAPI-Extensions/Proxy.cs. Please remove it. (It's large, so I don't even want it added and then deleted again; please make a new, squashed changeset without it). Thanks.

stephen-turner · 2015-01-13T15:19:12Z

XenAdmin/ConsoleView/VNCTabView.cs

@@ -741,6 +761,11 @@ public void Unpause()
                vncScreen.Unpause();
        }

+        private bool RDPDiabledOnCreamOrGreater(IXenConnection conn)


Typo in function name: should be Disabled not Diabled.

cheng-z · 2015-01-14T03:44:20Z

Hi @stephen-turner I already check on all comments and thanks for your great work which help me a lot.
The second round of reword is done. Please help to check the comment and code again.
Thanks a lot.

Signed-off-by: Cheng Zhang <cheng.zhang@citrix.com>

Zhengchai · 2015-01-16T09:40:57Z

Thank you @stephen-turner Thank you for your review and @cheng--zhang for the discussion, do we have more comments for the codes change to be merged?

stephen-turner · 2015-01-16T10:41:40Z

Sorry, I still need to do another review, but it's been a really busy week with the train planning.

stephen-turner · 2015-01-16T15:16:31Z

I have scheduled this for Monday. Apologies for the delay.

stephen-turner · 2015-01-19T14:26:35Z

The code looks OK to me now, but I wanted to do some smoke testing too. Could you confirm which branch and build has all the changes (xapi and guest agent) on it? I tried with trunk-ring3 build 90554, but it didn't work right: I saw "switch to RDP" when RDP was turned off; no button when RDP was turned on; and I never saw "turn on RDP". But maybe I just haven't got the guest agent changes.

cheng-z · 2015-01-20T04:58:40Z

Hi @stephen-turner, I am so sorry about the test on trunk-ring3. Because this feature also need code change for Windows PV tool, the commit is not merged in trunk-ring3, so the RDP status in trunk-ring3 will not be the correct one. You can use trunk branch build as a test environment. The trunk branch included all changes for this feature.
I also equipped my DT box dt65 with the latest trunk build for your reference.
Thanks.

stephen-turner · 2015-01-20T09:24:07Z

OK, thanks, I'll try that.

stephen-turner · 2015-01-20T15:09:49Z

This still isn't working well for me. It doesn't appear to be responding to changes made via the in-guest UI. And when I press "Turn on XenDesktop", the button gets greyed out, but it never connects. I have also seen a state where the remote desktop option is set, but with a warning that I need to configure the Windows firewall too, which I thought is supposed to happen automatically.

I think we should have a meeting to discuss this, rather than continuing to do it in the pull request. I'll set one up for tomorrow.

stephen-turner · 2015-01-21T08:49:55Z

It turns out that the problems were caused by not having all the guest agent changes on the server I was testing on. It looks good now, so I'm accepting this pull request. There are some minor bugs, but we will take them as separate items.

CP-10204: XenCenter: guest RDP control

cheng-z force-pushed the CP-10204 branch from c5e8182 to 1b60fb6 Compare January 5, 2015 08:36

stephen-turner reviewed Jan 8, 2015
View reviewed changes

CP-10204: XenCenter: guest RDP control

6b6b515

Signed-off-by: Cheng Zhang <cheng.zhang@citrix.com>

cheng-z force-pushed the CP-10204 branch 3 times, most recently from e9d1eab to 6a49c83 Compare January 9, 2015 09:48

cheng-z force-pushed the CP-10204 branch 5 times, most recently from e626f47 to f3c79d0 Compare January 12, 2015 08:45

CP-10204: Rework comments

98992d3

Signed-off-by: Cheng Zhang <cheng.zhang@citrix.com>

cheng-z force-pushed the CP-10204 branch from f3c79d0 to 98992d3 Compare January 12, 2015 09:28

stephen-turner reviewed Jan 13, 2015
View reviewed changes

cheng-z force-pushed the CP-10204 branch from ca1cb69 to 963dd95 Compare January 14, 2015 03:33

CP-10204: Rework comments - part 2

961edb3

Signed-off-by: Cheng Zhang <cheng.zhang@citrix.com>

cheng-z force-pushed the CP-10204 branch from 963dd95 to 961edb3 Compare January 14, 2015 03:47

stephen-turner pushed a commit that referenced this pull request Jan 21, 2015

Merge pull request #275 from cheng--zhang/CP-10204

10226d3

CP-10204: XenCenter: guest RDP control

stephen-turner merged commit 10226d3 into xenserver:master Jan 21, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CP-10204: XenCenter: guest RDP control #275

CP-10204: XenCenter: guest RDP control #275

cheng-z commented Dec 8, 2014

stephen-turner commented Dec 10, 2014

huizh commented Dec 15, 2014

stephen-turner commented Dec 15, 2014

huizh commented Dec 15, 2014

cheng-z commented Jan 5, 2015

stephen-turner commented Jan 5, 2015

cheng-z commented Jan 6, 2015

stephen-turner commented Jan 6, 2015

stephen-turner Jan 8, 2015

cheng-z Jan 9, 2015

stephen-turner commented Jan 9, 2015

stephen-turner commented Jan 9, 2015

cheng-z commented Jan 12, 2015

stephen-turner commented Jan 13, 2015

stephen-turner Jan 13, 2015

cheng-z commented Jan 14, 2015

Zhengchai commented Jan 16, 2015

stephen-turner commented Jan 16, 2015

stephen-turner commented Jan 16, 2015

stephen-turner commented Jan 19, 2015

cheng-z commented Jan 20, 2015

stephen-turner commented Jan 20, 2015

stephen-turner commented Jan 20, 2015

stephen-turner commented Jan 21, 2015

CP-10204: XenCenter: guest RDP control #275

CP-10204: XenCenter: guest RDP control #275

Conversation

cheng-z commented Dec 8, 2014

stephen-turner commented Dec 10, 2014

huizh commented Dec 15, 2014

stephen-turner commented Dec 15, 2014

huizh commented Dec 15, 2014

cheng-z commented Jan 5, 2015

stephen-turner commented Jan 5, 2015

cheng-z commented Jan 6, 2015

stephen-turner commented Jan 6, 2015

stephen-turner Jan 8, 2015

Choose a reason for hiding this comment

cheng-z Jan 9, 2015

Choose a reason for hiding this comment

stephen-turner commented Jan 9, 2015

stephen-turner commented Jan 9, 2015

cheng-z commented Jan 12, 2015

stephen-turner commented Jan 13, 2015

stephen-turner Jan 13, 2015

Choose a reason for hiding this comment

cheng-z commented Jan 14, 2015

Zhengchai commented Jan 16, 2015

stephen-turner commented Jan 16, 2015

stephen-turner commented Jan 16, 2015

stephen-turner commented Jan 19, 2015

cheng-z commented Jan 20, 2015

stephen-turner commented Jan 20, 2015

stephen-turner commented Jan 20, 2015

stephen-turner commented Jan 21, 2015