Salt: pkg.installed does not seem to refresh the repo database, no matter what

[jf]

Description of Issue/Question

With the following state file

install:
  pkg.installed:
    - name: some-packagename
    - refresh: True

both salt-ssh, and regular salt fail to call apt-get update (I'm expecting it to be called, based on https://docs.saltstack.com/en/latest/ref/states/all/salt.states.pkg.html#salt.states.pkg.installed) on an Ubuntu system (really doubt this is ubuntu-specific, though). This is deduced by me simply by looking at /var/cache/apt/lists, and noting that no new files are created for a new repo I've got in /etc/apt/sources.list.d (running apt-get update manually creates these files though).

Setup

See SLS file above (the - refresh: True line can be removed as well, and the effect will still be the same). Create any valid file in /var/lib/apt/sources.list.d

Steps to Reproduce Issue

See description above.

Versions Report

Salt Version:
           Salt: 2016.11.0

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 1.5
          gitdb: 0.5.4
      gitpython: 0.3.2 RC1
          ioflo: Not Installed
         Jinja2: 2.7.2
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: 0.9.1
   msgpack-pure: Not Installed
 msgpack-python: 0.4.6
   mysql-python: 1.2.3
      pycparser: Not Installed
       pycrypto: 2.6.1
         pygit2: Not Installed
         Python: 2.7.6 (default, Oct 26 2016, 20:30:19)
   python-gnupg: Not Installed
         PyYAML: 3.10
          PyZMQ: 14.0.1
           RAET: Not Installed
          smmap: 0.8.2
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.0.5

System Versions:
           dist: Ubuntu 14.04 trusty
        machine: x86_64
        release: 3.13.0-103-generic
         system: Linux
        version: Ubuntu 14.04 trusty

[jf]

As a note, this would appear to be the same problem as https://github.com/saltstack/salt/issues/38064

[damon-atkins]

With out - refresh: True a highstate will by default will call only once pkg.refresh_db Their was a bug in older releases which resulted in pkg.refresh_db being called multiple times for installed or latest (can not remember which one it was).

It needs to be able to write to salt's cachedir to create a flag file that a refresh is required.

[Ch3LL]

@damon-atkins i'm not certain what you are stating? Are you stating that this is not an a bug?

From my test case i can see adding - refresh: True is indeed not working`

git bisect shows: cd16f7f9bb827a60210d88771b8842a432bdfb7f

Heres a docker container for whoever wants to quickly replicate:

1. docker run -it -v /home/ch3ll/git/salt/:/testing/ ch3ll/issues:38090 (where /home/ch3ll/git/salt is a local cloned git repo)
2. echo "" > /var/log/salt/minion ; salt-call --local state.sls test -ldebug; grep -i update /var/log/salt/minion

And you will see that apt-get update does not show up in the log. The state itself will error out cuz the package does not exist but the key is that you will see apt-ge tupdate does not run.

[terminalmage]

@Ch3LL @gtmanfred already pinged me about this, I did a bisect too and found the same issue.

@damon-atkins I'm reverting your changes to the rtag behavior in https://github.com/saltstack/salt/pull/38113. We'll need to figure out a better approach to whatever you were trying to do.

[damon-atkins]

The change was to fix refresh being called for every package install within highstate. The effect on Windows was a complete slow down. We all gave this a good test, so I am a bit surprised. I think we just need to look at why the new function is not picking up the correct logic I hope....

I will be hard press to look at this before Christmas. But putting the original code back will cause large issues for windows, and minor issues for Linux. see https://github.com/saltstack/salt/pull/36222#issuecomment-246481366

The idea was to consolidate the logic, to simplified the rest of the code. (but it needs to work) and we all thought it did.

[damon-atkins]

@Ch3LL did you test it using highstate?

@jf did you call your sls with state or highstate?

[damon-atkins]

Including @UtahDave as one of the original testers

[Ch3LL]

I called the state with state.sls. To clarify the exact command: salt-call --local state.sls test -ldebug

[damon-atkins]

my guess is mod_init(low) is not being called (it does not have access to the refresh value). mod_init should be called and this puts in the flag to do the refresh.

[damon-atkins]

The question then is should highstate and state both call mod_init?

[jf]

I actually tried to do my own troubleshooting before filing this, but gave up on looking at the code (specifically the rtag stuff, which was key to trying to understand how the system was behaving with regards to the refresh value). I can confirm, however, that the rtag stuff (including _refresh_tag_file()) is being called, state.sls or whatever.

[damon-atkins]

@jf The original code and the rework code should work the same way. when mod_init called it creates a file called refresh_db Then when installed is called or latest is called they look for this flag file and if it exists it calls pkg.refresh_db and then the flag file is removed. It took me a fair amount of time to understand the original code and what it was trying to do.

Thanks for trying to debug it.

@Ch3LL If the original code goes into 2016.11.1 then the release notes will need to provide a warning for Windows.

[jf]

thanks, @damon-atkins for the explanation. As a note, you might want to change that reference (@"jr"). Somebody just got pulled into a conversation in which he's not involved... Has happened to me before (there's some guy who just had to take my username + 3 dashes), and I know how irritating it can be.

[terminalmage]

@damon-atkins mod_init is always invoked, the code for this is in the state compiler.

The original rtag logic has served us well, it never should have been changed. The extra refreshes on Windows were from an oversight when refreshing was added to some of the win_pkg functions to ensure that there was package metadata available. We weren't removing the rtag after the initial refresh was performed within _find_install_targets(). https://github.com/saltstack/salt/pull/38156 corrects this.

[damon-atkins]

In the end salt owns the code. Writing windows specific code, will just lead to more specific code, when it should all be generic. The problem exists on other platforms as far as I could tell, not just windows. Its just windows is the hardest hit. It would take me less than half a day to fix the new code.

The new code just put the logic in one spot/function, instead of spread around different parts of code. i.e. code reuse. Also found that flag file was not being removed in one spot, when it should be.

oversight when refreshing was added to some of the win_pkg

I did not notice that it was that much different to other pkg modules, but maybe I missed something.

Looks like I should have spent more time testing non-windows. I apologies for the bug.

[terminalmage]

Frankly, I don't think you're being realistic expecting a "one size fits all" approach to the refresh logic. The reason that there is Windows-specific code in the state is because Windows acts fundamentally differently from any other platform. It doesn't have _true_ repositories or a package manager.

If you're going to say that other platforms are affected, then you need to provide actual details, not just assertions.

[damon-atkins]

because Windows acts fundamentally differently.

The new code I added to refresh in win_pkg allows it to behaviour like most over package mangers. This issue you have mention might possible be gone now. That was the aim of some of the changes to get win_pkg closer to the behaviour of other package managers.

I never wanted to touch state pkg code, but it was the only way win_pkg code was going to be accepted.
I did what I thought was best to improve the code, remove the bug which in state pkg so the win_pkg code would be accepted. I apologies for the bug.

[damon-atkins]

Provide sample of bug in #38156 for Fedora. where the cache is clean twice, but this is only seems to be when latest and installed are both used and refresh is True.

[terminalmage]

As noted in my response to the example you provided, there is no bug with that behavior, it seems you are just mistaken about how the refresh logic is supposed to work.

I'm confident that #38113 and #38156 resolve this matter.