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 --file-locks checkpoint/restore option #12333

Merged
merged 2 commits into from
Nov 18, 2021

Conversation

rst0git
Copy link
Contributor

@rst0git rst0git commented Nov 17, 2021

What this PR does / why we need it:

CRIU supports checkpoint/restore of file locks. This feature is required to checkpoint/restore containers running applications such MySQL.

How to verify it?

sudo podman run --name some-mysql -e MYSQL_ROOT_PASSWORD=my-secret-pw -d mysql:latest

sudo podman container checkpoint --file-locks some-mysql

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 17, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 17, 2021
@TomSweeneyRedHat
Copy link
Member

Quick review LGTM, but you'll need to update the man pages with the new option before the CI will let this get through.

@rst0git rst0git force-pushed the file-locks branch 5 times, most recently from 95a637b to c421070 Compare November 17, 2021 19:50
Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

Very quick review: you need to remove -l and resubmit (or, alternately, skip if remote).

test/e2e/checkpoint_test.go Outdated Show resolved Hide resolved
test/e2e/checkpoint_test.go Outdated Show resolved Hide resolved
@rst0git rst0git force-pushed the file-locks branch 2 times, most recently from 6685bde to 2c29790 Compare November 17, 2021 22:11
@rst0git rst0git force-pushed the file-locks branch 2 times, most recently from c4b246b to c351d21 Compare November 17, 2021 22:18
Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

Some minor suggestions (on the tests only; I don't feel qualified to comment on the rest).

Please hold off on re-pushing: all of CI is currently broken. There's a fix in progress, but you may need to rebase on main once that's in. Sorry for the inconvenience.

test/e2e/checkpoint_test.go Outdated Show resolved Hide resolved
test/e2e/checkpoint_test.go Show resolved Hide resolved
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2021
@edsantiago
Copy link
Member

Flake is a combination of #12273 and #12306. Restarting.

@edsantiago
Copy link
Member

Other CI failures are real:

Running: podman [options] container restore e6b6a148e5d9abfe053ce8bc0d93d3e2fcef1790e4d4f2a7773ab691d65609f7
Error: OCI runtime error: restore: unrecognized option '--file-locks'

@rst0git
Copy link
Contributor Author

rst0git commented Nov 18, 2021

Other CI failures are real:

Running: podman [options] container restore e6b6a148e5d9abfe053ce8bc0d93d3e2fcef1790e4d4f2a7773ab691d65609f7
Error: OCI runtime error: restore: unrecognized option '--file-locks'

Thanks, this should be fixed now.

@edsantiago
Copy link
Member

LGTM. @containers/podman-maintainers PTAL

@umohnani8
Copy link
Member

LGTM

@edsantiago
Copy link
Member

Aw, phooey. @rst0git there's a merge conflict, looks like #12342

@rst0git
Copy link
Contributor Author

rst0git commented Nov 18, 2021

Aw, phooey. @rst0git there's a merge conflict, looks like #12342

Thanks, I've rebased the branch.

@mheon
Copy link
Member

mheon commented Nov 18, 2021

@adrianreber Mind doing a quick review?

Code LGTM

CRIU supports checkpoint/restore of file locks. This feature is
required to checkpoint/restore containers running applications
such as MySQL.

Signed-off-by: Radostin Stoyanov <radostin@redhat.com>
Signed-off-by: Radostin Stoyanov <radostin@redhat.com>
@rhatdan
Copy link
Member

rhatdan commented Nov 18, 2021

/approve
/lgtm
Thanks @rst0git

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 18, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 18, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, rhatdan, rst0git

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit b24110e into containers:main Nov 18, 2021
@adrianreber
Copy link
Collaborator

@adrianreber Mind doing a quick review?

Code LGTM

I am too late, but looks good. One thing, however, shouldn't there be a check if the runtime actually supports --file-locks? There is a PR referenced for crun 1.4, but currently it will fail with crun. Although I think there are a couple of things (pre-dump for example) were we do not have support in crun but only in runc.

@rst0git rst0git deleted the file-locks branch November 19, 2021 09:29
@rst0git
Copy link
Contributor Author

rst0git commented Nov 19, 2021

shouldn't there be a check if the runtime actually supports --file-locks?

We could check what is the runtime and its version and if it is an older version of crun we could:

  1. Display an error message to update crun and/or mention that users could add file-locks in /etc/criu/default.conf;
  2. Or generate a config file for CRIU and specify the option in this way.

I think there are a couple of things (pre-dump for example)

For --pre-checkpoint we could display appropriate error message, and I will look into adding pre-dump support in crun.

@adrianreber What do you think?

@rhatdan
Copy link
Member

rhatdan commented Nov 19, 2021

I am not crazy about adding assumptions like that to the code, since over time they will just become stale. That should probably be a distributions problem, in that it should ensure the OCI runtimes are new enough.

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants