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

Account for empty meta in reboot_shutdown.py #2514 #2518

Conversation

FroggyFlox
Copy link
Member

Fixes #2514
@phillxnet, @Hooverdan96, ready for review.

As detailed in #2514, a scheduled task of type reboot fails due to a omission to account for an empty taskdefinition meta as it is the case for reboot tasks. This pull request thus simply adds a check for an empty meta before trying to extract information from it.

Demonstration

A scheduled task of type reboot was created. When it was time, the reboot command is indeed issued:

Broadcast message from root@rockdev (Sat 2023-03-11 17:45:02 EST):

The system is going down for reboot at Sat 2023-03-11 17:48:02 EST!

image

To verify other tasks using the reboot_shutdown.py script are still functional, a task of type shutdown was created. When it was time, the shutdown command is indeed triggered:

Broadcast message from root@rockdev (Sat 2023-03-11 17:55:02 EST):

The system is going down for poweroff at Sat 2023-03-11 17:58:02 EST!

[11/Mar/2023 17:55:02] DEBUG [scripts.scheduled_tasks.reboot_shutdown:150] System shutdown scheduled

image

Unit tests

Three new unit tests are added to cover the reboot_shutdown.py script. Note, however, that the part of main() dealing with rtc wake-up time is not covered as I'm not that familiar with working with time data in Python. I'm not sure I want to risk messing users' tasks scheduling as a result.

Testing

rockdev:/opt/rockstor/src/rockstor # poetry run django-admin test
Creating test database for alias 'default'...
Creating test database for alias 'smart_manager'...
System check identified no issues (0 silenced).
..........................................................................................................................................................................................................................................................
----------------------------------------------------------------------
Ran 250 tests in 13.574s

OK
Destroying test database for alias 'default'...
Destroying test database for alias 'smart_manager'...

We currently assume a taskdefinition meta is always filled with information
when running the reboot_shutdown.py script. While this is the case for tasks
of type shutdown and suspend, tasks of type reboot only have an empty Dict
as meta.

This commit thus first checks for an empty meta before trying to extract
information from it.

Add unit tests coverage for reboot_shutdown.py.
@FroggyFlox
Copy link
Member Author

@phillxnet, I realized I forgot to update some text and names in the new tests so I've pushed an additional commit (82acd08). My apologies for the extra commit :(. Let me know if you'd like me to redo the PR with only the str.format() bits separated.

Copy link
Member

@phillxnet phillxnet left a comment

Choose a reason for hiding this comment

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

@FroggyFlox Thanks for finding and seeing to this one. Much appreciated.

Test rpm build OK with the following test results:

cd src/rockstor/

  • poetry run django-admin test
    ..........................................................................................................................................................................................................................................................

Ran 250 tests in 26.534s

Thanks also for upping our testing in this area.

From a local console of a 4.5.8-2518 rpm build scheduled to shutdown at 17:30 we have:

Broadcast message from root@rleap15-4 (Mon 2023-03-20 17:32:02 WET):

The system is going down for reboot at Mon 2023-03-20 17:33:02 WET!

Broadcast message from root@rleap15-4 (Mon 2023-03-20 17:33:02 WET):

The system is going down for reboot NOW!

@phillxnet
Copy link
Member

As a follow-up to the review I also tested (in KVM) a shutdown task to be successfully:

Mar 20 17:42:01 rleap15-4 cron[1109]: (*system*) RELOAD (/etc/cron.d/rockstortab)
Mar 20 17:42:01 rleap15-4 CRON[14904]: (root) CMD (/opt/rockstor/.venv/bin/st-system-power 2 \*-*-*-*-*-*)

Broadcast message from root@rleap15-4 (Mon 2023-03-20 17:42:01 WET):

The system is going down for poweroff at Mon 2023-03-20 17:45:01 WET!

Mar 20 17:42:01 rleap15-4 systemd-logind[675]: Creating /run/nologin, blocking further logins...
Mar 20 17:42:02 rleap15-4 CRON[14903]: (root) CMDEND (/opt/rockstor/.venv/bin/st-system-power 2 \*-*-*-*-*-*)

Broadcast message from root@rleap15-4 (Mon 2023-03-20 17:43:02 WET):

The system is going down for poweroff at Mon 2023-03-20 17:45:01 WET!
...
Mar 20 17:43:24 rleap15-4 systemd-helper[15020]: running timeline cleanup for 'root'.
Mar 20 17:43:24 rleap15-4 systemd-helper[15020]: running empty-pre-post cleanup for 'root'.
Mar 20 17:43:24 rleap15-4 systemd[1]: snapper-cleanup.service: Deactivated successfully.

Broadcast message from root@rleap15-4 (Mon 2023-03-20 17:44:02 WET):

The system is going down for poweroff at Mon 2023-03-20 17:45:01 WET!

Mar 20 17:44:24 rleap15-4 systemd[1]: snapperd.service: Deactivated successfully.

Broadcast message from root@rleap15-4 (Mon 2023-03-20 17:45:02 WET):

The system is going down for poweroff NOW!

Connection to rleap15-4.lan closed by remote host.

@phillxnet phillxnet merged commit 3ca00b4 into rockstor:testing Mar 20, 2023
@FroggyFlox FroggyFlox deleted the 2514_Scheduled_tasks_of_type_Reboot_fail_to_run branch May 27, 2023 15:41
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.

2 participants