Improve operation workflow concurrency model

[didier-wenzek]

Proposed changes

Change the way MQTT is used to trigger state updates along an operation workflow

• MQTT is no more used inside the agent to move forward a command request
• The new source of truth is a CommandBoard used by the operation supervisor
• MQTT is used to init new command request
• MQTT is used to notify the progress of a command request
• The CommandBoard is persisted and restored when the agent restart
• The await-agent-restart can be used to validate a state transition on agent restart

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
• Documentation Update (if none of the other choices apply)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

• wrong state executed after workflow restart during a partition swap #2495

Checklist

• I have read the CONTRIBUTING doc
• I have signed the CLA (in all commits with git commit -s)
• I ran cargo fmt as mentioned in CODING_GUIDELINES
• I used cargo clippy as mentioned in CODING_GUIDELINES
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)

Further comments

[codecov]

Codecov Report

Attention: 169 lines in your changes are missing coverage. Please review.

Comparison is base (141c1d0) 76.0% compared to head (bd4ff60) 75.9%.

Additional details and impacted files

Files	Coverage Δ
...dge_agent/src/tedge_operation_converter/builder.rs	90.2% <100.0%> (+0.1%)	⬆️
crates/core/tedge_api/src/workflow/error.rs	0.0% <ø> (ø)
...tedge_agent/src/tedge_operation_converter/tests.rs	91.8% <92.8%> (+0.2%)	⬆️
crates/core/tedge_agent/src/agent.rs	0.0% <0.0%> (ø)
crates/core/tedge_api/src/workflow/state.rs	80.4% <57.1%> (+2.7%)	⬆️
crates/core/tedge_api/src/messages.rs	83.7% <81.2%> (-0.2%)	⬇️
crates/core/tedge_api/src/workflow/mod.rs	58.6% <50.0%> (-2.5%)	⬇️
...edge_agent/src/tedge_operation_converter/config.rs	0.0% <0.0%> (ø)
crates/core/tedge_api/src/workflow/toml_config.rs	68.6% <0.0%> (-1.9%)	⬇️
crates/core/tedge_api/src/workflow/on_disk.rs	37.5% <37.5%> (ø)
... and 2 more

... and 6 files with indirect coverage changes

[github-actions]

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
378	0	3	378	100	59m18.85s

[albinsuresh]

LGTM

Not approving yet as I'm aware of your plans to move the awaiting logic into an action.

[albinsuresh]

Not for now, but we can eventually consider moving that timestamp into the GenericCommandState payload itself as external processes acting on those state transitions might also benefit from it.

[albinsuresh]

Is this safe? Shouldn't we be returning the on_failed target of these commands, as we can't expect to resume commands just like that from their current states, if they were interrupted in the middle of their execution last time, right? I mean, for it to work, we should be sure that the script actions provided by the users are also resumable even after a first incomplete attempt, right?

[albinsuresh]

After making that comment, I realised that it has been like this since we started supporting state transitions using MQTT retained messages, which would have behaved exactly the same way, when the same retained messages are re-delivered to the agent post-restart. I'm just not sure if our users would be aware of the impact: "their scripts should essentially be resumable"

[didier-wenzek]

Very good point. I opt for this implementation precisely for the reason you give: "this been like this since we started supporting state transitions using MQTT retained messages".

And yes the scripts should be resumable for that to work. We can add an option to "fail on retry".

[albinsuresh]

Just leaving some doc rewording suggestions. Happy to approve once the test passes.

[didier-wenzek]

Happy to approve once the test passes.

There is indeed a race condition making the native reboot test flaky when the system successfully reboot. The issue is that currently the agent ignores SIGTERM signals when executing an operation action. In this specific case the timeout awaiting the agent to restart is delaying the restart. As a temporary workaround I increased the timeout used by the test and have the agent stopped by a SIGKILL. The proper fix is to have the agent stop on SIGTERM. I will address that.

[albinsuresh]

LGTM