-
Notifications
You must be signed in to change notification settings - Fork 554
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
Conversation
|
@KentaTada could you update the PR? |
827fc2b
to
324f30a
Compare
@AkihiroSuda @tianon |
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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
324f30a
to
5210ede
Compare
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 <[email protected]>
5210ede
to
36bb632
Compare
(Silly) questions;
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). |
I think windows is out of the scope of this PR, and windows support can be added in the |
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) |
@thaJeztah Can we merge this? |
"timeOffsets": { | ||
"type": "object", | ||
"additionalProperties": { | ||
"$ref": "defs-linux.json#/definitions/TimeOffsets" | ||
} |
There was a problem hiding this comment.
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
@KentaTada Do you plan to open a PR for runc (or other runtimes)? |
Hello @AkihiroSuda san Recently, I don't have enough resources to focus on the container runtime. |
@chethanah san Especially, I have no idea about the special-handling case for time namespace. |
|
👋 Hi @KentaTada @chanezon FYI: crun's implementation containers/crun#1184 |
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 [email protected]