refresh: True not working with pkg.installed state

[ghost]

Description of Issue/Question

Hi, first of all this is a bug with Windows minions that appears to affect minions with version 2015.8.0+, and I believe this to be a regression introduced at the same time that winrepo-ng was introduced).

Specifically, I am trying to use refresh: True with the pkg.latest state, but it's not working. pkg.installed won't work unless pkg.refresh_db has been ran first.

Setup

Here is an example sls file

chrome installed:
   pkg.latest:
     - name: chrome
     - refresh: true

Steps to Reproduce Issue

(Include debug logs if possible and relevant.)

salt '011000' saltutil.clear_cache
salt '011000' state.apply chrome_managed

The eventual output includes the following

011000:
----------
          ID: chrome installed
    Function: pkg.latest
        Name: chrome
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "C:\salt\bin\lib\site-packages\salt\state.py", line 1591, in call
                  **cdata['kwargs'])
                File "C:\salt\bin\lib\site-packages\salt\states\pkg.py", line 1392, in latest
                  cur = __salt__['pkg.version'](*desired_pkgs, **kwargs)
                File "C:\salt\bin\lib\site-packages\salt\modules\win_pkg.py", line 197, in version
                  val = __salt__['pkg_resource.version'](*names, **kwargs)
                File "C:\salt\bin\lib\site-packages\salt\modules\pkg_resource.py", line 184, in version
                  pkgs = __salt__['pkg.list_pkgs'](versions_as_list=True, **kwargs)
                File "C:\salt\bin\lib\site-packages\salt\modules\win_pkg.py", line 236, in list_pkgs
                  name_map = _get_name_map()
                File "C:\salt\bin\lib\site-packages\salt\modules\win_pkg.py", line 951, in _get_name_map
                  name_map = get_repo_data().get('name_map', {})
                File "C:\salt\bin\lib\site-packages\salt\modules\win_pkg.py", line 934, in get_repo_data
                  'Windows repo cache doesn\'t exist, pkg.refresh_db likely '
              CommandExecutionError: Windows repo cache doesn't exist, pkg.refresh_db likely needed
     Started: 15:15:23.521000
    Duration: 10.0 ms
     Changes:

Versions Report

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)

salt-master is 2015.8.8.2

minions tried were 2015.8.8.0-3, 2015.8.8-2.

[ghost]

This also fails with salt master 2016.3.0 with minion 2016.3.0

[Ch3LL]

@hrumph just to clarify you are expecting this to work without haveing to run salt '<minion>' pkg.refresh_db manually. Just want ot make sure you know that you can indeed get this working you just need to run that command although I think we need to add the ability to not have to do that.

@twangboy is a user expected to have to run pkg.refresh_db manually before running pkg.installed on a minion? If so I think it would be good to add this behavior

[ghost]

@Ch3LL, to my mind this should cause a refresh_db to be done once within a state run (but not more than once). If it doesn't do that, then I don't understand the point of it, or at least I would like to see the point of it made clearer in the documentation.

Also, I may be misremembering what I saw here and I may not have done a proper inspection, I thought i saw that the cache was not refreshing, even if I had set it up manually initially. I can look into this later on to either confirm or deny this, but let's hear from @twangboy first

[Ch3LL]

@hrumph I totally agree this should be able to run within a state module. Just wanted to make sure you knew you could run the pkg.refresh_db to get your environment up and running. Thanks

[ghost]

Hi @Ch3LL. I've been trying to communicate with @twangboy first about this. First of all I'd like to point out that in in the code, the following fragment

    refresh = bool(
        salt.utils.is_true(refresh)
        or (os.path.isfile(rtag) and refresh is not False)
    )

got replaced with the following code

    refresh = bool(salt.utils.is_true(refresh) or
                   (os.path.isfile(rtag) and salt.utils.is_true(refresh))
                   )

I think this was a mistake because now that I see it I believe that a state run is always supposed to do a refresh_db once unless its being forced not to by a "refresh: False". Is everyone ok with my doing a PR to put this back the way it was and if so, what branch should it go it? This won't conclusively solve the problem (which I believe I can fix) but I think this should be done first before the bug is tackled.

In my initial bug report I was ignorant of how the system was supposed to work. I now understand that I shouldn't have to specify "refresh: True" anywhere in my state sls formulas.

[twangboy]

Perhaps we should change the default to refresh=True in the state function definition if that is the desired default behavior. @terminalmage ??

[ghost]

Hi @twangboy, I believe that the original problem was with the documentation and of course the documentation should be fixed. Using the original logic refresh=True forces a refresh for that state, just as refresh=False will do the opposite and ensure that no reresh occurs in that state. So going by the original logic we wouldn't want the default set to refresh=True because that would cause a refresh to happen each and every time (barring manually setting refresh to something else). The idea (as I now understand it) was that the mod_init function checks to see what function is being invoked and if its "installed" or "latest" it will set rtag and rhwn return True, causing it to not get called again in the state run. rtag being set is meant to trigger a db refresh during a state run (even when refresh is set to None). after rtag is used this way it's then cleared. The use of True and False with refresh is meant to force either a refresh_db or no refresh of the db (even when rtag is set). If I'm wrong I guess I'll eat crow. If I'm right I guess in that case the documentation should be fixed because I didn't understand it either.

[terminalmage]

Around the same time the winrepo-ng stuff was introduced, code was added that will raise an exception when a pkg.refresh_db is run on Windows and there is no winrepo database present. In the past, we would silently fail and it would just appear that there are no packages available. The fact that this exception is not being caught in the pkg.latest state has led to the error you see.

In particular, the traceback observed in this report is because the pkg.version check (which requires the winrepo cache) happens before the pkg.latest_version call that would have refreshed the winrepo database, so I've also moved this check to after the call to pkg.latest_version.

I've caught a few other instances where a CommandExecutionError may be raised, and wrapped them in try/except clauses to gracefully fail these states and get rid of the traceback.

[terminalmage]

See #34093 for these changes.

[ghost]

Hi @terminalmage. I know what the cause of the bug that I reported is and I was contemplating how to fix it when I noticed the recent change in how the value of "refresh" was set. I recognise that it's a separate issue but I'm having trouble accepting the change since I think that the original logic was right (or it was right relative to what whoever coded it at first wanted it to do). Let' just say that I'm in a state of confusion right now. I would like the original logic put back (the way I see it).

[ghost]

@terminalmage I don't really agree with the fix. If it's determined that refresh is supposed to be true at that time then pkg.version should call refresh_db (we can pass the refresh paramater to pkg.version as a kwarg).

[terminalmage]

Yeah, and if someone passed False as the value for refresh, then it would raise an exception, so we still need to catch these exceptions.

I'm talking with @twangboy about the refresh behavior, since he authored that code. This is the intended behavior:

• refresh is None -- if refresh has happened already (via another pkg state), no refresh will be done. If refesh hasn't happened already, perform a refresh.
• refresh is True -- refresh happens regardless of whether or not one has been done already in this Salt run
• refresh is False -- refresh does not happen regardless of whether or not one has been done already in this Salt run

[ghost]

@terminalmage yes I agree. That is also what I think that the code should be doing. I think that there may have been some misunderstandings due to the documentation.

[terminalmage]

What you need to understand though, is that the change you talked about above to the refresh behavior, did not arise until 2016.3.0, and the exception issue regarding checking the current version existed in the 2015.8 release cycle. So it's not just one issue.

[terminalmage]

#34098 restores the prior refresh logic to the 2016.3 release branch.

[terminalmage]

#34100 updates the documentation to reflect the correct behavior of the refresh parameter.

[ghost]

@terminalmage, yes I know it's two issues. I discovered the first while thinking about the second and wanted to proceed by fixing the first one first. BTW I didn't even know how it was supposed to work until I looked at the code so the state of the documentation was to blame (thanks for fixing the documentation BTW). That's why my original bug report doesn't make much sense. I had a plan to fix the second issue also but if you or @twangboy are taking of care of it then that's great. Anyway if it isn't fixed soon I'll do a a PR but I'd like to see #34098 in the devel branch first because the fix probably belongs in devel since I would be enhancing the behaviour of the pkg.version function a bit (so it can do a refresh_db if needed).

[terminalmage]

I'm not sure why you would need to open a PR, everything has been addressed now.

[ghost]

@terminalmage,
so far what has been addressed is a documentation issue and the erroneous change to the value of the refresh variable. I agree with both of these changes. However the state shouldn't fail just because refresh_db wasn't called in time for win_pkg.version. In the case of windows the work of calling refresh db should be done by win_pkg.version which should be passed the value of the refresh variable in kwargs (or at least that's what I plan to have the PR do).

[damon-atkins]

Re: Windows
In 2015.x not sure about 2016.x the sls files were being fetched from the master all the time, i think it was pkg.latest. If you run it in debug mode you can see it.... I think the refresh should be time based, i.e. a minimum time.

It also depends if people want to test their code without impacting others. I suppose they could use a different environment.

[terminalmage]

I updated #34093 to add a check for the refresh param to pkg.list_pkgs (which is where the traceback actually occurs when invoking pkg.version).

[ghost]

Ok thanks, it sounds like you've got this wrapped up. Look forward to testing this out.

[ghost]

@terminalmage, I don't know what's going on. I am inspecting the code and now I really want to know what's going on.

I see the following

    cur = __salt__['pkg.version'](*desired_pkgs, **kwargs)

    try:
        cur = __salt__['pkg.version'](*desired_pkgs, **kwargs)
    except CommandExecutionError as exc:
        ret['result'] = False
        ret['comment'] = exc.strerror
        return ret

so pkg. version is being called both inside and outside the try block which makes no sense. Also in the code I don't see where the value of refreshed was put into kwargs before being passed to pkg.version. Also we need this to be a special windows case because we don't want to do double refreshes in Windows (once for pkg. version and then once again for pkg. latest_version). Maybe I'm misunderstanding things right now in which case I'm sorry but there something wrong here (I think).

[ghost]

Wait I'm sorry @terminalmage. Ok i think things are probably good, except I don't think that pkg.version should be called twice, once outside and inside the cur block

[ghost]

I mean merge it but get rid of the double call to pkg.version. I think everything should be ok now.

[terminalmage]

Yeah, good catch. When I was making the additional commits to this PR, I accidentally had originally made them against the wrong branch, and had to cherrypick them onto the branch I used for the pull request, and I did not correctly resolve the merge conflict.

I've removed the extraneous call to pkg.version.

[ghost]

THanks a lot @terminalmage. Sorry for being such an obstreperous dingbat. I'm looking forward to working with you and @twangboy in the future on some things I have in mind.

[terminalmage]

Haha, no problem. I'd rather have someone tell me where I screwed something up, than silently ignore it to avoid hurting my feelings. 😄