Salt: cmd.run cwd should not be checked before preconditions

[eeaston]

Interesting catch-22 issue here.
Say you have:

TaskA:
   cmd.run:
    - name: mkdir /tmp/a
    - unless:  which B

TaskB:
   cmd.run:
   - name: echo "hello" > /bin/B
   - unless: which B
   - cwd: /tmp/a
   - require: 
      - TaskA

The first time this provisions, it works fine. Then the machine reboots and /tmp is gone.
The next time it provisions, it will fail because the cwd for TaskB no longer exists, and wont be created because the 'unless' precondition of TaskA is now fulfilled. We're stuck.

I know this seems a silly case, but you can conceivably see a number of chained build steps doing something like this.

I've had a stab at fixing this here at eeaston/salt@3273ae on the 2014.1 branch, but it's a bit of an API change. The changes were to move the cwd existence checking until after the precondition checking, and also remove the cwd from the context of the 'unless' and 'onlyif' tests.

If you'd like me to continue on this I can (I'll write some tests and stuff), but the removal of the cwd from the 'unless' and 'onlyif' checks will need to be documented.
What say ye?
Cheers,
Ed

[s0undt3ch]

Let's also wait for other opinions about this particular issue since I currently can't say yes or no to your changes(I'm not around a computer right now). What I can say is, whatever changes you do, please make them against the develop branch.

Pedro Algarvio @ Phone

----- Reply message -----
From: "eeaston" [email protected]
To: "saltstack/salt" [email protected]
Subject: [salt] cmd.run cwd should not be checked before preconditions (#11497)
Date: Tue, Mar 25, 2014 10:04
Interesting catch-22 issue here.

Say you have:

TaskA:
cmd.run:

• name: mkdir /tmp/a
• unless: which B

TaskB:
cmd.run:

• name: echo "hello" > /bin/B
• unless: which B
• cwd: /tmp/a
• require:
• TaskA

The first time this provisions, it works fine. Then the machine reboots and /tmp is gone.

The next time it provisions, it will fail because the cwd for TaskB no longer exists, and wont be created because the 'unless' precondition of TaskA is now fulfilled. We're stuck.

I know this seems a silly case, but you can conceivably see a number of chained build steps doing something like this.

I've had a stab at fixing this here at eeaston/salt@3273ae on the 2014.1 branch, but it's a bit of an API change. The changes were to move the cwd existence checking until after the precondition checking, and also remove the cwd from the context of the 'unless' and 'onlyif' tests.

If you'd like me to continue on this I can (I'll write some tests and stuff), but the removal of the cwd from the 'unless' and 'onlyif' checks will need to be documented.

What say ye?

Cheers,

Ed

—
Reply to this email directly or view it on GitHub.

[basepi]

Yes, all pull requests should be against the develop branch, not the 2014.1 branch.

I'd be OK with moving the cwd check below the unless check. I think that's where most people would expect it to be anyway, but I could be wrong.

[frumpel]

I have a real use case for this (CentOS 6 / salt 2014.7) - and I think the precondition check isn't just wrong for CWD but the whole cmd.run execution:

The example may seem contrived but it's based on Oracle's installer package being dirty and (a) leaving files in places other than the current working directory and (b) using unzip in a way where it tries to ask whether to unzip a file over top of an existing one even it it doesn't have a terminal :(

This fails:

# Install jdk 1.6.0_32 from tarball in AWS

{% set major_version = 6 %}
{% set minor_version = 32 %}

/tmp/jdk/:
  file.directory:
    - user: root
    - group: root
    - unless: test -d /usr/java/jdk1.{{major_version}}.0_{{minor_version}}

/tmp/jdk/jdk-{{major_version}}u{{minor_version}}-linux-x64-rpm.bin:
  file.managed:
    - source: https://s3-us-west-2.amazonaws.com/smart-smex-salt-bucket/Linux/CentOS/Binaries/jdk-{{major_version}}u{{minor_version}}-linux-x64-rpm.bin
    - source_hash: md5=b30130fce5769b72608bbfc83ab1de4a
    - user: root
    - group: root
    - unless: test -d /usr/java/jdk1.{{major_version}}.0_{{minor_version}}
    - require:
      - file: /tmp/jdk/

expand_jdk_{{major_version}}_{{minor_version}}:
  cmd.run:
    - name : /bin/sh /tmp/jdk/jdk-{{major_version}}u{{minor_version}}-linux-x64-rpm.bin -noregister
    - cwd: /tmp/jdk
    - unless: test -d /usr/java/jdk1.{{major_version}}.0_{{minor_version}}
    - require:
      - file: /tmp/jdk/jdk-{{major_version}}u{{minor_version}}-linux-x64-rpm.bin

# We need to do this because otherwise the stupid sfx extractor hangs when 
# checking whether we really want to overwrite the already present files
delete_jdk_{{major_version}}_{{minor_version}}:
  cmd.run:
    - name: rm -rf /tmp/jdk /root/jdk-6u32-linux-amd64.rpm
    - unless: test -d /usr/java/jdk1.{{major_version}}.0_{{minor_version}}
    - require: 
      - cmd: expand_jdk_{{major_version}}_{{minor_version}}

local:
----------
          ID: /tmp/jdk/
    Function: file.directory
      Result: True
     Comment: unless execution succeeded
     Started: 03:36:41.847419
    Duration: 32.95 ms
     Changes:   
----------
          ID: /tmp/jdk/jdk-6u32-linux-x64-rpm.bin
    Function: file.managed
      Result: True
     Comment: unless execution succeeded
     Started: 03:36:41.889589
    Duration: 23.648 ms
     Changes:   
----------
          ID: expand_jdk_6_32
    Function: cmd.run
        Name: /bin/sh /tmp/jdk/jdk-6u32-linux-x64-rpm.bin -noregister
      Result: False
     Comment: Desired working directory "/tmp/jdk" is not available
     Started: 03:36:41.922704
    Duration: 1.255 ms
     Changes:   
----------
          ID: delete_jdk_6_32
    Function: cmd.run
        Name: rm -rf /tmp/jdk /root/jdk-6u32-linux-amd64.rpm
      Result: False
     Comment: One or more requisite failed
     Started: 
    Duration: 
     Changes:   

Summary
------------
Succeeded: 2
Failed:    2
------------

Note how the FOURTH stanza also fails. It simply fails on the require line even though it doesn't have a cwd. Without looking at the code I would interpret this as the require being evaluated before the unless.

The following succeeds but it's ugly because on subsequent runs it will create and destroy directories and thus always look like something is changing:

# Install jdk 1.6.0_32 from tarball in AWS

{% set major_version = 6 %}
{% set minor_version = 32 %}

/tmp/jdk/:
  file.directory:
    - user: root
    - group: root

/tmp/jdk/jdk-{{major_version}}u{{minor_version}}-linux-x64-rpm.bin:
  file.managed:
    - source: https://s3-us-west-2.amazonaws.com/smart-smex-salt-bucket/Linux/CentOS/Binaries/jdk-{{major_version}}u{{minor_version}}-linux-x64-rpm.bin
    - source_hash: md5=b30130fce5769b72608bbfc83ab1de4a
    - user: root
    - group: root
    - unless: test -d /usr/java/jdk1.{{major_version}}.0_{{minor_version}}
    - require:
      - file: /tmp/jdk/

expand_jdk_{{major_version}}_{{minor_version}}:
  cmd.run:
    - name : /bin/sh /tmp/jdk/jdk-{{major_version}}u{{minor_version}}-linux-x64-rpm.bin -noregister
    - cwd: /tmp/jdk
    - unless: test -d /usr/java/jdk1.{{major_version}}.0_{{minor_version}}
    - require:
      - file: /tmp/jdk/jdk-{{major_version}}u{{minor_version}}-linux-x64-rpm.bin

# We need to do this because otherwise the stupid sfx extractor hangs when 
# checking whether we really want to overwrite the already present files
delete_jdk_{{major_version}}_{{minor_version}}:
  cmd.run:
    - name: rm -rf /tmp/jdk /root/jdk-6u32-linux-amd64.rpm
    - require: 
      - cmd: expand_jdk_{{major_version}}_{{minor_version}}

local:
----------
          ID: /tmp/jdk/
    Function: file.directory
      Result: True
     Comment: Directory /tmp/jdk updated
     Started: 03:38:40.618939
    Duration: 2.23 ms
     Changes:   
              ----------
              /tmp/jdk:
                  New Dir
----------
          ID: /tmp/jdk/jdk-6u32-linux-x64-rpm.bin
    Function: file.managed
      Result: True
     Comment: unless execution succeeded
     Started: 03:38:40.621698
    Duration: 32.782 ms
     Changes:   
----------
          ID: expand_jdk_6_32
    Function: cmd.run
        Name: /bin/sh /tmp/jdk/jdk-6u32-linux-x64-rpm.bin -noregister
      Result: True
     Comment: unless execution succeeded
     Started: 03:38:40.655846
    Duration: 32.614 ms
     Changes:   
----------
          ID: delete_jdk_6_32
    Function: cmd.run
        Name: rm -rf /tmp/jdk /root/jdk-6u32-linux-amd64.rpm
      Result: True
     Comment: Command "rm -rf /tmp/jdk /root/jdk-6u32-linux-amd64.rpm" run
     Started: 03:38:40.689592
    Duration: 33.351 ms
     Changes:   
              ----------
              pid:
                  3832
              retcode:
                  0
              stderr:

              stdout:


Summary
------------
Succeeded: 4 (changed=2)
Failed:    0
------------
Total states run:     4

[Reiner030]

Hi,

Problem is still open...

# salt-call --version
salt-call 2014.7.5 (Helium)

The "origin" state i found in a salt-template:

unpack-nexus-tarball:
  cmd.run:
    - name: curl -L '{{ nexus.source_url }}' | tar xz
    - user: {{ project_name }}
    - cwd: {{ nexus.download_dir }}
{%-   if pillar['project']['name'] != None %}
    - unless: test -d {{ project_base_path }}/{{ project_name }}/nexus-{{ nexus.version }}
    - require:
        - file: {{ project_base_path }}/{{ project_name }}
{%-   else %}
    - unless: test -d {{ nexus.real_home }}
    - require:
        - file: {{ nexus.prefix }}
{%-   endif %}
        - file: {{ nexus.download_dir }}
{%- endif %}

creates in each run a "{{ nexus.download_dir }} not found" error when the folder didn't exists.
I had to fix it to use the cwd in the command itself:

unpack-nexus-tarball:
  cmd.run:
    - name: curl -L '{{ nexus.source_url }}' | tar xzC {{ nexus.download_dir }}
    - user: {{ project_name }}
    - onlyif: test -d {{ nexus.download_dir }}
{%- if pillar['project']['name'] != None %}
    - unless: test -d {{ project_base_path }}/{{ project_name }}/nexus-{{ nexus.version }}
    - require:
        - file: {{ project_base_path }}/{{ project_name }}
{%- else %}
    - unless: test -d {{ nexus.real_home }}
    - require:
        - file: {{ nexus.prefix }}
{%- endif %}

to get the state work without created tmp_dir / if no updates are available and its not created...

[rallytime]

How does the fix in #32293 work for everyone that has commented here?

[rallytime]

Fixed in #32418.

[taylor-peterson]

I''m having this same issue on 2016.03.0

[sastorsl]

I'm seeing something similar to this with cmd.run + cwd + onlyif

<state>:
  - cmd.run: <do-something-to> /my/full/path
  - cwd: /my/full
  - onlyif:
    - /usr/bin/test -d /my/full/path

I would expect that the state is skipped if the onlyif test fails, but instead I'm getting:

 Comment: An exception occurred in this state: Traceback (most recent call last):
            File "/usr/lib/python2.7/site-packages/salt/state.py", line 1733, in call
              **cdata['kwargs'])
            File "/usr/lib/python2.7/site-packages/salt/loader.py", line 1651, in wrapper
              return f(*args, **kwargs)
            File "/usr/lib/python2.7/site-packages/salt/states/cmd.py", line 821, in run
              cret = mod_run_check(cmd_kwargs, onlyif, unless, creates)
            File "/usr/lib/python2.7/site-packages/salt/states/cmd.py", line 332, in mod_run_check
              cmd = __salt__['cmd.retcode'](entry, ignore_retcode=True, python_shell=True, **cmd_kwargs)
            File "/usr/lib/python2.7/site-packages/salt/modules/cmdmod.py", line 1773, in retcode
              password=kwargs.get('password', None))
            File "/usr/lib/python2.7/site-packages/salt/modules/cmdmod.py", line 460, in _run
              .format(cwd)
          CommandExecutionError: Specified cwd '/my/full/path' either not absolute or does not exist

My version

$ salt --versions-report
Salt Version:
           Salt: 2016.3.2

Dependency Versions:
           cffi: Not Installed
       cherrypy: 3.2.2
       dateutil: 1.5
          gitdb: 0.5.4
      gitpython: 0.3.2 RC1
          ioflo: Not Installed
         Jinja2: 2.7.2
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: 0.21.1
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.7
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
         pygit2: Not Installed
         Python: 2.7.5 (default, Oct 11 2015, 17:47:16)
   python-gnupg: Not Installed
         PyYAML: 3.10
          PyZMQ: 14.7.0
           RAET: Not Installed
          smmap: 0.8.1
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.0.5

System Versions:
           dist: redhat 7.2 Maipo
        machine: x86_64
        release: 3.10.0-327.13.1.el7.x86_64
         system: Linux
        version: Red Hat Enterprise Linux Server 7.2 Maipo