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

[#12] Adding scheduled shutdown #110

Draft
wants to merge 1 commit into
base: development
Choose a base branch
from
Draft

[#12] Adding scheduled shutdown #110

wants to merge 1 commit into from

Conversation

DT3264
Copy link
Collaborator

@DT3264 DT3264 commented Jun 2, 2023

[#12] Adding scheduled shutdown

This commit adds support for a new global directive "ShutdownTime", like ContestTimes or ConfigExpirationTime, that schedules a shutdown at the time defined.

@DT3264 DT3264 requested a review from equetzal June 2, 2023 20:19
@DT3264 DT3264 force-pushed the pr110 branch 2 times, most recently from 6ab444a to 3d29238 Compare June 3, 2023 04:56
@DT3264 DT3264 changed the title [ #12 ] Adding scheduled shutdown [#12] Adding scheduled shutdown Jun 3, 2023
@equetzal equetzal force-pushed the development branch 10 times, most recently from f477176 to 9b027cc Compare June 8, 2023 03:30
@equetzal equetzal changed the base branch from development to main June 8, 2023 03:38
@equetzal equetzal changed the base branch from main to development June 8, 2023 03:38
Copy link
Owner

@equetzal equetzal left a comment

Choose a reason for hiding this comment

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

Have not tested, please rock-test it before landing.
Address de documentation comment. LGTM in overall!

Copy link
Owner

@equetzal equetzal left a comment

Choose a reason for hiding this comment

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

Overall good, the system does shutdown correctly and does the work.
I've found some problems that are needed to be corrected before merging, so let's tackle them.
Good work on to this! :D

base-system/usrroot/usr/lib/hsync/libhapply.so Outdated Show resolved Hide resolved
base-system/usrroot/usr/lib/hsync/libhapply.so Outdated Show resolved Hide resolved
@equetzal
Copy link
Owner

Hey @DT3264, I was notified for review on this, but I don't see any upcoming change after my last review. Was this request right?

@DT3264
Copy link
Collaborator Author

DT3264 commented Aug 10, 2023

@equetzal There's no need to re-review this one yet, dunno why that review was requested

@DT3264
Copy link
Collaborator Author

DT3264 commented Aug 10, 2023

@equetzal I now asked for review intentionally, I amended the requested changes

Copy link
Owner

@equetzal equetzal Aug 11, 2023

Choose a reason for hiding this comment

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

I'll test this ASAP but in the meanwhile please rework this doc similar as you did with ipman. Try to be more detailed about what this is doing, talk about the roundings of the time, explain what happens on edge cases for someone to be clear about what happens in those cases.
Remember that the whole point of documentation is to find the specifics on how something works without requiring someone to got deep on the code. An example of this was the discovery we had about the date %s '' which had that 'special' case of empty string, and we catch that on the docs not the code.

This commit adds support for a new global directive "ShutdownTime", like ContestTimes or ConfigExpirationTime, that schedules a shutdown at the time defined.
Copy link
Owner

@equetzal equetzal left a comment

Choose a reason for hiding this comment

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

I've found several issues so I'll be needing to hold this feature because it's not ready for a release. The bug of not being able to change mode and how hard is to recover from it makes this severe, so we might be needing to fully change the approach we're taking for this feature.
Also, you need to polish your testing skills, you're not being able to find multiple bugs in several PRs that you craft. I know this environment is not like regular coding, this is why you need to become creative when it's about testing and really try your feature in all kind of circunstancies.

Great work on this, but we need to keep working on this one.

if [ "$SECONDS_REMAINING" -gt "$ROUND_THRESHOLD" ]; then
MINUTES_TO_SHUTDOWN=$((MINUTES_TO_SHUTDOWN + 1))
fi
MINUTES_TO_SHUTDOWN=$((SECONDS_TO_SHUTDOWN / 60))
Copy link
Owner

Choose a reason for hiding this comment

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

So you're basically overriding the rounding you did before, so the system keeps doing the exact same thing of poweroff on mode change.

Copy link
Owner

Choose a reason for hiding this comment

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

To better test this scenario, make a 2min long contest and schedule a shutdown at the middle. This is what should happens:
min 0 - Mode change to contest
min 1 - Shutdown

This is what happens:
min 0 - Shutdown

Please fix this behavior, and also let's rise an issue for make an effort to make with more precise as seen with the mode change that it's performed timely on second 0

Copy link
Owner

Choose a reason for hiding this comment

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

After deleting the line 416 the schedule works more properly

SHUTDOWN_TIME="none"
fi
log "Cancelling scheduled shutdown (if any)"
shutdown -c
Copy link
Owner

Choose a reason for hiding this comment

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

Ok, so Idk what this thing is doing on the back, but it seems like if you schedule a poweroff, you cannot change the system mode anymore. I would be needing to go and watch the journal in detail, but I only get a broadcast message that the system will poweroff in X min and I'm not able to turn up lightdm up again.
I think it is also related to unmounting the usrchanges/ branch but not sure.
To repro, schedule a contest (as large as you want) and schedule a shudown within the contest. You won't be able to change mode to contest.

@DT3264
Copy link
Collaborator Author

DT3264 commented Aug 12, 2023

I think I'll rework this PR later on after the OMI thing has happened, in the meantime I'll leave a thought I had about this feature

Maybe the shutdown thing could be simpler, just a service asking each minute
"Is the current datetime equal or greater than the shutdown datetime provided?"
And to avoid conflicts with the mode change, maybe if there's a mode change nearby skip the shutdown until the mode change had taken place,

wdyt?

@equetzal
Copy link
Owner

I think I'll rework this PR later on after the OMI thing has happened, in the meantime I'll leave a thought I had about this feature

Maybe the shutdown thing could be simpler, just a service asking each minute "Is the current datetime equal or greater than the shutdown datetime provided?" And to avoid conflicts with the mode change, maybe if there's a mode change nearby skip the shutdown until the mode change had taken place,

wdyt?

I like the approach of instead of scheduling the shutdown with shutdown, do it like "schedule a script execution" which when executed actually powers off the machine. This way we can implement this in numerous ways, we could use at, cron, systemd-run, etc. And the shutdown op would always be like 'shutdown now'. Inside, the op could be a systemd-service that conflicts with both hsync.service and happly.service so that the execution waits until the directives are applied to then shutdown the system.

About the thing with mode change, is not -that- important, the problem is the accuracy of the time, because if I do some op at min 0 and shutdown at min 1 I don't expect the system to shutdown before, but this can be solved by the systemd service I told you.

@DT3264 DT3264 marked this pull request as draft September 2, 2023 21:38
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