Fix Set-Service -Status Stopped runs failed on the service with dependencies

zhenggu commented 6 years ago

PR Summary

Fixes #5517

In the previous version, if one service has dependency services, it cannot be stopped by "Set-Service -Stop", this PR fixes this issue.

And also add the Parameter "-Force" to Set-Service, when it is enabled, the function will stop it's dependent services, and then stop the target's service itself.

Without the parameter "-Force" it will raise a exception, if the service has running dependent services.

PR Checklist

[x] PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
[x] Summarized changes
[x] Change is not breaking
[x] Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
[x] This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.
User-facing changes
- [x] User-facing Documentation needed
  - [x] Issue filed - Issue link: https://github.com/PowerShell/PowerShell-Docs/pull/2858
Testing - New and feature
- [x] Make sure you've added a new test if existing tests do not effectively test the code changed
  - [ ] Add [feature] if the change is significant or affects feature tests
    
    This change is

msftclas commented 6 years ago

All CLA requirements met.

markekraus commented 6 years ago

I'm curious if this is even the right direction. Doesn't Stop-Service -Force do this already? Shouldn't Set-Service only support a a very basic stop? The -Status on Set-Service is redundant to stop-service which should be the de facto way to stop services.

iSazonov commented 6 years ago

We have a similar question in #5471

zhenggu commented 6 years ago

I think it is necessary to remove the ServicesDependedOn check for it will block the normal function. and for parameter "Force", we can discuss it in other thread, if the parameter may be not acceptable.

iSazonov commented 6 years ago

@SteveL-MSFT Could you please review the PR with #5471 on PowerShell committee?

mklement0 commented 6 years ago

Yes, Set-Service -Status duplicates Start-Service / Resume-Service / Stop-Service / Suspend-Service functionality, but:

it is a one-stop solution with more of a desired-state feel.
it offers functionality that Start-Service doesn't have: if the target service's startup mode is currently Disabled, Start-Service invariably fails. By contrast, you can use Set-Service -StartupType Manual -State Running in such a case, for instance.

On the flip side, it currently doesn't support -Force in order to stop a service that have dependents - which I think should be fixed too.

As for the issue at hand:

Preventing a service from stopping because it depends on other services - as opposed to having other services depend on it (having dependents) - is an arbitrary and nonsensical restriction.

I presume it was introduced based on a misconception and should therefore be removed, which will make -Status Stopped work as expected, as long as the target service doesn't have dependents.

@iSazonov: How is #5471 related to this?

iSazonov commented 6 years ago

@mklement0 Thanks for useful comments!

How is #5471 Start-Process: add parameter 'ExitTimeout' related to this?

Design review needed.

markekraus commented 6 years ago

@mklement0

it is a one-stop solution with more of a desired-state feel.

Yea, but stopping a service is NOT setting a service. We have specific verbs for that kind of action. if there is a current bug in how it operates, that should be fixed, but adding -force for set- to only affect the stop is ridiculous. We have the stop verb, use it for advanced stop functionality. If you can get away with basic stop on set, so be it, that's what stop is for. If I could go back in time I would say the set verb should not have had this functionality to begin with. if it did, then the stop verb should not have been included.

On your second point about start-service, I will say that set is also not start and including it originally is silly, IMO. The correction there is to add a -force to start if it is not already there that enables the service if it is not disabled and starts any services it depends on.

I'm just trying to keep things simple here and it seems like this PR is going in the wrong direction from simple. There is nothing wrong with doing something like set-service @params -PassThru | stop-service or set-service @params -PassThru | start-service. That keeps to the do one thing philosophy. increasing complexity in a command to do another thing when there is another command that does that one thing is an odd design choice. (setting aside any perceived convenience in doing so).

mklement0 commented 6 years ago

I don't why it was decided to provide duplicate functionality in Set-Service, but given that it was:

fixing the bug is a must, as we agree.
however, not duplicating all the functionality seems silly, if adding -Force is all that it takes, especially given that, as demonstrated, in other aspects it can do more than one of its counterparts.
(On a side note: status value suspended should probably be added as an alias for paused).

Adding a -Force switch to Start-Service is not a solution, because there is no sensible default for the startup type that invariably has to change in that case - unless you add -StartupType to Start-Service too, but that would then duplicate what Set-Service already does.

Yes, you can argue that the -Status parameter should never have been implemented, but, looking at the bigger picture, the Set verb has always had desired-state aspects in certain cmdlets, resulting in duplication as well; two examples:

You can use Set-Variable without ever having to touch New-Variable - the variable is created on demand.
You can use Set-Content to create a file on demand, without needing to use New-Item.

And, finally, another aspect in which Set-Service -Status provides extra (again, desired-state) functionality is that -Status Running either starts or resumes (continues) a service, as appropriate, whereas applying Start-Service to a suspended (paused) service results in an error.

markekraus commented 6 years ago

however, not duplicating all the functionality seems silly,

No, it doesn't at all. Set- is for setting, Stop- is for stopping. if the ability to stop on set- goes beyond anything basic, you should use stop-. The ability to stop on set- is a convenience that arguably should not be there (and I'm not advocating for its removal, for clarity).

Tell me, absent -Status Stopped, what purposed does -Force have? If I see -Force on Set-Service is expect it to allow me to force service settings. In this case, the only thing force is good for is stopping the service. We already have Stop-Service -Force for that. For desired state, you are not likely going to be forcing the service to stop. Again, this adds complexity and redundancy, and I'm arguing for simplicity.

Edit: as a compromise, if everyone really thinks that having set- force stop a service is not as crazy as I think it is, how about -Status StoppedForced or something instead of -Force switch that only works for a specific single value on a single parameter?

Adding a -Force switch to Start-Service is not a solution, because there is no sensible default for the startup type that invariably has to change in that case

I disagree, setting it to manual, when you are "manually" forcing the service to start certainly serves as a sensible setting. But this is a discussion for another time, lets not continue it in this thread.

SteveL-MSFT commented 6 years ago

@iSazonov committee is on taking time off this week for Thanksgiving (US holiday), will resume next week

SteveL-MSFT commented 6 years ago

@PowerShell/powershell-committee reviewed this. Given that -Status Stopped is already there, it would make sense to support -Force for Set-Service

adityapatwardhan commented 6 years ago

@zhenggu Please add tests at : https://github.com/PowerShell/PowerShell/blob/master/test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1

markekraus commented 6 years ago

@SteveL-MSFT I would like the comittee to reconsider the approval of -Force for this PR. RE: https://github.com/PowerShell/PowerShell/pull/6113#discussion_r167348028 and a few other discussions we have had here. The -Force in this PR not only applies to a single parameter, it also only applies to a single option for that parameter. I still believe including a ForceStopped or StoppedForced option for -Status is the better design choice and that we should avoid -Force abuse.

SteveL-MSFT commented 6 years ago

@PowerShell/powershell-committee reviewed this. The only applicable parameter to -Force is -Status (when value is Stopped) and there's no applicability to other parameters for this cmdlet. The usage is consistent with other cmdlets like Stop-Service, so committee continues to recommend using -Force for this case.

adityapatwardhan commented 6 years ago

@zhenggu Thanks for your contribution. Please add the PR submission template and update the appropriate fields. https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#pull-request---submission

stale[bot] commented 6 years ago

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days. Thank you for your contributions.

stale[bot] commented 6 years ago

This PR has be automatically close because it is stale. If you wish to continue working on the PR, please first update the PR, then reopen it. Thanks again for your contribution. Community members are welcome to grab these works.

zhenggu commented 5 years ago

I want to reopen this PR, but how can I do it?

iSazonov commented 5 years ago

@zhenggu The PR is reopened. Thanks!

zhenggu commented 5 years ago

Thanks @iSazonov

zhenggu commented 5 years ago

any idea about CodeFactor issues? Seems no issue is caused by my code.

