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

[master] Deprecate the multiprocessing process long names #55053

Merged
merged 3 commits into from
Oct 20, 2019

Conversation

s0undt3ch
Copy link
Collaborator

What does this PR do?

Deprecate the multiprocessing process long names.

Who on earth named these classes?!

Oh. It was me.
I must have been insane at the time because these names are absurd.

This PR should only be merged after #55047 since it includes code from that PR to provide the deprecation by date feature.

Previous Behavior

We had insane names two classes.

New Behavior

We have proper names for those two classes.
The previous names still exist but are deprecated and set to be removed on the first release after Jan, 1st, 2022.

Tests written?

No, this is just deprecating class names.

Commits signed with GPG?

Yes

@s0undt3ch s0undt3ch requested a review from dwoz October 18, 2019 09:06
@s0undt3ch s0undt3ch requested a review from a team as a code owner October 18, 2019 09:06
@s0undt3ch s0undt3ch force-pushed the hotfix/i-was-insane branch 2 times, most recently from 2e3b3d5 to f40beb5 Compare October 18, 2019 10:17
@rares-pop
Copy link
Contributor

The change looks good, but we can do more cleanup vis-à-vis of this class. More specifically regarding the _is_child abstraction leakage (where any class that inherits from this has to specify '_is_child = True' in order to behave correctly.

@s0undt3ch
Copy link
Collaborator Author

@rares-pop _is_child seems like an artefact of old code that was left behind. You mean remove these occurrences?

@s0undt3ch s0undt3ch force-pushed the hotfix/i-was-insane branch from f40beb5 to 4fe3e80 Compare October 18, 2019 13:16
@rares-pop
Copy link
Contributor

I don't think we can remove it (without refactoring the code, which is what I was suggesting). The Windows behavior relies on this in order to work properly (the only reason it's there is so that on Windows, those log_queue and log_queue_level gets pickled/unpickled, so the logging works in the spawned processes. Without this, the logs won't get processed and they will get leaked (I still have an open PR with enabling logging for salt-api, as opposed to leak the logs)).

What I think should happen here is to have another class on the inheritance chain that can have the '_is_child=True', and have client classes inherit from that, so they are not forced to say a random '_is_child=True' which they have no knowledge about. I heard about that the hard way. :)

@s0undt3ch
Copy link
Collaborator Author

Well, this PR is pre-work for #55043 which refactors logging, mostly because it has grew too complicated and on windows it does leak(we can't switch to pytest on windows right now because on this multiprocessing leakage).

However, _is_child is no longer in use(at least on the master branch), it just looks like some leftovers from whatever tweaks we once did.
I'm removing those keys in this PR.

@rares-pop
Copy link
Contributor

rares-pop commented Oct 18, 2019

Oh, I wasn't aware of the other one. I'll take a look on it.

I do use both salt-master and salt-minion, along with salt-api on Windows, and have systems running for months. I can't say they are not leaking, but I can say that in case they do, it's not obvious and a showstopper. LE: I lied - I know there are leaks, they are mentioned in the below referenced PR.

If you get rid of '_is_child', please make sure it still works on Windows.

This is the aforementioned PR: #52471

@s0undt3ch s0undt3ch force-pushed the hotfix/i-was-insane branch from 4fe3e80 to d9c9c24 Compare October 18, 2019 15:16
@dwoz
Copy link
Contributor

dwoz commented Oct 19, 2019

The is_child attribute was removed by me in 2019.2.1. It is safe to remove it from these classes and everything still works on windows.

Who on earth named these classes?!

Oh. It was me.
I must have been insane at the time because these names are absurd.
@s0undt3ch s0undt3ch force-pushed the hotfix/i-was-insane branch from d9c9c24 to 4cfd891 Compare October 19, 2019 07:47
@dwoz dwoz merged commit c157cb7 into saltstack:master Oct 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants