pkg.removed state fails with implicit package version

[morganwillcock]

I was trying to downgrade some software and had issues removing a newer version. In other pkg operations not specifying a version string will imply to operate with the newest version, but for pkg.removed this leads to the state being failed if another version is installed.

Here is an example using 7zip. I have two versions available:

salt salt-test pkg.list_available 7_zip

salt-test:
    - 9.20.00.0
    - 15.14.00.0

I have the older version installed:

salt salt-test pkg.version 7_zip

salt-test:
    9.20.00.0

I'll use two states, one implying the latest version and one which lists it explicitly:

remove_7_zip:
  pkg.removed:
    - name: 7_zip

remove_7_zip_again:
  pkg.removed:
    - name: 7_zip
    - version: 15.14.00.0

The first state result is a failure, but the changes indicate that it (logically) should have passed as the latest version string seems to have made it to the module ("15.14.00.0 not installed"):

salt salt-test state.apply

salt-test:
----------
          ID: remove_7_zip
    Function: pkg.removed
        Name: 7_zip
      Result: False
     Comment: The following packages failed to remove: 7_zip.
     Started: 17:31:02.709000
    Duration: 234.0 ms
     Changes:   
              ----------
              7_zip:
                  ----------
                  current:
                      15.14.00.0 not installed
----------
          ID: remove_7_zip_again
    Function: pkg.removed
        Name: 7_zip
      Result: True
     Comment: All specified packages (matching specified versions) are already absent
     Started: 17:31:05.486000
    Duration: 62.0 ms
     Changes:   

Summary for salt-test
------------
Succeeded: 1 (changed=1)
Failed:    1
------------
Total states run:     2

Having no copy of 7zip installed lets the first state pass:

salt salt-test pkg.remove 7_zip version=9.20.00.0

salt-test:
    ----------
    7_zip:
        ----------
        new:
        old:
            9.20.00.0

salt salt-test state.apply

salt-test:
----------
          ID: remove_7_zip
    Function: pkg.removed
        Name: 7_zip
      Result: True
     Comment: All specified packages are already absent
     Started: 17:58:29.492000
    Duration: 62.0 ms
     Changes:   
----------
          ID: remove_7_zip_again
    Function: pkg.removed
        Name: 7_zip
      Result: True
     Comment: All specified packages are already absent
     Started: 17:58:29.570000
    Duration: 47.0 ms
     Changes:   

Summary for salt-test
------------
Succeeded: 2
Failed:    0
------------
Total states run:     2

The documentation doesn't seem to say what should happen if you don't set the version string in the state, but it looks like something is getting lost in translation between the pkg state and the pkg module. It also doesn't state which pkg providers are supported so I've only tested this on Windows for the moment.

Tested on Salt 2016.3.1.

[ghost]

Can confirm. I tested this and believe I saw the same problem.

[ghost]

Hi again @morganwillcock. I have reviewed the code and technically this may not a bug at the moment, but I think it should be. When pkg.remove doesn't get a version passed to it, it attempts to uninstall the latest version.

The following code fragment in pkg.remove is the probable culprit

       # Get latest version if no version passed, else use passed version
        if not version:
            version_num = _get_latest_pkg_version(pkginfo)
        else:
            version_num = version

I consider that if you call pkg.remove, with no version specified, then every instance of that package should be removed. Even if this were a breaking change I think it should be done because the uninstalled state isn't so pleasant to work with if you have to add extra jinja to get it to do what you want (or however you end up handling this state of affairs). @terminalmage and @twangboy, what are your thoughts on this?

Edit: the code is in win_pkg. Other providers may handle it differently.

[morganwillcock]

@hrumph thanks for investigating further. I agree that removing any version of the installed package is one of the more logical (and useful) outcomes, but the pkg.removed docs say:

Don't do anything if the package is installed with an unmatching version.

...so maybe this is going to have to follow what other pkg providers are doing (although I imagine a lot of the package providers are only going to provide a single version of a package).

There is also the issue than win_pkg can match installed products against versions that haven't been defined (same displayname, different version), so you may not have the uninstall logic available for the version which you have installed.

[ghost]

@morganwillcock,
yes I guess multiple versions of the same thing may not be possible (i was thiking about kernels and how I see a comma-separated list of versions for the version info) but I consider that to be a bit of a side-issue. OTO i don't think I understand what you are saying about the matching.
For instance if I have Firefox esr 45.0.1 installed and the following formula

firefox-esr:
  '45.2.0':
    full_name: 'Mozilla Firefox 45.1.0 ESR (x86 en-US)'
    installer: 'https://download-installer.cdn.mozilla.net/pub/firefox/releases/45.2.0esr/win32/en-US/Firefox%20Setup%2045.2.0esr.exe'
    install_flags: '/s'
    uninstaller: '45.0.1\Mozilla Firefox\uninstall\helper.exe'
    uninstall_flags: '/S'
    msiexec: False
    locale: en_US
    reboot: False

Then when I run

salt -t 600 '000004' pkg.version "firefox-esr"

I get an empty result. OTO when I run

salt -t 600 '000004' pkg.version "Mozilla Firefox 45.1.0 ESR (x86 en-US)"

I get

000004:
    45.1.0

So as far as I know win_pkg has to match on both the version and the displayName (exception for 'latest'). If I'm wrong or don't understand what you are saying, could you give me an example to set me straight?

Anyway, so the way I see it pkg.remove should try to remove whatever pkg.version comes up with.

[morganwillcock]

As an example, I've got a package definition for Chrome that only contains a single version (i've not updated this in a few months):

salt salt-test pkg.list_available google_chrome
salt-test:
    - 50.0.2661.94

...but Chrome has upgraded itself, and I have no uninstall logic for this version (package GUID will most likely have changed, maybe even install location once the 64 bit build is the default)

salt salt-test pkg.version google_chrome
salt-test:
    52.0.2743.82

...so there is no way that a state or module can reliably remove this version, since it won't know how to do so.

[morganwillcock]

@hrumph I think Firefox is a demonstration of how an inconsistent displayname for the package can change behaviour, technically every time it upgrades it's viewed as a different product, so the version comparisons for the package are redundant.

[ghost]

@morganwillcock, it's an artifical example I made up, and I made sure that the displayname matched what I have installed. So it matches what I have installed but the version is different. Can I see your sls file? I want to better understand what's going on.

[ghost]

@morganwillcock also what version is your salt minion? Maybe this is a behaviour that changed.

[ghost]

My bad sorry man. I reproduced the behaviour (had the first line commented out in my sls file.) OK really sorry about that. This is news to me but all the same I don't think its game-changing. Like I said, I believe that salt should do the uninstall when it can.

[rallytime]

Hi @morganwillcock and @hrumph - Thanks for the discussion here. I agree that the current behavior is a bug (and confusing). A solution to this issue was just submitted yesterday in #34864. Can you guys give that a try and confirm that it fixes this issue for you?

[ghost]

Hi @rallytime, I haven't tested anything. The way I see it #34864 is a good patch and should get merged. AFAICS @jmacfar just wants to put things back the way they were, which is better than the way things are now. Without his patch, when multiple versions of something are installed, you can't just remove one of them. I don't think it addresses this issue however. Even after his patch, when version is None, salt will want to remove whatever it considers the latest version to be. I also don't think that this is a regression. The behaviour is what whoever designed this intended. I just don't think its a very good design decision and hope to change people's minds about this.

What I want salt to do, is, whenever version is None, I want salt to try to remove all existing versions. I understand what @morganwillcock is saying, in that salt may not always know how to that, but that shouldn't stop salt from doing whatever it can in this direction.

[ghost]

@rallytime since you have kept the bug label, would a PR to make the changes I have in mind be accepted?

[rallytime]

@hrumph Ah, thanks for summing everything up - I see what you're saying. I am not sure how the other pkg modules handle this, or if this is generally a windows issue, but perhaps an option can be added to say "remove everything if any packages of this name are found". That way current behavior isn't disrupted for anyone (especially when other pkg modules might behave differently on other distributions) but still allows for you to purge all packages when the version doesn't exist.

@terminalmage This is more your area of expertise than mine. I'd love it if you could weigh in on this conversation.

[ghost]

@rallytime the current behaviour is so absurd that I find it hard to imagine anyone depending on it. If you have program X installed and want to uninstall X, then remove X should remove X, not remove X only if X happens to be the latest version, otherwise complain.

[morganwillcock]

@hrumph I think you can get same result by calling pkg.list_available in a template and looping over the result to apply the pkg.removed state on all known versions. This should always succeed but would also give you a chance to manually set the ordering or dependencies for the uninstall actions (as part of the template rendering).

Personally I would rather have pkg.removed operate in a similar way to pkg.installed, just for consistency. e.g.

jre_x86:
  pkg.installed

I might have got 10 versions of Java defined so that I can uninstall them, but this won't install all 10.

[ghost]

@morganwillcock, how the case of multiple versions of the same thing is handled is a side issue. It wouldn't bother me if that situation were handled by not doing anything (when the version paramater is None). However when there is only version of something installed it is absolutely clear to me what pkg.remove (with version=None) should attempt to do in that case. In that case it should attempt to remove the thing that's installed. Anyway I guess I'd be ok with a special boolean param to make it behave this way but it's not my first choice.

[damon-atkins]

No version should = remove all versions.

[morganwillcock]

@rallytime, sorry for the delay in testing #34864. I've just tried it but unfortunately it doesn't seem to make a difference for me.

salt-test:
----------
          ID: remove_7_zip
    Function: pkg.removed
        Name: 7_zip
      Result: False
     Comment: The following packages failed to remove: 7_zip.
     Started: 13:26:01.085000
    Duration: 223.0 ms
     Changes:   
              ----------
              7_zip:
                  ----------
                  current:
                      15.14.00.0 not installed
Summary for salt-test
------------
Succeeded: 0 (changed=1)
Failed:    1
------------
Total states run:     1

[ghost]

One thing that could be done is having a bool called something like "classicMode". Default would be false and you would get the new behaviour by default but when it was true you would get the old behaviour.

[morganwillcock]

How would you decide which environment you should be using? You won't know where each package was installed from or even which environments are relevant. I don't think this approach is viable and, in the short term at least, this just needs to align with the win_pkg module docs.

:param str version:
The version of the package to be uninstalled. If this option is used to
to uninstall multiple packages, then this version will be applied to all
targeted packages. Recommended using only when uninstalling a single
package. If this parameter is omitted, the latest version will be
uninstalled.

[ghost]

@morganwillcock, it's already got a "saltenv" param in the devel version, so that's how we know what environment to use. As for what the docs presently say I (and I believe @damon-atkins) believe the status quo to be a bit of a design flaw. I guess we need to hear from @terminalmage to get the final word on what changes would and would not be accepted. I also wonder what @twangboy has to say.

[morganwillcock]

This is actually more broken than I first realised, as reported in #36302 if the version you are uninstalling through a state is not the newest available version, the action will fail. Uninstalling using the module directly is okay.

{% if grains['cpuarch'] == 'AMD64' %}
  {% set program_files_x86 = '%PROGRAMFILES(X86)%' %}
{% else %}
  {% set program_files_x86 = '%PROGRAMFILES%' %}
{% endif %}
audacity:
{% for version in '2.1.2', '2.1.1' %}
  '{{ version }}':
    full_name: 'Audacity {{ version }}'
    installer: 'salt://win/repo-ng/audacity/audacity-win-{{ version }}.exe'
    install_flags: '/SP- /VERYSILENT /NORESTART /MERGETASKS="!desktopicon"'
    uninstaller: '{{ program_files_x86 }}\Audacity\unins000.exe'
    uninstall_flags: '/SILENT'
    refresh: True
    msiexec: False
    use_scheduler: False
    locale: en_US
    reboot: False
    cache_dir: False
{% endfor %}

salt salt-test pkg.version audacity

salt-test:
    2.1.1

cat test_remove.sls

audacity:
  pkg.removed:
    - version: '2.1.1'

salt salt-test state.apply test_remove

salt-test:
----------
          ID: audacity
    Function: pkg.removed
      Result: False
     Comment: The following packages failed to remove: audacity.
     Started: 20:38:58.131000
    Duration: 188.0 ms
     Changes:   
              ----------
              audacity:
                  ----------
                  current:
                      2.1.2 not installed
Summary for salt-test
------------
Succeeded: 0 (changed=1)
Failed:    1
------------
Total states run:     1
Total run time: 188.000 ms

[damon-atkins]

@morganwillcock could you prefix the title with win_pkg: so its easy to find all win_pkg issues.

[morganwillcock]

@damon-atkins I'm not 100% sure that the problem is unique to win_pkg, as the version comparisons are done in the pkg state. The win_pkg module works as expected, when called directly.

[terminalmage]

Sorry for not getting to this sooner. A couple points here:

• @morganwillcock is right, version comparisons are done in the pkg state, at least in terms of selecting which package should be uninstalled.
• We do pass the version info to pkg.remove to remove the package, however we do not universally support removing a specific version across all pkg virtual modules. Part of this could just be lack of support in that particular package manager, and part of this could just be oversight. This bears further investigation.
• When we invoke pkg.remove on Windows, we do take the version into account. As already mentioned, when no explicit version is passed, we just remove the newest version. I agree with what others have said, a pkg.removed state with no specific version implies that all versions should be removed. We should change this behavior in win_pkg.py.
• The way we handle removing the "newest version" is wrong. We are simply doing sorting the package DB metadata for a given package and returning the newest version. But if 1.0.0 is installed and 2.0.0 is the newest in the pkg DB, then win_pkg.py will try to remove 2.0.0 and report failure. This was touched upon by @morganwillcock a couple days ago.

I plan to spend some time next week looking more deeply into this and working on a fix.

[ghost]

@terminalmage are you still going to look into this?

[terminalmage]

@hrumph Yes. I have been pulled away multiple times after starting to look into this, so I haven't been able to give it the time I need to. I'm sorry for that.

[ghost]

Hi @terminalmage I keep thinking that I might have a stab at this myself but I think that the matter is sufficiently complex that I would rather that members of the salt team such as yourself did this. I'm not just a whiner whining about something and not making contributions. I've got a new state module and run module in the works, and once you've dealt with this issue there's a specific contribution I'd like to make to the software system, but I don't want to do it until this issue is resolved.

[terminalmage]

Understood. I'll try to work on this soon.

[terminalmage]

This should be fixed in #39379.

[morganwillcock]

I think this is going to hit a related problem #33119. When trying to uninstall it's only caching the executable from the server, but to remove the software you may need everything in the directory.

[ghost]

Thanks for this @Terminal mage. @morganwillcock, I don't believe that #33119 should hold up this PR.

[cachedout]

Fixed via #39379. Closing.