Skip to content

Conversation

@zhenggu
Copy link
Contributor

@zhenggu zhenggu commented Nov 22, 2017

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


This change is Reviewable

…ents services And 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. (PowerShell#5517)
@msftclas
Copy link

msftclas commented Nov 22, 2017

CLA assistant check
All CLA requirements met.

Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

@mklement0 Could you please review the PR?


if ((servicedependedon != null) && (servicedependedon.Length > 0))
{
WriteNonTerminatingError(service, null, "ServiceIsDependentOnNoForce", ServiceResources.ServiceIsDependentOnNoForce, ErrorCategory.InvalidOperation);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should remove the resource string from RESX file it is no longer used.

if (!service.Status.Equals(ServiceControllerStatus.Stopped))
{
//check for the dependent services as set-service dont have force parameter
//check for the dependent services
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please correct the comment "// Check ...."

@markekraus
Copy link
Contributor

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
Copy link
Collaborator

We have a similar question in #5471

@zhenggu
Copy link
Contributor Author

zhenggu commented Nov 23, 2017

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
Copy link
Collaborator

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

@mklement0
Copy link
Contributor

mklement0 commented Nov 23, 2017

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
Copy link
Collaborator

@mklement0 Thanks for useful comments!

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

Design review needed.

@markekraus
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

markekraus commented Nov 23, 2017

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
Copy link
Member

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

@TravisEz13 TravisEz13 added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Dec 7, 2017
@SteveL-MSFT
Copy link
Member

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

@SteveL-MSFT SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Jan 4, 2018
@daxian-dbw daxian-dbw requested a review from anmenaga February 1, 2018 17:51
@adityapatwardhan adityapatwardhan changed the title Fix the Set-Service -Status Stopped issue #5517 Fix the Set-Service -Status Stopped issue Feb 1, 2018
@adityapatwardhan
Copy link
Member

@markekraus
Copy link
Contributor

markekraus commented Feb 9, 2018

@SteveL-MSFT I would like the comittee to reconsider the approval of -Force for this PR. RE: #6113 (comment) 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 SteveL-MSFT added Review - Committee The PR/Issue needs a review from the PowerShell Committee and removed Committee-Reviewed PS-Committee has reviewed this and made a decision labels Feb 10, 2018
@SteveL-MSFT
Copy link
Member

@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.

@SteveL-MSFT SteveL-MSFT added the Committee-Reviewed PS-Committee has reviewed this and made a decision label Feb 21, 2018
@TravisEz13
Copy link
Member


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

namespace TestService

missing copyright headers

Copy link
Member

@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r8, 6 of 7 files at r11, 1 of 1 files at r12.
Reviewable status: :shipit: complete! all files reviewed

@zhenggu
Copy link
Contributor Author

zhenggu commented Aug 7, 2018

@TravisEz13 Please help to review again. Thanks.

Copy link
Member

@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r13.
Reviewable status: :shipit: complete! all files reviewed

@zhenggu
Copy link
Contributor Author

zhenggu commented Aug 16, 2018

Does this PR need another review?

@anmenaga
Copy link

@adityapatwardhan This looks ready for merge.

@adityapatwardhan
Copy link
Member

@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

The template is here: https://github.com/PowerShell/PowerShell/blob/master/.github/PULL_REQUEST_TEMPLATE.md

@zhenggu
Copy link
Contributor Author

zhenggu commented Aug 22, 2018

@adityapatwardhan for this PR adds a "-Force" parameter for Set-Service, but I am not sure should I modify https://github.com/PowerShell/PowerShell-Docs/blob/staging/reference/6/Microsoft.PowerShell.Management/Set-Service.md before this PR merged or it should be changed after it this PR is merged?

@iSazonov
Copy link
Collaborator

@zhenggu Please open an issue or PR in PowerShell-Docs repo and add the link here.

@zhenggu
Copy link
Contributor Author

zhenggu commented Aug 24, 2018

@iSazonov Thanks, will add it

zhenggu added a commit to zhenggu/PowerShell-Docs that referenced this pull request Aug 27, 2018
@zhenggu
Copy link
Contributor Author

zhenggu commented Aug 27, 2018

@adityapatwardhan have added the document, and all the actions are ready, please help to review.

sdwheeler pushed a commit to MicrosoftDocs/PowerShell-Docs that referenced this pull request Aug 27, 2018
@adityapatwardhan
Copy link
Member

adityapatwardhan commented Aug 27, 2018

@zhenggu The tests are classified as Feature so I pushed an empty commit with [Feature] to run all tests. If all tests pass, I will go ahead and merge.

@adityapatwardhan
Copy link
Member

@zhenggu Please have a look at the failures and for the next commit have [Feature] in your commit message to ensure all tests are executed. See this for more details.

@zhenggu
Copy link
Contributor Author

zhenggu commented Aug 28, 2018

@adityapatwardhan, all test cases passed, please help to merge.

@adityapatwardhan adityapatwardhan merged commit 358e8ab into PowerShell:master Aug 28, 2018
@adityapatwardhan
Copy link
Member

@zhenggu Thank you for your contribution!

@zhenggu
Copy link
Contributor Author

zhenggu commented Aug 28, 2018

Thanks everyone's help to my first PR

@iSazonov
Copy link
Collaborator

@zhenggu Thanks for your contribution and patience! I hope you will continue.

joeyaiello pushed a commit to MicrosoftDocs/PowerShell-Docs that referenced this pull request Sep 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Set-Service -Status Stopped should not prevent stopping of services without dependents only because they depend on others

9 participants