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

plugins/reboot: fix boot time command for FreeBSD #82383

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

ckoehne
Copy link

@ckoehne ckoehne commented Dec 8, 2023

SUMMARY

The boot time is not a fixed value [1]. It's adjusted by NTP. That's not only an issue of FreeBSD. E.g. OpenBSD has the same issue [2]. On FreeBSD we can use the boot_id sysctl [3]. Unfortunately, OpenBSD doesn't have such a sysctl. So, we keep the boottime for now.

[1] https://cgit.freebsd.org/src/commit/?id=a512d0ab009eedf2f1876fce86d6386bfee626d8
[2] https://github.com/openbsd/src/blob/dd1c5868edaa80b7ad9df54e8b3eae1c49c42319/sys/kern/kern_tc.c#L735
[3] https://cgit.freebsd.org/src/commit/?id=34086d5bda29cc583755fc8948f59c3b61f8ce7d

ISSUE TYPE
  • Bugfix Pull Request
ADDITIONAL INFORMATION

@ansibot ansibot added bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. labels Dec 8, 2023
@ckoehne
Copy link
Author

ckoehne commented Dec 12, 2023

Note that the sysctl is not available on FreeBSD 12. Not sure if it's important. Additionally, FreeBSD 12 reaches EOL in a few days: https://www.freebsd.org/security/#sup

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Dec 12, 2023
@ansibot

This comment was marked as resolved.

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Dec 12, 2023
@webknjaz webknjaz requested a review from bcoca December 12, 2023 12:27
@webknjaz
Copy link
Member

Note that the sysctl is not available on FreeBSD 12. Not sure if it's important. Additionally, FreeBSD 12 reaches EOL in a few days: https://www.freebsd.org/security/#sup

@ckoehne we don't run FreeBSD 12 in CI for the devel branch, but I think that some older stable and still supported versions do (stable-2.15 and older). This effectively means that we can accept a backport into the stable-2.16 branch for as long as it's framed as a bugfix.

Generally, we prefer having regression tests bundled with PRs. There's pre-existing ones @ https://github.com/ansible/ansible/blob/devel/test/units/plugins/action/test_reboot.py / https://github.com/ansible/ansible/tree/devel/test/integration/targets/reboot.

Do you think it would be possible to come up with an integration test? Keep in mind, though, that reboots are expensive so it's probably cheaper to piggy-back on one of the pre-existing tests. I moved some of the older ones to unit-tests to cut the CI usage some time ago.

@@ -0,0 +1,2 @@
bugfixes:
- reboot - change the boot time command on FreeBSD because the current one is not stable (https://github.com/ansible/ansible/pull/82383)
Copy link
Member

@webknjaz webknjaz Dec 12, 2023

Choose a reason for hiding this comment

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

Note that this appears in the changelog, along with other entries. Release notes generally refer to things that changed compared to the previous release. In that context, the end-users (the main target audience reading the changelog) wouldn't understand what's "the current one" in that context.

Could you try re-phrasing it? Using the past tense or structures like "X is now Y" may be helpful.

@ckoehne
Copy link
Author

ckoehne commented Dec 12, 2023

Note that the sysctl is not available on FreeBSD 12. Not sure if it's important. Additionally, FreeBSD 12 reaches EOL in a few days: https://www.freebsd.org/security/#sup

@ckoehne we don't run FreeBSD 12 in CI for the devel branch, but I think that some older stable and still supported versions do (stable-2.15 and older). This effectively means that we can accept a backport into the stable-2.16 branch for as long as it's framed as a bugfix.

Generally, we prefer having regression tests bundled with PRs. There's pre-existing ones @ https://github.com/ansible/ansible/blob/devel/test/units/plugins/action/test_reboot.py / https://github.com/ansible/ansible/tree/devel/test/integration/targets/reboot.

Do you think it would be possible to come up with an integration test? Keep in mind, though, that reboots are expensive so it's probably cheaper to piggy-back on one of the pre-existing tests. I moved some of the older ones to unit-tests to cut the CI usage some time ago.

I'm not sure if it's possible to reproduce it reliable. It's a sporadic issue which only occurs when:

  1. ansible calls ssh target@ip shutdown -r
  2. FreeBSD updates the system time e.g. due to NTP -> boot time changes
  3. ansible is able to reconnect before the target rebooted -> ansible thinks that the target already rebooted
  4. The target reboots

So, we would have to somehow delay the shutdown and then changing the system time after calling shutdown.

The boot time is not a fixed value [1]. It's adjusted by NTP. That's not
only an issue of FreeBSD. E.g. OpenBSD has the same issue [2]. On
FreeBSD we can use the boot_id sysctl [3]. Unfortunately, OpenBSD
doesn't have such a sysctl. So, we keep the boottime for now.

[1] https://cgit.freebsd.org/src/commit/?id=a512d0ab009eedf2f1876fce86d6386bfee626d8
[2] https://github.com/openbsd/src/blob/dd1c5868edaa80b7ad9df54e8b3eae1c49c42319/sys/kern/kern_tc.c#L735
[3] https://cgit.freebsd.org/src/commit/?id=34086d5bda29cc583755fc8948f59c3b61f8ce7d

Signed-off-by: Corvin Köhne <c.koehne@beckhoff.com>
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Dec 12, 2023
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants