dsccommunity / xRemoteDesktopSessionHost

This module contains DSC resources for the management and configuration of Microsoft Remote Desktop Session Host (RDSH).

MIT License

35 stars 47 forks source link

xRDSessionDeployment: Deploy multiple session hosts #89

Closed jeremyciak closed 11 months ago

jeremyciak commented 3 years ago

Pull Request (PR) description

This small change should allow for people to supply a string array (multiple values) for the SessionHost parameter of the xRDSessionDeployment resource.

This Pull Request (PR) fixes the following issues

77

Task list

[x] Added an entry under the Unreleased section of file CHANGELOG.md. Entry should say what was changed, and how that affects users (if applicable).
[x] Resource documentation added/updated in README.md.
[x] Resource parameter descriptions added/updated in README.md, schema.mof and comment-based help.
[ ] Comment-based help added/updated.
[ ] Localization strings added/updated in all localization files as appropriate.
[ ] Examples appropriately added/updated.
[x] Unit tests added/updated. See DSC Community Testing Guidelines.
[ ] Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
[x] New/changed code adheres to DSC Community Style Guidelines.

This change is

danielboth commented 3 years ago

Hi @jeremyciak, nice work on yet another PR! I didn't do a full review yet, but I did give it some thought on my side. Allowing multiple session hosts as a DSC user, I would expect DSC to "make it so", but can we actually do that leveraging the current code? As in, this would work the first time, but adding more hosts would not do anything, since the test is only checking the connection broker at this time.

I think the reason this resource supports the SessionHost parameter is because it is mandatory on New-RDSessionDeployment, but I feel adding / removing servers from the deployment should be done by deploying a configuration that uses the xRDServer resource to each machine that needs to be added. Your thoughts?

jeremyciak commented 3 years ago

@danielboth I remember when I first came to DSC to address my RDS deployment needs. I instinctively assumed the parameters for this resource would line up with the corresponding cmdlet that it ends up calling. I ended up working around this in the manner you've described but I think extending this resource in this manner is more intuitive.

I'm on the fence of whether this should be extended to mange ongoing state or if this resource should really only reflect initial deployment. I think even before I made these changes if you updated the SessionHost parameter I don't think it would do anything.

Which direction do you want to go? Feel free to fold in input from others you may know in the community as well!

jeremyciak commented 3 years ago

Just following up on this...

kcighon commented 3 years ago

@jeremyciak - same fix is needed on MSFT_xRDSessionCollection

johlju commented 2 years ago

This PR proposes removing one of the Key properties. I need help understanding here. Does this key not uniquely identifies a particular RDSH instance? Is it possible to install two RDSH instance on one node, if so, what uniquely identifies each instance?

uw-dc commented 1 year ago

My personal take on this, is that xRDSessionDeployment should be able to handle multiple session hosts.

I think the test should compare the desired session hosts with the existing session hosts, in the case that a deployment exists, and the set should use Add-RDSessionHost and Remove-RDSessionHosts to add and remove session hosts as required to make the desired session hosts match the existing session hosts.

I would be tempted to merge this PR and raise a new issue to work on the enhancement describe above to the test-targetresource and set-targetresource functions.

uw-dc commented 1 year ago

This PR proposes removing one of the Key properties. I need help understanding here. Does this key not uniquely identifies a particular RDSH instance? Is it possible to install two RDSH instance on one node, if so, what uniquely identifies each instance?

I think it is sufficient to identify a deployment by RDS Connection Broker alone. I've not seen an RDS Broker serving two different RDS Deployments in any of the supported deployments I've researched. I suspect that the service runs on given and unique set of ports and I don't think it is possible to configure multiple instances of the connection broker service (tssdis) bound to different IP addresses ... in which case, each IP address would be associated to a different FQDN anyway.