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

[BUG] 3006.8 DeprecationWarning salt.utils.psutil_compat #66467

Closed
2 tasks done
RolandRosenfeld opened this issue May 6, 2024 · 16 comments
Closed
2 tasks done

[BUG] 3006.8 DeprecationWarning salt.utils.psutil_compat #66467

RolandRosenfeld opened this issue May 6, 2024 · 16 comments
Assignees
Labels

Comments

@RolandRosenfeld
Copy link

Description
I use salt-minion on Debian 12 (bookworm) from
https://repo.saltproject.io/salt/py3/debian/12/amd64/3006 bookworm main
repository.
Since upgrading from 3006.7 to 3006.8
salt-call state.apply test=true
reports the following derecation warning on every call:

/opt/saltstack/salt/lib/python3.10/site-packages/salt/utils/psutil_compat.py:16: DeprecationWarning: Please stop importing 'salt.utils.psutil_compat' and instead import 'psutil' directly as there's no longer a need for a compatability layer. The 'salt.utils.psutil_compat' will go away on Salt 3008.0 (Argon).
  salt.utils.versions.warn_until(

Since I use the onedir install, which bundles all python modules in salt-common package, I'd expect that this warning does not appear but is fixed.

I observe this issue on all my Debian 12 servers after the upgrade from 3006.7 to 3006.8.

Setup

  • VM (Virtualbox, KVM, etc. please specify)
  • onedir packaging

Steps to Reproduce the behavior
Upgrade from 3006.7 to 3006.8.
salt-call state.apply test=true
(adding any state name results in the same warning)

A remote salt myhost state.apply test=true call writes the same message to the journal of salt-minion.

Expected behavior
No deprecation warnings on console or log if I use only bundled SaltStack 3006.8 software.

Versions Report

salt --versions-report ```yaml Salt Version: Salt: 3006.8

Python Version:
Python: 3.10.14 (main, Apr 3 2024, 21:30:09) [GCC 11.2.0]

Dependency Versions:
cffi: 1.14.6
cherrypy: 18.6.1
dateutil: 2.8.1
docker-py: Not Installed
gitdb: Not Installed
gitpython: Not Installed
Jinja2: 3.1.3
libgit2: Not Installed
looseversion: 1.0.2
M2Crypto: Not Installed
Mako: Not Installed
msgpack: 1.0.2
msgpack-pure: Not Installed
mysql-python: Not Installed
packaging: 22.0
pycparser: 2.21
pycrypto: Not Installed
pycryptodome: 3.19.1
pygit2: Not Installed
python-gnupg: 0.4.8
PyYAML: 6.0.1
PyZMQ: 23.2.0
relenv: 0.16.0
smmap: Not Installed
timelib: 0.2.4
Tornado: 4.5.3
ZMQ: 4.3.4

System Versions:
dist: debian 12 bookworm
locale: utf-8
machine: x86_64
release: 6.1.0-20-amd64
system: Linux
version: Debian GNU/Linux 12 bookworm

</details>
Server and minion are identical except that server has `cherrypy: unknown`.
@RolandRosenfeld RolandRosenfeld added Bug broken, incorrect, or confusing behavior needs-triage labels May 6, 2024
Copy link

welcome bot commented May 6, 2024

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at saltproject@vmware.com. We’re glad you’ve joined our community and look forward to doing awesome things with you!

@RolandRosenfeld
Copy link
Author

I dived a little deeper into this issue and found out, that psutil_compat isn't imported or used at any point in salt.
But the file is still excecuted.
The problem seems to be, that every python file in /opt/saltstack/salt/lib/python3.10/site-packages/salt/utils is executed/compiled on state.apply also files which aren't imported or used.
I did a little test: copied psutil_compat.py to test.py (in the same directory) and modified the deprecation warning a little. As a result the next 'state.apply' showed my two deprecation warnings, one from 'psutil_compat.py' and the other from test.py. I also noticed, that __pycache__/test.cpython-310.pyc was created.

So it seems that we should remove psutil_compat.py from the salt-common package (or make sure that is isn't executed unintentionally), since the depecation warning on salt-call state.apply or in the log (when state.apply is executed from master) is quite annoying.

@tjyang
Copy link
Contributor

tjyang commented May 6, 2024

@RolandRosenfeld , I saw this error message also. for thanks the detail analysis. Hope salt core dev team can acknowledge and fix it in the next release. (this was not a "hope" but was expected before Broadcomm's purchase).

@RolandRosenfeld
Copy link
Author

As a workaround the following gets rid of the warning. Hopefully I don't have to extend the version guard:

{% if grains.saltversion == "3006.8" %}
/opt/saltstack/salt/lib/python3.10/site-packages/salt/utils/psutil_compat.py:
  file.absent

/opt/saltstack/salt/lib/python3.10/site-packages/salt/utils/__pycache__/psutil_compat.cpython-310.pyc:
  file.absent
{% endif %}

@tjyang
Copy link
Contributor

tjyang commented May 6, 2024

@RolandRosenfeld
Thanks for this salt tip. I will add this into my salt-minion restart formula, which also have another file removal for version 3007 minions.

@slinrain
Copy link

As I investigated this warning shows in all states where using state.pkg (pkg.installed, pkg.purged, pkg.latest etc). In other states doesn't reproduce.

@clayoster
Copy link
Contributor

The DeprecationWarning message is now occurring in version 3007.1. I'm also only noticing it when using state.apply for state files which include pkg state modules. This only seems to be happening on Red Hat based systems and not Debian/Ubuntu.

@matt-work2023
Copy link

while this is just a minor annoyance, if this is a quick fix can it be added to 3006.9 / 3007.2 milestones? Thanks!

@dmurphy18
Copy link
Contributor

There have been Deprecation Warning in use for years, wondering why a deprecation warning is a problem now, given it has been general practice for years. Deprecation Warnings are there for a reason.

@matt-work2023
Copy link

Definitely not a "problem", but also did NOT get this warning in earlier versions of salt.
No worries if you think it doesn't warrant priority - seems mostly cosmetic... but from the above it seemed like it might be a quick fix.

@dmurphy18
Copy link
Contributor

@RolandRosenfeld @matt-work2023 Deprecation Warnings are normal practice for Salt, and vary depending on what is being deprecated. Please consider closing this issue, as the deprecation warning will be removed, generally Salt has deprecation warning for items which will be removed in the next major version of Salt that will be released, see https://docs.saltproject.io/en/latest/topics/development/deprecations.html and release notes for 3007.0 (search for Deprecation) as an example https://docs.saltproject.io/en/latest/topics/releases/3007.0.html

@dmurphy18 dmurphy18 self-assigned this Jul 23, 2024
@dmurphy18 dmurphy18 added expected-behavior intended functionality Release-Notes and removed Bug broken, incorrect, or confusing behavior needs-triage labels Jul 23, 2024
@RolandRosenfeld
Copy link
Author

@dmurphy18 My understanding of a deprecation warning is, that there is a TODO for the developers to get rid of the deprecated code. Since it is still there in 3006.8, I created this ticket to make sure that it isn't overseen by the developers.

Except this, for me as a user it is quite annoying to get this warning on every "salt-call state.apply" on STDERR. It breaks one of my scripts that tells me that there is a state change pending on this server, so I had to write a workaround.
From my point of view a deprecation warning should never pop up to a user of a stable release, but being solved (by removing the call of the deprecated function) before the release to stable.

The above ticket is even more wired, since the deprecated stuff isn't really used but its pure existence in the directory resulted in the (annoying) warning, so removing the (unused) file solves the issue (why not do so in the next point release?).

I think that the issue should be closed when the warning no longer pops up...

@dmurphy18
Copy link
Contributor

@RolandRosenfeld So by your reasoning, every Deprecation Warning should have an issue attached to it, just so developers don't forget there is a Deprecation Warning.

This is not the first Deprecation Warning, and users have been dealing with them for over a decade, without the need for associated issues. Please consider closing this issue, as Salt is operating normally.

@markschuh
Copy link

@dmurphy18, I can remember some Deprecation Warnings in salt from the past - and the majority of them were really helpful, because they had warned me - as an end user of salt using some own state files - to do the needed changes in my own states early enough before some module functionality was changed or removed in future versions.

But this specific Deprecation Warning seems very different to me: From outside the core salt code I have no control over this Warning - because it is not triggered by my own states. Instead it seems to be salt/loader/lazy.py that scans through all the module files and tries to import all the different module files. And such triggers the Deprecation Warning for psutil_compat.

I share the feeling with @RolandRosenfeld that this DeprecationWarning looks quite useless to me as an end user. The addressee of the Warning Please stop importing 'salt.utils.psutil_compat' and instead .... were more the Salt Core Developer themselves.

Of course there might be some edge cases somewhere, where some salt end user really still explicitly imports psutil_compat. Though I can't imagine, this still happens - at least not on some OS within the currently supported list of OS.

Is there any chance, the module could detect, it were imported by lazy - and suppress the DeprecationWarning in this case? For the related warning in other modules the dir() scan of the lazy module typically do not trigger a warning. But for psutil_compat the warning was placed in the root context of the module file - because there isn't any function there. I'm afraid, the detection of the source of the import is not possible. And introduction of some "blacklist" in the lazy scan were too much effort for such an edge case.

So I personally follow the advise of @RolandRosenfeld and will just remove the related files from the salt package file content after installation of any salt version below 3008 in order to get rid of the Deprecation Warning.

@dmurphy18
Copy link
Contributor

@RolandRosenfeld @markschuh Looking at the contents of the file

Should be removed once support for psutil <2.0 is dropped. (eg RHEL 6)

It appears that this file should have been removed long ago, esp. since onedir where using supplied psutil 5.8.0 on 3006.x.
Discussing with team to remove it, esp. since not used in Salt, and no more classic packaging depending on OS supplied dependencies.

@dmurphy18
Copy link
Contributor

Closing since PR #66763 was just merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants