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

image: core -- Reserve start_time field #905

Merged
merged 1 commit into from
Jan 14, 2020

Conversation

valeriyvdovin
Copy link
Contributor

@valeriyvdovin valeriyvdovin commented Jan 10, 2020

To ensure consistency of runtime environment processes within a container need to see same start time values over suspend/resume cycles. We introduce new field to the core image structure to store start time of a dumped process. Later same value would be restored to a newly created task. In future the feature is likely to be pulled here, so we reserve field id in the core's protobuf descriptor.

@adrianreber
Copy link
Member

You need to sign off your patches like described at https://criu.org/How_to_submit_patches#Sign_your_work

And a bit more details why this change, even as simple as it is, is needed.

To ensure consistency of runtime environment processes within a
container need to see same start time values over suspend/resume
cycles. We introduce new field to the core image structure to
store start time of a dumped process. Later same value would be
restored to a newly created task. In future the feature is likely
to be pulled here, so we reserve field id in protobuf descriptor.

Signed-off-by: Valeriy Vdovin <valeriy.vdovin@virtuozzo.com>
@0x7f454c46
Copy link
Member

@valeriyvdovin why do you want to reserve it?
The reason why tty_* fields were reserved is to make Openvz's CRIU version images compatible with the mainstream version. Is that the case in this PR?

@0x7f454c46
Copy link
Member

Ah, I've noticed your @virtuozzo.com address.
Not sure if we've seen any progress with tty features or even a roadmap when they would go in mainstream.
So, up to @avagin if he thinks it's worth to reserve that.

@mihalicyn
Copy link
Member

mihalicyn commented Jan 12, 2020

Related to #412 #296
It is the step to fix Oracle XE c/r

@valeriyvdovin
Copy link
Contributor Author

valeriyvdovin commented Jan 13, 2020

Ah, I've noticed your @virtuozzo.com address.
Not sure if we've seen any progress with tty features or even a roadmap when they would go in mainstream.
So, up to @avagin if he thinks it's worth to reserve that.

This 'start_time' field is rather straight-forward and is the most basic stat of a process and should be covered by criu's basic functionality, so in our vz criu we definitely want to support it to fix issues with processes that depend on correct value of this field and in the light of that I don't see any reason to not reserve it in mainline, so we could pull it here later

@Snorch
Copy link
Member

Snorch commented Jan 13, 2020

We've already had these problem in VZ7 criu, if we use some id in core image we have a risk that these same field id will be used in ms criu, and we won't to have compatible images between mscriu and vzcriu. Last time we've introduced task_core_entry_VZ730 =) https://src.openvz.org/projects/OVZ/repos/criu/browse/images/core.proto#27 And we don't want this pain again.

I'm not sure how much time it may take to implement start_time in mscriu, and how much time it will take to add proper interface for it in mskernel. But we can at least try implementing it in vzcriu and vzkernel first =).

@avagin avagin merged commit 2722495 into checkpoint-restore:criu-dev Jan 14, 2020
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.

7 participants