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 support for time namespace #1151

Merged
merged 1 commit into from
Feb 1, 2023

Conversation

KentaTada
Copy link
Contributor

The time namespace is a new kernel feature available in 5.6+ to
isolate the system monotonic and boot-time clocks.

I'm sorry I mistook and closed the existing request #1062

Signed-off-by: Kenta Tada Kenta.Tada@sony.com

@KentaTada
Copy link
Contributor Author

monotonicNanosecs and boottimeNanosecs do not use the negative number.
And I noticed that the value of offset-nanosecs is not greater than 999,999,999.
So I changed those types from int64 to uint32 in this new PR.

specs-go/config.go Outdated Show resolved Hide resolved
config.md Outdated Show resolved Hide resolved
@AkihiroSuda
Copy link
Member

@KentaTada could you update the PR?

@KentaTada
Copy link
Contributor Author

@AkihiroSuda @tianon
I updated this pr from your comments.
If I remember correctly, Secs was required and NanoSecs was optional because of the kernel implementation.
But we changed the definition of LinuxTimeOffset from specific to generic. So, I changed Secs to optional.
I may be slow to respond, but I'll check reviews. Sorry.

config-linux.md Outdated
Entry values are objects with the following properties:

* **`secs`** *(int64, OPTIONAL)* - is the offset of clock (in secs) in the container.
* **`nanosecs`** *(uint32, OPTIONAL)* - is the additional offset for secs (in nanosecs). The actual offset is secs plus nanosecs.
Copy link
Member

Choose a reason for hiding this comment

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

The actual offset is secs plus nanosecs.

Just to confirm, is this true for negative secs too?

Also curious why nanosecs is unsigned

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. I don't remeber the reason of this explanation.
I just copied the reviewed explanation in the previous PR #1062.
I'll just refer to man and modify it.

First of all,

And I noticed that the value of offset-nanosecs is not greater than 999,999,999.
So I changed those types from int64 to uint32 in this new PR.

The above specification depends on the current implementations.
When we consider future-proof, I'll switch back to int64.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for going back and forth, but nanosecs can remain unsigned, as it is stated in the kernel man page
https://man7.org/linux/man-pages/man7/time_namespaces.7.html

These offsets are expressed
relative to the clock values in the initial time namespace. The
offset-secs value can be negative, subject to restrictions noted
below; offset-nanosecs is an unsigned value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. I re-updated to remain unsigned.

config-linux.md Outdated
Entry values are objects with the following properties:

* **`secs`** *(int64, OPTIONAL)* - is the offset of clock (in seconds) in the container.
* **`nanosecs`** *(int64, OPTIONAL)* - is the offset of clock (in nanoseconds) in the container.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this must be smaller than 10^9(?) Do we want to mention the limits and that this is for fratcions of the seconds?

Copy link
Member

@AkihiroSuda AkihiroSuda Jan 24, 2023

Choose a reason for hiding this comment

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

It's already explained in the kernel doc: https://man7.org/linux/man-pages/man7/time_namespaces.7.html

EINVAL An offset-nanosecs value is greater than 999,999,999.

The time namespace is a new kernel feature available in 5.6+ to
isolate the system monotonic and boot-time clocks.

Signed-off-by: Kenta Tada <Kenta.Tada@sony.com>
@thaJeztah
Copy link
Member

(Silly) questions;

  • Do Windows containers allow for time to be namespaced? (possibly not for this proposal, but in case it's added to the Windows counterpart in future)
  • As the discussion above appears to be focussing on signedness and size of integers (because we're trying to do a 1:1 mapping of the Linux kernel to the Spec), I'm wondering if we MUST follow the linux Kernel here, or if it would be beneficial to pick our own definition (does RFC3399 officially support nanoseconds and a "duration", or is there an RFC for that?).

Perhaps it's not an option, and I know many of the options were a 1:1 mapping of kernel features (out of convenience?) I also know that some of those choices made it less agnostic/portable than desired, which is why I was considering if another format would be beneficial / future proof (as long as it's not ambiguous, and documented how runtimes must interpret the config and convert it to (currently) what Linux provides).

@AkihiroSuda
Copy link
Member

windows

I think windows is out of the scope of this PR, and windows support can be added in the config-windows.md spec separately in the future when somebody needs it.

@thaJeztah
Copy link
Member

I think windows is out of the scope of this PR, and windows support can be added in the config-windows.md spec separately in the future when somebody needs it.

For sure! Mostly exploring if there's an platform-agnostic way to specify this configuration (and if there is, and it wouldn't be "crippling" either side, the same format could be used for Windows)

@AkihiroSuda AkihiroSuda added this to the v1.1.0 milestone Feb 1, 2023
@AkihiroSuda
Copy link
Member

@thaJeztah Can we merge this?

@hqhq hqhq merged commit 7301c34 into opencontainers:main Feb 1, 2023
@AkihiroSuda AkihiroSuda mentioned this pull request Mar 28, 2023
Comment on lines +257 to +261
"timeOffsets": {
"type": "object",
"additionalProperties": {
"$ref": "defs-linux.json#/definitions/TimeOffsets"
}
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't seem correct.

I've opened a PR: #1193

@AkihiroSuda
Copy link
Member

@KentaTada Do you plan to open a PR for runc (or other runtimes)?

@KentaTada
Copy link
Contributor Author

Hello @AkihiroSuda san

Recently, I don't have enough resources to focus on the container runtime.
But @chethanah and team members are interested in implementing time namespace in runc.
If it is ok to send the PR to runc instead of me, I'll ask him to take over my activity.

@KentaTada
Copy link
Contributor Author

@chethanah san

Especially, I have no idea about the special-handling case for time namespace.
For example, when it comes to IPC namespace, it affects the changeable kernel parameters(fs.mqueue.*)
https://github.com/opencontainers/runc/blob/v1.1.7/libcontainer/configs/validate/validator.go#L184
So we need to investigate the properties related with time namespace at first.
Could you create the new issue in runc? If it is difficult or takes a lot of time, please feel free to ask me in the issue's page.

@AkihiroSuda
Copy link
Member

issue in runc

@utam0k
Copy link
Member

utam0k commented May 11, 2023

👋 Hi @KentaTada @chanezon FYI: crun's implementation containers/crun#1184

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.

8 participants