Skip to content

Fix Implicit Remoting test failure on nightly run #4323

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
mirichmo merged 1 commit into from
Jul 26, 2017
Merged

Fix Implicit Remoting test failure on nightly run #4323

mirichmo merged 1 commit into from
Jul 26, 2017

Conversation

@SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Jul 22, 2017 •
edited
Loading

[Feature]
Systems running WS2012R2 w/o WMF5 has WinRM with restriction for MaxConcurrentUsers between 1-100

So the default needs to be 100 to allow PSCore6 to work w/o requiring WMF5+

This is technically a Breaking Change as we had explicitly changed the allowed max to Int.MaxValue, but for most users this shouldn't be an issue.

@SteveL-MSFT SteveL-MSFT requested a review from mirichmo July 22, 2017 06:26
@SteveL-MSFT SteveL-MSFT added the Breaking-Change breaking change that may affect users label Jul 22, 2017
@SteveL-MSFT SteveL-MSFT changed the title [Daily] Fix Implicit Remoting test failure on nightly run [Feature] Fix Implicit Remoting test failure on nightly run Jul 22, 2017
@SteveL-MSFT SteveL-MSFT changed the title [Feature] Fix Implicit Remoting test failure on nightly run Fix Implicit Remoting test failure on nightly run Jul 22, 2017
@SteveL-MSFT SteveL-MSFT reopened this Jul 22, 2017
@SteveL-MSFT
Copy link
Member Author

SteveL-MSFT commented Jul 22, 2017

@TravisEz13 having trouble getting Feature tests to run

@iSazonov
Copy link
Collaborator

iSazonov commented Jul 22, 2017

WS2012R2 w/o WMF5 has WinRM with restriction for MaxConcurrentUsers between 1-100

I never knew that restriction! I tried and I can confirm.
It's also a very fantastic scenario - to a server should connect more than 100 users by WinRS!

I believe the fix does not need because the limit (1) can be changed in future, (2) can vary between systems and versions
If anybody catch the issue he get a standard error message.
So I believe the best fix is to enhance documentation https://msdn.microsoft.com/en-us/library/ee309367%28v=vs.85%29.aspx

@iSazonov
Copy link
Collaborator

iSazonov commented Jul 22, 2017

@SteveL-MSFT

[Feature]

"If in the first line of the last (most recent) commit description you add [Feature],
we will ensure that we will also run the tests tagged with Feature."

@TravisEz13
Copy link
Member

TravisEz13 commented Jul 22, 2017

#4304 was causing [Feature] not to work for PRs

@SteveL-MSFT
Copy link
Member Author

SteveL-MSFT commented Jul 23, 2017

@iSazonov the fix is only needed because any customer (and affects our AppVeyor CI) using WS2012/R2 without WMF5 installed will have this restriction. This means that by default, if they do new-pssessionconfigurationfile and register-pssessionconfigurationfile it will return this error. Unfortunately, we don't expose a way to set this value, they would have to understand WinRM XML configuration to make it work.

@daxian-dbw
Copy link
Member

daxian-dbw commented Jul 24, 2017

The 3 failures in implicit remoting will be resolved by #4326

@daxian-dbw daxian-dbw mentioned this pull request Jul 24, 2017
Merged
@iSazonov
Copy link
Collaborator

iSazonov commented Jul 24, 2017

@SteveL-MSFT Why do we rely on the default value? What if I want 200? I can install WMF5 and get 200, but this fix is blocking me. This error is rised on the server but is sent to the remote user.
This user does not know the configuration of the remote server, which can also be changed dynamically. If we want to inform the user, we must catch this exception and create our own.

@SteveL-MSFT
Copy link
Member Author

SteveL-MSFT commented Jul 24, 2017

@iSazonov we need a default as that value is required by WinRM. I guess the question is whether we want to support downlevel systems w/o WMF5 or not. Personally, I don't like making PSCore6 dependent on WMF5. cc @joeyaiello

@SteveL-MSFT SteveL-MSFT force-pushed the maxconcurrentusers branch 2 times, most recently from c2d3444 to 6dd595a Compare July 24, 2017 16:56
@SteveL-MSFT SteveL-MSFT added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Jul 24, 2017
@iSazonov
Copy link
Collaborator

iSazonov commented Jul 25, 2017

I don't like making PSCore6 dependent on WMF5 too.

@SteveL-MSFT SteveL-MSFT mentioned this pull request Jul 25, 2017
Merged
@SteveL-MSFT SteveL-MSFT removed the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Jul 25, 2017
@SteveL-MSFT
Copy link
Member Author

SteveL-MSFT commented Jul 25, 2017

Based on some internal discussion, I think where we ended up is to revert the default as it was changed due to customer impact (100 was too low) and in the cmdlet, if the registration fails, we try again with MaxConcurrentUsers=100.

@SteveL-MSFT SteveL-MSFT added the Committee-Reviewed PS-Committee has reviewed this and made a decision label Jul 25, 2017
iSazonov
iSazonov reviewed Jul 25, 2017
View reviewed changes
src/System.Management.Automation/engine/remoting/commands/CustomShellCommands.cs Outdated
Copy link
Collaborator

@iSazonov iSazonov Jul 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment should mention WMF 5.

Copy link
Member Author

@SteveL-MSFT SteveL-MSFT Jul 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WS2012R2 has PSv3 by default and I'm not sure if the Max limit was changed in WMF4 or WMF5, so I'll just mention WMF

@SteveL-MSFT SteveL-MSFT removed the Breaking-Change breaking change that may affect users label Jul 25, 2017
@SteveL-MSFT SteveL-MSFT requested a review from PaulHigin July 25, 2017 23:52
@SteveL-MSFT
Copy link
Member Author

SteveL-MSFT commented Jul 25, 2017

@mirichmo @PaulHigin since this is causing our nightly runs to fail, can you prioritize this review higher? thanks

mirichmo
mirichmo reviewed Jul 25, 2017
View reviewed changes
...tem.Management.Automation/engine/remoting/commands/NewPSSessionConfigurationOptionCommand.cs Outdated
Copy link
Member

@mirichmo mirichmo Jul 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the decision to retry, shouldn't this be reverted?

Copy link
Member Author

@SteveL-MSFT SteveL-MSFT Jul 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thought I did. Will fix.

@SteveL-MSFT
[Feature]
6c85e19 
Retry setting winrm config if it fails with MaxConcurrentUsers > 100
PaulHigin
PaulHigin approved these changes Jul 26, 2017
View reviewed changes
Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mirichmo mirichmo merged commit cf7b9f3 into PowerShell:master Jul 26, 2017
@SteveL-MSFT SteveL-MSFT deleted the maxconcurrentusers branch July 26, 2017 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov left review comments

@daxian-dbw daxian-dbw daxian-dbw approved these changes

+2 more reviewers

@mirichmo mirichmo mirichmo approved these changes

@PaulHigin PaulHigin PaulHigin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mirichmo mirichmo

Labels

Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@SteveL-MSFT @iSazonov @TravisEz13 @daxian-dbw @mirichmo @PaulHigin @msftclas