iSazonov commented 5 years ago

@zhenggu You must fix style issues in new code and changed code but you shouldn't fix all CodeFactor issues.

zhenggu commented 5 years ago

@iSazonov got it, I will take care of this

iSazonov commented 5 years ago

I don't see style errors in your code.

zhenggu commented 5 years ago

one question, There are many test cases uses pwsh.exe as the the binary file of the service in Set-Service.Tests.ps1, but actually we cannot run pwsh.exe as a service for it will not response any thing to the OS. As in this way, can I implement a new service by powershell to test the stop case, or I can just use wuauserv in #5517 as the service.

zhenggu commented 5 years ago

I have added one test case to check -Status Stopped can run normally to stop wuauserv.

zhenggu commented 5 years ago

Hi @iSazonov, can you please help to review again.

zhenggu commented 5 years ago

It will be difficult to add test cases -Force parameter, for I cannot find two services which has dependency relationship, and I can stop them both without any impact to the test windows server. Any suggestion?

iSazonov commented 5 years ago

We could create simple custom services for the test.

zhenggu commented 5 years ago

@iSazonov So can I put my custom service into test\powershell\Modules\Microsoft.PowerShell.Management\assets? And is there any variable which points to the source code base directory in the test case?

iSazonov commented 5 years ago

I guess it will be binary test module in /test/tools/. I can look WebListener as a sample.

zhenggu commented 5 years ago

@iSazonov Thanks very much

zhenggu commented 5 years ago

Have added one TestService written in C, but still don't know how to let the whole project call this CMakeFile.txt. And if it is called, will it be built in the same directory as pwsh? Please help to confirm them, and then I can continue. Thanks

iSazonov commented 5 years ago

@adityapatwardhan @JamesWTruher Could you please help? Perhaps there is more simple way to create test windows services to test *-Service cmdlets.

zhenggu commented 5 years ago

For I don't find a way to add a custom service, remove force flag and do the bug fix first.

iSazonov commented 5 years ago

@zhenggu You could look https://www.c-sharpcorner.com/UploadFile/naresh.avari/develop-and-install-a-windows-service-in-C-Sharp/

zhenggu commented 5 years ago

Thanks @iSazonov for your always support.

zhenggu commented 5 years ago

@iSazonov there is one interesting thing that when I try your way, System.ServiceProcess.ServiceBase is only provided by .net framework, not .net core, according: http://dotnet.github.io/dotnet-web/api/System.ServiceProcess.html, it seems there is one RP of dotnet to add this: https://github.com/dotnet/corefx/pull/22920, but it is in minestone 2.1.0, and we are still using 2.0.0 according the build document.

TravisEz13 commented 5 years ago

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1, line 339 at r4 (raw file):

Previously, zhenggu wrote…

Thanks @iSazonov I will modify it.

I don't think spooler always exists. Can we add a check if it exists ahead of time and skip the test if it doesn'?

TravisEz13 commented 5 years ago

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1, line 339 at r4 (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

I don't think spooler always exists. Can we add a check if it exists ahead of time and skip the test if it doesn'?

I verified. I believe this is the minimal set of services:

Status   Name               DisplayName
------   ----               -----------
Running  cexecsvc           Container Execution Agent
Running  CryptSvc           Cryptographic Services
Running  DcomLaunch         DCOM Server Process Launcher
Running  Dhcp               DHCP Client
Running  DiagTrack          Connected User Experiences and Tele...
Running  Dnscache           DNS Client
Running  EventLog           Windows Event Log
Stopped  KeyIso             CNG Key Isolation
Stopped  LanmanServer       Server
Running  LanmanWorkstation  Workstation
Stopped  lmhosts            TCP/IP NetBIOS Helper
Stopped  mpssvc             Windows Defender Firewall
Stopped  Netlogon           Netlogon
Stopped  NetSetupSvc        Network Setup Service
Running  nsi                Network Store Interface Service
Stopped  Power              Power
Running  ProfSvc            User Profile Service
Running  RpcEptMapper       RPC Endpoint Mapper
Running  RpcSs              Remote Procedure Call (RPC)
Running  SamSs              Security Accounts Manager
Running  Schedule           Task Scheduler
Stopped  seclogon           Secondary Logon
Running  SystemEventsBroker System Events Broker
Running  TimeBrokerSvc      Time Broker
Stopped  TrustedInstaller   TrustedInstaller
Running  UserManager        User Manager
Stopped  VaultSvc           Credential Manager
Stopped  WerSvc             Windows Error Reporting Service
Stopped  WinHttpAutoProx... WinHTTP Web Proxy Auto-Discovery Se...
Stopped  wisvc              Windows Insider Service

zhenggu commented 5 years ago

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1, line 339 at r4 (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

I verified. I believe this is the minimal set of services: ``` Status Name DisplayName ------ ---- ----------- Running cexecsvc Container Execution Agent Running CryptSvc Cryptographic Services Running DcomLaunch DCOM Server Process Launcher Running Dhcp DHCP Client Running DiagTrack Connected User Experiences and Tele... Running Dnscache DNS Client Running EventLog Windows Event Log Stopped KeyIso CNG Key Isolation Stopped LanmanServer Server Running LanmanWorkstation Workstation Stopped lmhosts TCP/IP NetBIOS Helper Stopped mpssvc Windows Defender Firewall Stopped Netlogon Netlogon Stopped NetSetupSvc Network Setup Service Running nsi Network Store Interface Service Stopped Power Power Running ProfSvc User Profile Service Running RpcEptMapper RPC Endpoint Mapper Running RpcSs Remote Procedure Call (RPC) Running SamSs Security Accounts Manager Running Schedule Task Scheduler Stopped seclogon Secondary Logon Running SystemEventsBroker System Events Broker Running TimeBrokerSvc Time Broker Stopped TrustedInstaller TrustedInstaller Running UserManager User Manager Stopped VaultSvc Credential Manager Stopped WerSvc Windows Error Reporting Service Stopped WinHttpAutoProx... WinHTTP Web Proxy Auto-Discovery Se... Stopped wisvc Windows Insider Service ```

have added a pre check of spooler for that.

zhenggu commented 5 years ago

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1, line 339 at r4 (raw file):

Previously, zhenggu wrote…

have added a pre check of spooler for that.

Done.

iSazonov commented 5 years ago

there is one interesting thing that when I try your way, System.ServiceProcess.ServiceBase is only provided by .net framework, not .net core,

The API is in WCP https://blogs.msdn.microsoft.com/dotnet/2017/11/16/announcing-the-windows-compatibility-pack-for-net-core/ . We use WCP in PowerShell Core and I think we can use it in test module too.

zhenggu commented 5 years ago

any other thing can I do for this PR?

SteveL-MSFT commented 5 years ago

@TravisEz13 can you continue your review?

TravisEz13 commented 5 years ago

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1, line 358 at r12 (raw file):

        (Get-Service $testservicename2).Status | Should -BeExactly "Stopped"

        # stop a service with running dependent service

It seems that this should be a separate it statement and Set-Service -Status Running $testservicename2 | Should -Not -Throw should be part of a BeforeEach

TravisEz13 commented 5 years ago

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Service.Tests.ps1, line 367 at r12 (raw file):

        (Get-Service $testservicename2).Status | Should -BeExactly "Running"

        # stop a service with running dependent service by -Force

This looks like a separate it as well, maybe a separate context because it may have a different BeforeEach

TravisEz13 commented 5 years ago

test/tools/TestService/Program.cs, line 1 at r12 (raw file):

using System.ServiceProcess;

missing copyright headers

TravisEz13 commented 5 years ago

test/tools/TestService/Service1.cs, line 1 at r12 (raw file):

using System.ServiceProcess;

missing copyright headers

PowerShell / PowerShell

Fix Set-Service -Status Stopped runs failed on the service with dependencies #5525

PR Summary

PR Checklist