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

Upgrade go-criu to v6 #15591

Merged
merged 1 commit into from
Nov 4, 2022
Merged

Conversation

snprajwal
Copy link
Contributor

@snprajwal snprajwal commented Sep 1, 2022

The v6.0.0 release of go-criu has deprecated the rpc and stats
packages in favour of the crit package. This commit provides the
changes required to use this version in podman.

Signed-off-by: Prajwal S N prajwalnadig21@gmail.com

None

@openshift-ci openshift-ci bot added do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None release-note-none and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Sep 1, 2022
@mheon
Copy link
Member

mheon commented Sep 1, 2022

Looks like you have build issues, but changes LGTM otherwise

@snprajwal
Copy link
Contributor Author

snprajwal commented Sep 1, 2022

Looks like you have build issues

There's a namespace conflict between the proto files from another dependency. I'll get this repaired and working shortly.

@TomSweeneyRedHat
Copy link
Member

LGTM
assuming happy tests

@snprajwal snprajwal marked this pull request as draft September 4, 2022 12:05
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 4, 2022
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 6, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2022
@snprajwal snprajwal marked this pull request as ready for review September 17, 2022 10:09
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 17, 2022
@snprajwal
Copy link
Contributor Author

@mheon @TomSweeneyRedHat would it be possible to override the CI constraint for binary deltas over 51200B? The remaining tests are passing :)

Copy link
Contributor

@rst0git rst0git left a comment

Choose a reason for hiding this comment

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

LGTM

@mheon
Copy link
Member

mheon commented Sep 19, 2022

@baude @Luap99 Looks like ~1mb of growth. I'm OK with merging, but you both have stronger opinions on this than I do.

@Luap99
Copy link
Member

Luap99 commented Sep 19, 2022

Eh, why exactly do we need this? +1 MB for no new features?
It looks like the dependency structure for go-criu/v6 is bad. It pulls in way to much because of the stupid protobuf code generation, one of the reason we dropped the k8s dep.

@rhatdan
Copy link
Member

rhatdan commented Sep 19, 2022

Can we work around this issue. One of the things Automotive wants out of us is guarantees that we prevent the start up time of Podman from going up. Each time it gets fatter, the total time of starting the podman command slows down. Automotive has a requirement to start container from boot in < 2 seconds.

@rst0git
Copy link
Contributor

rst0git commented Sep 19, 2022

why exactly do we need this? +1 MB for no new features?

In go-criu version 6.X we introduced support for image processing functionality (crit) and we plan to extend the podman inspect command with support for checkpoint container images. This functionality also replaces the existing stats API and would also allow us to enable pre-copy container migration.

@rhatdan
Copy link
Member

rhatdan commented Sep 19, 2022

Can you do something about the protobuf problem @Luap99 mentions?

@snprajwal
Copy link
Contributor Author

Can you do something about the protobuf problem @Luap99 mentions?

Although we do not need all the bindings for the stats API, they will be required to extend podman inspect for checkpointed images.

@rst0git
Copy link
Contributor

rst0git commented Sep 19, 2022

Can you do something about the protobuf problem @Luap99 mentions?

We briefly discussed this problem in checkpoint-restore/go-criu#95 (comment), but so far this is the best solution we have.

@adrianreber
Copy link
Collaborator

I agree over 1MB increase of binary size for just moving code around does not sound right. It is not clear why the binary size increases so much as non of the imported protobuf files are actually used. We need to structure go-criu differently.

@snprajwal
Copy link
Contributor Author

It is not clear why the binary size increases so much as non of the imported protobuf files are actually used.

Since all Go programs are statically linked, the Go compiler includes the entire library, even if just a single function is being used (in this case, crit/images). On the other hand, if we add a bunch of new features that use the same libraries, the size increase will be in the order of a few bytes, since nothing needs to be included from the imports.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2022
@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

@rst0git
Copy link
Contributor

rst0git commented Oct 31, 2022

@snprajwal Would you be able to update this pull request with go-criu version 6.3.0 (e.g., a71a19e)?
This version should be backwards compatible with v5 and therefore the size of the podman binary shouldn't change.

@github-actions github-actions bot removed the stale-pr label Nov 1, 2022
Signed-off-by: Prajwal S N <prajwalnadig21@gmail.com>
@rst0git
Copy link
Contributor

rst0git commented Nov 4, 2022

@Luap99 @mheon @rhatdan PTAL

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing this

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 4, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, rst0git, snprajwal

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-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2022
@rhatdan
Copy link
Member

rhatdan commented Nov 4, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2022
@openshift-merge-robot openshift-merge-robot merged commit afc8415 into containers:main Nov 4, 2022
@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 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 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. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants