feat: restart tedge-agent on tedge connect

[reubenmiller]

Proposed changes

Restart the tedge-agent service (instead of just starting it) when calling tedge connect <cloud> to ensure it matches any settings that may have changed (e.g. mqtt.client.port) since it was last started.

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

#3345

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: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...on/tedge_config/src/system_services/manager_ext.rs	0.00%	6 Missing ⚠️
crates/core/tedge/src/cli/connect/command.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

📢 Thoughts on this report? Let us know!

[github-actions]

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
556	1	2	557	99.82	1h34m57.152259s

Failed Tests

Name	Message	⏱️ Duration	Suite
tedge reconnect does not restart agent	MainPID=1092 != MainPID=1290	4.290 s	Tedge Connect Test

[reubenmiller]

Looks like we have a system test which covers the opposite case (introduced in 415ffaa, PR #2505). Investigating the PR where it was introduced and compare the two conflicting requirements.

Update

The PR introduced the following behaviour:

1. Don't stop the tedge-agent service when running tedge disconnect c8y (there is no change in behaviour here)
2. Only start the tedge-agent service when executing tedge connect or tedge reconnect (the new PR does the opposite to this now).

In my view, point 2 is partially incorrect. Whilst the cloud specific logic has moved out of the tedge-agent and into the mappers, the tedge-agent's configuration is still coupled to the tedge.toml configuration and there is no indication to the user when the tedge-agent needs to be restarted or even how to restart the tedge-agent.

Ideally it would be great if the tedge-agent could determine if it needs to be restarted or not based on the configuration changes, (or dynamically reload the configuration if changes are detected)...this would eliminate the need for this PR. @didier-wenzek @jarhodes314 Do you think dynamically reloading the configuration used by the tedge-agent would be feasible here?

[didier-wenzek]

Do you think dynamically reloading the configuration used by the tedge-agent would be feasible here?

Why not but this will be tedious. The issue is that the impact of a config change is deeply hidden in numerous independent components.

[albinsuresh]

From what I understood, the primary intent of the PR #2505 was just to prevent the agent from being stopped on a disconnect (which is even more relevant now since profiled connections are possible). Since this PR doesn't change that aspect, it is fine from that POV.

My only concern about restart on connect is the possible disruption to any operations that were in-progress (esp the built-in ones like software update). For example, if two profiled c8y connections: cloud and edge were running and a config download operation triggered from cloud was in-progress, tedge connect c8y --profile edge would unnecessarily disrupt the operation of the cloud. If tedge-agent supported seamless resumption of operations, this isn't a big deal. But that's not the case AFAIK (any incomplete operations are marked failed on restart), at least for the built-in operations.

[reubenmiller]

From what I understood, the primary intent of the PR #2505 was just to prevent the agent from being stopped on a disconnect (which is even more relevant now since profiled connections are possible). Since this PR doesn't change that aspect, it is fine from that POV.

My only concern about restart on connect is the possible disruption to any operations that were in-progress (esp the built-in ones like software update). For example, if two profiled c8y connections: cloud and edge were running and a config download operation triggered from cloud was in-progress, tedge connect c8y --profile edge would unnecessarily disrupt the operation of the cloud. If tedge-agent supported seamless resumption of operations, this isn't a big deal. But that's not the case AFAIK (any incomplete operations are marked failed on restart), at least for the built-in operations.

I don't think it will be an issue, as the tedge connect c8y and tedge reconnect c8y are generally only run when the user is fixing something, so the likelihood that the tedge-agent is already doing something like a software-update is unlikely (though I guess if you're connecting thin-edge.io to a second cloud instance, then the likelihood would increase). And even it it is, then the persistence of operations shouldn't be an issue for it.