Made the salt-ssh shell shim compatible with AIX. #11004

Merged
merged 1 commit into from Mar 7, 2014

Projects

None yet

3 participants

@dumol
Contributor
dumol commented Mar 6, 2014

salt-ssh fails for AIX targets, as discussed at https://groups.google.com/forum/#!searchin/salt-users/aix/salt-users/G_a53lNYPsA/Vt6-WWjP-uUJ :

$ salt-ssh -c ./etc/salt/ '*aix*' test.ping
aix-aix-5.3-ppc64:
    ----------
    retcode:
        127
    stderr:
        /bin/sh[28]: 0652-141: 0403-012 A test command parameter is not valid.
        /bin/sh[28]: 0652-141: 0403-012 A test command parameter is not valid.
        /bin/sh[28]: 0652-141: 0403-012 A test command parameter is not valid.
        /bin/sh[28]: 0652-141: 0403-012 A test command parameter is not valid.
        /bin/sh[47]: 0652-141: 0403-012 A test command parameter is not valid.
        /bin/sh[47]: 0652-141: 0403-012 A test command parameter is not valid.
        The following required Packages are missing:  md5 or md5sum

    stdout:

The list of fixes:

  • fixed shell tests
  • made variables definitions safer
  • replaced which with the POSIX shell built-in command -v
  • added code for MD5 checking through AIX's csum
  • added support for md5 binaries in non-standard locations
  • replaced current tar invocation with a more portable one
  • added one more error code for failed checksumming, this replaces the generic error that sometimes occurs on first call:
freebsd-freebsd-10.0-x86:
    ----------
    retcode:
        2
    stderr:
        /usr/local/bin/python2.7: can't open file '/tmp/.salt/salt-call': [Errno 2] No such file or directory

    stdout:

with a specific new one:

freebsd-freebsd-10.0-x86:
    ----------
    retcode:
        1
    stderr:
        Mismatched checksum for /tmp/.salt/salt-thin.tgz

    stdout:

The test fixes also prevents errors such as:

linux-ubuntu-1204-x64:
    ----------
    retcode:
        2
    stderr:
        /bin/sh: 35: [: /usr/bin/python2.7: unexpected operator
        /bin/sh: 60: [: /usr/bin/md5sum: unexpected operator

End result:

$ salt-ssh -c ./etc/salt/ '*aix*' test.ping
aix-aix-5.3-ppc64:
    True

Tested on a variety of OS's:

$ salt-ssh -c ./etc/salt/ '*' grains.item os oscodename osfinger os_family osfullname osarch cpuarch | grep stdout -A1
    stdout:
        {"osarch": "x86_64", "cpuarch": "x86_64", "osfullname": "Gentoo", "os_family": "Gentoo", "oscodename": "", "os": "Gentoo"}
--
    stdout:
        {"osarch": "x86_64", "cpuarch": "x86_64", "osfullname": "SUSE", "os_family": "Suse", "oscodename": "n.a", "os": "SUSE"}
--
    stdout:
        {"osarch": "i386", "cpuarch": "i686", "osfullname": "Ubuntu", "os_family": "Debian", "oscodename": "lucid", "osfinger": "Ubuntu-10.04", "os": "Ubuntu"}
--
    stdout:
        {"osarch": "x86_64", "cpuarch": "x86_64", "osfullname": "Red Hat Enterprise Linux AS", "os_family": "RedHat", "oscodename": "Nahant Update 8", "osfinger": "Red Hat Enterprise Linux AS-4", "os": "RedHat"}
--
    stdout:
        {"osarch": "amd64", "cpuarch": "x86_64", "osfullname": "Ubuntu", "os_family": "Debian", "oscodename": "precise", "osfinger": "Ubuntu-12.04", "os": "Ubuntu"}
--
    stdout:
        {"osarch": "i386", "os_family": "FreeBSD", "os": "FreeBSD", "cpuarch": "i386"}
--
    stdout:
        {"osarch": "0001963AD300", "os_family": "AIX", "os": "AIX", "cpuarch": "0001963AD300"}
--
    stdout:
        {"osarch": "sparc", "cpuarch": "sparc64", "osfullname": "Debian GNU/Linux", "os_family": "Debian", "oscodename": "", "os": "Debian"}
@salt-jenkins
Contributor

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2091/

@thatch45 thatch45 merged commit ea8e62e into saltstack:develop Mar 7, 2014

1 check failed

default Merged build finished.
Details
@thatch45
Member
thatch45 commented Mar 7, 2014

W00t! Thanks!

@dumol
Contributor
dumol commented Mar 7, 2014

Thank you too! I could refactor the shim shell code to:

  • use a function for the duplicate code for verifying required commands, say check_command()
  • move custom paths to the beginning of the shim shell code using variables (eg. SALT_TMP_PATH="/tmp/.salt", SALT_TGZ="$SALT_TMP_PATH/salt-thin.tgz"
  • use backticks instead of $() for beyond POSIX compatibility (think older Solaris /bin/sh and csh-style shells)
  • have a consistent indentation of 4 spaces
  • make if/then syntax consistent with existing shell code, eg. https://github.com/saltstack/salt/blob/75c2bd46b46b6edaac9fc083b40ca6563c1bda46/pkg/salt.bash

What do you think?

@dumol
Contributor
dumol commented Mar 12, 2014

@thatch45

Any thoughts on my previous comment? Thanks!

@thatch45
Member

Sorry, I missed this!
The only thing I am hesitant on is the $() because backticks can escape differently in some shells, but it may still be the most compatible approach. All other suggestions are awesome!

@dumol
Contributor
dumol commented Mar 15, 2014

Thanks @thatch45 , I'll try my hand in a new pull request and add Solaris 10 to the tested targets.

In regards to backticks, it is pretty much required to switch to them in order to support Solaris' /bin/sh. Perhaps I should first try my hand at supporting Solaris and then refactor the code, as its ancient /bin/sh should be the lowest common denominator.

I foresee problems with verifying the md5 checksum too in Solaris as it has no md5sum, md5 or csum commands by default. We should either use digest or ditch the current approach and use Python for that, eg. python -c "import hashlib;print hashlib.md5(open('$FILE','rb').read()).hexdigest()". Python is already launched once by the shell shim code to verify its version, so we could use it for verifying the MD5 sum too. What do you think?

@thatch45
Member

I agree, I think that using python is the best way here, and given this data we should go ahead and use backticks. Please add comments explaining these decisions right above the shim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment