Add filter status to ps module #61421

remyd1 · 2022-01-05T15:35:50Z

What does this PR do?

It allows to retrieve a list of processes according to their state, using psutil.

What issues does this PR fix or reference?

Fixes: #61420

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Docs
Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
Tests written/updated

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

waynew

Hey @remyd1 thanks for the PR! This seems like a useful addition (without checking to see if there's anything else that could accomplish this - I'm assuming that you did that already, given the feature request!)

One thing that this would require is tests.
If you need help writing tests, attend one of the Test Clinics on Twitch!

salt/modules/ps.py

remyd1 · 2022-01-17T09:32:51Z

I would still need a bit of help to write the corresponding test clinic. I looked at tour twitch streams but I am still kind of a bit lost.

remyd1 · 2022-01-19T16:17:18Z

Tests updated !

Thanks @waynew

Ch3LL

Just a small change request. Also there are failing tests related to this change that need to be fixed.

Ch3LL · 2022-03-15T20:26:55Z

salt/modules/ps.py

+    Returns a list of processes according to their states.
+    See https://psutil.readthedocs.io/en/latest/index.html\
+?highlight=status#process-status-constants
+


.. versionadded:: 3005

Well, 3006 now 😂

added (and updated the docstring a bit)

Ch3LL · 2022-08-31T19:24:23Z

@remyd1 are you able to come back to this PR and fix the tests and fix the feedback submitted?

github-actions · 2022-09-01T07:11:06Z

Hi! I'm your friendly PR bot!

You might be wondering what I'm doing commenting here on your PR.

Yes, as a matter of fact, I am...

I'm just here to help us improve the documentation. I can't respond to
questions or anything, but what I can do, I do well!

Okay... so what do you do?

I detect modules that are missing docstrings or "CLI Example" on existing docstrings!
When I was created we had a lot of these. The documentation for these
modules need some love and attention to make Salt better for our users.

So what does that have to do with my PR?

I noticed that in this PR there are some files changed that have some of these
issues. So I'm leaving this comment to let you know your options.

Okay, what are they?

Well, my favorite, is that since you were making changes here I'm hoping that
you would be the most familiar with this module and be able to add some other
examples or fix any of the reported issues.

If I can, then what?

Well, you can either add them to this PR or add them to another PR. Either way is fine!

Well... what if I can't, or don't want to?

That's also fine! We appreciate all contributions to the Salt Project. If you
can't add those other examples, either because you're too busy, or unfamiliar,
or you just aren't interested, we still appreciate the contributions that
you've made already.

Whatever approach you decide to take, just drop a comment here letting us know!

Detected Issues (click me)

Check Known Missing Docstrings...........................................Failed
- hook id: invoke
- exit code: 1
/home/runner/.cache/pre-commit/repo_20aoa51/py_env-python3/lib/python3.9/site-packages/_distutils_hack/init.py:33: UserWarning: Setuptools is replacing distutils.

warnings.warn("Setuptools is replacing distutils.")

The function 'kill_pid' on 'salt/modules/ps.py' does not have a 'CLI Example:' in its docstring

The function 'pkill' on 'salt/modules/ps.py' does not have a 'CLI Example:' in its docstring

The function 'pgrep' on 'salt/modules/ps.py' does not have a 'CLI Example:' in its docstring

Found 3 errors

Thanks again!

remyd1 · 2022-09-01T09:03:09Z

@remyd1 are you able to come back to this PR and fix the tests and fix the feedback submitted?

Hi @Ch3LL , unfortunately I have no more time to dig into those test cases. Moreover, I am a bit lost with Mock, MagicMock and associated Salt methods...

If anyone could fix these tests, it would be helpful.

Thanks,
Best regards,

waynew · 2022-10-17T18:09:39Z

@remyd1 I can take a look at this during tomorrow's Test Clinic. If you don't have the time/inclination to finish it up I may be able to get it sorted out 👍

remyd1 · 2022-10-18T11:50:05Z

Indeed, if you could check what is wrong here, it would be great 👍🏼

waynew · 2022-10-21T00:01:31Z

@remyd1 Didn't I look into this on Tuesday's Test Clinic? Were you able to update the tests here or do you need a bit more help to get this done?

remyd1 · 2022-10-23T13:24:27Z

Hi @waynew

No, sorry. As I said, I have less time to spend on this. Did you do stream some more tests on it ? Could you give me back you Twitch URL ? Anyway, I do not think I will be able to achieve this.

Thanks,

waynew · 2022-10-24T18:41:27Z

@remyd1 https://twitch.tv/saltprojectoss

waynew · 2022-11-15T22:19:05Z

@Ch3LL / @remyd1 I went ahead and updated this on today's Test Clinic. I also took the opportunity to migrate the unittest to pytest since it wasn't super duper involved.

Assuming tests all pass this should be good to go now 👍

Ch3LL · 2022-11-16T19:46:26Z

@waynew looks like there are related test failures

waynew · 2022-11-17T16:07:16Z

Doh! Yeah, looks like I didn't understand the API of the Process quite well
enough. Just need to mock a few more things - doing that on today's Test
Clinic!

Ch3LL · 2022-11-22T20:34:37Z

Looks like just window, mac and windows are failing now

…o their state

@Ch3LL

Renamed `filter` to `status` to match up with the psutil.Process.status. Also added a comment to call out the singular status vs multiple, and mention how that could be added in the future. Also added versionadded to the docs, re: @Ch3LL's feedback. Refactored the module a bit to pass tests, as well as simplify the return bits. There was no need to check that the return type was a generator - psutil.process_iter is going to yield always, and we don't really care about anything else getting mocked, we just want to iterate over it. Since psutil.Process already has a `.as_dict(attrs=...)` where we can filter by the attributes we care about, we can go ahead and use that instead of having to `pop` the ones we don't care about.

The problem: psutil has Process objects for different platforms, and those actually try to parse some system output bits that may not exist when we don't have a process with that PID. Since we're creating one with PID 42, we need to mock out all the shenanigans that the underlying Process will use. Specifically currently that's: - create_time - _parse_stat_file - status

On other platforms this attribute doesn't exist, but we don't really care. If it exists it shouldn't be a problem. So go ahead and create=True.

salt/modules/ps.py

Co-authored-by: Megan Wilhite <mwilhite@vmware.com>

waynew · 2022-12-16T18:10:01Z

@Ch3LL failures didn't look related - one looked like the underlying OS/Docker had an issue (completely unrelated to this module) and the others that I saw claimed to be a mem leak issue but... I'm guessing that's also a flaky test

remyd1 requested a review from a team as a code owner January 5, 2022 15:35

remyd1 requested review from waynew and removed request for a team January 5, 2022 15:35

waynew suggested changes Jan 13, 2022

View reviewed changes

salt/modules/ps.py Outdated Show resolved Hide resolved

salt/modules/ps.py Outdated Show resolved Hide resolved

remyd1 requested a review from waynew January 19, 2022 16:17

Ch3LL suggested changes Mar 15, 2022

View reviewed changes

Ch3LL added the has-failing-test label Mar 16, 2022

This comment was marked as outdated.

Sign in to view

waynew force-pushed the add-filter-status-to-ps-module branch from 426b32d to c814043 Compare November 15, 2022 22:17

waynew force-pushed the add-filter-status-to-ps-module branch 2 times, most recently from 530768f to 647b17b Compare November 29, 2022 21:55

remyd1 and others added 5 commits December 13, 2022 14:54

add filter status to ps module to get a list of processes according t…

0e98b13

…o their state

adding changelog

258dcbd

modifications from @waynew PR#61421

b37a356

add some sample tests

a3374c7

need to add some test coverage to ps module for status filter

7e6e676

remyd1 and others added 8 commits December 13, 2022 14:54

reformat

2044ec0

port modules.ps tests to pytest

d31d974

Go ahead and use extra data

cf68548

Go ahead and create _parse_stat_file

909d48b

On other platforms this attribute doesn't exist, but we don't really care. If it exists it shouldn't be a problem. So go ahead and create=True.

Mock osx and bsd attrs as well

b1b5d09

more bsd time keys

6a2cc33

waynew force-pushed the add-filter-status-to-ps-module branch from 2001793 to 6a2cc33 Compare December 13, 2022 20:55

rename filter to status

0181f3d

waynew previously approved these changes Dec 15, 2022

View reviewed changes

Ch3LL suggested changes Dec 15, 2022

View reviewed changes

salt/modules/ps.py Outdated Show resolved Hide resolved

Add .0 to version

c534bd1

Co-authored-by: Megan Wilhite <mwilhite@vmware.com>

waynew dismissed their stale review via c534bd1 December 15, 2022 14:56

Ch3LL approved these changes Dec 15, 2022

View reviewed changes

Ch3LL added the Sulfur v3006.0 release code name and version label Dec 15, 2022

waynew added this to the Sulphur v3006.0 milestone Dec 16, 2022

Merge branch 'master' into add-filter-status-to-ps-module

c6ead08

Ch3LL merged commit f43de41 into saltstack:master Dec 22, 2022

waynew removed the has-failing-test label Jan 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add filter status to ps module #61421

Add filter status to ps module #61421

remyd1 commented Jan 5, 2022 •

edited by waynew

Loading

waynew left a comment

remyd1 commented Jan 17, 2022 •

edited

Loading

remyd1 commented Jan 19, 2022 •

edited

Loading

Ch3LL left a comment

Ch3LL Mar 15, 2022

waynew Oct 17, 2022

waynew Nov 15, 2022

Ch3LL commented Aug 31, 2022

This comment was marked as outdated.

github-actions bot commented Sep 1, 2022

remyd1 commented Sep 1, 2022

waynew commented Oct 17, 2022

remyd1 commented Oct 18, 2022

waynew commented Oct 21, 2022

remyd1 commented Oct 23, 2022

waynew commented Oct 24, 2022

waynew commented Nov 15, 2022

Ch3LL commented Nov 16, 2022

waynew commented Nov 17, 2022

Ch3LL commented Nov 22, 2022

waynew commented Dec 16, 2022

Add filter status to ps module #61421

Add filter status to ps module #61421

Conversation

remyd1 commented Jan 5, 2022 • edited by waynew Loading

What does this PR do?

What issues does this PR fix or reference?

Merge requirements satisfied?

Commits signed with GPG?

waynew left a comment

Choose a reason for hiding this comment

remyd1 commented Jan 17, 2022 • edited Loading

remyd1 commented Jan 19, 2022 • edited Loading

Ch3LL left a comment

Choose a reason for hiding this comment

Ch3LL Mar 15, 2022

Choose a reason for hiding this comment

waynew Oct 17, 2022

Choose a reason for hiding this comment

waynew Nov 15, 2022

Choose a reason for hiding this comment

Ch3LL commented Aug 31, 2022

This comment was marked as outdated.

github-actions bot commented Sep 1, 2022

Hi! I'm your friendly PR bot!

remyd1 commented Sep 1, 2022

waynew commented Oct 17, 2022

remyd1 commented Oct 18, 2022

waynew commented Oct 21, 2022

remyd1 commented Oct 23, 2022

waynew commented Oct 24, 2022

waynew commented Nov 15, 2022

Ch3LL commented Nov 16, 2022

waynew commented Nov 17, 2022

Ch3LL commented Nov 22, 2022

waynew commented Dec 16, 2022

remyd1 commented Jan 5, 2022 •

edited by waynew

Loading

remyd1 commented Jan 17, 2022 •

edited

Loading

remyd1 commented Jan 19, 2022 •

edited

Loading