Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Terminate execution for criu that does not meet version requirements #4431

Closed
wants to merge 2 commits into from
Closed

Conversation

yangzhao02
Copy link
Contributor

This pull request is used to solve When I use runc and criu 3.9, it crashed . When the criu version does not meet the requirements, execution will be terminated directly to prevent unpredictable behavior.

@lifubang
Copy link
Member

lifubang commented Oct 9, 2024

Thanks, could you have a test case?

@@ -186,7 +186,7 @@ func criuNsToKey(t configs.NamespaceType) string {

func (c *Container) handleCheckpointingExternalNamespaces(rpcOpts *criurpc.CriuOpts, t configs.NamespaceType) error {
if !c.criuSupportsExtNS(t) {
return nil
return fmt.Errorf("criu version does not meet the requirements")
Copy link
Contributor

@kolyshkin kolyshkin Oct 9, 2024

Choose a reason for hiding this comment

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

Maybe something more specific, like

return fmt.Errorf("criu lacks support for external %s namespace (old criu version?)", configs.NsName(t))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's better. I will change it later.

@kolyshkin
Copy link
Contributor

Thanks, could you have a test case?

It's hard to add a test as it would require installing some old criu version.

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

Should we do the same in handleRestoringExternalNamespaces?

Signed-off-by: yangzhao.hjh <yangzhao.hjh@alibaba-inc.com>
@yangzhao02
Copy link
Contributor Author

Should we do the same in handleRestoringExternalNamespaces?

Good idea, there is the same problem in handleRestoringExternalNamespaces. And I have do the same in handleRestoringExternalNamespaces.

@kolyshkin
Copy link
Contributor

CI failures appear to be flakes (filed #4437). Failed CI jobs restarted.

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

@yangzhao02 please avoid adding merge commits (use git pull --rebase to rebase your working tree)

@yangzhao02
Copy link
Contributor Author

This pull request has been moved to #4440

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants