Should file.rename return success when target exists and force is not used? #49748

DickerDackel · 2018-09-22T13:44:10Z

Description of Issue/Question

Use-case:

Move a pre-packaged configuration of software package-x to package-x.conf.example
Place my own package-x.conf there instead

Works correct for the first run, when package-x.conf.example doesn't exist yet.
Fails on consecutive runs, because package-x.conf.example already exists.

Using 'force=True' is not an option, because after the second state.apply, the packaged version would be lost.

Shouldn't the file.rename report a "success, renamed file already in correct state" instead of a failure when force is not used?

Misunderstanding? Or am I doing something non-idiomatic here and there's a better way?

Setup

# Backup distro provided config as example
/etc/package-x/package-x.conf.example:
    file.rename:
        - source: /etc/package-x/package-x.conf

# Push our own config up there
/etc/package-x/package-x.conf:
    file.managed:
        - source: salt://roles/package-x/package-x.conf
        - user: root
        - group: root
        - mode: 644

Steps to Reproduce Issue

Initial run

salt:/srv/salt# salt $HOSTNAME state.apply roles/package-x
salt:
----------
          ID: /etc/package-x/package-x.conf.example
    Function: file.rename
      Result: True
     Comment: Moved "/etc/package-x/package-x.conf" to "/etc/package-x/package-x.conf.example"
     Started: 15:34:37.288072
    Duration: 0.507 ms
     Changes:
              ----------
              /etc/package-x/package-x.conf.example:
                  /etc/package-x/package-x.conf
----------
          ID: /etc/package-x/package-x.conf
    Function: file.managed
      Result: True
     Comment: File /etc/package-x/package-x.conf updated
     Started: 15:34:37.288700
    Duration: 22.224 ms
     Changes:
              ----------
              diff:
                  New file
              mode:
                  0644

Summary for salt
------------
Succeeded: 2 (changed=2)
Failed:    0
------------
Total states run:     2
Total run time:  22.731 ms


-------------------------------------------
Summary
-------------------------------------------
# of minions targeted: 1
# of minions returned: 1
# of minions that did not return: 0
# of minions with errors: 0
-------------------------------------------
salt:/srv/salt#

next run

salt:/srv/salt# salt $HOSTNAME state.apply roles/package-x
salt:
----------
          ID: /etc/package-x/package-x.conf.example
    Function: file.rename
      Result: False
     Comment: The target file "/etc/package-x/package-x.conf.example" exists and will not be overwritten
     Started: 15:35:23.268438
    Duration: 0.567 ms
     Changes:
----------
          ID: /etc/package-x/package-x.conf
    Function: file.managed
      Result: True
     Comment: File /etc/package-x/package-x.conf is in the correct state
     Started: 15:35:23.269182
    Duration: 17.892 ms
     Changes:

Summary for salt
------------
Succeeded: 1
Failed:    1
------------
Total states run:     2
Total run time:  18.459 ms


-------------------------------------------
Summary
-------------------------------------------
# of minions targeted: 1
# of minions returned: 1
# of minions that did not return: 0
# of minions with errors: 1
-------------------------------------------
ERROR: Minions returned with non-zero exit code
salt:/srv/salt#

Versions Report

salt:/srv/salt# salt --versions-report
Salt Version:
           Salt: 2018.3.2

Dependency Versions:
           cffi: Not Installed
       cherrypy: unknown
       dateutil: Not Installed
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.7.2
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.5 (default, Jul 13 2018, 13:06:57)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 15.3.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.4

System Versions:
           dist: centos 7.5.1804 Core
         locale: UTF-8
        machine: x86_64
        release: 4.18.7-1.el7.elrepo.x86_64
         system: Linux
        version: CentOS Linux 7.5.1804 Core

salt:/srv/salt#

The text was updated successfully, but these errors were encountered:

DickerDackel · 2018-09-22T13:50:48Z

Addendum: file.copy behaves as expected, an already existing version is not a failure but returns success:

So, using file.copy is a proper workaround, but still file.rename should behave consistently.

salt:/srv/salt# salt $HOSTNAME state.apply roles/package-x
salt:
----------
          ID: /etc/package-x/package-x.conf.example
    Function: file.copy
      Result: True
     Comment: The target file "/etc/package-x/package-x.conf.example" exists and will not be overwritten
     Started: 15:48:48.944001
    Duration: 6.656 ms
     Changes:
----------
          ID: /etc/package-x/package-x.conf
    Function: file.managed
      Result: True
     Comment: File /etc/package-x/package-x.conf is in the correct state
     Started: 15:48:48.950778
    Duration: 13.554 ms
     Changes:

Summary for salt
------------
Succeeded: 2
Failed:    0
------------
Total states run:     2
Total run time:  20.210 ms


-------------------------------------------
Summary
-------------------------------------------
# of minions targeted: 1
# of minions returned: 1
# of minions that did not return: 0
# of minions with errors: 0
-------------------------------------------
salt:/srv/salt#

MTecknology · 2018-09-23T11:28:41Z

So, without - force: true, file.rename should report similar to file.copy with The target file "/foo" exists and will not be overwritten, yeah? I have to agree, this seems logical. I suspect it's an easy patch that I could try to look into, unless you'd like to take a stab at it.

Ch3LL · 2018-09-24T20:05:06Z

im not able to replicate this on 2018.3.2 with this state:

[root@d28a9d276619 /]# cat /srv/salt/test.sls
test:
  file.rename:
    - source: /tmp/test
    - name: /tmp/test2

on the second run i see:

local:
----------
          ID: test
    Function: file.rename
        Name: /tmp/test2
      Result: True
     Comment: Source file "/tmp/test" has already been moved out of place
     Started: 20:03:28.810408
    Duration: 0.649 ms
     Changes:   

Summary for local
------------
Succeeded: 1
Failed:    0
------------
Total states run:     1
Total run time:   0.649 ms

When you pasted your salt version it looks like that is the version on your master. What is your minion version?

DickerDackel · 2018-09-24T23:27:58Z

As shown in the top commandline of my test case: I used the master's own '$HOSTNAME', so basically that was a localhost test.

Edit: What I wanted to say: Master Version == Minion Version, since that's been the same host.

Your test case is incomplete though.

You're only moving the file out of the way, but if you replace the initial filename with a new one in a following file.managed state, that should trigger the issue. See the file.managed state in my sls on top.

Ch3LL · 2018-09-28T14:36:53Z

thanks for clarifying that. I used your exact state and can now replicate it and I agree this should be reporting success. will label as a bug thanks :) as @MTecknology stated this should be an easy fix if you want to give it a go

DickerDackel · 2018-09-29T19:48:54Z

Sure, just wanted to make sure in advance, that my expectation was indeed correct.

I'll take a look.

DickerDackel · 2018-09-30T20:45:40Z

I'm reading your "Contributing" documentation right now, but that fix is so trivial, that a git pull request feels massively overkill for me, so here's a context diff.

I think the return ret['comment'] is fine, so the only thing that's changed is the removal of the False return value (ret['result'] is initialized on top of the function with True).

salt:/usr/lib/python2.7/site-packages/salt/states# diff -u file.py.orig file.py
--- file.py.orig        2018-09-30 22:30:20.926611941 +0200
+++ file.py     2018-09-30 22:33:09.458135593 +0200
@@ -5573,7 +5573,6 @@
         if not force:
             ret['comment'] = ('The target file "{0}" exists and will not be '
                               'overwritten'.format(name))
-            ret['result'] = False
             return ret
         elif not __opts__['test']:
             # Remove the destination to prevent problems later
salt:/usr/lib/python2.7/site-packages/salt/states#

damon-atkins · 2018-10-01T16:53:08Z

Mmm this is a behavior change. Someone might rely on the fact it errors. Another option is to add a failhard option for your use case.

MTecknology · 2018-10-01T19:16:16Z

Yes, this is a behavior change, but this discussion shows that the previous behavior was unexpected. The use case presented here seems substantially more likely than a use case that expects a failure in this situation.

This change makes the behavior between file.copy and file.rename match, as seen in the below unit tests.

file.copy:

comt2 = ('The target file "{0}" exists and will not be overwritten'.format(name))
ret.update({'comment': comt2, 'result': True})
self.assertDictEqual(filestate.copy(name, source, preserve=True), ret)

file.rename:

comt = ('The target file "{0}" exists and will not be overwritten'.format(name))
ret.update({'comment': comt, 'result': True})
self.assertDictEqual(filestate.rename(name, source), ret)

@gtmanfred

* upstream/develop: (379 commits) Update unit test to match expected behavior change. Couple lint fixes. Adding some tests for the vault execution module. Don't return False when target exists. (Fixes: saltstack#49748) Option print_result should be true by default Adding some changes for the status referncing saltstack#49204, fixing mis-spelling of lattrs on line 4514 per request from @gtmanfred Updating syntax to be better and with new lines Skip test_local_pkg_install for now: this is causing the test suite to hang Skip test_local_pkg_install for now: this is causing the test suite to hang Adding timeout to all pipelines so that the build aborts Removed blacklist of pkg_resources Remove development stub. Ughh... Lintfix: mute fopen warning Lintfix: explicitly close filehandle Mute pylint: file handler explicitly needed Fix six import Fix unit tests Lintfix: use salt.utils.files.fopen instead of open Lintfix: Uses of an external blacklisted import 'six': Please use 'import salt.ext.six as six' ...

Ch3LL added cannot-reproduce cannot be replicated with info/context provided info-needed waiting for more info labels Sep 24, 2018

Ch3LL added this to the Approved milestone Sep 24, 2018

MTecknology mentioned this issue Sep 30, 2018

Don't return False when target exists (file.rename) #49843

Merged

Ch3LL added the fixed-pls-verify fix is linked, bug author to confirm fix label Oct 1, 2018

terminalmage closed this as completed in a8b6e25 Oct 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Should file.rename return success when target exists and force is not used? #49748

Should file.rename return success when target exists and force is not used? #49748

DickerDackel commented Sep 22, 2018

DickerDackel commented Sep 22, 2018

MTecknology commented Sep 23, 2018

Ch3LL commented Sep 24, 2018

DickerDackel commented Sep 24, 2018 •

edited

Loading

Ch3LL commented Sep 28, 2018

DickerDackel commented Sep 29, 2018

DickerDackel commented Sep 30, 2018

damon-atkins commented Oct 1, 2018

MTecknology commented Oct 1, 2018

Should file.rename return success when target exists and force is not used? #49748

Should file.rename return success when target exists and force is not used? #49748

Comments

DickerDackel commented Sep 22, 2018

Description of Issue/Question

Setup

Steps to Reproduce Issue

Initial run

next run

Versions Report

DickerDackel commented Sep 22, 2018

MTecknology commented Sep 23, 2018

Ch3LL commented Sep 24, 2018

DickerDackel commented Sep 24, 2018 • edited Loading

Ch3LL commented Sep 28, 2018

DickerDackel commented Sep 29, 2018

DickerDackel commented Sep 30, 2018

damon-atkins commented Oct 1, 2018

MTecknology commented Oct 1, 2018

DickerDackel commented Sep 24, 2018 •

edited

Loading