Skip to content

runtime: deprecate runc --criu / -criu-path option#6496

Merged
estesp merged 1 commit intocontainerd:mainfrom
thaJeztah:deprecate_criu_opt
Mar 23, 2022
Merged

runtime: deprecate runc --criu / -criu-path option#6496
estesp merged 1 commit intocontainerd:mainfrom
thaJeztah:deprecate_criu_opt

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

relates to:

runc option --criu is now ignored (with a warning), and the option will be
removed entirely in a future release. Users who need a non- standard criu
binary should rely on the standard way of looking up binaries in $PATH.

@thaJeztah
Copy link
Copy Markdown
Member Author

FWIW, I noticed various Import gogoproto/gogo.proto is unused warnings when running make generate; not sure if changes are needed?

make generate
+ bin/protoc-gen-gogoctrd
+ protos
github.com/containerd/containerd/api/events/container.proto:22:1: warning: Import gogoproto/gogo.proto is unused.
github.com/containerd/containerd/api/events/namespace.proto:21:1: warning: Import gogoproto/gogo.proto is unused.
github.com/containerd/containerd/api/services/diff/v1/diff.proto:21:1: warning: Import gogoproto/gogo.proto is unused.
github.com/containerd/containerd/api/services/version/v1/version.proto:22:1: warning: Import gogoproto/gogo.proto is unused.
github.com/containerd/containerd/api/types/mount.proto:21:1: warning: Import gogoproto/gogo.proto is unused.
github.com/containerd/containerd/runtime/linux/runctypes/runc.proto:5:1: warning: Import gogoproto/gogo.proto is unused.
github.com/containerd/containerd/runtime/v2/runc/options/oci.proto:5:1: warning: Import gogoproto/gogo.proto is unused.
+ generate

@kzys
Copy link
Copy Markdown
Member

kzys commented Jan 31, 2022

@thaJeztah That's odd. I have touched a bunch of protoc/protobuild stuff, but these changes shouldn't be reached to containerd automatically.

flag.StringVar(&workdirFlag, "workdir", "", "path used to storge large temporary data")
flag.StringVar(&runtimeRootFlag, "runtime-root", process.RuncRoot, "root directory for the runtime")
flag.StringVar(&criuFlag, "criu", "", "path to criu binary")
flag.StringVar(&criuFlag, "criu", "", "path to criu binary (deprecated: do not use)")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The entire shim v1 itself is already deprecated, so maybe this message isn't necessary

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, I doubt many people will use it manually from the command-line, but when running the command, it's not printed that it's deprecated, and I think it's useful to at least make it visible that the option itself should not be used?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The v1 shim is deprecated but working. The flag is deprecated and it doesn't work (ignored). So I'd like to keep the message.

@thaJeztah
Copy link
Copy Markdown
Member Author

@AkihiroSuda @crosbymichael ptal

Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@kzys
Copy link
Copy Markdown
Member

kzys commented Mar 8, 2022

@AkihiroSuda Looks good to merge?

@AkihiroSuda
Copy link
Copy Markdown
Member

Needs rebase

@kzys
Copy link
Copy Markdown
Member

kzys commented Mar 8, 2022

You are right. I was assuming that GitHub shows a list of conflicted files when it needs rebase. Where does that go? :(

@thaJeztah
Copy link
Copy Markdown
Member Author

You are right. I was assuming that GitHub shows a list of conflicted files when it needs rebase. Where does that go? :(

I think it performs that check "lazily" (so when you open the page), and it sometimes takes some time to show up.

Would be great if they did so "pro-actively" and added a visible warning in overviews (and possibly allowing to filter on it (etc))

I'll try to find some time to rebase tomorrow

@kolyshkin
Copy link
Copy Markdown
Contributor

@thaJeztah can you please rebase this?

runc option --criu is now ignored (with a warning), and the option will be
removed entirely in a future release. Users who need a non- standard criu
binary should rely on the standard way of looking up binaries in $PATH.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the deprecate_criu_opt branch from 598b386 to d2013d2 Compare March 23, 2022 13:43
@thaJeztah
Copy link
Copy Markdown
Member Author

Rebased 👍

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Mar 23, 2022

Build succeeded.

@thaJeztah thaJeztah closed this Mar 23, 2022
@thaJeztah thaJeztah reopened this Mar 23, 2022
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Mar 23, 2022

Build succeeded.

@estesp estesp merged commit 36dcc76 into containerd:main Mar 23, 2022
@thaJeztah thaJeztah deleted the deprecate_criu_opt branch March 23, 2022 15:50
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