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

Make automatic updates nicer #4676

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

Conversation

morgaesis
Copy link

Thank you for contributing to rpm-ostree.

If you are adding functionality to tree composes, please add
a corresponding test to the compose-test suite. Similarly,
if adding a client-facing feature, consider the vmcheck
suite. Regressions fixes are also great candidates for new
tests.

If you're not sure where or how to add tests, don't hesitate
to ask for help from the maintainers.

Cheers!

@openshift-ci
Copy link

openshift-ci bot commented Oct 27, 2023

Hi @morgaesis. Thanks for your PR.

I'm waiting for a coreos member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@morgaesis morgaesis force-pushed the patch-1 branch 3 times, most recently from bc149f6 to 7675343 Compare October 27, 2023 00:19
@@ -5,4 +5,5 @@ ConditionPathExists=/run/ostree-booted

[Service]
Type=simple
Nice=10
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately this won't do what you want here, because ultimately this just calls into rpm-ostreed.service.

I think by default one would need to use systemd drop-ins for rpm-ostreed.service locally to have the desired effect.

In theory perhaps though, we could better differentiate "background updates" from "foreground activity" and the daemon could automatically lower its priority.

Hmm. Possibly there's a way we could inherit niceness and other properties from this unit.

Copy link
Author

Choose a reason for hiding this comment

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

Wow, you're quick to reply 😮
Isn't the systemd service the right place to configure this? The service has a key made for this. Configuring "niceness" in the process itself is extra complexity, which would probably just end up calling nicectl from within anyway...

Copy link
Author

Choose a reason for hiding this comment

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

I moved the niceness key to the daemon. When would you want rpm-ostreed to run at 0 niceness? It's such a heavy command, that I would always like it to be nice. Running the command manually or when automatic updates are applied, my system stutters.

Copy link
Member

Choose a reason for hiding this comment

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

Does this really help you though?

I suspect the main problem is not CPU usage but I/O bandwidth for most people.

Copy link
Author

Choose a reason for hiding this comment

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

Indeed, for the most part. There are still CPU bound parts

Writing OSTree commit: rpm-ostree start-daemon is CPU intensive (1-2 cores, i guess)
When I tried adding a new package, rpm-ostree start-daemon was using ~180% CPU.

Then, Nice is maybe not the right solution. I just want my system to run smoothly when the updates randomly start.

Copy link
Author

Choose a reason for hiding this comment

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

Or what.... man pages suggest that setting nice for the process also makes the I/O nice
https://linux.die.net/man/1/ionice

@cgwalters
Copy link
Member

I think what would make me most comfortable here is a design where the automatic update path only has Nice set. Implementing that is a bit tricky; by far the simplest would be adding a nice flag for all transactions, and having the automatic update one set it by default.

Basically I'm less certain that we want to default to doing Nice=10 for all user-interactive invocations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants