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

NAS-133058 / 25.04 / simplify service.terminate_process #15194

Merged
merged 7 commits into from
Dec 12, 2024
Merged

Conversation

yocalebo
Copy link
Contributor

psutil is expensive in about every way imaginable. This method doesn't require using it so I've simplified the method to use built-in modules provided by python.

  1. add validation to terminate_process since it allowed terminating pid 0 or middlewared (itself)
  2. make it private, there is no reason for this to be a public method (UI, TC doesn't call it)

@yocalebo yocalebo requested a review from a team December 12, 2024 19:02
@bugclerk bugclerk changed the title simplify service.terminate_process NAS-133058 / 25.04 / simplify service.terminate_process Dec 12, 2024
@bugclerk
Copy link
Contributor

@yocalebo yocalebo requested review from themylogin and a team December 12, 2024 20:11
Copy link
Contributor

@themylogin themylogin left a comment

Choose a reason for hiding this comment

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

Thank you

Returns:
boolean: True if process was terminated with SIGTERM, false if SIGKILL was used
"""
if pid == 0 or pid == os.getpid():
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at kill(2) man page I wonder whether we should also eliminate all negative values of pid.

@yocalebo yocalebo merged commit c3ba0b4 into master Dec 12, 2024
2 checks passed
@yocalebo yocalebo deleted the fix-svc-term branch December 12, 2024 20:52
@bugclerk
Copy link
Contributor

This PR has been merged and conversations have been locked.
If you would like to discuss more about this issue please use our forums or raise a Jira ticket.

@truenas truenas locked as resolved and limited conversation to collaborators Dec 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants