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

This needs discussion, since this breaks SUSE #34865

Merged
merged 1 commit into from
Jul 22, 2016

Conversation

thatch45
Copy link
Contributor

@thatch45 thatch45 commented Jul 21, 2016

What does this PR do?

This needs discussion, since this breaks salt-ssh on SUSE, it seems that there is something wrong with python-xml in the Python version shipped with OpenSUSE, or so the removed comment says. But we can't ship bins in the thin tarball since they will break on systems with an alternative version of python or alternative architecture.

What issues does this PR fix or reference?

Fix #26171

@isbm Do you know anything about this? Or know who best to consult on the SUSE side?

But it does fix all other distros....
@isbm
Copy link
Contributor

isbm commented Jul 22, 2016

@thatch45 I am not quite sure about the way it is fixed. Was it tried on JeOS, which, of course has things watered down to the very minimum or some sort of custom installation? We do have these things separated. So if XML is required (usually is), then the package python-xml from the standard repo additionally needs to be installed. Maybe it is a better idea to pre-check the environment first and then install missing things, instead? Ironically, salt.modules.zypper requires xml module too.

As of a typical, say, Leap installation:

$ cat /etc/os-release | grep PRETTY
PRETTY_NAME="openSUSE Leap 42.1 (x86_64)"

$ rpm -qf $(python -c 'import xml;print xml.__file__')
python-xml-2.7.9-21.3.x86_64

...or SLE installation (well, python-xml is a bit older there, of course):

$ cat /etc/os-release | grep PRETTY
PRETTY_NAME="SUSE Linux Enterprise Server 12 SP1"

$ rpm -qf $(python -c 'import xml;print xml.__file__')
python-xml-2.7.9-20.6.x86_64

Or maybe I am missing some context from #26171?

@thatch45
Copy link
Contributor Author

Your comments are very helpful! Let me set up the context. Salt-ssh works by sending all salt sources in a tarball call the thin down to a target system. This thin tarball contains all of the salt sources and all of the salt deps so they can run on the target system. To make this work we need to ship only python sources, no bins, since the bins are dependent on the build of python architecture they were built against. This is what then causes salt-ssh to fail when running from cent6 to cent7, and subsequently it could fail when running from say Leap to tumbleweed, because the xml libs are compiled python modules.
So we do just need to take them out, and we might just need to say that you need python-xml installed on the target if it is SUSE, since shipping bins in the thin will always cause this to break.

@thatch45
Copy link
Contributor Author

So I think that my conclusion is to merge this in, if we want to add a fix to automatically install python-xml on SUSE we should do it here:
https://github.com/saltstack/salt/blob/develop/salt/client/ssh/ssh_py_shim.py
That file is sent over to the target system after the thin tarball is sent over and then executed to run said thin tarball. Due to how it is sent it does need to dep only core python standard libs and needs to stay small (it is send as base64 encoded data inside a shell HEREDOC piped to ssh)

@thatch45
Copy link
Contributor Author

Ok, since I think this is the greater of the 2 options I am going to merge it in

@thatch45 thatch45 merged commit 584d760 into saltstack:2016.3 Jul 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants