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

Configurable restart command #1577

Merged

Conversation

cmosd
Copy link
Contributor

@cmosd cmosd commented Nov 10, 2022

Proposed changes

This PR makes the restart command configurable.

This is the current status of the PR:

  • Add key to system.toml
  • Move reusable code into library code (from tedge to tedge_config)
  • Implement restart from system.toml in tedge_agent
  • Fix/add tests
  • Check that it works.

For some reason the restart feature does not work on my laptop. Running sudo -u tedge init 6 fails to restart, stating that the device can not be restarted since there are users logged in. The suggestion is to use systemctl reboot -i. I am not sure if this is only for my laptop or not. I tried also making a debian package for tedge agent and the error there is slightly different:

Nov 10 14:25:28 pop-os tedge_agent[34932]: sudo: a terminal is required to read the password; either use the -S option to read from standard input or configure an askpass helper
Nov 10 14:25:28 pop-os tedge_agent[34932]: sudo: a password is required
Nov 10 14:25:28 pop-os sudo[34932]: pam_unix(sudo:auth): conversation failed
Nov 10 14:25:28 pop-os sudo[34932]: pam_unix(sudo:auth): auth could not identify password for [tedge]
Nov 10 14:25:28 pop-os sudo[34932]:    tedge : command not allowed ; PWD=/ ; USER=root ; COMMAND=init 6
Nov 10 14:25:28 pop-os tedge_agent[34850]: 2022-11-10T13:25:28.631403597Z ERROR sm-agent: tedge_agent::agent: Command returned non 0 exit code.

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

#1017

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

@cmosd cmosd force-pushed the improvement/1017/configurable-restart-command branch 2 times, most recently from eff3923 to 8a82e39 Compare November 14, 2022 10:41
@cmosd cmosd changed the title Improvement/1017/configurable restart command Configurable restart command Nov 15, 2022
@cmosd
Copy link
Contributor Author

cmosd commented Nov 16, 2022

This PR now works with a known bug: #1135 and #1579

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.

Nice to have reduce to one the number of functions with test/no-test versions.

Comment on lines +673 to +674
let mut sync_command = std::process::Command::new(SUDO);
sync_command.arg(SYNC);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this sync command can be part of the user configuration.

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.

Nice. However, the tests need to be fixed (you failed to revert there the change making config.init optional).

Copy link
Member

@rina23q rina23q left a comment

Choose a reason for hiding this comment

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

Quickly reviewed (going to do more later). I am not convinced to move system_services directory to /crates/common/tedge_config. It is definitely not tedge_config, which is related to our tedge config ... command. My suggestion is to move directly /crates/common.

@cmosd cmosd force-pushed the improvement/1017/configurable-restart-command branch from fa75244 to cabebd9 Compare November 17, 2022 16:44
@didier-wenzek
Copy link
Contributor

Quickly reviewed (going to do more later). I am not convinced to move system_services directory to /crates/common/tedge_config. It is definitely not tedge_config, which is related to our tedge config ... command. My suggestion is to move directly /crates/common.

I agree that moving code related to system config into tedge_config change the original purpose of the former. However, we have already too many small crates that makes no sense in a context that is not thin-edge. This is why I prefer to have a larger crate that groups many thin-edge configuration related features.

@rina23q
Copy link
Member

rina23q commented Nov 18, 2022

I agree that moving code related to system config into tedge_config change the original purpose of the former. However, we have already too many small crates that makes no sense in a context that is not thin-edge. This is why I prefer to have a larger crate that groups many thin-edge configuration related features.

Having one larger crate makes sense. However, how the tedge_config crate looks is quite messy because of re-purposing the crate. In my opinion, if we make a new directory tedge_config and put all tedge_config_*.rs files to there, it will look better.

./src/
├── config_setting.rs
├── error.rs
├── lib.rs
├── models
│   ├── connect_url.rs
│   ├── file_path.rs
│   ├── flag.rs
│   ├── ipaddress.rs
│   ├── mod.rs
│   ├── port.rs
│   └── templates_set.rs
├── settings.rs
├── system_services
│   ├── command_builder.rs
│   ├── error.rs
│   ├── manager_ext.rs
│   ├── manager.rs
│   ├── managers
│   │   ├── config.rs
│   │   ├── general_manager.rs
│   │   └── mod.rs
│   ├── mod.rs
│   └── services.rs
├── tedge_config_defaults.rs
├── tedge_config_dto.rs
├── tedge_config_location.rs
├── tedge_config_repository.rs
└── tedge_config.rs

@cmosd cmosd force-pushed the improvement/1017/configurable-restart-command branch from 85d161c to 116c9fd Compare November 21, 2022 20:38
@didier-wenzek
Copy link
Contributor

didier-wenzek commented Nov 22, 2022

I agree that moving code related to system config into tedge_config change the original purpose of the former. However, we have already too many small crates that makes no sense in a context that is not thin-edge. This is why I prefer to have a larger crate that groups many thin-edge configuration related features.

Having one larger crate makes sense. However, how the tedge_config crate looks is quite messy because of re-purposing the crate. In my opinion, if we make a new directory tedge_config and put all tedge_config_*.rs files to there, it will look better.

It's a good idea to add a tedge_config sub-module. However, I would do that in another different PR. There are already too many files impacted by this PR simply because of a new crate/module.

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

alexandru solomes added 4 commits November 22, 2022 14:05
Signed-off-by: alexandru solomes <solo@softwareag.com>
Allows one to query config for `system.toml` in other components, not
just in `tedge`.

Signed-off-by: alexandru solomes <solo@softwareag.com>
Signed-off-by: alexandru solomes <solo@softwareag.com>
Signed-off-by: alexandru solomes <solo@softwareag.com>
@cmosd cmosd force-pushed the improvement/1017/configurable-restart-command branch from 116c9fd to e0a14d7 Compare November 22, 2022 12:06
@cmosd cmosd merged commit 27a01dd into thin-edge:main Nov 22, 2022
@cmosd cmosd mentioned this pull request Nov 28, 2022
11 tasks
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