Loader nomerge: Don't allow modules to "merge" #20540
Conversation
jacksontj
force-pushed
the
loader_nomerge
branch
2 times, most recently
from
be46b79
to
6f2c752
Compare
|
Thanks for merging the submodule support. removing the "merge" option looks like fun. |
|
The intent of your patch seems to make this test invalid on purpose, right? |
|
Yes, although the last time i submitted this that one test didn't fail-- but the equivalent non-ssh one did ;) |
jacksontj
force-pushed
the
loader_nomerge
branch
from
6421f72
to
06aa3c3
Compare
|
I really dislike this PR, we have a lot of deployments that use this feature. I don't think that being confused is a valid reason here. |
jfindlay
added
the
Pending-Discussion
|
@jacksontj I will only let this in if it is config gated, this is going to break a lot of people's environments and should not be on by default |
|
@thatch45 We talked about it a lot in another PR. Its not so much a case of "im confused" its more of a case of "we can't know what its doing". From our other conversation it sounds like we agreed this would be a good feature to kill off-- which will require some time (since people could be using the feature). From chatting in the other PR it seems that there isn't really a valid use-case that requires this complexity that couldn't be solved in a more simple way. Do you have a use-case that requires this? If not I think its a good idea to start the process of removing this feature. |
|
Looks like we have a number of test failures here |
|
Yea, from your last comment it sounded like this was still under dispute-- I need to add some deprecation warnings etc. and find the few places that On Tue, Feb 17, 2015 at 8:17 AM, Thomas S Hatch notifications@github.com
|
|
Sounds good, with the deprecation warnings I am ok putting this in develop |
Since now logging this, we've noticed its a bit noisy. To cut down on that I've changed the log level to debug. In addition, I changed the loader to keep a dict of mod -> error so that the caller (if so interested) can look up the error later. I switched caller and minion to use this new API. Now if you send a job to a minion witha module that isn't loaded you get a return like: ``` 'mod_name'' __virtual__ returned False: <error message here> ``` Fixes saltstack#21185
…sages-- and will be useful for stopping module merging down the road
Historically we have allowed modules to rename themselves (using __virtual__) to be the same name as a module we have already loaded. What this means is you can have 2 modules which end up being "foo", and all of their functions are placed in the loader dict (__salt__, for example). This means that all functions in the loader dict with the "foo" prefix don't come from the same code and are not gauranteed to be interoperable-- which is *very* confusing. The only use-cases we've seen could easily be implemented using the depends decorator or something similar Comes with the side-effect of increasing "miss" performance on the LazyDict
jacksontj
force-pushed
the
loader_nomerge
branch
from
06aa3c3
to
7e5e54a
Compare
|
This will break #21314 in its current form. |
|
This will also break the deb apache modules. We won't be merging this for a
|
|
@thatch45, @jacksontj, what think ye all about this pull request? |
|
I am going to close this out for a while, once activity starts again we can re-open |
Since we were checking if mod_init is in the loader dict-- and we still allow for module merging (saltstack#20540) we will do a load_all for every module without mod_init. This patch makes it so we load w/e module the function is in, then check if we have loaded a mod_init. This cuts out ~600ms on my local simple state.sls runs
Since we were checking if mod_init is in the loader dict-- and we still allow for module merging (saltstack#20540) we will do a load_all for every module without mod_init. This patch makes it so we load w/e module the function is in, then check if we have loaded a mod_init. This cuts out ~600ms on my local simple state.sls runs
Since we were checking if mod_init is in the loader dict-- and we still allow for module merging (saltstack#20540) we will do a load_all for every module without mod_init. This patch makes it so we load w/e module the function is in, then check if we have loaded a mod_init. This cuts out ~600ms on my local simple state.sls runs
In the current loader (and previous one) you are allowed to create a module with a virtual_name which can add its functions to the namespace (within salt) of another module. This is very confusing, and inconsistent (not to mention hard to debug). From talking it out in #20481 we determined there aren't use-cases that require this functionality, so we are going to remove it and move the few cases over to decorators, load, or something similarly less-magical.