Salt: FreeBSD packaging install performance regression

[ari]

This commit: https://github.com/saltstack/salt/commit/f9ef30aabe16d920563c802d82e220db01972cc1 causes a major performance regression in FreeBSD since it means that every single state run forces all packages to be reinstalled even if they are already the correct version.

Since the use case of downgrading packages is rare, this commit should be removed. Or if you want a better approach, a version comparison should check if the correct version is already installed.

The problem line is this:

if salt.utils.platform.is_freebsd():

logs look like this:

[INFO    ] Executing state pkg.installed for [net-mgmt/iftop]
[DEBUG   ] Could not LazyLoad pkg.normalize_name: 'pkg.normalize_name' is not available.
[DEBUG   ] Could not LazyLoad pkg.check_db: 'pkg.check_db' is not available.
[DEBUG   ] Could not LazyLoad pkg.normalize_name: 'pkg.normalize_name' is not available.
[INFO    ] Executing command ['pkg', 'install', '-yf', 'net-mgmt/iftop'] in directory '/root'
[INFO    ] Executing command ['pkg', 'info', '-ao'] in directory '/root'
[DEBUG   ] Could not LazyLoad pkg.hold: 'pkg.hold' is not available.
[INFO    ] The following packages were installed/updated: net-mgmt/iftop

[ari]

This regression is adding 40-60 seconds per highstate on my system with about a dozen packages defined in salt.

[garethgreenaway]

@ari thanks for the report. @rickyninja Looks like this was a change that you made awhile ago, any ideas what might be causing the issues reported? thanks!

[rickyninja]

I didn't realize it when I made this change, but it looks like doing a force install is equivalent to doing a reinstall.

I don't know where to grab the version_cmp value offhand, but looks like this could be changed to only add the force flag if it's FreeBSD and the cmp value is equal to -1.

if salt.utils.is_freebsd() and cmp == -1:
    force = True    # Downgrades need to be forced.

https://github.com/saltstack/salt/commit/f9ef30aabe16d920563c802d82e220db01972cc1#diff-e8ecdef70c9c678790ca9e0e07c9ab77R1249

[ari]

You added version_cmp in the same https://github.com/saltstack/salt/commit/f9ef30aabe16d920563c802d82e220db01972cc1 commit. Note also this is only helpful if the user actually passes a forced version in the state.

[rickyninja]

@ari I'm aware I added the version_cmp function. I'm talking about the value returned when the function runs. If that value is used as I proposed in the code snippet above, it may fix the problem you discovered.

I don't know if the value is in scope where needed, and I don't have time to dig in deeper right now to find out. I made this change over a year ago, so the full code/context is no longer fresh in my mind.

[ari]

'force=True' should only be hit if all the following are true:

1. allow_updates = False
2. version is not null
3. version != installed_version

Even if you caught just the first two cases now it would avoid a lot of unnecessary pkg installs.

[rallytime]

@ari This should be fixed with #45174. Can you confirm?

[ari]

Well, yes it should fix the problem, but now downgrades are impossible. Granted its a pretty rare situation, but still...

[rallytime]

@eradman Can you swing back around here? We should be able to pass the force options via kwargs so that we can maintain the downgrade functionality. Does that sound good to you @ari?

[eradman]

@rallytime you're observation about communicating the force kwarg is good. This does make downgrades possible on FreeBSD

[rallytime]

@eradman Party! Thanks for fixing that so quickly. I'll back-port both of the the fixes listed here so this will be fixed for 2017.7.3.

[rallytime]

@ari - since we're passing **kwargs through, forcing a downgrade is still possible. See #45298 for more information. I have back-ported this original fix and it will be available in 2017.7.3.