Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend tedge-agent with config and log management #2436

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

albinsuresh
Copy link
Contributor

@albinsuresh albinsuresh commented Nov 9, 2023

Proposed changes

Instead of creating a new tedge-device-management plugin with the additional features, the existing agent itself was extended with the newer feature, guarded behind a v1 flag. The extended agent is installed as /usr/bin/tedge-agent-v1 along with the existing tedge-agent and can be spawned from there, after stopping the running tedge-agent instance.

No system tests added for this specific variant as existing system tests for the dedicated operation plugins will be updated later by removing those plugins and using the updated agent instead.

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

#2433

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

Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Merging #2436 (f470f46) into main (35add19) will increase coverage by 0.1%.
The diff coverage is 0.0%.

Additional details and impacted files
Files Coverage Δ
crates/core/tedge/src/cli/mod.rs 73.6% <ø> (ø)
crates/core/tedge_agent/src/agent.rs 0.0% <ø> (ø)
crates/core/tedge_agent/src/main.rs 0.0% <0.0%> (ø)
crates/core/tedge_agent/src/lib.rs 0.0% <0.0%> (ø)
crates/core/tedge/src/main.rs 55.5% <0.0%> (-2.8%) ⬇️

... and 1 file with indirect coverage changes

Copy link
Contributor

github-actions bot commented Nov 9, 2023

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
359 0 3 359 100 59m6.683s

Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tedge-agent support for log and configuration is working as expected.

However:

  • As discussed offline, it would be better to use the symlink approach rather than a --v2 option. I.e. to have a tedge-agent-v2 symlink to tedge which would include all these plugins.
  • The tedge-log-plugin and tedge-configuration-plugin are somehow included twice as also wrapped by the tedge multi-command.
  • One needs to consider removing the use of sudo under the hood, as tedge-agent must then be launched as root. This will have to be done only when fully ready to deprecate the current packaging.

crates/core/tedge_agent/src/lib.rs Outdated Show resolved Hide resolved
@albinsuresh
Copy link
Contributor Author

  • As discussed offline, it would be better to use the symlink approach rather than a --v2 option. I.e. to have a tedge-agent-v2 symlink to tedge which would include all these plugins.

Yes.

  • The tedge-log-plugin and tedge-configuration-plugin are somehow included twice as also wrapped by the tedge multi-command.

Yes, those two plugins would be removed once the migration to the new agent is complete. We're keeping it just for the transition phase.

  • One needs to consider removing the use of sudo under the hood, as tedge-agent must then be launched as root. This will have to be done only when fully ready to deprecate the current packaging.

To be discussed.

Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Nice to see this is just a matter of moving around actor builders.

@albinsuresh
Copy link
Contributor Author

Approved. Nice to see this is just a matter to move around actor builders.

Yeah, it was a bit of a surprise to me too, even though it was clear that this shouldn't be that hard, based on the c8y-device-management plugin experience. But, I still didn't expect it to be so trivial. Glad to see that we did a pretty good job with the actor based architecture, making work like this so much easier.

Copy link
Contributor

@reubenmiller reubenmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@albinsuresh albinsuresh merged commit 7f6e084 into thin-edge:main Nov 15, 2023
18 checks passed
@albinsuresh albinsuresh deleted the feat/2433/tedge-agent-v2 branch November 23, 2023 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants