Pkg bypass remediation

[justindesilets]

What does this PR do?

Fixes a deprecated function call in the pkg.installed.bypass_file and pkg.installed.bypass_file_contains options.

Was: salt.utils.fopen Is: salt.utils.files.fopen

What issues does this PR fix or reference?

#50922 (comment)

Tests written?

Tests were added to tests.unit.states.test_pkg.py as:

• test_bypass_file
• test_bypass_file_contains

Commits signed with GPG?

No

[Ch3LL]

thanks for this fix! :) Looks like one of your tests is failing if you don't mind taking a look there.

[justindesilets]

@Ch3LL thank you. I saw the failed test. It was related to the default write behavior of the tempfile library. I've updated the function to write the temp_comment as a string only so both Python2 and Python3 can read it without any additional logic. So far it was passing tests on my laptop. Hopefully that is the last fix needed for the tests.

[max-arnold]

By the way, I just realized that there is another (more generic and powerful) feature that could achieve the same result: #51846

Something along the lines:

install_ntp:
  pkg.installed:
    - name: ntp
    - unless:
      - fun: file.file_exists
        args:
          - /etc/ntp.conf

And to check for file contents you can use fun: file.search or fun: file.sed_contains.

If that works for you, you might want to roll back your previous PR as well, to make Salt more pythonic (There should be one -- and preferably only one -- obvious way to do it.) 😎

[justindesilets]

@max-arnold I tried testing this. It looks like the - unless: option is not being honored by the pkg.installed state. Doing a trace on the minion, I still see it reaching all the way down to run Executing command [u'rpm', u'-qa', u'--queryformat', u'%{NAME}_|-%{EPOCH}_|-%{VERSION}_|-%{RELEASE}_|-%{ARCH}_|-(none)_|-%{INSTALLTIME}'] in directory '/root'. This is keeping the state run at around 8-10 seconds. There might be a reason why it is not being evaluated as expected. If it is possible that this gets the same result I am happy to roll back the feature. I will continue to look into this throughout the day.

[max-arnold]

That is a bummer. Maybe @gtmanfred could chime in to help figure out why unless/onlyif doesn't work for pkg.installed.

The other downside is that unless/onlyif is more verbose than bypass, especially if you have lots of packages.

[justindesilets]

@max-arnold I'm now testing on the dev branch. It looks like it is working. While it is faster, it is still taking over a second on my Vagrant vs the 6-9ms with the bypass function. I welcome opinions on if the speed difference is worth the added feature.

file.file_exists

(py27) [root@bslm-022 py27]# salt -c ./etc/salt '*' state.apply nginx
saltdev:
----------
          ID: install_nginx
    Function: pkg.installed
        Name: nginx
      Result: True
     Comment: unless condition is true
     Started: 16:44:25.330337
    Duration: 1780.0 ms

▽
install_nginx:
     Changes:

Summary for saltdev
------------
Succeeded: 1
Failed:    0
------------
Total states run:     1
Total run time:   1.780 s
(py27) [root@bslm-022 py27]# cat /srv/salt/nginx/init.sls
install_nginx:
  pkg.installed:
    - name: nginx
    - unless:
      - fun: file.file_exists
        args:
          - /etc/nginx/nginx.conf

file.sed_contains

(py27) [root@bslm-022 py27]# salt -c ./etc/salt '*' state.apply nginx
saltdev:
----------
          ID: install_nginx
    Function: pkg.installed
        Name: nginx
      Result: True
     Comment: unless condition is true
     Started: 16:49:20.050361
    Duration: 1230.0 ms
     Changes:

Summary for saltdev
------------
Succeeded: 1
Failed:    0
------------
Total states run:     1
Total run time:   1.230 s
(py27) [root@bslm-022 py27]# cat /srv/salt/nginx/init.sls
install_nginx:
  pkg.installed:
    - name: nginx
    - unless:
      - fun: file.sed_contains
        args:
          - /etc/nginx/nginx.conf
          - 'user www-data;'

[max-arnold]

@justindesilets I have no opinion on the speed difference, so it is up to you to decide (you have more context on how significant this issue is depending on the number of packages).

If you want more feedback, I guess you can ask other folks in the Slack channel.

Or maybe someone from SaltStack could weigh in.

[mchugh19]

It sounds like the the bypass feature is used to work around the slow path of multiple calls to package databases, which sounds like a similar use-case for the aggregate system: https://docs.saltstack.com/en/latest/ref/states/aggregate.html#in-states

I'm just a contributor with no architectural authority, but to be the bypass feature seems like a hack on the package module which should be removed. For the cases where it is truly needed, as @max-arnold points out, the requisite unless and execution module solution offers similar functionality with increased execution time, but with more generic and extendable invocation.

[github-abcde]

On the other hand, the feature already exists within the salt codebase, albeit broken, which this PR is fixing. That alone warrants this PR despite the discussion on whether or not the feature itself is wanted/needed/optimal in the first place.

[justindesilets]

I am fine with the bypass feature being removed. The only reason I added it in the first place is that I was unaware of any existing method to achieve the same result. I'd like to update the documentation with the examples talked about in this thread, but I am not sure what would be the best location for that? To maximized the effectiveness, you would want to use aggregate in combination with the unless requisite. Do anyone have any suggestions as to where something like this could be documented and highlighted for other users to find?

[max-arnold]

I think that salt.states.pkg module docs is the most logical place for this kind of performance notes: https://docs.saltstack.com/en/latest/ref/states/all/salt.states.pkg.html#salt.states.pkg.installed

To add a hyperlink to the new unless/onlyif feature https://docs.saltstack.com/en/develop/ref/states/requisites.html#unless you can use the following rst syntax:

.. seealso:: unless and onlyif

    You can use the :ref:`unless <unless-requisite>` or 
    :ref:`onlyif <onlyif-requisite>` syntax to ...

    EXAMPLE HERE

    :ref:`Unless Requisite <unless-requisite>`

You could also add a note here https://docs.saltstack.com/en/latest/ref/states/aggregate.html

To build the docs on your local machine you probably need to install this https://github.com/saltstack/salt/blob/develop/requirements/static/py2.7/docs.txt and then run make html

[justindesilets]

Thank you @max-arnold. I should be able to work on a new PR for updating the documentation this week. I don't know the process for having a feature pulled back out. Are you able to provide any guidance on this?

[max-arnold]

Since the feature is not yet released (feature freeze for Neon is expected in a few days), you can just create a PR (or reuse this one) with a reverse commit (use git revert HASH)

[max-arnold]

@justindesilets

While it is faster, it is still taking over a second on my Vagrant vs the 6-9ms with the bypass function. I welcome opinions on if the speed difference is worth the added feature.

I think the unless/onlyif slowness might be caused by mod_run_check tests:

salt/salt/state.py

Lines 1942 to 1943 in 35646c5

	if ('unless' in low and '{0[state]}.mod_run_check'.format(low) not in self.states) or \
	('onlyif' in low and '{0[state]}.mod_run_check'.format(low) not in self.states):

The LazyLoader attempts to load every state module to find the possible pkg.mod_run_check function and that operation is slow.

CC: @gtmanfred