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

podman machine: Adjust Chrony makestep config #17661

Merged
merged 1 commit into from
Mar 2, 2023

Conversation

xordspar0
Copy link
Contributor

@xordspar0 xordspar0 commented Feb 28, 2023

This allows Chrony to update the system time when it has drifted far from NTP time. By default Chrony only makes slight adjustments, but in the case where a user's laptop lid has been shut for a while and then the machine is resumed, the VM system time could be hours or days behind real time, and it my never catch up if Chrony only makes slight changes.

Fixes #11541

Does this PR introduce a user-facing change?

Fixed a bug where podman machine VMs could have their system time drift behind real time. New machines will no longer be affected by this.

@openshift-ci openshift-ci bot added release-note do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 28, 2023
@xordspar0 xordspar0 force-pushed the chrony-makestep branch 2 times, most recently from 64dc17c to 95ab027 Compare February 28, 2023 19:28
@xordspar0 xordspar0 marked this pull request as ready for review February 28, 2023 19:54
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 28, 2023
@xordspar0
Copy link
Contributor Author

What would a test for this look like? Can you do something like podman machine init and then check that the new files are there? That's what I did in my manual testing, but I don't know if that's possible in the automated test suite.

@TomSweeneyRedHat
Copy link
Member

@xordspar0 IDK if a test is possible, @n1hility would probably have the best answer for that. If a test isn't possible, you can use the tag [NO NEW TESTS NEEDED] in your commit description to avoid the test errors you're seeing currently.

The code LGTM, but it's way out of my wheelhouse.

@xordspar0
Copy link
Contributor Author

xordspar0 commented Mar 2, 2023

I should have included a link the chrony.conf docs to make it easier on reviewers. I'm using confdir to set up a directory where I can drop a file, and in another file I setting makestep, which sets how many and how large adjustments chrony can make to the clock. The main config file already has a makestep directive, but the last one wins. I would comment out or replace the original makestep, but I don't see a way to do that with the Ignition DSL.

https://chrony.tuxfamily.org/doc/3.4/chrony.conf.html

This allows Chrony to update the system time when it has drifted far
from NTP time. By default Chrony only makes slight adjustments, but in
the case where a user's laptop lid has been shut for a while and then
the machine is resumed, the VM system time could be hours or days behind
real time, and it may never catch up if Chrony only makes slight
changes.

[NO NEW TESTS NEEDED]

Fixes containers#11541

Signed-off-by: Jordan Christiansen <[email protected]>
@n1hility
Copy link
Member

n1hility commented Mar 2, 2023

I think testing the actual time sync would be pretty complicated and likely would lead to flakes. If you do it I would definitely blow away the podman machine instance afterwords. Verifying the configs exist would be straight forward, just an enhancement to the machine tests that perhaps uses podman machine ssh to cat / Grep the files. Not sure how valuable that test would be. I’d probably just do [NO NEW TESTS NEEDED]. The change LGTM. Wdyt @baude ?

@baude
Copy link
Member

baude commented Mar 2, 2023

/approve
/lgtm
/hold

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Mar 2, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 2, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude, xordspar0

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 2, 2023
@rhatdan
Copy link
Member

rhatdan commented Mar 2, 2023

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 2, 2023
@openshift-merge-robot openshift-merge-robot merged commit ce67bbf into containers:main Mar 2, 2023
@xordspar0 xordspar0 deleted the chrony-makestep branch March 2, 2023 14:01
@ashley-cui
Copy link
Member

/cherry-pick v4.4

@openshift-cherrypick-robot
Copy link
Collaborator

@ashley-cui: new pull request created: #17871

In response to this:

/cherry-pick v4.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 5, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OS X, podman machine time stops sometime
8 participants