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

Refactor order of actions in guest reboot implementations #3469

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

happz
Copy link
Collaborator

@happz happz commented Jan 15, 2025

Things worked, but the code was not easy to understand and follow:

  • it wasn't obvious custom reboot command and hard reboots are not compatible;
  • it wasn't obvious that custom reboot command can come only from reboot request data stored by tmt-reboot;
  • the ordering of hard, soft and soft-with-custom-command was unclear,
  • command of reboot() was not the same as command of perform_reboot();
  • the relationship between reboot() and perform_reboot() was unclear;
  • often all known arguments were passed to super().reboot() or perform_reboot(), which was not incorrect, but harder for humans and linters to reason about;
  • docstrings of reboot() methods were incomplete or outdated,
  • plugins did not mention their special behavior in docstrings of reboot() method.

To hopefully improve the situation, the patch makes the following changes:

  • uses @overload to clearly mark that hard=True means no custom reboot command;
  • it makes reboot handling in TestInvocation code more verbose to make the distinction between hard and soft reboots which should make the custom reboot command clearly attached to soft reboots only;
  • all reboot() methods now follow the same order of actions: handling hard reboot first, soft reboots second, with less dense code;
  • perform_reboot() now accepts "action" callback, not "command", and it is responsibility of reboot() to feed perform_reboot() wit the proper action (running remote command, local command, or calling testcloud API, and so on);
  • reduces "blind" passing of known values: super().reboot(hard=hard) is correct, but impossible for a type checker to map to the "hard or custom command" split; using hard=True|False helps linters narrow the situation;
  • cleans up docstrings, makes them as same as possible with plugin-specific additions and notes where necessary;
  • and mention reboot support levels in plugin docstrings.

Pull Request Checklist

  • implement the feature
  • write the documentation
  • adjust plugin docstring

Things worked, but the code was not easy to understand and follow:

* it wasn't obvious custom reboot `command` and hard reboots are not
  compatible;
* it wasn't obvious that custom reboot command can come only from reboot
  request data stored by `tmt-reboot`;
* the ordering of hard, soft and soft-with-custom-command was unclear,
* `command` of `reboot()` was not the same as `command` of
  `perform_reboot()`;
* the relationship between `reboot()` and `perform_reboot()` was
  unclear;
* often all known arguments were passed to `super().reboot()` or
  `perform_reboot()`, which was not incorrect, but harder for humans and
  linters to reason about;
* docstrings of `reboot()` methods were incomplete or outdated,
* plugins did not mention their special behavior in docstrings of
  `reboot()` method.

To hopefully improve the situation, the patch makes the following
changes:

* uses `@overload` to clearly mark that `hard=True` means no custom
  reboot command;
* it makes reboot handling in `TestInvocation` code more verbose to
  make the distinction between hard and soft reboots which should make
  the custom reboot command clearly attached to soft reboots only;
* all `reboot()` methods now follow the same order of actions: handling
  hard reboot first, soft reboots second, with less dense code;
* `perform_reboot()` now accepts "action" callback, not "command", and
  it is responsibility of `reboot()` to feed `perform_reboot()` wit the
  proper action (running remote command, local command, or calling
  testcloud API, and so on);
* reduces "blind" passing of known values: `super().reboot(hard=hard)`
  is correct, but impossible for a type checker to map to the "hard or
  custom command" split; using `hard=True|False` helps linters narrow
  the situation;
* cleans up docstrings, makes them as same as possible with
  plugin-specific additions and notes where necessary;
* and mention reboot support levels in plugin docstrings.
@happz happz added step | provision Stuff related to the provision step code | style Code style changes not affecting functionality command | reboot Support for rebooting guests during `tmt run` and the `tmt-reboot` command code | no functional change "No Functional Change" intended. Patch should not change tmt's behavior in any way. labels Jan 15, 2025
@happz happz added this to the 1.42 milestone Jan 15, 2025
@happz happz added the ci | full test Pull request is ready for the full test execution label Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci | full test Pull request is ready for the full test execution code | no functional change "No Functional Change" intended. Patch should not change tmt's behavior in any way. code | style Code style changes not affecting functionality command | reboot Support for rebooting guests during `tmt run` and the `tmt-reboot` command step | provision Stuff related to the provision step
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant