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

Add -lp/--low-priority/KOMETA_LOW_PRIORITY to allow Kometa to run at lower/higher priorities #2397

Open
wants to merge 10 commits into
base: nightly
Choose a base branch
from

Conversation

YozoraXCII
Copy link
Contributor

Description

Credits to @planetrocky for the initial Pull Request.

This change allows the user to specify the priority that Kometa run at on Windows/Linux/Mac (and should work in Docker too).

The options available are:
25: Below Normal
50: Normal (default)
75: Above Normal
90: High
100: Realtime

Issues Fixed or Closed

Closes #2304 (supersedes)

Type of Change

  • New feature (non-breaking change which adds functionality)

Checklist

  • Updated Documentation to reflect changes
  • Updated the CHANGELOG with the changes

@YozoraXCII
Copy link
Contributor Author

I would greatly appreciate any testing on this, particularly the KOMETA_PRIORITY=XX as I haven't been able to test this yet.

Docker image is kometateam/kometa:run-nicely

@planetrocky
Copy link

planetrocky commented Jan 1, 2025

@YozoraXCII just carried out a visual code-review.

I can’t see low_priority() being called anywhere?

Originally I only added lowering the priority; otherwise in Docker you have to add —cap-add SYS_NICE to the Docker container if you want to use a negative nice value (that’s a privileged operation in Linux and requires root).
I was also concerned that users might just run their Docker with --privileged rather than the fine-grained permissions with —cap-add.

Should the definition be changed to reflect it is now changing Kometa’s run process priority, not just lowering/nice-ing the process.

@YozoraXCII
Copy link
Contributor Author

@YozoraXCII just carried out a visual code-review.

I can’t see low_priority() being called anywhere?

Originally I only added lowering the priority; otherwise in Docker you have to add —cap-add SYS_NICE to the Docker container if you want to use a negative nice value (that’s a privileged operation in Linux and requires root). I was also concerned that users might just run their Docker with --privileged rather than the fine-grained permissions with —cap-add.

Should the definition be changed to reflect it is now changing Kometa’s run process priority, not just lowering/nice-ing the process.

Let me take this away and think about how to handle it. I may need to reduce the scope if we're going to potentially run into privilege issues.

@YozoraXCII YozoraXCII added the status:more-info-needed More information is needed label Jan 2, 2025
@planetrocky
Copy link

@YozoraXCII just carried out a visual code-review.
I can’t see low_priority() being called anywhere?
Originally I only added lowering the priority; otherwise in Docker you have to add —cap-add SYS_NICE to the Docker container if you want to use a negative nice value (that’s a privileged operation in Linux and requires root). I was also concerned that users might just run their Docker with --privileged rather than the fine-grained permissions with —cap-add.
Should the definition be changed to reflect it is now changing Kometa’s run process priority, not just lowering/nice-ing the process.

Let me take this away and think about how to handle it. I may need to reduce the scope if we're going to potentially run into privilege issues.

