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 I/O priority #3783

Merged
merged 1 commit into from
Mar 31, 2024
Merged

Add I/O priority #3783

merged 1 commit into from
Mar 31, 2024

Conversation

utam0k
Copy link
Member

@utam0k utam0k commented Mar 24, 2023

@AkihiroSuda
Copy link
Member

Could you squash the commits?

@utam0k
Copy link
Member Author

utam0k commented Jun 12, 2023

@AkihiroSuda Sure. Done.

go.mod Outdated Show resolved Hide resolved
utils_linux.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

left a few comments, mostly nits

@utam0k utam0k force-pushed the io-prio branch 5 times, most recently from 223043c to ca36f1e Compare June 15, 2023 12:17
@@ -58,7 +58,7 @@ you use `runc` directly in something like a `systemd` unit file. To disable
this `LISTEN_FDS`-style passing just unset `LISTEN_FDS`.

**Be very careful when passing file descriptors to a container process.** Due
to some Linux kernel (mis)features, a container with access to certain types of
to some Linux kernel misfeatures, a container with access to certain types of
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to fix it to pass the checking code spell.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't seem relevant to the PR

Copy link
Member Author

Choose a reason for hiding this comment

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

But CI won't allow this word... if I edit this file, CI won't go through unless I fix this part.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this hunk. We'll deal with CI later.

@utam0k utam0k force-pushed the io-prio branch 2 times, most recently from 076fa1b to 9f7d118 Compare June 15, 2023 12:30
@utam0k utam0k requested a review from AkihiroSuda June 15, 2023 23:36
@utam0k utam0k force-pushed the io-prio branch 3 times, most recently from a1980b1 to cff9be2 Compare October 21, 2023 05:50
@utam0k utam0k requested a review from AkihiroSuda October 21, 2023 06:12
@lifubang
Copy link
Member

lifubang commented Jan 1, 2024

This PR can be merged once you do a rebase and resolve the conflicts, and I can take over this PR if you are in busy status.

utils_linux.go Outdated Show resolved Hide resolved
@kolyshkin
Copy link
Contributor

@utam0k please avoid merge commits -- do a rebase instead

@utam0k
Copy link
Member Author

utam0k commented Feb 20, 2024

@utam0k please avoid merge commits -- do a rebase instead

Done ✅

@AkihiroSuda
Copy link
Member

@kolyshkin PTAL

@lifubang lifubang dismissed kolyshkin’s stale review March 16, 2024 01:03

All request changes have been addressed.

@lifubang
Copy link
Member

@utam0k Please help to do a rebase to main and solve the conflicts. Thanks.

@@ -9,6 +9,7 @@ Spec version | Feature | PR
-------------|------------------------------------------|----------------------------------------------------------
v1.1.0 | `SECCOMP_FILTER_FLAG_WAIT_KILLABLE_RECV` | [#3862](https://github.com/opencontainers/runc/pull/3862)
v1.1.0 | `.process.ioPriority` | [#3783](https://github.com/opencontainers/runc/pull/3783)
Copy link
Member

Choose a reason for hiding this comment

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

This line should be now removed

@@ -9,6 +9,7 @@ Spec version | Feature | PR
-------------|------------------------------------------|----------------------------------------------------------
v1.1.0 | `SECCOMP_FILTER_FLAG_WAIT_KILLABLE_RECV` | [#3862](https://github.com/opencontainers/runc/pull/3862)
v1.1.0 | `.process.ioPriority` | [#3783](https://github.com/opencontainers/runc/pull/3783)
v1.1.0 | rsvd hugetlb cgroup | TODO ([#3859](https://github.com/opencontainers/runc/issues/3859))
Copy link
Member

Choose a reason for hiding this comment

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

This should be a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

Seems to have been implemented in:

Signed-off-by: utam0k <[email protected]>
Priority: tc.priority,
}
config := &configs.Config{
Rootfs: "/var",
Copy link
Member

Choose a reason for hiding this comment

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

(This value looks unusual, but it doesn’t matter here)

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems we need to set an existing dir. I tried to delete this line and got following error.

 validator_test.go|869| iopriority: 0, expected nil, got error invalid rootfs: stat : no such file or directory

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

Copy link
Member

@lifubang lifubang left a comment

Choose a reason for hiding this comment

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

Thank!

@lifubang lifubang merged commit ea38465 into opencontainers:main Mar 31, 2024
45 checks passed
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.

4 participants