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

SaltSSH cross-major version support #46684

Merged

Conversation

@isbm
Copy link
Contributor

@isbm isbm commented Mar 23, 2018

What does this PR do?

Adds two major features, fixes various bugs, refactores code.

Previous Behavior

  • SaltSSH cannot work if your Python interpreter major version differs to what you are trying to connect.
  • SaltSSH cannot work (or fails) if your target machine is an outdated Py2.6 box.

New Behavior

  • SaltSSH can be installed on a box with Py 3.x and still run code on Py 2.x. Requirements: there should be Salt installed as a library to the alternative interpreter.

  • Advanced mode: SaltSSH can be also configured to be packed alongside with an older version of Salt, say 2015.8 and still run on Py2.6 box in order to update it to the latest version, if this is required.

Another Improvements

  • Now Salt library (and other considerably share-able libraries) are no longer copied twice, but is placed into the common directory.

  • salt-call is rewritten so it now understand namespaces

Example

$ salt-ssh --version
salt-ssh 2018.3.0-408-g4f84226 (Oxygen)
$ python --version
Python 3.5.2
$ salt-ssh root@cyclone.suse.de grains.get pythonversion
cyclone.suse.de:
    - 2
    - 6
    - 9
    - final
    - 0
$ salt-ssh root@cyclone.suse.de pkg.info_installed kernel-default attr=build_date,version,vendor
cyclone.suse.de:
    ----------
    kernel-default:
        ----------
        build_date:
            2017-12-31T11:16:04Z
        vendor:
            SUSE LINUX Products GmbH, Nuernberg, Germany
        version:
            3.0.101

Tests written?

Yes

@ghost ghost requested review from Mar 23, 2018
@isbm
Copy link
Contributor Author

@isbm isbm commented Mar 23, 2018

@isbm isbm changed the title SaltSSH major version support across the machines SaltSSH cross-major version support Mar 23, 2018
SaltSSH also can pack whole additional, another version of Salt. For example, if there
would be an old box, running only outdated and unsupported Python 2.6, it is still
would be possible from within Python 3.5 or newer machine to access it. This feature
requires an additional configuration /etc/salt/master file as follows:
Copy link
Collaborator

@brejoc brejoc Mar 27, 2018

Could we rephrase that a little bit? Perhaps like this:

SaltSSH now works across different major Python versions. Python 2.7 ~ Python 3.x are now supported transparently. Requirement is, however, that the SaltMaster should have installed Salt, including all related dependencies for Python 2 and Python 3. Everything needs to be importable from the respective Python environment.

SaltSSH can bundle up an arbitrary version of Salt. If there would be an old box for example, running an outdated and unsupported Python 2.6, it is still possible from a SaltMaster with Python 3.5 or newer to access it. This feature requires an additional configuration in /etc/salt/master as follows:

Copy link
Contributor Author

@isbm isbm Mar 27, 2018

Fixd.

@@ -163,11 +165,15 @@ def unpack_thin(thin_path):
old_umask = os.umask(0o077) # pylint: disable=blacklisted-function
tfile.extractall(path=OPTIONS.saltdir)
tfile.close()
checksum_path = os.path.normpath(os.path.join(OPTIONS.saltdir, "thin_checksum"))
Copy link
Collaborator

@brejoc brejoc Mar 27, 2018

Shouldn't this be checksum_file?

Copy link
Contributor Author

@isbm isbm Mar 27, 2018

Why? os.path.normpath is just normalises path, and does not what os.path.basename does.

Copy link
Collaborator

@brejoc brejoc Mar 27, 2018

I'm talking about the name of checksum_path. Isn't thin_checksum the file you are writing to? So this should be checksum_file instead. Just a nitpick.

@@ -39,10 +39,10 @@ def __virtual__():
'''
Only load when the platform has zfs support
'''
if __grains__['zfs_support']:
if __grains__.get('zfs_support'):
Copy link
Collaborator

@brejoc brejoc Mar 27, 2018

Are the changes to zfs.py and zpool.py part of the changes to SaltSSH?

Copy link
Contributor Author

@isbm isbm Mar 27, 2018

In a way. 😉 They remove tracebacks in the logs every time when you call SaltSSH no matter on what platform.

elif py_ver == 3:
pymap.append('py3:3:0')

for ns, cfg in _six.iteritems(copy.deepcopy(extended_cfg) or {}):
Copy link
Collaborator

@brejoc brejoc Mar 27, 2018

Why are you deepcopying extended_cfg here?

elif compress == 'zip':
thin_ext = 'zip'
thintar = os.path.join(thindir, 'thin.' + thin_ext)
thintar = os.path.join(thindir, 'thin.' + (compress == 'gzip' and 'tgz' or 'zip'))
Copy link
Collaborator

@brejoc brejoc Mar 27, 2018

While this neatly tucks this into one line, it is not easier to read. But I guess this is a matter of taste.

Copy link
Member

@thatch45 thatch45 Mar 28, 2018

While I agree with @brejoc here, I am big van of more vertical code, I would not call this a show stopper by any means

It is also possible to add several alternative versions of Salt. One of the way is to
generate a Minimal tarball using runners and include it. However, this is possible only
when there is a running alternative Salt version, installed on the Master machine.
Copy link
Collaborator

@brejoc brejoc Mar 27, 2018

Could this also be rephrased? I'm a little bit confused. But perhaps it's just me. 😆

Copy link
Collaborator

@brejoc brejoc Mar 27, 2018

Okay, here is my take. Let me know if this still makes sense.

It is also possible to use several versions of Salt. You can for instance generate a minimal tarball using runners and include that. But this is only possible, when this specific Salt version is also installed on the SaltMaster.

Copy link
Contributor Author

@isbm isbm Mar 27, 2018

Almost. 😉 I took your version and added a bit of things. I think that @terminalmage will come up here at some point and anyways comb that up, since he is very good at it. 😜

except ImportError:
HAS_CERTIFI = False
certifi = None
Copy link
Collaborator

@brejoc brejoc Mar 27, 2018

Isn't SaltStack conventionally using uppercase variable names for those import flags? Or does this only apply to modules?

Copy link
Contributor Author

@isbm isbm Mar 27, 2018

No, not really. We have uppercase vars old style, but we also now have this way too elsewhere. Besides, I'd like to see how then your code will look like when you want to filter-out unavailable modules in get_tops but using uppercase vars instead. 😉

Copy link
Contributor

@cachedout cachedout Apr 11, 2018

This has been an informal convention but we do need to add this sort of thing to a style guide.

self.wipe = 'False'
else:
self.wipe = 'True' if self.opts.get('ssh_wipe') else 'False'
self.wipe = self.opts.get('ssh_wipe')
Copy link
Member

@dincamihai dincamihai Mar 27, 2018

This changes the type of self.wipe from string 'True'/'False' to, maybe, 'True'/None.
Is this intentional?

Copy link
Member

@dincamihai dincamihai Mar 27, 2018

I say maybe because I don't know if self.opts.get('ssh_wipe') returns string 'True'

Copy link
Contributor

@mcalmer mcalmer Mar 28, 2018

You should check this deeper. @isbm found out that the problems we see only exists when we use the wipe option.

Copy link
Member

@thatch45 thatch45 Mar 28, 2018

This just looks wrong. We should not be changing the wipe behavior

return __virtualname__
else:
return (False, "The zfs module cannot be loaded: zfs not supported")
return False, "The zfs module cannot be loaded: zfs not supported"
Copy link
Member

@dincamihai dincamihai Mar 27, 2018

Copy link
Contributor Author

@isbm isbm Mar 27, 2018

No, but then you will see the traceback all over the place. I could open another PR, but well, doesn't really hurt.

'''
# This list is subject to change
shareable = ['salt', 'jinja2',
'msgpack', 'certifi']
Copy link
Member

@dincamihai dincamihai Mar 27, 2018

What does it mean that the list is hardcoded to these packages?

Copy link
Contributor Author

@isbm isbm Mar 27, 2018

These are "known shareable packages and dependencies" that should be sitting in the common directory for every Python version. I see no reason to have this configurable at this point, even so these are also imported all over the place anyway. Maybe @thatch45 has a better idea, but for now I would prefer it this way.

'''
tops = []
for mod in [salt, jinja2, yaml, tornado, msgpack, certifi, singledispatch,
singledispatch_helpers, ssl_match_hostname, markupsafe, backports_abc]:
Copy link
Member

@dincamihai dincamihai Mar 27, 2018

This is another hardcoded list of packages. Do we want it like this?

Copy link
Contributor Author

@isbm isbm Mar 27, 2018

These are not hardcoded, but imports in the import section. Some of them are None, if they are not available.

Copy link
Contributor

@gtmanfred gtmanfred Apr 10, 2018

Also, this was hardcoded before, so i don't see much difference moving it to a list.

@isbm isbm force-pushed the isbm-saltssh-multimajor-py-version-support branch from 958330a to bad31fc Mar 27, 2018
@rallytime
Copy link
Contributor

@rallytime rallytime commented Mar 27, 2018

re-run all

NOTE: Py2 version of this test is not required, as code shares the same spot across the versions.
:return:
'''
Copy link
Member

@dincamihai dincamihai Mar 28, 2018

This test fails. This is the fix:

+        from salt.utils.thin import tempfile
+        tempfile.mkdtemp.return_value = ''

Copy link
Contributor Author

@isbm isbm Apr 5, 2018

I've added fix here: 3e6712d

@isbm isbm force-pushed the isbm-saltssh-multimajor-py-version-support branch from 0e539cc to 4f84226 Apr 5, 2018
@isbm isbm force-pushed the isbm-saltssh-multimajor-py-version-support branch from 6ff4b5f to 00e66fc Apr 6, 2018
@rallytime
Copy link
Contributor

@rallytime rallytime commented Apr 10, 2018

@thatch45 and @gtmanfred Can you guys review this addition?

Copy link
Contributor

@gtmanfred gtmanfred left a comment

I only had one nitpick change.

I am really impressed that this passed all our other ssh tests, including the enable_ssh_minions tests.

Also excited, Tom and I had talked about doing something like this where we just shipped everything, and I was supposed to work on it for fluorine, so I am happy I do not need to anymore :)

@@ -3397,7 +3397,7 @@ def _filter_matches(_match, _data, _opts):
ext_matches = self._master_tops()
for saltenv in ext_matches:
top_file_matches = matches.get(saltenv, [])
if self.opts['master_tops_first']:
if self.opts.get('master_tops_first'):
Copy link
Contributor

@gtmanfred gtmanfred Apr 10, 2018

master_tops_first should probably just be set to False in salt.config instead of switching to a get here.

Copy link
Contributor Author

@isbm isbm Apr 10, 2018

It is false. But in some cases it is just key error. So I do .get instead. Which is still False.

Copy link
Contributor

@gtmanfred gtmanfred Apr 10, 2018

Right, it is a key error because it is not defined in salt.config

All that is needed here for master_tops_first to always be defined is to add it to the default opts dictionary.

https://github.com/saltstack/salt/blob/2017.7/salt/config/__init__.py#L1523

Copy link
Contributor Author

@isbm isbm Apr 11, 2018

@gtmanfred added the option to the config by default. Leaving .get because in case disappears, it won't crash. Doesn't hurt anyway. 😉

Copy link
Contributor

@gtmanfred gtmanfred Apr 11, 2018

sounds good

'''
tops = []
for mod in [salt, jinja2, yaml, tornado, msgpack, certifi, singledispatch,
singledispatch_helpers, ssl_match_hostname, markupsafe, backports_abc]:
Copy link
Contributor

@gtmanfred gtmanfred Apr 10, 2018

Also, this was hardcoded before, so i don't see much difference moving it to a list.

@isbm
Copy link
Contributor Author

@isbm isbm commented Apr 11, 2018

@cachedout Anything else needed to be done here?

Copy link
Contributor

@cachedout cachedout left a comment

There are some nitpicks here but overall I think this is going enough to get in and do some cleanup in follow-on PRs.

@cachedout cachedout merged commit cbe1cfa into saltstack:develop Apr 11, 2018
6 of 10 checks passed
6 of 10 checks passed
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #18017 — ABORTED
Details
codeclimate 20 issues to fix
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #21735 — FAILURE
Details
@wip[bot]
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #24135 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #16289 — SUCCESS
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #3971 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #8801 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #20908 — SUCCESS
Details
@isbm isbm deleted the isbm-saltssh-multimajor-py-version-support branch May 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants