file.directory clean does not work as expected

[ryanwalder]

So I want Salt to clear out the /etc/apt/sources.list.d folder of any sources.list files it doesn't manage.

First attempt:

core-apt-sources-dir-clean:
  file.directory:
    - name: /etc/apt/sources.list.d
    - user: root
    - group: root
    - mode: 755
    - clean: True
    - require:
      - pkgrepo: core-apt-ubuntu

core-apt-ubuntu:
  pkgrepo.managed:
    - file: /etc/apt/sources.list.d/ubuntu.list
    - name: deb [arch=amd64] https://{{ repouser }}:{{ repopass }}@repo.company.com/ubuntu/ {{ grains['lsb_distrib_codename'] }} production
    - key_url: salt://core/apt/files/aptly.pub

Nope:

----------
          ID: core-apt-ubuntu
    Function: pkgrepo.managed
        Name: deb [arch=amd64] https://user:[email protected]/ubuntu/ trusty production
      Result: True
     Comment: Configured package repo 'deb [arch=amd64] https://user:[email protected]/ubuntu/ trusty production'
     Started: 14:11:25.777941
    Duration: 4029.592 ms
     Changes:
              ----------
              repo:
                  deb [arch=amd64] https://user:[email protected]/ubuntu trusty production
----------
          ID: core-apt-sources-dir-clean
    Function: file.directory
        Name: /etc/apt/sources.list.d
      Result: True
     Comment: Files cleaned from directory /etc/apt/sources.list.d
     Started: 14:11:29.807755
    Duration: 0.886 ms
     Changes:
              ----------
              removed:
                  - /etc/apt/sources.list.d/ubuntu.list

Ok, maybe it can only exclude something if it's a file:

core-apt-sources-dir-clean:
  file.directory:
    - name: /etc/apt/sources.list.d
    - user: root
    - group: root
    - mode: 755
    - clean: True
    - require:
      - file: core-apt-ubuntu

core-apt-ubuntu:
  file.managed:
    - name: /etc/apt/sources.list.d/ubuntu.list
    - source: salt://core/apt/files/ubuntu.list.jinja
    - template: jinja
    - user: root
    - group: root
    - mode: 644
    - makedirs: True

----------
          ID: core-apt-ubuntu
    Function: file.managed
        Name: /etc/apt/sources.list.d/ubuntu.list
      Result: True
     Comment: File /etc/apt/sources.list.d/ubuntu.list updated
     Started: 14:36:44.264193
    Duration: 4.971 ms
     Changes:
              ----------
              diff:
                  New file
              mode:
                  0644
----------
          ID: core-apt-sources-dir-clean
    Function: file.directory
        Name: /etc/apt/sources.list.d
      Result: True
     Comment: Files cleaned from directory /etc/apt/sources.list.d
     Started: 14:36:44.270779
    Duration: 0.973 ms
     Changes:
              ----------
              removed:
                     - /etc/apt/sources.list.d/ubuntu.list

Nope.

The docs are a little sparse:

clean
    Make sure that only files that are set up by salt and required by this function are kept. If this option is set then everything in this directory will be deleted unless it is required.
require
    Require other resources such as packages or files
exclude_pat
    When 'clean' is set to True, exclude this pattern from removal list and preserve in the destination.

Which to me reads that if something is required the end file it produces (if in that directory) won't get removed, which doesn't appear to be the case.

I also gave exclude_pat a go with no joy:

core-apt-sources-dir-clean:
  file.directory:
    - name: /etc/apt/sources.list.d
    - user: root
    - group: root
    - mode: 755
    - clean: True
    - exclude_pat: '(myrepo|ubuntu).list'

Also tried (myrepo|ubuntu) & (myrepo|ubuntu)\.list with no joy.

Tested on salt versions v2015.5.0 to v2015-5-5 and 2015.8 (all from git using the bootstrap), running on Ubuntu 14.04.

[jfindlay]

@ryanwalder, thanks for the report.

[bbinet]

I think this was a duplicate of #8646 which was fixed by #27748.
If confirmed, this issue can be closed.

[VynceMontgomery]

I suspect this is related to https://groups.google.com/forum/#!topic/salt-users/x-3zCIA_s4w which was at the time linked to two other bugs, both now closed but in ways that clearly didn't affect this issue. @ryanwalder can you check if your tests work if you use what is currently the -name onto the id line?

[ryanwalder]

Ok, so it's partially resolved.

It now works as expected with you manage a file directly (adds file, does not delete)

core-apt-sources-dir-clean:
  file.directory:
    - name: /etc/apt/sources.list.d
    - user: root
    - group: root
    - mode: 755
    - clean: True
    - require:
      - file: core-apt-ubuntu

core-apt-ubuntu:
  file.managed:
    - name: /etc/apt/sources.list.d/ubuntu.list
    - source: salt://salt-test/files/ubuntu.list.jinja
    - template: jinja
    - user: root
    - group: root
    - mode: 644
    - makedirs: True

local:
----------
          ID: core-apt-ubuntu
    Function: file.managed
        Name: /etc/apt/sources.list.d/ubuntu.list
      Result: True
     Comment: File /etc/apt/sources.list.d/ubuntu.list updated
     Started: 15:47:57.372843
    Duration: 23.23 ms
     Changes:   
              ----------
              diff:
                  New file
              mode:
                  0644
----------
          ID: core-apt-sources-dir-clean
    Function: file.directory
        Name: /etc/apt/sources.list.d
      Result: True
     Comment: Directory /etc/apt/sources.list.d is in the correct state
     Started: 15:47:57.396344
    Duration: 1.262 ms
     Changes:   

Summary for local
------------
Succeeded: 2 (changed=1)
Failed:    0
------------
Total states run:     2
Total run time:  24.492 ms

However it does not work as expected if salt creates the file (pkgrepo.managed adds the file, file.directory clean removes the file)

core-apt-sources-dir-clean:
  file.directory:
    - name: /etc/apt/sources.list.d
    - user: root
    - group: root
    - mode: 755
    - clean: True
    - require:
      - pkgrepo: core-apt-ubuntu

core-apt-ubuntu:
  pkgrepo.managed:
    - file: /etc/apt/sources.list.d/ubuntu.list
    - name: deb http://gb.archive.ubuntu.com/ubuntu/ {{ grains['lsb_distrib_codename'] }} main
    - require_in:
      - file: core-apt-sources-dir-clean

local:
----------
          ID: core-apt-ubuntu
    Function: pkgrepo.managed
        Name: deb http://gb.archive.ubuntu.com/ubuntu/ trusty main
      Result: True
     Comment: Configured package repo 'deb http://gb.archive.ubuntu.com/ubuntu/ trusty main'
     Started: 15:52:26.025460
    Duration: 3560.877 ms
     Changes:   
              ----------
              repo:
                  deb http://gb.archive.ubuntu.com/ubuntu trusty main
----------
          ID: core-apt-sources-dir-clean
    Function: file.directory
        Name: /etc/apt/sources.list.d
      Result: True
     Comment: Files cleaned from directory /etc/apt/sources.list.d
     Started: 15:52:29.586594
    Duration: 3.488 ms
     Changes:   
              ----------
              removed:
                  - /etc/apt/sources.list.d/ubuntu.list

Summary for local
------------
Succeeded: 2 (changed=2)
Failed:    0
------------
Total states run:     2
Total run time:   3.564 s

Version info:

vagrant@salt-test:~$ salt --versions
Salt Version:
           Salt: 2015.8.0-743-g2ad01fe

Dependency Versions:
         Jinja2: 2.7.2
       M2Crypto: 0.21.1
           Mako: Not Installed
         PyYAML: 3.10
          PyZMQ: 14.0.1
         Python: 2.7.6 (default, Jun 22 2015, 17:58:13)
           RAET: Not Installed
        Tornado: 4.3
            ZMQ: 4.0.4
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
        libnacl: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.3.0
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
         pygit2: Not Installed
   python-gnupg: Not Installed
          smmap: Not Installed
        timelib: Not Installed

System Versions:
           dist: Ubuntu 14.04 trusty
        machine: x86_64
        release: 3.13.0-65-generic
         system: Ubuntu 14.04 trusty

[roskens]

_gen_keep_files() isn't checking lowstates for any entries that require the file.directory being created.

--- salt/states/file.py.orig        2016-11-22 11:52:40.000000000 -0600
+++ salt/states/file.py        2016-12-03 15:51:16.422171951 -0600
@@ -403,6 +403,11 @@
                                 keep.update(_process(fn))
                     else:
                         keep.add(fn)
+    lowstates = [low for low in __lowstate__ if 'require' in low]
+    for low in lowstates:
+        for req in low['require']:
+            if 'file' in req and req['file'] == name:
+                keep.add(low['name'])
     return list(keep)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[ryanwalder]

This is still an issue.

[stale]

Thank you for updating this issue. It is no longer marked as stale.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[ryanwalder]

This is still an issue

[stale]

Thank you for updating this issue. It is no longer marked as stale.

[onmeac]

(running salt 3000)

We too have seen this issue, it gets even weirder when one uses keyword arguments for requisites (which we do).

Will not clean files:

make_dir:
  file.directory:
    - name: /test

clean_dir:
  file.directory:
    - name: /test
    - clean: True
    - require:
      - file: make_dir

manage_file:
  file.managed:
    - name: /test/somefile
    - require_in:
      - file: clean_dir

Cleans files:

make_dir:
  file.directory:
    - name: /test

clean_dir:
  file.directory:
    - name: /test
    - clean: True
    - require:
      - make_dir

manage_file:
  file.managed:
    - name: /test/somefile
    - require_in:
      - clean_dir

One of our many workarounds is to create the directory using cmd.run and then manage the "clean: True" using file.directory.

[sagetherage]

We will see if the core team can get to this in Aluminium, but cannot commit due to time and resource constraints. If anyone in the community would like to open an PR we will definitely review it and see if we can merge it sooner rather than later. :)

[sagetherage]

I am not convinced there is a workaround here so I am changing the severity up one level until determined otherwise. We will take a look at this in next couple of weeks.

[waynew]

@sagetherage I think we should downgrade the severity here, unless someone can show otherwise it looks like the exclude_pat should work.

I just tried with this:

clean_dir:
  file.directory:
    - name: /etc/apt/sources.list.d/
    - clean: True
    - exclude_pat: '*ubuntu.list*'

blerp-repo:
  pkgrepo.managed:
    - file: /etc/apt/sources.list.d/ubuntu.list
    - name: deb [arch=amd64] https://some.example.com/ubuntu/ {{ grains['lsb_distrib_codename'] }} production
    - require_in:
      -  file: clean_dir

This will not remove the expected file.

It will remove other files from the directory. (There are some other wonky things happening with file.directory when not using exclude_pat, but.... seems like we do have a reasonable workaround here)

[javierbertoli]

@waynew, sorry, but I fail to see how can exclude_pat help in the particular example that originated this bug report?

Usually, a formula managing an application and providing a repository for it, will add a file under the /etc/apt/sources.list.d directory.

So say that I have, originally, this situation:

/etc/apt/sources.list.d/manual_repo_a.list
/etc/apt/sources.list.d/manual_repo_b.list
/etc/apt/sources.list.d/manual_repo_c.list

Then, I add 2 formulas, with repos, which add these entries

/etc/apt/sources.list.d/salt_managed_repo_a.list
/etc/apt/sources.list.d/salt_managed_repo_b.list

And I add the resource

clean_dir:
  file.directory:
    - name: /etc/apt/sources.list.d/
    - clean: True

The expected behavior is that salt would remove the manual_repo_* entried and keep the salt_managed_* ones. (The names are patterned here for the sake of discussion, but in real life, there's no convention on these files naming other than the extension .list)

The bug describes that salt will delete ALL the entries and re-create the salt_managed_* ones, each time it's run. Or delete them at all, depending on the order of the states.

With the require/_in workaround, it will work/fail depending the case (see the example I provided above).

And unless I'm not understanding it correctly, the workaround you propose means that each time a new formula or piece of repo-managing code is added to a node, the exclude_pat pattern will need to be modified to reflect this? If this is the case, it does not seem to be a workaround at all.

Am I missing something?

If not, imho, the severity should stand the same, right?

[waynew]

That's exactly the workaround, adding the managed files to the exclude_pat -- or in your example which is actually quite a bit better, just have the exclude pattern be salt_managed_* and for any states/formulas/etc. added they would need to set the repo filename to 👈.

If this didn't work (say, exclude_pat didn't exist, or that functionality was broken) then there wouldn't be a workaround.

---

On a different part of this topic, I haven't looked at the underlying code here, but it feels like either we're missing a useful API to pass around file(names) in requisites, or some other way to communicate what files should be under Salt's management. That would allow clean to work as expected, since it could simply be something like

clean_dir:
  file.directory:
    - clean: True
    - skip_salt_managed_files: True

That would be handy to have not only for this state, but I'm sure others could also benefit.

[ygersie]

for now I worked around it like so:

/etc/ssh/authorized_keys:
  file.directory:
    - clean: true
# "exclude_list" is a list of files to be excluded from being removed by the
# "clean" operation. Salt would otherwise keep removing and adding the files everytime.
# The `or ""` makes sure the output of the list append isn't None for each key.
{% set exclude_list = [] %}
{% for user in salt['pillar.get']('ssh:authorized_keys', {}) %}
{{ exclude_list.append(user + ".pub") or "" }}
{% endfor %}
    - exclude_pat: {{ exclude_list }}

[waynew]

After further experimentation and really digging into this, it appears that the existing behavior is basically correct. That is to say, this was always (apparently) only intended to ignore files that were managed by a file state.

For the original issue, the expected spelling is:

core-apt-sources-dir-clean:
  file.directory:
    - name: /etc/apt/sources.list.d
    - user: root
    - group: root
    - mode: 755
    - clean: True
    - require:
      - file: core-apt-ubuntu

core-apt-ubuntu:
  pkgrepo.managed:
    - file: /etc/apt/sources.list.d/ubuntu.list
    - name: deb [arch=amd64] https://{{ repouser }}:{{ repopass }}@repo.company.com/ubuntu/ {{ grains['lsb_distrib_codename'] }} production
    - key_url: salt://core/apt/files/aptly.pub
  # vvvvvv --Add this-- vvvvvv
  file.managed:
    - name: /etc/apt/sources.list.d/ubuntu.list
    - create: False
    - replace: False

(create/replace: False aren't strictly necessary, but they do help avoid warnings)

It would be neat if all states/modules could specify the files that they're managing, but without some potentially pretty impressive changes that isn't possible. Instead, this particular functionality simply depends on the required state being a file state - in the above example that was added under the original ID, but it's fine if it's a different ID, as long as the requisite is spelled correctly. And the require can be in either direction with a require from file.directory or require_in from the other state.

I've opened a PR to make all of this very explicit in the documentation as well as provide some examples. Since the bug that used to be there was resolved as mentioned earlier up this comment chain, and this isn't really a bug in the code, and I've got the PR open, I'm going to go ahead and close this issue.

[frebib]

One mechanism that Salt has had since 3001 is the global creates requisite, which could easily be a good shim for telling clean: true not delete files, but that doesn't work. Would that be a reasonable workaround? It'd be simple to implement as the file module already looks at required states, so the creates files could be added to the list of files to keep.