Skip to content
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.

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

Add timeout parameter to win_service functions #49144

Merged
merged 9 commits into from Aug 22, 2018

Conversation

Projects
None yet
3 participants
@twangboy
Copy link
Contributor

commented Aug 15, 2018

What does this PR do?

Adds a timeout parameter to the following functions in win_service:

  • start
  • stop
  • restart
  • delete

Fix some documentation formatting issues

What issues does this PR fix or reference?

#48163

Tests written?

No

Commits signed with GPG?

Yes

Add timeout parameter
Add a timeout parameter to start, stop, restart, and delete

@salt-jenkins salt-jenkins requested a review from saltstack/team-windows Aug 15, 2018

Add timeout support to the state
Remove redundent net stop command from the stop function
Remove extra _enable call on Windows that was causing incorrect state
return comments
@damon-atkins
Copy link
Member

left a comment

Please use "time code" for timeout rather than attempts

@@ -409,38 +411,33 @@ def start(name):

attempts = 0
while info(name)['Status'] in ['Start Pending', 'Stopped'] \
and attempts <= RETRY_ATTEMPTS:
and attempts <= timeout:

This comment has been minimized.

Copy link
@damon-atkins

damon-atkins Aug 16, 2018

Member

The time out is doco as seconds not attempts please use the following code

    end_t = time.time() + timeout
    info_results =  info(name)
    while info_results['Status'] in ['Start Pending', 'Stopped'] \
          and time.time() < end_t:
        wait_time = info_results['WaitHint']/1000 if WaitHint in info_results and info_results['WaitHint'] else 0.25
        time.sleep(wait_time)
        info_results =  info(name)

PS WaitHint looks not to be a status but the time to wait before checking again. You need to check if its always return by info().
Two options for
end_t = time.time() + timeout

  • Place before initial start request to windows so its a timeout for the whole process of starting. i.e. before the try.
  • Or Place just before this loop and document its timeout for waiting for the state to become running after the request is made which itself as a 30 second timeout therefore the maximum timeout out is 30 seconds + timeout provided in seconds.

win32serviceutil.StartService(name) has its own timeout of 30 seconds.
https://docs.microsoft.com/en-us/windows/desktop/api/winsvc/nf-winsvc-startservicectrldispatchera
https://docs.microsoft.com/en-us/windows/desktop/api/winsvc/nf-winsvc-startservicea

could place the code in a private function.

def _wait_while_status_is(service_name,end_time,*service_states)
    info_results =  info(service_name)
    while info_results['Status'] in service_states and time.time() < end_time:
        # From MS. Do not wait longer than the wait hint. A good interval is  one-tenth of the
        # wait hint but not less than 1 second  and not more than 10 seconds.
        wait_time = info_results['WaitHint']/1000 if WaitHint in info_results and info_results['WaitHint'] else 0.25
        time.sleep(wait_time)
        info_results =  info(service_name)
    return info_results

Then call
_wait_while_status_is(name, end_t, 'Start Pending', 'Stopped')

@@ -450,26 +447,35 @@ def stop(name):

attempts = 0
while info(name)['Status'] in ['Running', 'Stop Pending'] \
and attempts <= RETRY_ATTEMPTS:
and attempts <= timeout:

This comment has been minimized.

Copy link
@damon-atkins
@@ -1282,7 +1319,7 @@ def delete(name):
win32service.CloseServiceHandle(handle_svc)

attempts = 0
while name in get_all() and attempts <= RETRY_ATTEMPTS:
while name in get_all() and attempts <= timeout:

This comment has been minimized.

Copy link
@damon-atkins
2017_win_service_damon
Merge this if you like the suggestion.
@damon-atkins

This comment has been minimized.

Copy link
Member

commented Aug 16, 2018

open a PR against your fix.

twangboy added some commits Aug 16, 2018

Fix start/stop functions
Fix typo in the dict key 'Status'
Standardize docs
Fix some lint issues
Rename the new helper function to _status_wait
Add urls to code comments
Add version added statements
Improve timeout in delete
Improved error handling
@damon-atkins

This comment has been minimized.

Copy link
Member

commented Aug 17, 2018

Hi twangboy,
win32serviceutil.[Start|Stop]Service(name) has a 30 seconds time out to kick off starting/stopping the service.

So base on the current code the timeout will be 30 (ish) + 90 (default) = 120 timeout.

Some people may expect the function to return after 90 seconds with the current explanation and raise an issue that the timeout does not work.

To prevent issues being raised I suggest maybe one of these options ,
a) move the end_t = time.time() + int(timeout) above the try if keeping the current description of timeout
OR
b) timeout description should say its for Stop Pending to Stopped / Start Pending to Running

From what I have read it seems win32serviceutil.[Start|Stop]Service(name) should initially result in a Pending status or fail.

Everything else looks good.

# than 10 seconds.
# https://docs.microsoft.com/en-us/windows/desktop/services/starting-a-service
# https://docs.microsoft.com/en-us/windows/desktop/services/stopping-a-service
wait_time = info_results['Status_WaitHint'] / 1000

This comment has been minimized.

Copy link
@damon-atkins

damon-atkins Aug 17, 2018

Member

Choose which is best to read.

# wait hint is in ms.
wait_time = info_results['Status_WaitHint'] / 1000 if info_results['Status_WaitHint'] else 0
if wait_time <= 1:
    wait_time = 1
elif wait_time > 10:
    wait_time = 10
# remove the else.

OR

wait_time = info_results['Status_WaitHint']
if wait_time <= 10000:  # 10 seconds
     wait_time = 1  # 10sec/10
elif wait_time > 100000: # 100 seconds
     wait_time = 10 # 100sec/10
else:
     wait_time = wait_time  / 10000 # convert to seconds, and divide by 10
@twangboy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2018

@damon-atkins Could you please review this one last time. I implemented the changes you suggested.

@damon-atkins
Copy link
Member

left a comment

DONE PR update with some more words around the timeout

@gtmanfred gtmanfred merged commit 1900aff into saltstack:2017.7 Aug 22, 2018

8 checks passed

WIP ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details

@twangboy twangboy deleted the twangboy:fix_48163 branch Dec 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.