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

spec: add scheduler entity #1188

Merged
merged 1 commit into from
May 11, 2023
Merged

Conversation

giuseppe
Copy link
Member

extend the process struct to represent scheduling attributes for a process based on the sched_setattr(2) syscall.

I've been playing with crun and it is currently implemented as an annotation:

but I think this makes sense as part of the runtime-specs without the custom hack.

config.md Outdated Show resolved Hide resolved
config.md Outdated Show resolved Hide resolved
config.md Outdated Show resolved Hide resolved
specs-go/config.go Outdated Show resolved Hide resolved
config.md Show resolved Hide resolved
specs-go/config.go Outdated Show resolved Hide resolved
@rhatdan
Copy link
Contributor

rhatdan commented Mar 18, 2023

LGTM
@kolyshkin @mrunalp PTAL

Copy link
Contributor

@mrunalp mrunalp left a comment

Choose a reason for hiding this comment

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

LGTM

@utam0k
Copy link
Member

utam0k commented Mar 21, 2023

Thanks, @giuseppe

Policy LinuxSchedulerPolicy `json:"policy"`

// Nice is the nice value for the process, which affects its priority.
Nice int32 `json:"nice"`
Copy link
Member

@utam0k utam0k Mar 21, 2023

Choose a reason for hiding this comment

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

If you know, please let me know, does this apply to process groups?:pray: Or only one process?
I tried reading the man page but it just wasn't clear to me.
https://man7.org/linux/man-pages/man2/sched_setattr.2.html

Copy link
Member Author

@giuseppe giuseppe Mar 21, 2023

Choose a reason for hiding this comment

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

from my understanding that is only for one process or thread:

The sched_setattr() system call sets the scheduling policy and
       associated attributes for the thread whose ID is specified in
       pid.  If pid equals zero, the scheduling policy and attributes of
       the calling thread will be set

do you see a different behavior?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with you. I thought the user would expect the nice value to be applied to all processes in the container? What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

How about specifying whether it applies to the container init process alone or to all processes in the container?

* `SCHED_BATCH`
* `SCHED_ISO`
* `SCHED_IDLE`
* `SCHED_DEADLINE`
Copy link
Member

Choose a reason for hiding this comment

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

SCHED_DEADLINE seems to sometimes return an EBUSY error. Do you want the error numbers to follow these when the set fails?
https://man7.org/linux/man-pages/man2/sched_setattr.2.html

There seems to be some difficulty in using SCHED_DEADLINE with cpuset. can we consider removing SCHED_DEADLINE from support?
https://www.kernel.org/doc/Documentation/scheduler/sched-deadline.txt

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it is required to support it, I am just listing the possible values for now (e.g. SCHED_ISO is not even implemented yet)

Copy link
Member

Choose a reason for hiding this comment

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

I will leave it up to you to specify about it in the oci-spec, but there are some things that probably cannot be handled. I thought users might expect to be able to use them.

Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

@giuseppe
Sorry if I am causing trouble by leaving comments from the outside. If it doesn't seem necessary and the maintainer is satisfied, please ignore it and merge.

@AkihiroSuda AkihiroSuda added this to the v1.1.0 milestone Mar 28, 2023
@AkihiroSuda AkihiroSuda mentioned this pull request Mar 28, 2023
extend the process struct to represent scheduling attributes for a
process based on the sched_setattr(2) syscall.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe
Copy link
Member Author

@thaJeztah could you PTAL?

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM (thanks for the ping; sorry, thought I already did 🙈 ❤️)

@AkihiroSuda AkihiroSuda merged commit 0983f1d into opencontainers:main May 11, 2023
@thaJeztah
Copy link
Member

Any more reviewers needed, or are we good to go? (forgot how many we want for such changes in this repository 🙈)

@thaJeztah
Copy link
Member

Ah! Thanks @AkihiroSuda 😂

@AkihiroSuda AkihiroSuda mentioned this pull request May 22, 2023
12 tasks
@AkihiroSuda AkihiroSuda mentioned this pull request Jun 26, 2023
12 tasks
utam0k added a commit to utam0k/runc that referenced this pull request Aug 4, 2023
utam0k added a commit to utam0k/runc that referenced this pull request Aug 4, 2023
utam0k added a commit to utam0k/runc that referenced this pull request Aug 4, 2023
lifubang pushed a commit to lifubang/runc that referenced this pull request Sep 25, 2023
lifubang pushed a commit to lifubang/runc that referenced this pull request Sep 25, 2023
lifubang pushed a commit to lifubang/runc that referenced this pull request Sep 25, 2023
lifubang pushed a commit to lifubang/runc that referenced this pull request Sep 25, 2023
lifubang pushed a commit to lifubang/runc that referenced this pull request Sep 28, 2023
lifubang pushed a commit to lifubang/runc that referenced this pull request Sep 28, 2023
lifubang pushed a commit to lifubang/runc that referenced this pull request Sep 28, 2023
lifubang pushed a commit to lifubang/runc that referenced this pull request Oct 3, 2023
lifubang pushed a commit to lifubang/runc that referenced this pull request Oct 3, 2023
lifubang pushed a commit to lifubang/runc that referenced this pull request Oct 3, 2023
lifubang pushed a commit to lifubang/runc that referenced this pull request Oct 4, 2023
Spec: opencontainers/runtime-spec#1188
Fix: opencontainers#3895

Co-authored-by: utam0k <[email protected]>
Signed-off-by: utam0k <[email protected]>
Signed-off-by: lifubang <[email protected]>
lifubang pushed a commit to lifubang/runc that referenced this pull request Oct 4, 2023
Spec: opencontainers/runtime-spec#1188
Fix: opencontainers#3895

Co-authored-by: utam0k <[email protected]>
Signed-off-by: utam0k <[email protected]>
Signed-off-by: lifubang <[email protected]>
lifubang added a commit to lifubang/runc that referenced this pull request Oct 4, 2023
Spec: opencontainers/runtime-spec#1188
Fix: opencontainers#3895

Co-authored-by: lifubang <[email protected]>
Signed-off-by: utam0k <[email protected]>
Signed-off-by: lifubang <[email protected]>
dims pushed a commit to dims/libcontainer that referenced this pull request Oct 19, 2024
Spec: opencontainers/runtime-spec#1188
Fix: opencontainers/runc#3895

Co-authored-by: lifubang <[email protected]>
Signed-off-by: utam0k <[email protected]>
Signed-off-by: lifubang <[email protected]>
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.

6 participants