-
Notifications
You must be signed in to change notification settings - Fork 5.5k
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pkg.installed does not seem to refresh the repo database, no matter what #38090
Comments
As a note, this would appear to be the same problem as #38064 |
With out It needs to be able to write to salt's cachedir to create a flag file that a refresh is required. |
@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 git bisect shows: cd16f7f Heres a docker container for whoever wants to quickly replicate:
And you will see that |
@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 #38113. We'll need to figure out a better approach to whatever you were trying to do. |
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 #36222 (comment) 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. |
Including @UtahDave as one of the original testers |
I called the state with |
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. |
The question then is should highstate and state both call mod_init? |
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 |
@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. |
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. |
@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 |
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.
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. |
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. |
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. |
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. |
Description of Issue/Question
With the following state file
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
(runningapt-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
The text was updated successfully, but these errors were encountered: