Fix py2 thin dir issues #48234
Fix py2 thin dir issues #48234
Conversation
ghost
requested review from
| @@ -1106,7 +1106,10 @@ def shim_cmd(self, cmd_str, extension='py'): | |||
| shim_tmp_file.write(salt.utils.to_bytes(cmd_str)) | |||
|
|
|||
| # Copy shim to target system, under $HOME/.<randomized name> | |||
| target_shim_file = '.{0}.{1}'.format(binascii.hexlify(os.urandom(6)), extension) | |||
| target_shim_file = '.{0}.{1}'.format( | |||
| binascii.hexlify(os.urandom(6)).decode('ascii'), | |||
There was a problem hiding this comment.
You likely only want to decode under py3 right?
There was a problem hiding this comment.
Good catch. I think we are alright though; decoding on python 2 doesn't cause an issue because the unicode string is decoded by format.
>>> '{}'.format(binascii.hexlify(os.urandom(6)).decode('ascii'))
'1f84d3dc6284'
There was a problem hiding this comment.
@dwoz I have some questions before we move forward:
- How exactly you've concluded that the problem is lack of
concurrent.futuresimport? Why this is affecting only Python 2 if the same module is not anywhere added also to Python 3?Could you explain why Python 3 survives without this module at all? Because this module has to be added the same way as in Python 2. Yet it is not added and yet it works. Why?
Unless I am missing something, the concurrent.futures is only relevant to a transport under normal operation between Master and Minion, and so it is used by Tornado in tornado.concurrent.Futures class. Salt SSH is copying all the states to the target machine and executes all that locally without transport involved. So technically, this module is not supposed to be even triggered under Salt SSH operation.
In the original issue I see no evidence that this is about this module. And if it is, I still would like to see the evidence that lack of import of this module causing this (tracebacks, root of the cause etc).
And a very clear reproducer.
|
Python 3 includes |
|
@s0undt3ch right. Forgot to look at Any idea what exactly happening there and why? |
|
@isbm Tornado is being included in the thin dir which is causing the need for |
What does this PR do?
Fixes a missing thin directory dependency when running under python2 and adds regression test for fix.
What issues does this PR fix or reference?
#46507
Previous Behavior
Running salt-call in thin directory will fail due to missing concurrent.futures import which can break salt-ssh under Python 2.
New Behavior
salt-ssh runs under both Python 2 and Python 3
Tests written?
Yes
Commits signed with GPG?
Yes