propose dev reload (similar to Quarkus)

[tarilabs]

Following up on similar idea from #513,
and the "Report on: Kubernetes + Event Driven Ansible using Rulebooks" document shared with the team,
this PR proposes a "dev reload" similar to Quarkus dev mode,
that will monitor for rulebook changes and auto-reload when supplied on cli with --dev.

In the following, I provide a demo of its use.

"alternative 0" with the command line parameter NOT supplied, behaves as before:

Now I launch the same, but with --dev command line parameter:

it does not auto-terminate, and waits for file changes.
Now I want to modify from using the generic source to the webhook source:

I wanted to send analogous events as those from the generic source, but through the webhook by using the postman application, but my rule is not triggering anymore 🤔 ...
[this was intentional for the purpose of the demo]

Ah yes! 😉 I should update the condition to reflect accordingly:

In the previous screenshot, I just:

1. changed the rulebook
2. saved the rulebook
3. the ansible-rulebook dev mode downscaled and restarted automatically

As I send the same event from postman:

It is now working as expected.

I hope this demo exemplify the intended workflow of the proposed dev mode for ansible-rulebook, inspired by Quarkus dev mode.

[Alex-Izquierdo]

It produces the following log entry:

2023-05-25 18:58:15,645 - asyncio - ERROR - _GatheringFuture exception was never retrieved
future: <_GatheringFuture finished exception=DevReloadException('Rulebook file changed, raising exception so to asyncio.FIRST_EXCEPTION in order to reload')>

To prevent it, you need the return_exceptions=True here: https://github.com/ansible/ansible-rulebook/blob/main/ansible_rulebook/engine.py#L285

This is a question for @mkanoor:
asyncio.gather returns an awaitable, is there a reason to not use await in https://github.com/ansible/ansible-rulebook/blob/main/ansible_rulebook/engine.py#L285 ?

[Alex-Izquierdo]

In addition to the linting issues, It might need to refactor some test due to recursion issues. Maybe a good approach could be encapsulate the logic of the run function to avoid recursive calls.

[tarilabs]

It produces the following log entry:

2023-05-25 18:58:15,645 - asyncio - ERROR - _GatheringFuture exception was never retrieved
future: <_GatheringFuture finished exception=DevReloadException('Rulebook file changed, raising exception so to asyncio.FIRST_EXCEPTION in order to reload')>

To prevent it, you need the return_exceptions=True here: https://github.com/ansible/ansible-rulebook/blob/main/ansible_rulebook/engine.py#L285

thank you, done in 81683a9

This is a question for @mkanoor: asyncio.gather returns an awaitable, is there a reason to not use await in https://github.com/ansible/ansible-rulebook/blob/main/ansible_rulebook/engine.py#L285 ?

Not sure as I haven't touched that line, I assume it was that way as anyway the result of gather is not needed?
eg., even if it was

unused = await asyncio.gather(*ruleset_tasks, return_exceptions=True)

the unused was not used anywhere, so not necessary to await? At least I intended it that way? 🙃

In addition to the linting issues, It might need to refactor some test due to recursion issues. Maybe a good approach could be encapsulate the logic of the run function to avoid recursive calls.

T̵h̵a̵n̵k̵ ̵y̵o̵u̵,̵ ̵a̵n̵d̵ ̵I̵ ̵a̵m̵ ̵f̵a̵c̵i̵n̵g̵ ̵t̵h̵i̵s̵ ̵p̵r̵o̵b̵l̵e̵m̵,̵ ̵b̵u̵t̵ ̵I̵'̵m̵ ̵n̵o̵t̵ ̵s̵u̵r̵e̵ ̵I̵ ̵f̵u̵l̵l̵y̵ ̵u̵n̵d̵e̵r̵s̵t̵o̵o̵d̵ ̵y̵o̵u̵r̵ ̵f̵e̵e̵d̵b̵a̵c̵k̵.̵
̵C̵o̵u̵l̵d̵ ̵y̵o̵u̵ ̵p̵r̵o̵v̵i̵d̵e̵ ̵a̵ ̵b̵i̵t̵ ̵m̵o̵r̵e̵ ̵c̵o̵n̵t̵e̵x̵t̵/̵e̵x̵a̵m̵p̵l̵e̵?̵ ̵A̵l̵s̵o̵ ̵a̵l̵w̵a̵y̵s̵ ̵a̵v̵a̵i̵l̵a̵b̵l̵e̵ ̵o̵n̵ ̵c̵a̵l̵l̵ ̵i̵f̵ ̵p̵r̵e̵f̵e̵r̵r̵e̵d̵

EDIT: I believe I solved ^ with 8d795da, lmk what you think about that?

Thank you so far 🙏

[tarilabs]

I would assume I just need to now fix the remaining linting issues... checking

[Alex-Izquierdo]

I have tested it a little with good results. LGTM

[benthomasson]

I really like hot-reload features for development. As long as people are developing against non-production systems this is probably OK. If people are developing against production systems this could run a set of rules that is not intended to be run together due to auto-saving on editors.

[benthomasson]

Add a big warning banner in the output and a critical level log that says that the rules will be reloaded. Also add a critical level log that says that rules were reloaded when a hot reload happens.

[benthomasson]

This feature is for development environment only and does not need production testing other than that it doesn't break production execution.

[tarilabs]

friendly bump / ping @mkanoor @Alex-Izquierdo can this be merged after Ben's approval #516 (review) or something need further amendment in your pov, please?

[Alex-Izquierdo]

I think that as a feature that we are going to support from now on, it should have an e2e test.

[tarilabs]

I think that as a feature that we are going to support from now on, it should have an e2e test.

Since this has been raised from May 25, and I will have less opportunity to work on this "full time", can I ask you for a little guidance how to best setup and e2e test that interacts with this "as non-worker mode" and make file change that triggers the reload, please?

In other words, I'm very happy to add an e2e test, but I would really need a "bootstrap" guidance.

[Alex-Izquierdo]

Hi @tarilabs I don't want to go on too long here. At a quick thought I would say that you can run the process in the background, make some simple modification in the rulebook and check that you have the expected output from the expected restart.

This test can serve as a reference:

ansible-rulebook/tests/e2e/test_runtime.py

Line 198 in 98345aa

def test_terminate_process_sigint():

In this case the process is simply a fork, you can also launch it with async. As you like.

Our strategy in many places is always the same: use the debug action in the rulebook to print some string that is verified in the test. If you inspect other existing tests you will see that we use the pytest_check plugin to do the trick to run multiple assertions in the same test without stop the test. This is to optimize the execution time and verify multiple cases in the same rulebook instead of creating a new process for each testcase. Here you would not need it.

Anycase, you have to update the command helper with the new flag:
https://github.com/ansible/ansible-rulebook/blob/main/tests/e2e/utils.py#L18

I don't have much time either to work on it but if it's too much trouble I can try to take some time to do the test and make a PR to your branch.

[tarilabs]

@Alex-Izquierdo added test as required and per your suggestions with 43ed370 . The test is passing locally, now 🤞 on CI :)

[Alex-Izquierdo]

Thank you very much for attending my request. I think the e2e test is fine, anyway I leave you a couple of suggestions if you want to improve it.