I wrote a long reply about this. However I a got BSOD on my Windows machine before hitting comment :(
IMG_2515

@planetrocky
Copy link

planetrocky commented Jan 2, 2025

@YozoraXCII just carried out a visual code-review.
I can’t see low_priority() being called anywhere?
Originally I only added lowering the priority; otherwise in Docker you have to add —cap-add SYS_NICE to the Docker container if you want to use a negative nice value (that’s a privileged operation in Linux and requires root). I was also concerned that users might just run their Docker with --privileged rather than the fine-grained permissions with —cap-add.
Should the definition be changed to reflect it is now changing Kometa’s run process priority, not just lowering/nice-ing the process.

Let me take this away and think about how to handle it. I may need to reduce the scope if we're going to potentially run into privilege issues.

In the previous PR we discussed this, there was a question about adding the ability to also increase the process priority. I was reluctant for a few reasons:

  1. Long-running processing jobs such as Kometa are generally best at normal/default scheduling priority. Lowering Kometa's process priority benefits the system by improving responsiveness, user interactivity, and reducing the impact on system processes that run at higher priority. Setting Kometa to match system-level high-priority processes is not optimal. Kometa can be a long-running job with heavy I/O and would contend for scheduling time with system-level jobs. High-priority tasks often take resources exclusively and should release them as soon as possible. Contention at this scheduling level could degrade performance and perhaps stability.

    Often in multiprocessing, there will be synchronization or locking of shared resources. If a lock, mutex, or semaphore is taken, anything else dependent on that will block. The design might run blocking aspects at a higher priority to reduce the impact. Setting Kometa to a normal/default user priority allows system high-priority processes to take precedence in scheduling and prevents competition for resources with critical system operations.

    This is why increasing a process priority is always a privileged operation. Lowering Kometa's process priority below normal (adding a nice value to the base priority) enables interactive processes or other short-running jobs to take precedence. In my use case, I run a Linux Docker image for Kometa updates, typically on my desktop. When I am making many changes, adding things, and testing new overlays, I often have Kometa running at a lower priority for many hours every evening. This is precisely what the Linux nice mechanism was designed for. Modern OS schedulers are adept at maximizing resource utilization.

  2. Under Linux and Docker, an ordinary user may lower the process priority by setting a nice value, which is encouraged. However, setting a negative nice value (high priority) requires UID=0 (root) privileges. Docker provides --cap-add to permit fine-grained control over which privileged operations a non-root Docker instance may execute. For this case, the appropriate capability would be --cap-add SYS_NICE.

    Some users, however, might run the Docker instance as root (--privileged), which is unnecessary and more dangerous. Some Docker GUIs offer a simple checkbox to run as privileged, which undermines container isolation. Docker themselves describe this mode as follows:

    This flag exists to allow special use-cases, like running Docker within Docker.

Escalate container privileges (--privileged)

Escalate container privileges

The --privileged flag gives the following capabilities to a container:

  • Enables all Linux kernel capabilities
  • Disables the default seccomp profile
  • Disables the default AppArmor profile
  • Disables the SELinux process label
  • Grants access to all host devices
  • Makes /sys read-write
  • Makes cgroups mounts read-write

In other words, the container can then do almost everything that the host can do. This flag exists to allow special use-cases, like running Docker within Docker.

⚠️Warning

Use the --privileged flag with caution. A container with --privileged is not a securely sandboxed process. Containers in this mode can get a root shell on the host and take control over the system.

For most use cases, this flag should not be the preferred solution. If your container requires escalated privileges, you should prefer to explicitly grant the necessary permissions, for example by adding individual kernel capabilities with --cap-add.

For more information, see Runtime privilege and Linux capabilities

Win32 API Priority Classes

https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-setpriorityclass

ABOVE_NORMAL_PRIORITY_CLASS may be OK, and should be the maximum option.

HIGH_PRIORITY_CLASS has this comment (emphasis mine):

Process that performs time-critical tasks that must be executed immediately. The threads of the process preempt the threads of normal or idle priority class processes. An example is the Task List, which must respond quickly when called by the user, regardless of the load on the operating system. Use extreme care when using the high-priority class, because a high-priority class application can use nearly all available CPU time.

REALTIME_PRIORITY_CLASS has this comment:

Process that has the highest possible priority. The threads of the process preempt the threads of all other processes, including operating system processes performing important tasks. For example, a real-time process that executes for more than a very brief interval can cause disk caches not to flush or cause the mouse to be unresponsive.

@YozoraXCII
Copy link
Contributor Author

@YozoraXCII just carried out a visual code-review.
I can’t see low_priority() being called anywhere?
Originally I only added lowering the priority; otherwise in Docker you have to add —cap-add SYS_NICE to the Docker container if you want to use a negative nice value (that’s a privileged operation in Linux and requires root). I was also concerned that users might just run their Docker with --privileged rather than the fine-grained permissions with —cap-add.
Should the definition be changed to reflect it is now changing Kometa’s run process priority, not just lowering/nice-ing the process.

Let me take this away and think about how to handle it. I may need to reduce the scope if we're going to potentially run into privilege issues.

In the previous PR we discussed this, there was a question about adding the ability to also increase the process priority. I was reluctant for a few reasons:

In the previous PR, we discussed adding the ability to also increase the process priority. I was reluctant for a few reasons:

  1. Long-running processing jobs such as Kometa are generally best at normal/default scheduling priority. Lowering Kometa's process priority benefits the system by improving responsiveness, user interactivity, and reducing the impact on system processes that run at higher priority. Setting Kometa to match system-level high-priority processes is not optimal. Kometa can be a long-running job with heavy I/O and would contend for scheduling time with system-level jobs. High-priority tasks often take resources exclusively and should release them as soon as possible. Contention at this scheduling level could degrade performance and perhaps stability.
    Often in multiprocessing, there will be synchronization or locking of shared resources. If a lock, mutex, or semaphore is taken, anything else dependent on that will block. The design might run blocking aspects at a higher priority to reduce the impact. Setting Kometa to a normal/default user priority allows system high-priority processes to take precedence in scheduling and prevents competition for resources with critical system operations.
    This is why increasing a process priority is always a privileged operation. Lowering Kometa's process priority below normal (adding a nice value to the base priority) enables interactive processes or other short-running jobs to take precedence. In my use case, I run a Linux Docker image for Kometa updates, typically on my desktop. When I am making many changes, adding things, and testing new overlays, I often have Kometa running at a lower priority for many hours every evening. This is precisely what the Linux nice mechanism was designed for. Modern OS schedulers are adept at maximizing resource utilization.
  2. Under Linux and Docker, an ordinary user may lower the process priority by setting a nice value, which is encouraged. However, setting a negative nice value (high priority) requires UID=0 (root) privileges. Docker provides --cap-add to permit fine-grained control over which privileged operations a non-root Docker instance may execute. For this case, the appropriate capability would be --cap-add SYS_NICE.
    Some users, however, might run the Docker instance as root (--privileged), which is unnecessary and more dangerous. Some Docker GUIs offer a simple checkbox to run as privileged, which undermines container isolation. Docker themselves describe this mode as follows:

    This flag exists to allow special use-cases, like running Docker within Docker.

Escalate container privileges (--privileged)

Escalate container privileges

The --privileged flag gives the following capabilities to a container:

  • Enables all Linux kernel capabilities
  • Disables the default seccomp profile
  • Disables the default AppArmor profile
  • Disables the SELinux process label
  • Grants access to all host devices
  • Makes /sys read-write
  • Makes cgroups mounts read-write

In other words, the container can then do almost everything that the host can do. This flag exists to allow special use-cases, like running Docker within Docker.

⚠️Warning
Use the --privileged flag with caution. A container with --privileged is not a securely sandboxed process. Containers in this mode can get a root shell on the host and take control over the system.
For most use cases, this flag should not be the preferred solution. If your container requires escalated privileges, you should prefer to explicitly grant the necessary permissions, for example by adding individual kernel capabilities with --cap-add.
For more information, see Runtime privilege and Linux capabilities

Not ignoring this, just haven't had the headspace to read and contemplate all this - it's a bit above my knowledge level if I'm honest lol.

@planetrocky
Copy link

planetrocky commented Jan 14, 2025

Not ignoring this, just haven't had the headspace to read and contemplate all this - it's a bit above my knowledge level if I'm honest lol.

There's zero urgency! No worries, and no stress! :)
I'm super busy with work.

TLDR; don't raise process priority above normal! :)
Raising priority above normal scheduling priority is restricted to a privileged operation as it has implications!

In Windows (the Win32 API) ABOVE_NORMAL_PRIORITY_CLASS may be OK, and should be the maximum option I think.

kometa.py Outdated
def low_priority(niceness):
"""Set the priority of the process based on the niceness level."""

isWindows = platform.system() == "Windows"

Choose a reason for hiding this comment

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

I'd changed this to:
isWindows = os.name == 'nt'

os.name will only return one of ['posix', 'nt', 'java']

platform.system() returns many aliases and the platform library is for detailed system information.

https://docs.python.org/3/library/os.html#os.name

os.name
The name of the operating system dependent module imported. The following names have currently been registered: 'posix', 'nt', 'java'.

@planetrocky
Copy link

I added a comment on the isWindows code check; copied here for visibility:

I'd changed this to:
isWindows = os.name == 'nt'

os.name will only return one of 'nt' or 'posix' for Python 3.8 ... 3.13. Python 2.7 will still return 'nt' or 'posix', and some other dead platforms. Python 3.0 you will never get, but has 'ce' and 'riscos'

  • 'nt' = Windows
  • 'posix' = Unix

platform.system() returns many aliases and the platform library is for detailed system information.

https://docs.python.org/3/library/os.html#os.name

os.name
The name of the operating system dependent module imported. The following names have currently been registered: 'posix', 'nt', 'java'.

@meisnate12 meisnate12 added status:review-requested An additional review has been requested and removed status:more-info-needed More information is needed labels Jan 15, 2025
@YozoraXCII
Copy link
Contributor Author

@planetrocky would you mind reviewing the most recent changes and doing some tests?

@meisnate12 meisnate12 changed the title Add -pr/--priority/KOMETA_PRIORITY to allow Kometa to run at lower/higher priorities Add -lp/--low-priority/KOMETA_LOW_PRIORITY to allow Kometa to run at lower/higher priorities Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docker Build Pull Request in Docker status:review-requested An additional review has been requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants