salt: Inconsistent pillar data from git_pillar

What is actually missing here which could prevent progress? Does anyone know if the patch is still needed and/or functional with salt 3004?

one of the ideas ive had about this is change it so that each dynamic env gets its own git folder to cache data rather than one single git folder where its branch switching constantly as requests for different envs come in

that would

• a speed up pillar lookups across dynamic envs as it doesnt need to swap between branches
• b stop the inconsisent data as it will be looking at a git cache folder specific to that branch/ref

OK, I’m fairly certain I know what is going on. The checkout happens here, while the Pillar compilation happens later (here). The checkout lock is only preventing a simultaneous attempt at checking out the repo, but once the checkout is complete, the lock is removed. So minion A checks out its branch, but between when minion A checks it out and compiles the pillar SLS, minion B has separately checked out a different branch.

To fix this, the locking must be more intelligent and last until Pillar compilation is complete. This means we need to have a way for minions which need the same branch to be able to keep the lock alive so that we’re not having each invocation of git_pillar block. So essentially, if any minions need the master branch at a given time, the lock must remain in place. This would probably entail creating lock files named in the format checkout.<branch_or_tag_name>.<minion_id>.lk instead of checkout.lk. The contextmanager which obtains the checkout lock should also be invoked from within the git_pillar code and not the checkout() func in salt.utils.gitfs.

https://github.com/saltstack/salt/pull/65017

Changes:

• Place remotes branch/tag in its own folder ../salt/../hash/branch - Done
  • This includes locks and links
• Clear Minion and Master Cache on salt version change - Done
  • This is so people don’t need to manually clear salt cache when upgrading
• Add salt working dir ../salt/../hash/branch/.git/.salt
  • This is so we decrease the chances of stepping on git
• add fetch_on_fail checkout(self, fetch_on_fail=True) - Done
  • This is so gitpillar does not need worry if a fetch is required when working with dynamic branches
• fix clean_old_remotes - Done
• fix fetch for __env__
  • add fetch_request - when Gitbase.fetch_remotes is called the current dynamic branch will fetch and all its sister branches will have a fetch_request file placed in its salt working dir. When sister branch inits or goes to checkout it will see the fetch_request and run fetch

TODO later PR:

• remove old remotes branch’s/tags

  • Make a timestamp file every time a gitprovier object is made in remote branch
  • clear_old_remotes() will remove and remote branch that has out lived its time to live.

  I would appreciate it if you guys give it a try and/or look.

I doubt it’s related, but I was getting a Template does not exist error because pillar/init.py was computing the top cache as an empty string while using pillarenv in the minion config. I submitted a fix as #39516. Figured I’d mention it as this is the only sortof recent issue with “template does not exist” in it.

the cve detail aren’t yet public

https://saltproject.io/security-announcements/2023-08-10-advisory/

https://github.com/saltstack/salt/commit/6120bcac2ee79b6e8b104612941432841eb0c8c3

I have the same idea. It takes more spaces but accelerate the process. The problem is how to cleanup old branches. I think it is important to clear directories of old branches after a certain period.

@Vaarlion nice catch

it would look like this issue only occurs on the master when running a salt-call Any other minion don’t have that issue during a salt-call, and the master don’t have issue when i use salt to target him directly

If someone pick this up, i hop this would help.

Worktrees weren’t the way to go. Adding a global lock on the ext_pillar function to make it atomic solves the problem.

@peter-slovak we actually do this already for gitfs, using the Python bindings rather than the CLI. The trouble with this is that git_pillar still invokes the salt.pillar.Pillar class (used also for the conventional pillar_roots data) to parse the top file and load the pillar matches. A hybrid option, where the top file is parsed first, and then the pillar matches are extracted directly from the refs, may work. But doing this separately for each minion may cause its own performance issues.

I’ll give this some more thought, and discuss with my fellow engineers.

There is this error in the master log, but not explicitly tied to gitfs locking - although suspicious enough:

2017-02-15 23:07:46,460 [salt.template    ][ERROR   ][27564] Template does not exist:

The other is a warning, presumably because git_pillar_global_lock is set to False (but setting it to True changes nothing in the behavior):

2017-02-15 23:07:44,920 [salt.utils.gitfs ][WARNING ][27568] git_pillar_global_lock is enabled and checkout lockfile /var/cache/salt/master/git_pillar/b4092923fbdfd694387b13fcfbb90712d13692542d47e70913049848f672b8a6/.git/checkout.lk is present for git_pillar remote '__env__ ssh://git@git.domain.tld/group/salt.git'. Process 27585 obtained the lock

Besides that, I ran some tests on two minions:

• minion api-1 is in environment dev and has the pillar item role: internal set
• minion api-2 is in environment dev2 and has the pillar item role: foobar set
• pillar has been synced from master with saltutil.refresh_pillar

These are the results:

(dev) root@api-1:~$ for i in {1..20}; do salt-call pillar.get role | grep -q internal && echo 'Pillar correct' || echo 'Race lost!'; done
Pillar correct
Pillar correct
Pillar correct
Pillar correct
Pillar correct
Pillar correct
Pillar correct
Race lost!
Pillar correct
Pillar correct
Pillar correct
Pillar correct
Pillar correct
Race lost!
Pillar correct
Pillar correct
Pillar correct
Pillar correct
Pillar correct
Pillar correct

(dev2) root@api-2:~$ for i in {1..20}; do salt-call pillar.get role | grep -q foobar && echo 'Pillar correct' || echo 'Race lost!'; done
Pillar correct
Pillar correct
Pillar correct
Pillar correct
Pillar correct
Race lost!
Race lost!
Race lost!
Pillar correct
Race lost!
Pillar correct
Pillar correct
Pillar correct
Pillar correct
Pillar correct
Pillar correct
Pillar correct
Pillar correct
Pillar correct
Pillar correct

So that definitely looks like a race. Now get this:

root@saltmaster:~$ salt 'api-*' saltutil.refresh_pillar
api-2:
    True
api-1:
    True
root@saltmaster:~$ salt 'api-*' pillar.item role  # this one is correct
api-1:
    ----------
    role:
        internal
api-2:
    ----------
    role:
        foobar
root@saltmaster:~$ salt 'api-*' saltutil.refresh_pillar
api-2:
    True
api-1:
    True
root@saltmaster:~$ salt 'api-*' pillar.item role
api-1:
    ----------
    role:
        foobar
api-2:
    ----------
    role:
        foobar

Unfortunately I don’t have an in-depth knowledge of how the master handles simultaneous pillar compilation for more minions, but my best guess is that some stray checkout doesn’t adhere to the lock you mentioned. I’ll do my best to look into git_pillar implementation in the next few days and see if we can easily mimick the behavior of gitfs for state files; that is having a separate directory for each dynamic environment. Thanks for any help until then.