Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "<tiamat> python" subcommand to allow execution or arbitrary scripts via bundled Python runtime #62388

Merged
merged 5 commits into from
Aug 8, 2022

Conversation

lukasraska
Copy link
Contributor

What does this PR do?

Introduces new python subcommand for Tiamat-based Salt packages in order to run scripts and applications with the same Python runtime as Salt.

What issues does this PR fix or reference?

Fixes: #62381

Previous Behavior

Users were not able to utilize Python Client API as no Python "executable" was exposed

New Behavior

Users can run external python scripts and applications via /path/to/salt python script.py command

Merge requirements satisfied?

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

Commits signed with GPG?

Yes

@lukasraska lukasraska requested a review from a team as a code owner July 27, 2022 10:10
@lukasraska lukasraska requested review from dwoz and removed request for a team July 27, 2022 10:10
@lukasraska
Copy link
Contributor Author

@Ch3LL here you go, I've targetted it against the freeze branch as I've noticed the issue is in Phosphorus milestone.

I've tested following usecases manually:

  1. Argument passthrough
root@ip-172-31-45-201:~# cat test_args.py 
import sys

print(sys.argv)
root@ip-172-31-45-201:~# /tmp/tmpm5yodb3j/salt/dist/salt python test_args.py arg1 arg2 arg3
['test_args.py', 'arg1', 'arg2', 'arg3']
  1. Exit code passthrough
root@ip-172-31-45-201:~# cat test_exit.py 
import sys

sys.exit(42)
root@ip-172-31-45-201:~# /tmp/tmpm5yodb3j/salt/dist/salt python test_exit.py 
root@ip-172-31-45-201:~# echo $?
42
  1. Salt library interaction (the freeze branch still seems to represent as 2016.3.1 version)
root@ip-172-31-45-201:~# cat test_salt_shebang.py 
#!/tmp/tmpm5yodb3j/salt/dist/salt python
import salt.version

print("\n".join(salt.version.versions_report()))
root@ip-172-31-45-201:~# /tmp/tmpm5yodb3j/salt/dist/salt python test_salt_shebang.py 
Salt Version:
          Salt: 2016.3.1+46009.gbe2c8d402b
 
Dependency Versions:
          cffi: 1.14.6
      cherrypy: 18.6.1
      dateutil: 2.8.1
     docker-py: Not Installed
         gitdb: 4.0.9
     gitpython: 3.1.27
        Jinja2: 3.1.0
       libgit2: Not Installed
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 1.0.2
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: 2.17
      pycrypto: Not Installed
  pycryptodome: 3.9.8
        pygit2: Not Installed
        Python: 3.10.4 (main, Jul 19 2022, 11:24:48) [GCC 11.2.0]
  python-gnupg: 0.4.8
        PyYAML: 5.4.1
         PyZMQ: Not Installed
         smmap: 5.0.0
       timelib: 0.2.4
       Tornado: 4.5.3
           ZMQ: Not Installed
 
System Versions:
          dist: ubuntu 22.04 jammy
        locale: utf-8
       machine: x86_64
       release: 5.15.0-1011-aws
        system: Linux
       version: Ubuntu 22.04 jammy
  1. Shebang functionality
root@ip-172-31-45-201:~# cat test_salt_shebang.py 
#!/tmp/tmpm5yodb3j/salt/dist/salt python
import salt.version

print("\n".join(salt.version.versions_report()))
root@ip-172-31-45-201:~# ./test_salt_shebang.py 
Salt Version:
          Salt: 2016.3.1+46009.gbe2c8d402b
 
Dependency Versions:
          cffi: 1.14.6
      cherrypy: 18.6.1
      dateutil: 2.8.1
     docker-py: Not Installed
         gitdb: 4.0.9
     gitpython: 3.1.27
        Jinja2: 3.1.0
       libgit2: Not Installed
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 1.0.2
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: 2.17
      pycrypto: Not Installed
  pycryptodome: 3.9.8
        pygit2: Not Installed
        Python: 3.10.4 (main, Jul 19 2022, 11:24:48) [GCC 11.2.0]
  python-gnupg: 0.4.8
        PyYAML: 5.4.1
         PyZMQ: Not Installed
         smmap: 5.0.0
       timelib: 0.2.4
       Tornado: 4.5.3
           ZMQ: Not Installed
 
System Versions:
          dist: ubuntu 22.04 jammy
        locale: utf-8
       machine: x86_64
       release: 5.15.0-1011-aws
        system: Linux
       version: Ubuntu 22.04 jammy
  1. Relative imports
root@ip-172-31-45-201:~# cat test_app/__init__.py 
import sub

print(sub.test())
root@ip-172-31-45-201:~# cat test_app/sub.py 
def test():
    return "ok"
root@ip-172-31-45-201:~# /tmp/tmpm5yodb3j/salt/dist/salt python test_app/__init__.py 
ok
  1. Exception handling
root@ip-172-31-45-201:~# cat test_exception.py 
raise Exception("test")
root@ip-172-31-45-201:~# /tmp/tmpm5yodb3j/salt/dist/salt python test_exception.py 
Traceback (most recent call last):
  File "/root/.pyenv/versions/3.10.4/envs/tmpyprx9b6s/bin/salt", line 113, in <module>
    redirect(sys.argv)
  File "/root/.pyenv/versions/3.10.4/envs/tmpyprx9b6s/bin/salt", line 93, in redirect
    python_runtime()
  File "/root/.pyenv/versions/3.10.4/envs/tmpyprx9b6s/bin/salt", line 70, in python_runtime
    exec(f.read())
  File "<string>", line 1, in <module>
Exception: test
[85680] Failed to execute script 'salt' due to unhandled exception!
[ERROR   ] An un-handled exception was caught by Salt's global exception handler:
Exception: test
Traceback (most recent call last):
  File "/root/.pyenv/versions/3.10.4/envs/tmpyprx9b6s/bin/salt", line 113, in <module>
    redirect(sys.argv)
  File "/root/.pyenv/versions/3.10.4/envs/tmpyprx9b6s/bin/salt", line 93, in redirect
    python_runtime()
  File "/root/.pyenv/versions/3.10.4/envs/tmpyprx9b6s/bin/salt", line 70, in python_runtime
    exec(f.read())
  File "<string>", line 1, in <module>
Exception: test

Here I'm a bit unsure whether I shouldn't rather wrap the whole exec in try/except and just print the exception and exit with 1 (as Python does it), because I don't particularly like the Salt exception handler mixing things up here

And also the tests are not written in Salt test suite as I don't believe there is any example test I could use to test this behaviour. Will ask on Slack if anybody has any idea.

@Ch3LL Ch3LL added the Phosphorus v3005.0 Release code name and version label Jul 27, 2022
@Ch3LL
Copy link
Contributor

Ch3LL commented Jul 27, 2022

Our package builds and tests run here: https://gitlab.com/saltstack/open/salt-pkg/ so tests for the packages would need to be added here. This does create a bit of an issue, since we would have to merge this in before adding a test in that repo. We are working on for next release to get the builds in the Salt repo, so you can build and test from the same place. Once this is reviewered and merged I don't mind writing up the tests real quick if you don't mind helping review it and point out any issues.

Ch3LL
Ch3LL previously approved these changes Jul 27, 2022
run.py Outdated Show resolved Hide resolved
run.py Outdated Show resolved Hide resolved
run.py Outdated Show resolved Hide resolved
waynew
waynew previously approved these changes Jul 29, 2022
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally I'm for this (per our discussion on Slack, we can't reliably access, for instance sys.executable or the pythonexecutable grain), I was a bit concerned that except Exception was either too broad or too narrow, but I think it's actually just right.

I tested this via some code like this:

import sys
import traceback

code = '''
print('cool')
exit(42)  # this line can change
print('not here though')
'''

try:
    exec(code)
except Exception:
    print(traceback.print_exc())
    exit(1)
print('done?')

This exits, as expected, with status code 42

✦ ❯ python something.py 
cool

/tmp/fun via 🐍 v3.8.10 
✦ ❯ echo $?
42
  • raise Exception('whoops') will print out the stack trace, and exit with 1.
  • input('Whatever:') and hitting ctrl+c displayed the KeyboardInterrupt trace, as well as had exit code of 1
  • adding a try: except KeyboardInterrupt around the input line, including a raise in the except block
    try:
        input('cool')
    except KeyboarInterrupt:
        print('okay')
        raise
    
    also worked as expected. Changing the raise in that block to exit(13) correctly exited with status 13

I banged around a bit more trying a few different cases and this logic all handled it appropriately. As long as we get the test code in salt-pkg as @Ch3LL mentioned, I'm 👍 for this.

run.py Outdated Show resolved Hide resolved
run.py Outdated Show resolved Hide resolved
Co-authored-by: Pedro Algarvio <pedro@algarvio.me>
@Ch3LL
Copy link
Contributor

Ch3LL commented Aug 8, 2022

Here is the MR where I'm adding tests around this new subcommand. https://gitlab.com/saltstack/open/salt-pkg/-/merge_requests/193

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Phosphorus v3005.0 Release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants