[3006.x] Revert changes from PR 65186

[dmurphy18]

What does this PR do?

Reverts code changes introduced in Salt 3006.5, PR #65186.
The fix is incomplete and is causing issues, hence revert to previous code and introduce the fix with better and fuller tests at a later point

What issues does this PR fix or reference?

Fixes:#65692

Previous Behavior

Upgrade from 3006.4 to 3006.5 was removing any external minions that had been cached, that is, directory /var/cache/salt/minion/extmods/grains, other commands also removing the cached external grains.

New Behavior

Upgrade from 3006.4 to 3006.5 was preserves any external minions that had been cached, that is, directory /var/cache/salt/minion/extmods/grains and external grains no longer removed

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

• Docs
• Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
• Tests written/updated

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

[lkubb]

@dmurphy18

I went ahead and upgraded a non-essential minion to 3006.5. The behavior I found was exactly as described in #65692 (comment). Custom grains were completely broken with salt-call (not --local).

I applied #65738 (this patch, sorry, wrote this for the thread initially), which makes them usable again, but every single salt-call logs an error:

[root@testminion ~]# salt-call grains.get foo
[ERROR   ] Failed to sync grains module: 'master_uri'
local:
    bar
[root@testminion ~]# salt-call test.ping
[ERROR   ] Failed to sync grains module: 'master_uri'
local:
    True

Even with #65738, a simple salt-call --local test.ping still wipes custom grains without being asked for:

[root@testminion ~]# salt-call grains.get foo
[ERROR   ] Failed to sync grains module: 'master_uri'
local:
    bar
[root@testminion ~]# salt-call --local test.ping
local:
    True
[root@testminion ~]# salt-call grains.get foo
[ERROR   ] Failed to sync grains module: 'master_uri'
local:

Are you sure the initial fix is even necessary? It introduces overhead and potential flakiness for avoiding a single salt-call saltutil.sync_all on masterless minions.

[dmurphy18]

@lkubb I haven't finished with the PR, wanted to a get a PR run in to check on it. Don't have access like I used to, to VMs, to test and unit-test fixes. There is an internal Slack channel that is used for internal review before anything is ready for merge etc., and not close, so please wait till I have a full solution merged.

Recommend attending Open hours tomorrow, going to be a discussion on Salt moving forward under Broadcom.

[dmurphy18]

@lkubb btw: salt-call --local test.ping still wipes custom grains.
Will double check with 3006.4 etc

[lkubb]

No worries, just trying to help. :) The behavior of wiping the grains with a single --local invocation is new in 3006.5 (for anything other than saltutil.sync_(all|grains) of course). Other module types are not wiped.

I still think the correct fix is to document custom modules better tbh.

Sadly I will likely not be able to make it tomorrow, will it be posted to the Youtube channel again?

[dmurphy18]

@lkubb Should be posted to youtube eventually, the person who does that will be on PTO (like so many) till the New Year, so may be a while, but should be up sometime in January

[dmurphy18]

Waiting for nightly build fixes (they are responsible for the test failures) before proceeding with this fix