minion/master file.recurse needs to stop using clean: True

[iggy]

The salt code (and myself) put files in /etc/salt/{master,minion}.d

The "clean: True" is killing all that. It probably shouldn't do that.

salt-minion:
  file.recurse:
    - name: {{ salt.get('config-path', '/etc/salt') }}/minion.d
    - template: jinja
    - source: salt://salt/files/minion.d
    - clean: True
    - context:
        standalone: False

[nmadhok]

I think there should be a way to specify this in pillar with the default being clean:True

[iggy]

The default will erase files that salt itself is writing out. So... that seems like a bad default to me.

Maybe @thatch45 can weigh in on how important it is to keep that _schedule.conf file around or not (it was a commit from him that added that persist function).

[nmadhok]

@iggy there can be config files present in that directory even before the installation of salt using salt-formula. If this is the case, we wouldn't want them to be there and would want salt-formula to put a set of new config files there. Perhaps backing up the old config files or maybe renaming them to have a .bak extension would be a good idea so if you want to recover the config file, you can do so manually. Thoughts @thatch45 @basepi ?

[basepi]

Sounds like a good solution to me. Backups are always good.

[iggy]

That doesn't answer the question of whether it's safe to just obliterate files that the salt master is writing.

[basepi]

I haven't looked closely enough at this formula to know the answer to that question. Is this state running during salt-minion startup? Additionally, I don't think the minion writes files to this directory -- it only writes files to the pki directory as far as I know.

[iggy]

I put a link to the code in the original post. It's utils.schedule doing it. It is definitely the minion doing the writing. I didn't write these files myself.

The only thing I don't know is if these files shouldn't be erased (as they are now with this formula).

[basepi]

Oh, I see, it must be a scheduled state run of some sort or something.

[basepi]

I only meant that the main startup of the minion doesn't write those files.

[iggy]

Let me know if you need more info.

bjackson@dev-salt01:~$ cat /etc/salt/minion.d/_schedule.conf 
schedule:
  __mine_interval: {function: mine.update, jid_include: true, maxrunning: 2, minutes: 5}

[nmadhok]

@basepi @iggy backing up the old config files and renaming them to have a .bak extension would be a good idea. @iggy Can you submit a PR for that change?

[iggy]

I'm unlikely to do a PR for that seeing as I'm against it using "clean: True" to begin with.

[basepi]

If you don't clean: True, how can you ensure that configuration deployed by the master takes effect? Other files in master.d might override what you're trying to do. You want to own the configuration, right?

[nmadhok]

I agree with @basepi

[iggy]

I understand where you guys are coming from. My issue is that the clean: True is going to wipe out files that salt is writing for a reason (presumably).

[aboe76]

So i'm reading this and is because salt minion has _schedule.conf in /etc/salt/minion.d
in some way, and the salt-formula cleans out the master.d/ and minion.d folders and pushes its own
_defaults.conf file there.

Only thing I can think of is why salt-formula doesn't create _schedule.conf with configurable settings...
salt-formula should configure/control all configuration files.

@basepi and @nmadhok is _schedule.conf not basically the default mine scheduler of the minion? why is this even in a separate file? shouldn't this be default in /etc/salt/minion....

[basepi]

Well, having it in a different file means salt can manage the whole file. I just think this _schedules.conf should be in the cache or somewhere else. I think minion.d/ should be reserved exclusively for user configs. I'll try to get some time with @thatch45 to discuss.

[aboe76]

@basepi I think that is the most sensible approach, 👍

[nmadhok]

@iggy If you clone this formula on a fresh VM and install salt-minion using it, does it place _schedule.conf under /etc/salt/minion.d/ ? If not then the default behavior of clean: True is correct and it would be nice to backup files already present under that folder (in case someone already had salt installed and had configuration files lying there). Also, we can add a way for the user to specify in the pillar if they don't want to delete old config files. But i still think that the defaults are correct and should be to remove all old config files and put new ones.

From where did you get this config file? What generated it? Was it generated by salt-formula?

[iggy]

I'm not beating this poor dead horse any more until basepi gets some input from thatch about why the minion is writing files to the config directory and whether that is going to change (but at this point, it's in a release, so we're stuck with it).

[nmadhok]

@iggy Sure. Not a problem.

[basepi]

Had a conversation this morning about _schedule.conf. The short and easy fix is to make the clean: True configurable via pillar. The longer, better fix is to add functionality to file.recurse to allow it to ignore certain filename patterns. In this case, it would ignore files with leading underscores, as we can assume those files were put down by salt.

[nmadhok]

@basepi What should be the default? I still think it should be clean: True by default.

[iggy]

If it's clean: True by default it erases config files that Salt writes out. I don't think it's a good idea for this formula to be destroying files that Salt expects to exist. I can just hear the IRC channel now... "My scheduled jobs quit working constantly... HALP MEH!!!!!!!!!1!!1!!"

[nmadhok]

@iggy I'm a little confused right now. When you install salt using the salt formula, does it create or place the config file that you're talking about?

[iggy]

The way I understand it is, that file is the way that salt.states.schedule persists scheduled jobs across service restarts. So it's created by Salt itself (not the formula).

https://github.com/saltstack/salt/blob/26acc5c1d476284174ba988439a2cc08ce66dc61/salt/utils/schedule.py#L302

[basepi]

Yep, it's completely independent of the formula, and ideally the formula shouldn't mess with it. The ideal solution is to have a whitelist for clean so it doesn't clean certain files but I don't think that functionality is available yet.

[tjstansell]

Why not just use exclude_pat: _* in the file.recurse definition?

[basepi]

Actually, that's the correct fix. I was stuck on the first half of the exclude_pat docs:

> exclude_pat
> Exclude this pattern from the source when copying. If both include_pat and exclude_pat are supplied, then it will apply conditions cumulatively. i.e. first select based on include_pat, and then within that result apply exclude_pat.

While it actually works on the clean as well:

> Also, when 'clean=True', exclude this pattern from the removal list and preserve in the destination.

[nmadhok]

@basepi Should I revert 8ebb7f5 ?

[iggy]

It would probably be easier to just change the default. I've had a couple commits on top of this that might make it not revert cleanly.

[basepi]

Ya, this is not a hard fix to make. =)

[iggy]

And you'll have to rename _defaults.conf as it'll be excluded from the original copy (with exclude_pat: _*)

[basepi]

Good point.

[nmadhok]

@iggy Can you submit a PR with the appropriate changes? Change the defaults as well. Thanks for the awesome work!