No change on execute opereations (operations.apk: change detection not working correctly)

Gaming4LifeDE commented 3 years ago

Describe the bug

When running an upgrade through apk, it doesn't correctly detect if anything was actually updated.
Example deploy:

apk.packages(
    name = "Refresh repo list",
    update = True
)

update = apk.packages(
    name = "Update everything",
    upgrade = True
)

if update.changed:
    server.reboot(delay=5, interval=1, reboot_timeout=300)

In this case, the server will always reboot. on the actual output it will also always say "success" instead of "no change". See the output below. I ran the deploy twice directly after each other. This is the output of the second run:

--> Starting operation: Refresh repo list 
    [192.168.2.13] Success

--> Starting operation: Update everything 
    [192.168.2.13] Success

--> Starting operation: Server/Reboot (delay=5, interval=1, reboot_timeout=300)
    [192.168.2.13] Connected
    [192.168.2.13] Success

To Reproduce

See above

Expected behavior

I expect the operation to yield "no change" instead of "success". It might be neccessary to analyze stdout to make this happen. You could just check if there's more than one line...

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-5.4.67-09356-gf3ed4c0c4a31-x86_64-with-debian-10.8
  Release: 5.4.67-09356-gf3ed4c0c4a31
  Machine: x86_64
pyinfra: v1.3.6
Executable: /usr/local/bin/pyinfra
Python: 3.7.3 (CPython, GCC 8.3.0)

How was pyinfra installed (source/pip)?
pip

Consider including output with -vv and --debug.

--> Starting operation: Update everything 
[pyinfra.api.operations] Starting operation Update everything on 192.168.2.13
[pyinfra.api.connectors.ssh] Running command on 192.168.2.13: (pty=None) sh -c 'apk upgrade'
[pyinfra.api.connectors.ssh] Waiting for exit status...
[pyinfra.api.connectors.ssh] Command exit status: 0
[192.168.2.13] Success

Fizzadar commented 3 years ago

This is expected behaviour - no change applies when no commands are run, in this case apk upgrade is being executed, which counts as a change - even if the command itself doesn't do anything.

There's a possibility of checking the output and pyinfra could potentially display Success (no change) or similar...

Gaming4LifeDE commented 3 years ago

That's what makes sense form a code perspective but if you read the execution log you'd expect "no change" to be no change and "successful" meaning it was successfully changed. I think this should be thought about again.

Offtopic: Is there a way to execute one operation with a different connector? I wonder if a terraform connector would be feasible. Then you could have a deploy which can first provision the host with terraform and then install stuff onto it via ssh.

Fizzadar commented 3 years ago

I see this both ways; I think ultimately there's three valid results from an operation:

executed & changed (in the apk update case)
executed & unchanged (in the apk upgradesecond run case)
no execute / no change

The problem with 2 is detecting it - change/no change is decided during the fact gathering phase, before any commands are actually executed. To detect at this point whether any packages will upgrade requires some kind of apk fact/command to identify this. Potentially apk upgrade -s could do this - but won't take into account any apk update calls beforehand.

So I agree I think it'd be great to highlight this different situation, however I can't currently think of a reasonable way to achieve it without changing how pyinfra executes. One option is the Success (no change) flag which could highlight this specifically during the execution phase only.

Re: different connector - not currently! There an issue for a @terraform connector implementation, however, which should make this possible. Currently I'd advise simply calling terraform within the inventory code, and parsing state to get the SSH details.

Gaming4LifeDE commented 3 years ago

How about this: before actually running the upgrade command you check if there's available updates and change the state accordingly. Would this work?

Fizzadar commented 3 years ago

Sort of - the problem is the fact gathering phase which determines if an operation us success or no change happens before any operations execute, ie any call to apt.update would break any check. For your example of apk.update followed by apk.upgrade, the sequence of events is roughly:

Fact gathering phase:

[Prepare apk.update] Push apk update command to be executed later
[Prepare apk.upgrade] Check for any pending upgrade packages (using a fact that executes apk upgrade -s immediately)
[Prepare apk.upgrade] Nothing shows up, push nothing and flag operation as No change

Execution phase: apk update is executed and the deploy exits

The problem here is that during execution any fact that identifies whether packages will be upgraded becomes invalid afterapk update is called. This is a limitation of the two phase execution model, which makes it quite hard to fix/improve :/

Gaming4LifeDE commented 3 years ago

So I think the only way to really make things like this work (or for example if you're provisioning with Terraform to be able to retrieve the ip) there needs to be a hook which forces pyinfra to check for certain facts only (which would keep the performance impact minimal)

pyinfra-dev / pyinfra