julienlavergne commented 2 years ago

Describe the bug

When a symbolic link exists, points to a directory and contains a trailing separator, using files.link to create a link to the same folder without the trailing separator will not replace the existing link.

According to the doc: If the link exists and points to a different target, pyinfra will remove it and recreate a new one pointing to then new target.

To Reproduce

$ pyinfra inventories/local.py tasks/link.py -vvv --debug
--> Loading config...
--> Loading inventory...
    [pyinfra_cli.inventory] Creating fake inventory...
    [pyinfra_cli.inventory] Checking possible group_data directory: /pyinfra
    [pyinfra_cli.inventory] Looking for group data in: /pyinfra/group_data/**.py
    [pyinfra_cli.inventory] Looking for group data in: /pyinfra/group_data/**.py
    [pyinfra_cli.inventory] Looking for group data in: /pyinfra/group_data/**.py
    [pyinfra_cli.inventory] Looking for group data in: /pyinfra/group_data/all.py
    [pyinfra_cli.inventory] Checking possible group_data directory: /pyinfra/inventories

--> Connecting to hosts...
    [pyinfra.connectors.ssh] Connecting to: host05 ({'allow_agent': True, 'look_for_keys': True, 'hostname': 'host05', '_pyinfra_ssh_forward_agent': None, '_pyinfra_ssh_config_file': None, '_pyinfra_ssh_known_hosts_file': None, '_pyinfra_ssh_strict_host_key_checking': None, 'username': 'myuser', 'timeout': 10})
    [pyinfra.connectors.sshuserclient.client] Loading SSH config: None
    No host key for host05 found in known_hosts
    [host05] Connected
    [pyinfra.api.state] Activating host: host05

--> Preparing operations...
    Loading: tasks/link.py
    [pyinfra.api.operation] Adding operation, {'Link with separator'}, opOrder=(0, 5), opHash=dfa9e3b5bdf0060fa4b8730330366e286292da34
    [pyinfra.api.facts] Getting fact: files.Link (path=/home/myuser/link) (ensure_hosts: None)
    [pyinfra.connectors.ssh] Running command on host05: (pty=None) sh -c '! (test -e /home/myuser/link || test -L /home/myuser/link ) || ( stat -c '"'"'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N'"'"' /home/myuser/link 2> /dev/null || stat -f '"'"'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY'"'"' /home/myuser/link )'
[host05] >>> sh -c '! (test -e /home/myuser/link || test -L /home/myuser/link ) || ( stat -c '"'"'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N'"'"' /home/myuser/link 2> /dev/null || stat -f '"'"'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY'"'"' /home/myuser/link )'
    [pyinfra.connectors.ssh] Waiting for exit status...
    [pyinfra.connectors.ssh] Command exit status: 0
    [host05] Loaded fact files.Link (path=/home/myuser/link)
    [pyinfra.api.facts] Getting fact: files.Directory (path=/home/myuser) (ensure_hosts: None)
    [pyinfra.connectors.ssh] Running command on host05: (pty=None) sh -c '! (test -e /home/myuser || test -L /home/myuser ) || ( stat -c '"'"'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N'"'"' /home/myuser 2> /dev/null || stat -f '"'"'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY'"'"' /home/myuser )'
[host05] >>> sh -c '! (test -e /home/myuser || test -L /home/myuser ) || ( stat -c '"'"'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N'"'"' /home/myuser 2> /dev/null || stat -f '"'"'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY'"'"' /home/myuser )'
[host05] user=root group=root mode=lrwxrwxrwx atime=1651542672 mtime=1541732484 ctime=1541732484 size=23 '/home/myuser' -> '/RTGDEVHKMA05/home/myuser'
    [pyinfra.connectors.ssh] Waiting for exit status...
    [pyinfra.connectors.ssh] Command exit status: 0
    [host05] Loaded fact files.Directory (path=/home/myuser)
    [pyinfra.api.facts] Getting fact: files.Link (path=/home/myuser) (ensure_hosts: None)
    [pyinfra.connectors.ssh] Running command on host05: (pty=None) sh -c '! (test -e /home/myuser || test -L /home/myuser ) || ( stat -c '"'"'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N'"'"' /home/myuser 2> /dev/null || stat -f '"'"'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY'"'"' /home/myuser )'
[host05] >>> sh -c '! (test -e /home/myuser || test -L /home/myuser ) || ( stat -c '"'"'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N'"'"' /home/myuser 2> /dev/null || stat -f '"'"'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY'"'"' /home/myuser )'
[host05] user=root group=root mode=lrwxrwxrwx atime=1651542672 mtime=1541732484 ctime=1541732484 size=23 '/home/myuser' -> '/RTGDEVHKMA05/home/myuser'
    [pyinfra.connectors.ssh] Waiting for exit status...
    [pyinfra.connectors.ssh] Command exit status: 0
    [host05] Loaded fact files.Link (path=/home/myuser)
    [host05] noop: directory /home/myuser already exists (as a link)
    [pyinfra.api.operation] Adding operation, {'Link without separator'}, opOrder=(0, 6), opHash=b774d0fdabe26575c0b79a9b49f216bd9af1d0e4
    [host05] noop: link /home/myuser/link already exists
    [host05] Ready: tasks/link.py

--> Proposed changes:
    Groups: local / **
    [host05]   Operations: 2   Commands: 1

--> Beginning operation run...
--> Starting operation: Link with separator
    [pyinfra.api.operations] Starting operation Link with separator on host05
    [pyinfra.connectors.ssh] Running command on host05: (pty=None) sh -c 'ln -s target/ /home/myuser/link'
[host05] >>> sh -c 'ln -s target/ /home/myuser/link'
    [pyinfra.connectors.ssh] Waiting for exit status...
    [pyinfra.connectors.ssh] Command exit status: 0
    [host05] Success

--> Starting operation: Link without separator
    [pyinfra.api.operations] Starting operation Link without separator on host05
    [host05] No changes

--> Results:
    Groups: local / **
    [host05]   Successful: 2   Errors: 0   Commands: 1/1

tasks/link.py

import os
from pyinfra import host
from pyinfra.operations import files

files.link(name="Link with separator", path=os.path.join(host.data.home_dir_path, host.data.ssh_user, "link"), target="target/")
files.link(name="Link without separator", path=os.path.join(host.data.home_dir_path, host.data.ssh_user, "link"), target="target")

inventories/local.py

dev_phy = [
    ("host05", {"tools_dir_path": "/tools", "ssh_user": "myuser"})
]

group_data/all.py

home_dir_path = "/home"

Result

link -> target/

Expected behavior

I would expect the link to be replaced if the old link is not strictly the same. It is not possible for pyinfra to know if my link is supposed to refer to a directory or file.

Final link should be /home/link -> target

Meta

Include output of pyinfra --support.

--> Support information:

If you are having issues with pyinfra or wish to make feature requests, please
check out the GitHub issues at https://github.com/Fizzadar/pyinfra/issues .
When adding an issue, be sure to include the following:

System: Linux
  Platform: Linux-3.10.0-1160.59.1.el7.x86_64-x86_64-with-glibc2.17
  Release: 3.10.0-1160.59.1.el7.x86_64
  Machine: x86_64
pyinfra: v2.0.1
Executable: /home/**/.local/bin/pyinfra
Python: 3.9.4 (CPython, GCC 4.8.5 20150623 (Red Hat 4.8.5-44))

How was pyinfra installed (source/pip)? pip

Fizzadar commented 2 years ago

Hi @julienlavergne on what kind of target machine are you seeing this? I cannot replicate on MacOS local nor in an Ubuntu 20 container:

$ pyinfra @docker/ubuntu:20.04 test.py
...
--> Proposed changes:
    Groups: @docker
    [@docker/ubuntu:20.04]   Operations: 2   Commands: 3

--> Beginning operation run...
--> Starting operation: Link with separator
    [@docker/ubuntu:20.04] Success

--> Starting operation: Link without separator
    [@docker/ubuntu:20.04] Success
...
    [@docker/ubuntu:20.04] docker build complete, image ID: 0430ecd73769

$ docker run  0430ecd73769  ls -lh
total 48K
...
lrwxrwxrwx   1 root root    6 Apr 16 09:37 testo-link -> target

julienlavergne commented 2 years ago

I provided python uname output in the description. This is Red Hat 7. I have same result on CentOS 7, CentOS 6 and Red Hat 6.

sysadmin75 commented 2 years ago

I might be missing the true nature of the bug, but to me, this does not feel like a bug. This behavior is the same for ln -s.

In this example, I'm demonstrating how creating a symlink using a slash at the end of the target behaves.

[kris@NX-01 link-test]$ pwd
/tmp/link-test
[kris@NX-01 link-test]$ ll
total 0
[kris@NX-01 link-test]$ mkdir target
[kris@NX-01 link-test]$ ll
total 0
drwxrwxr-x 2 kris kris 40 Apr 23 10:49 target
[kris@NX-01 link-test]$ ln -s target/ link
[kris@NX-01 link-test]$ ll
total 0
lrwxrwxrwx 1 kris kris  7 Apr 23 10:49 link -> target/
drwxrwxr-x 2 kris kris 40 Apr 23 10:49 target
[kris@NX-01 link-test]$ ln -sf target link
[kris@NX-01 link-test]$ ll
total 0
lrwxrwxrwx 1 kris kris  7 Apr 23 10:49 link -> target/
drwxrwxr-x 2 kris kris 60 Apr 23 10:49 target

In this example, I'm demonstrating the same as above, but I'm not using a slash in the target. This shows that the order of operations matters.

[kris@NX-01 link-test]$ ll
total 0
drwxrwxr-x 2 kris kris 60 Apr 23 10:54 target
[kris@NX-01 link-test]$ 
[kris@NX-01 link-test]$ ln -s target link
[kris@NX-01 link-test]$ ll
total 0
lrwxrwxrwx 1 kris kris  6 Apr 23 10:55 link -> target
drwxrwxr-x 2 kris kris 60 Apr 23 10:54 target
[kris@NX-01 link-test]$ ln -sf target/ link
[kris@NX-01 link-test]$ ll
total 0
lrwxrwxrwx 1 kris kris  6 Apr 23 10:55 link -> target
drwxrwxr-x 2 kris kris 60 Apr 23 10:55 target

julienlavergne commented 2 years ago

I might be missing the true nature of the bug, but to me, this does not feel like a bug. This behavior is the same for ln -s.

In this example, I'm demonstrating how creating a symlink using a slash at the end of the target behaves.

[kris@NX-01 link-test]$ pwd
/tmp/link-test
[kris@NX-01 link-test]$ ll
total 0
[kris@NX-01 link-test]$ mkdir target
[kris@NX-01 link-test]$ ll
total 0
drwxrwxr-x 2 kris kris 40 Apr 23 10:49 target
[kris@NX-01 link-test]$ ln -s target/ link
[kris@NX-01 link-test]$ ll
total 0
lrwxrwxrwx 1 kris kris  7 Apr 23 10:49 link -> target/
drwxrwxr-x 2 kris kris 40 Apr 23 10:49 target
[kris@NX-01 link-test]$ ln -sf target link
[kris@NX-01 link-test]$ ll
total 0
lrwxrwxrwx 1 kris kris  7 Apr 23 10:49 link -> target/
drwxrwxr-x 2 kris kris 60 Apr 23 10:49 target

In this example, I'm demonstrating the same as above, but I'm not using a slash in the target. This shows that the order of operations matters.

[kris@NX-01 link-test]$ ll
total 0
drwxrwxr-x 2 kris kris 60 Apr 23 10:54 target
[kris@NX-01 link-test]$ 
[kris@NX-01 link-test]$ ln -s target link
[kris@NX-01 link-test]$ ll
total 0
lrwxrwxrwx 1 kris kris  6 Apr 23 10:55 link -> target
drwxrwxr-x 2 kris kris 60 Apr 23 10:54 target
[kris@NX-01 link-test]$ ln -sf target/ link
[kris@NX-01 link-test]$ ll
total 0
lrwxrwxrwx 1 kris kris  6 Apr 23 10:55 link -> target
drwxrwxr-x 2 kris kris 60 Apr 23 10:55 target

You created another symlink inside your target folder, that is why you cannot see the new link. Use ln -nfs to create your links.

Fizzadar commented 2 years ago

Is it possible this is caused by #801?

@julienlavergne would it be possible to get the full run with -vvv --debug flags added? I cannot replicate this which makes it hard to debug!

julienlavergne commented 2 years ago

Yes, here is the debug log. I changed some names for privacy but I did not remove any line.

pyinfra-dev / pyinfra

files.link did not remove old link pointing to "different" target #791

Describe the bug

To Reproduce

tasks/link.py

inventories/local.py

group_data/all.py

Result

Expected behavior

Meta