saltstack / salt

Software to automate the management and configuration of any infrastructure or application at scale. Get access to the Salt software package repository here:
https://repo.saltproject.io/
Apache License 2.0
14.15k stars 5.48k forks source link

Recursive requisite found, using - name is treated as equivalent to ID #5667

Closed boltronics closed 5 years ago

boltronics commented 11 years ago

My requirements are simple: Create a file if it does not already exist, and append a line (if the line does not already exist).

I used the following:

/testfile:
  file:
    - touch
    - unless:
      - test -f /testfile

Append /testfile:
  file.append:
    - name: /testfile
    - text:
      - 'some text'
    - require:
      - file: /testfile

This gives me a "Recursive requisite found" error. Apparently, the " -name: /testfile" line causes the require to fail. Essentially, - name: appears to be treated as an ID. Maybe that's the way Salt is supposed to work, but then take this example:

Touch /testfile:
  file.touch:
    - name: /testfile
    - unless:
      - test -f /testfile

Append /testfile:
  file.append:
    - name: /testfile
    - text:
      - 'some text'
    - require:
      - file: Touch /testfile

If - name: is the same as the ID, the above should also fail, yet it works fine. I don't see the logic of this behaviour, nor to my colleagues. If this behaviour can be explained, it should ideally be documented somewhere. Otherwise, I'll consider it a bug.

Cheers, Adam

basepi commented 11 years ago

Yes, name is treated as an ID. In fact, either the top level definition or the name can be used to refer to that state in a require. So your second version is what you have to do in this instance, as both file.touch and file.append are going to share one of their names.

It's not a bug, it's just a side effect of allowing either the top level name or the defined name to be used as an ID.

boltronics commented 11 years ago

Ahh.. got it. "name:" isn't actually a real ID (so the same "name:" entry in each state doesn't conflict with each other) but "require:" allows it to be used like an ID for convenient reference. I believe all has become clear now. Thanks!

And since this bug report can serve as notice to anyone else who comes across this issue, I guess that is sufficient to close this bug. ;)

Cheers!

alwinmark commented 10 years ago

I still think its a bug since its an unexpected side effect. I don't see the benefit using the name attribute as an internally identifier but I ran into the same issue like boltronics.

I would really appreciate if you could fix this side effect

alwinmark commented 10 years ago

sth like:

/testfile:
  file:
    - touch
    - unless:
      - test -f /testfile

Append /testfile:
  file.append:
    - name: /testfile
    - text:
      - 'some text'
    - require:
      - file.touch: /testfile

would be ok, too. Even it is still ugly, because you have to find this threat or try around a bit and you still are not able to append after another append state.

basepi commented 10 years ago

You're right, it's definitely less than ideal. I'm going to reopen the issue and mark it as a feature. Supporting that syntax will take some work, as it will require some substantial additions to the state compiler. But I agree, the side effect is less than ideal.

alwinmark commented 10 years ago

Thank you, you guys are great!

bemeyert commented 10 years ago

Hi all,

Are there any news on this one? This "bug" cost me nearly a day of debugging.

basepi commented 10 years ago

@bemeyert We haven't had a chance to look into this yet.

Can you clarify exactly what part of the problem you ran into and how exactly you wish it had functioned? Are you just looking for the same extra syntax @CansaSCityShuffle is looking for?

bemeyert commented 10 years ago

Hi @basepi ,

I wrote about this issue to the ML with the subject "how to manage files inside a managed directory". I just cut & paste the code here:

Before and not working:

File: jallah/init.sls
/opt/blah/jallah:
    file.directory:
        - file_mode: 0755
        - dir_mode:  0750
        - recurse:
            - mode
        - require:
            - file: /opt/blah
File: jallah/script.sls
/opt/blah/jallah/script.sh:
    file.managed:
        - source: salt://jallah/files/script.sh
        - mode:   0750
        - require:
            - sls: jallah
        - require_in:
            - service: jallah-service

After I found this bug and working fine:

(changes are marked with '=>'):
File: jallah/init.sls
=> set_permissions:
    file.directory:
=>      - name: /opt/blah/jallah
        - file_mode: 0755
        - dir_mode:  0750
        - recurse:
            - mode
        - require:
=>          - file: dir_blah
File: jallah/script.sls
=> install_script
    file.managed:
=>      - name: /opt/blah/jallah/script.sh
        - source: salt://jallah/files/script.sh
        - mode:   0750
        - require:
=>          - file: dir_blah
        - require_in:
            - service: jallah-service

So I had to play a bit with the IDs and names. But that was anything but obvious. Without this bug I never would have found the solution. Cheers

basepi commented 10 years ago

Thanks for the update. Your sls snippets seem incomplete, and you're requiring an ID that doesn't exist in your snippets. Can you add the relevant pieces so we can get a more exact idea of what was going wrong?

bemeyert commented 10 years ago

I hope that the information is now complete.

Before and not working:

File: jallah/init.sls

/opt/blah:
   file.recurse:
        - source:   salt://jallah/files/opt/blah
        - makedirs: True
/opt/blah/jallah:
    file.directory:
        - file_mode: 0755
        - dir_mode:  0750
        - recurse:
            - mode
        - require:
            - file: /opt/blah
jallah-service:
[...]

File: jallah/script.sls

include:
    - jallah
/opt/blah/jallah/script.sh:
    file.managed:
        - source: salt://jallah/files/script.sh
        - mode:   0750
        - require:
            - sls: jallah
        - require_in:
            - service: jallah-service

After I found this bug and working fine:

(changes are marked with '=>'):

File: jallah/init.sls

=> copy_jallah:
    file.recurse:
=>      - name:     /opt/jallah
        - source:   salt://jallah/files/opt/jallah
        - makedirs: True
=> set_permissions:
    file.directory:
=>      - name: /opt/blah/jallah
        - file_mode: 0755
        - dir_mode:  0750
        - recurse:
            - mode
        - require:
=>          - file: dir_blah
jallah-service:
[...]

File: jallah/script.sls

include:
    - jallah
=> install_script
    file.managed:
=>      - name: /opt/blah/jallah/script.sh
        - source: salt://jallah/files/script.sh
        - mode:   0750
        - require:
=>          - file: dir_blah
        - require_in:
            - service: jallah-service
basepi commented 10 years ago

I'm still not seeing the target dir_blah state in the second example, so I'm not sure how that relates. The first example, at least what you've shown me of the first example, looks like it should work, there aren't any naming conflicts that I can see. However, I'd still like to see the dir_blah state, as well as the whole jallah-service state, just for completeness, so I can get a complete picture of the requires in your setup. Nothing that you've shown me so far looks like it should have a problem, but I'm guessing there's something outside of the states you've shown that is causing the conflict.

bemeyert commented 10 years ago

That was a typer. "dir_blah" is supposed to be "copy_jallah". Hopefully this works now:

File: jallah/init.sls

=> copy_jallah:
    file.recurse:
=>      - name:     /opt/jallah
        - source:   salt://jallah/files/opt/jallah
        - makedirs: True
=> set_permissions:
    file.directory:
=>      - name: /opt/blah/jallah
        - file_mode: 0755
        - dir_mode:  0750
        - recurse:
            - mode
        - require:
=>          - file: copy_jallah
jallah-service:
    service:
        - running
        - name:    jallah
        - enable:  True
        - reload:  True
        - require:
            - pkg: required-pkgs 

File: jallah/script.sls

include:
    - jallah
=> install_script
    file.managed:
=>      - name: /opt/blah/jallah/script.sh
        - source: salt://jallah/files/script.sh
        - mode:   0750
        - require:
=>          - file: copy_jallah
        - require_in:
            - service: jallah-service
basepi commented 10 years ago

Thanks for the update, looks good now.

The latest example you posted works, right? So back to your original, non-working example. What error do you get? It seems like there are no conflicting names/state IDs in that example. If you look at the origin example by @boltronics, you can see that because one state has - name: /testfile and another state has a state ID of /testfile, there's a conflict when you try to require file: /testfile. In your example, @bemeyert, there are no conflicts so I'm trying to figure out where the problem would come from.

bemeyert commented 10 years ago

Y At last I got it right ;-)

I no longer have the complete output, but the error "recursive requisit" popped up while Salt worked on "/opt/blah/jallah/script.sh" in the state "jallah.script". I can "reconstruct" the error, but it would take some time and effort.

Thx for your patience

basepi commented 10 years ago

Well, glad you got it working, and it's probably the same issue as the original issue in this thread anyway, so we'll leave it for now, no need to reconstruct your error if it's working.

garrickp commented 9 years ago

Got bitten by this bug today.

For what it's worth, the explanation that requisites target either the name or the ID contradicts the documentation, which indicates that only an ID is used for targeting requisites.

timcharper commented 9 years ago

I just got bit by this :(

xterm-font:
  file.append:
    - name: /home/{{grains.user}}/.Xresources
    - text: |
        xterm*font:     *-fixed-*-*-*-19-*
    - require:
      - file: /home/{{grains.user}}/.Xresources
    - watch_in:
      - cmd: xrdb
xorg:
  pkg.installed

/home/{{grains.user}}/.Xresources:
  file.managed:
    - user: {{grains.user}}
    - mode: 600
xrdb:
  cmd.watch:
    - user: {{grains.user}}
    - name: xrdb -merge {{grains.user}}/.Xresources
    - require:
      - pkg: xorg
      - file: /home/{{grains.user}}/.Xresources

Output:

----------
          ID: xorg
    Function: pkg.installed
      Result: True
     Comment: Package xorg is already installed.
     Started: 14:59:03.335396
    Duration: 290.398 ms
     Changes:
----------
          ID: /home/scimmia/.Xresources
    Function: file.managed
      Result: True
     Comment: File /home/scimmia/.Xresources exists with proper permissions. No changes made.
     Started: 14:59:03.627602
    Duration: 2.071 ms
     Changes:
----------
          ID: xterm-font
    Function: file.append
        Name: /home/scimmia/.Xresources
      Result: False
     Comment: Recursive requisite found
     Started:
    Duration:
     Changes:
----------
          ID: xrdb
    Function: cmd.watch
        Name: xrdb -merge scimmia/.Xresources
      Result: False
     Comment: One or more requisite failed: custom.xterm-font
     Started:
    Duration:
     Changes:

Looks like my xterm-font requirement is being evaluated to refer to itself :( This behavior makes me sad.

timcharper commented 9 years ago

Workaround:

user-xresources:
  file.managed:
    - name: /home/{{grains.user}}/.Xresources
    - user: {{grains.user}}
    - mode: 600

xterm-font:
  file.append:
    - name: /home/{{grains.user}}/.Xresources
    - text: |
        xterm*font:     *-fixed-*-*-*-19-*
    - require:
      - file: user-xresources
    - watch_in:
      - cmd: xrdb
ScoreUnder commented 6 years ago

I've run into this yesterday/today. Took a bit of tribulation to figure out why a seemingly straight line of dependencies was treated as a loop:

mysqld_running:
  test.nop:
    - name: mysqld

mysql_root_user:
  test.nop:
    - require:
      - mysqld_running

my.cnf:
  test.nop:
    - require:
      - mysql_root_user

mysqld_restart:
  test.nop:
    - name: mysqld
    - watch:
      - my.cnf

In my case, I need to start mysql to set the root password, then I need to drop my.cnf in after setting the root password (otherwise the mysql client ceases to work correctly). However, the my.cnf file contains server settings too, so I then need to restart mysql after changing it.

The above test case has been simplified in the hopes that it makes it easier to see that there is no circular dependency, and easier to test whether a fix works or not without having to play with your local filesystem or mysql instance.

My current workaround is to have the restart only, and the first time you run the state you just let most of it fail and run it a second time.

While debugging this issue I hacked this script together: https://gist.github.com/ScoreUnder/6735d45ed85e2e5fb0bf5c9adc18478f

boltronics commented 6 years ago

@ScoreUnder That example doesn't actually error for me. :wink:

Your require statements are missing the module. eg. should be test: mysql_root_user.

For your example to be similar to my original issue, I would have expected to see a require on test: mysqld somewhere or something. You might have a different issue, or your example may not accurately represent the problem you have.

stale[bot] commented 5 years ago

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.

t0xicCode commented 5 years ago

Just got bit by it myself.

I ended up rewriting the entire state file to change the actions that would be taken: I wanted to ensure that a service was both dead and masked. Instead, I had to change the actions so that the entire package is removed instead (which means I lose the utilities).