[master] Fix 64433: Add dynamic loading of file_roots, pillar_roots, and thorium_roots

[bluesliverx]

What does this PR do?

Adds dynamic expansion of file/pillar/thorium roots config.

What issues does this PR fix or reference?

Fixes: #64433

Previous Behavior

The roots were expanded only once at startup.

New Behavior

The roots are expanded on every access of the environments within the file_roots, pillar_roots, and thorium_roots options.

Merge requirements satisfied?

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

Commits signed with GPG?

No

[bluesliverx]

@Ch3LL I have added tests and updated the documentation as requested. Let me know if you see anything else.

[bluesliverx]

Friendly ping for @dwoz/@twangboy for reviews :)

[dwoz]

Looked at this. I like the feature but am not sure about DynamicDict just yet. I'm going to chew on it for a bit before giving a final review.

[bluesliverx]

Looked at this. I like the feature but am not sure about DynamicDict just yet. I'm going to chew on it for a bit before giving a final review.

Thanks @dwoz, I should give you some background that the DynamicDict was written by someone else at my company, and it fell to me to get it into upstream Salt. The first thing I did is sat back and consider if there was a better way to do this, such as using a method in salt.config to retrieve the file roots or updating the opts when changes were detected (too hard for config on disparate filesystems such as NFS). In the end I ended up back with the dynamic dict since it provides a seamless interface to module writers, etc. i.e. opts['file_roots'] will work in either case with no changes to salt modules or third party modules.

We have been using pretty much this exact implementation for about 5 years now on clusters of 30k+ of salt minions, and it works well for us. With that said, I'm open to suggestions, just let me know.

[bluesliverx]

@dwoz, not trying to be pushy here, just curious if you had any further thoughts on this? (just in case it dropped off your radar :) )

[dwoz]

Sorry about the delayed response here. I was hoping to come around to allowing the DynamicDict thing to fly. However, that hasn't happened. I do no't like the 'magic' of dynamic dict and I do not like adding something other than simple data types to config and subsequently opts. If we're going to implement this feature, and I think we should. I think it'd be better to have a method in config or utils to grab the file_roots as needed and where needed.

[bluesliverx]

@dwoz I appreciate the honest feedback, and completely understand. Should the method in config/utils handle updating the file_roots or be used for actual retrieval? My only worry about doing it there is that something (even an execution module not in this repo) will use file_roots that will not be updated with the latest changes.

[bluesliverx]

@dwoz, wondering if I could get a little bit of guidance here with my previous question:

Should the method in config/utils handle updating the file_roots or be used for actual retrieval? My only worry about doing it there is that something (even an execution module not in this repo) will use file_roots that will not be updated with the latest changes.

I'm happy to work on this, but just want to set the direction first so that I don't do unnecessary or unwanted work. Thank you!

[dwoz]

@bluesliverx Looks like this is causing some tests to fail

[bluesliverx]

@dwoz, I'm a bit confused. You said you didn't want this PR merged in its current form. I'm looking for some feedback on what exactly you were thinking (see my last comment for questions there) before I rework it and bring it up to date with the latest master. Or has that opinion changed?

[dwoz]

@dwoz, I'm a bit confused. You said you didn't want this PR merged in its current form. I'm looking for some feedback on what exactly you were thinking (see my last comment for questions there) before I rework it and bring it up to date with the latest master. Or has that opinion changed?

I don't have the bandwidth to dive in and try and come up with a better solution. If the tests all passed I'd be more inclined to let it go without being my preferred implementation.

@s0undt3ch Thoughts on this one?

[bluesliverx]

Sorry @twangboy, thanks for the review! I'm still fixing up some tests that were affected by this change. I'm getting close but since I can't seem to run tests locally anymore (not sure why it freezes for me now but worked previously...), so I just have to run it up here unfortunately.

[twangboy]

No worries. Let me know when you're ready for a review.

[bluesliverx]

@twangboy I think this might be ready, but it's no longer running the build for some reason, so hard to tell. I was really close on the last commit though.

[bluesliverx]

@twangboy @dwoz this should be ready now.

[twangboy]

Sometimes it just takes a while for the jobs to start, looks like they're running now

[twangboy]

Not sure why those tests are failing

[bluesliverx]

@twangboy, yeah, me either. I've been trying to watch master, because I believe it is failing to build master in general and has nothing to do with my change: https://github.com/saltstack/salt/actions?query=branch%3Amaster

[twangboy]

Yeah, we've made a lot of fixes on 3006.x. Those are now being merged forward to 3007.x and then to master. Maybe the fixes are there.

[twangboy]

This will need a review from @dwoz

[twangboy]

You got some pre-commit and lint failures that need to be addressed

[twangboy]

@bluesliverx Would you mind rebasing and resolving the conflicts?

[bluesliverx]

@twangboy, sure thing. Apologies that this has been languishing. I have one consistently failed test that I can't seem to get to pass, and without a windows machine I'm finding this extremely difficult to test locally.