Issues/29643 fix invalid retcode

[DmitryKuzmenko]

Fixes: #29643
Relates to: #28569

Problem:
Salt highstate is executed. One of states failed. Result retcode is 0.
Expected result retcode: 2.

Explanation:
The problem is the following: minion instance have functions LazyLoader instance keeping context inside. It calls modules.state.highstate() that is loaded by the loader and injected with the context instance. This state module function creates state.State instance that creates it's own LazyLoader instance with another context' instance and executes states. Some states uses modules and during modules loading the new context' instance is injected into the modules instead of the old one. By the LazyLoader design it searches a module to load by matching file names. If no exact nor partial match found it loads modules one-by-one and checks does the module registered the desired name with it's __virtual__ function. So loading using virtual modules such pkg could produce loading of the state module with the new loader and injecting the new context' into it.
When states execution is finished modules.state.highstate() function writes retcode value into the module context' dict that was updated by loader. But when it returns minon reads the value from its context dict it keeps in the old loader.

Fix:
I've changed state module to pass it's __context__ object to State object that is uses it when it creates the new LazyLoader instance. This keeps the context instance in all modules called indirectly from the state module.

[DmitryKuzmenko]

@jacksontj could you please provide your opinion?

[cachedout]

Might also be related to #30674

[meggiebot]

Need this in 2016.3 as well

[jacksontj]

LGTM :)

[DmitryKuzmenko]

@jacksontj great! Thanks!

[plastikos]

I have a patch that does the same thing that I've been using with 2015.8 for several months. Unfortunately the same patch does not work with the new tornado code that resets __context__ when yield is hit. Have you tested this with the Tornado code?

[plastikos]

This is the patch I've been using since October/November 2015:

diff -ur salt-2015.8.0.2015.10.02.old/salt/modules/state.py salt-2015.8.0.2015.10.02/salt/modules/state.py
--- salt-2015.8.0.2015.10.02.old/salt/modules/state.py  2015-10-02 11:56:17.000000000 -0700
+++ salt-2015.8.0.2015.10.02/salt/modules/state.py  2015-11-05 10:31:49.463717209 -0800
@@ -183,7 +183,7 @@
     conflict = _check_queue(queue, kwargs)
     if conflict is not None:
         return conflict
-    st_ = salt.state.State(__opts__)
+    st_ = salt.state.State(__opts__, context=__context__)
     err = st_.verify_data(data)
     if err:
         __context__['retcode'] = 1
@@ -224,7 +224,7 @@
         raise SaltInvocationError(
             'Pillar data must be formatted as a dictionary'
         )
-    st_ = salt.state.State(__opts__, pillar)
+    st_ = salt.state.State(__opts__, pillar, context=__context__)
     ret = st_.call_high(data)
     _set_retcode(ret)
     return ret
@@ -258,7 +258,7 @@
     conflict = _check_queue(queue, kwargs)
     if conflict is not None:
         return conflict
-    st_ = salt.state.HighState(__opts__)
+    st_ = salt.state.HighState(__opts__, context=__context__)
     if not tem.endswith('.sls'):
         tem = '{sls}.sls'.format(sls=tem)
     high_state, errors = st_.render_state(tem, saltenv, '', None, local=True)
@@ -283,7 +283,7 @@
     conflict = _check_queue(queue, kwargs)
     if conflict is not None:
         return conflict
-    st_ = salt.state.State(__opts__)
+    st_ = salt.state.State(__opts__, context=__context__)
     ret = st_.call_template_str(tem)
     _set_retcode(ret)
     return ret
@@ -529,7 +529,7 @@
     if 'pillarenv' in kwargs:
         opts['pillarenv'] = kwargs['pillarenv']

-    st_ = salt.state.HighState(opts, pillar, kwargs.get('__pub_jid'))
+    st_ = salt.state.HighState(opts, pillar, kwargs.get('__pub_jid'), context=__context__)
     st_.push_active()
     try:
         ret = st_.call_highstate(
@@ -681,7 +681,7 @@
             '{0}.cache.p'.format(kwargs.get('cache_name', 'highstate'))
             )

-    st_ = salt.state.HighState(opts, pillar, kwargs.get('__pub_jid'))
+    st_ = salt.state.HighState(opts, pillar, kwargs.get('__pub_jid'), context=__context__)

     if kwargs.get('cache'):
         if os.path.isfile(cfn):
@@ -779,7 +779,7 @@
             'Pillar data must be formatted as a dictionary'
         )

-    st_ = salt.state.HighState(opts, pillar)
+    st_ = salt.state.HighState(opts, pillar, context=__context__)
     st_.push_active()
     st_.opts['state_top'] = salt.utils.url.create(topfn)
     if saltenv:
@@ -820,7 +820,7 @@
             'Pillar data must be formatted as a dictionary'
         )

-    st_ = salt.state.HighState(__opts__, pillar)
+    st_ = salt.state.HighState(__opts__, pillar, context=__context__)
     st_.push_active()
     try:
         ret = st_.compile_highstate()
@@ -845,7 +845,7 @@
     if conflict is not None:
         assert False
         return conflict
-    st_ = salt.state.HighState(__opts__)
+    st_ = salt.state.HighState(__opts__, context=__context__)
     st_.push_active()
     try:
         ret = st_.compile_low_chunks()
@@ -883,7 +883,7 @@
         opts['test'] = __opts__.get('test', None)
     if 'pillarenv' in kwargs:
         opts['pillarenv'] = kwargs['pillarenv']
-    st_ = salt.state.HighState(opts)
+    st_ = salt.state.HighState(opts, context=__context__)
     if isinstance(mods, six.string_types):
         split_mods = mods.split(',')
     st_.push_active()
@@ -947,7 +947,7 @@
         opts['test'] = __opts__.get('test', None)
     if 'pillarenv' in kwargs:
         opts['pillarenv'] = kwargs['pillarenv']
-    st_ = salt.state.HighState(opts)
+    st_ = salt.state.HighState(opts, context=__context__)
     if isinstance(mods, six.string_types):
         mods = mods.split(',')
     st_.push_active()
@@ -1011,7 +1011,7 @@
     if 'pillarenv' in kwargs:
         opts['pillarenv'] = kwargs['pillarenv']

-    st_ = salt.state.HighState(opts, pillar)
+    st_ = salt.state.HighState(opts, pillar, context=__context__)
     if isinstance(mods, six.string_types):
         mods = mods.split(',')
     st_.push_active()
@@ -1052,7 +1052,7 @@
     conflict = _check_queue(queue, kwargs)
     if conflict is not None:
         return conflict
-    st_ = salt.state.HighState(opts)
+    st_ = salt.state.HighState(opts, context=__context__)
     errors = []
     top_ = st_.get_top()
     errors += st_.verify_tops(top_)
@@ -1104,7 +1104,7 @@
             'Pillar data must be formatted as a dictionary'
         )

-    st_ = salt.state.State(opts, pillar)
+    st_ = salt.state.State(opts, pillar, context=__context__)
     err = st_.verify_data(kwargs)
     if err:
         __context__['retcode'] = 1
@@ -1199,7 +1199,7 @@
         if not os.path.isdir(full):
             continue
         popts['file_roots'][fn_] = [full]
-    st_ = salt.state.State(popts, pillar=pillar)
+    st_ = salt.state.State(popts, pillar=pillar, context=__context__)
     ret = st_.call_chunks(lowstate)
     try:
         shutil.rmtree(root)
diff -ur salt-2015.8.0.2015.10.02.old/salt/state.py salt-2015.8.0.2015.10.02/salt/state.py
--- salt-2015.8.0.2015.10.02.old/salt/state.py  2015-10-02 11:56:17.000000000 -0700
+++ salt-2015.8.0.2015.10.02/salt/state.py  2015-11-05 10:30:26.976259138 -0800
@@ -593,13 +593,13 @@
     '''
     Class used to execute salt states
     '''
-    def __init__(self, opts, pillar=None, jid=None):
+    def __init__(self, opts, pillar=None, jid=None, context=None):
         if 'grains' not in opts:
             opts['grains'] = salt.loader.grains(opts)
         self.opts = opts
         self._pillar_override = pillar
         self.opts['pillar'] = self._gather_pillar()
-        self.state_con = {}
+        self.state_con = context or {}
         self.load_modules()
         self.active = set()
         self.mod_init = set()
@@ -3159,11 +3159,11 @@
     # a stack of active HighState objects during a state.highstate run
     stack = []

-    def __init__(self, opts, pillar=None, jid=None):
+    def __init__(self, opts, pillar=None, jid=None, context=None):
         self.opts = opts
         self.client = salt.fileclient.get_file_client(self.opts)
         BaseHighState.__init__(self, opts)
-        self.state = State(self.opts, pillar, jid)
+        self.state = State(self.opts, pillar, jid, context=context)
         self.matcher = salt.minion.Matcher(self.opts)

         # tracks all pydsl state declarations globally across sls files

[DmitryKuzmenko]

@plastikos I've just tested again with transport: tcp and tornado 4.3 and reproduced the problem. Am I right that it's the issue you've described in #30914?

[plastikos]

@DmitryKuzmenko, I'm pretty certain. This is the whole reason I didn't submit my patch above because it wasn't a general fix anymore with tornado.

[plastikos]

The retcode is still incorrect for me. There might be two bugs here such that this patch is necessary but not sufficient to get proper retcode pass-through to the master.