Skip to content

Conversation

@kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Jan 27, 2022

This was introduced in an initial commit, back in the day when criu was
a highly experimental thing. Today it's not; most users who need it have
it packaged by their distro vendor.

The usual way to run a binary is to look it up in directories listed in
$PATH. This is flexible enough and allows for multiple scenarios (custom
binaries, extra binaries, etc.). This is the way criu should be run.

Make --criu a hidden option (thus removing it from help). Remove the
option from man pages, integration tests, etc. Remove all traces of
CriuPath from data structures.

Add a warning that --criu is ignored and will be removed.

This used to be part of #3316.

This was introduced in an initial commit, back in the day when criu was
a highly experimental thing. Today it's not; most users who need it have
it packaged by their distro vendor.

The usual way to run a binary is to look it up in directories listed in
$PATH. This is flexible enough and allows for multiple scenarios (custom
binaries, extra binaries, etc.). This is the way criu should be run.

Make --criu a hidden option (thus removing it from help). Remove the
option from man pages, integration tests, etc. Remove all traces of
CriuPath from data structures.

Add a warning that --criu is ignored and will be removed.

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Contributor Author

This should be done 5 years ago but I guess better late than never.

runc also uses ps, busctl, systemctl, newuidmap/newgidmap etc, but do not provide any options to configure those -- $PATH is sufficient and flexible enough.


## [Unreleased]

### Deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Can be another PR, but maybe we should have docs/deprecation.md in addition to docs/experimental.md ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. It seems easy enough to search CHANGELOG.md for eprecat.

@kolyshkin
Copy link
Contributor Author

kolyshkin commented Jan 27, 2022

Now, an alternative would be to honor --criu but warn that it will be removed. I'd rather not do it, because:

  • most probably no one is using this option;
  • whoever uses it can employ a quick and easy fix:
-runc --criu /some/custom/criu/dir/criu ...
+PATH=/some/custom/criu/dir:$PATH runc

(or a two-liner if criu binary is not named criu).

The only question is, should using --criu result in an error (rather than a warning).

@kolyshkin kolyshkin mentioned this pull request Jan 27, 2022
7 tasks
@rhatdan
Copy link
Contributor

rhatdan commented Jan 27, 2022

LGTM

@kolyshkin kolyshkin mentioned this pull request Jan 27, 2022
@thaJeztah
Copy link
Member

I guess this is fine, as the flag is still present (so things won't hard-fail).

I stumbled upon this option in containerd, which (I think) may be using this flag; https://github.com/containerd/containerd/blob/77d53d2d230c3bcd3f02e6f493019a72905c875b/cmd/containerd-shim/main_unix.go#L60

Originally added in containerd/containerd@ab0cb4e, and I can see the --criu flag being set in containerd/go-runc; https://github.com/containerd/go-runc/blob/1c99ade6c04ed41bc4c9a8ac12b81b5fd4e14bcc/runc.go#L730-L732

So that code probably should be removed, and the option deprecated on containerd's side as well, @AkihiroSuda

@thaJeztah
Copy link
Member

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

@AkihiroSuda @cyphar good to go?

@kolyshkin
Copy link
Contributor Author

@adrianreber PTAL

@AkihiroSuda AkihiroSuda merged commit e9190d3 into opencontainers:main Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants