Test Results

55 tests  ±0   55 ✅ ±0   1s ⏱️ ±0s
 1 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 1f4c74c. ± Comparison against base commit 6c3a263.

♻️ This comment has been updated with latest results.

I will add test cases gradually.

Thanks for this PR. It looks like the right approach. Unfortunately, now that the PR exists, I am no longer sure this is right thing to do.

This PR pulls in so many dependencies (over 3000 files), that maybe this was not a good idea after all.

@adrianreber yes I noticed that also, I am not getting any solution of this problem right now, do you have any advise for me ?

This PR pulls in so many dependencies (over 3000 files), that maybe this was not a good idea after all.

I am not getting any solution of this problem right now, do you have any advise for me ?

It is definitely not a good idea to add all these dependencies. A much simpler solution would be to use something like exec.Command() and run the buildah binary instead.

This PR introduce a new command checkpointctl convert

What about using checkpointctl build instead of "convert" as it creates an OCI image similar to podman build and docker build?

Not sure about exec. Maybe it should just be a shell script. Not sure.

Not sure about exec. Maybe it should just be a shell script. Not sure.

If we use shell script, then where should we store it(maybe in $HOME/.checkpointctl/scripts) ?
and I don't think embedding the script into the go code as string and then writing them out to temporary files or executing them directlty from memory is a good idea.

No, if we offer a shell script it should follow what make install does.

It could also be somewhere in libexec and then be called by exec as suggested by Radostin.

Now if we use script, we have to ensure that user have buildah installed or we can add buildah installation as a part of checkpointctl installation.

Now if we use script, we have to ensure that user have buildah installed or we can add buildah installation as a part of checkpointctl installation.

Yes, we will document it and the actual dependencies is up to the package managers in the distributions. The script can also check for buildah and exit with an error message if it is not there.

@adrianreber @rst0git PTAL, If there is no major refactoring required, I can start working on tests.

Why does the PR now still add over 16000 lines of code? Something is not right.

Why does the PR now still add over 16000 lines of code? Something is not right.

yes I am checking that.

Codecov Report

Attention: Patch coverage is 8.62069% with 106 lines in your changes are missing coverage. Please review.

Project coverage is 72.81%. Comparing base (6c3a263) to head (8496613).

@@            Coverage Diff             @@
##             main     #125      +/-   ##
==========================================
- Coverage   78.73%   72.81%   -5.92%     
==========================================
  Files          11       13       +2     
  Lines        1260     1376     +116     
==========================================
+ Hits          992     1002      +10     
- Misses        201      307     +106     
  Partials       67       67              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Good first try. I must confess I am still unsure which direction we should go, but let's try and see where this leads us.

Please add your script to the shellcheck target and please also take a look at the tool shfmt and add it during GitHub Actions CI runs. I recently introduce shfmt in another project. You can take a look at what I did there: https://github.com/openhpc/ohpc/blob/3.x/tests/ci/Makefile#L74

For the destination directory of the script I am currently thinking about /usr/libexec/.... That seems to be Fedora specific, but the git package for example uses it also extensively for internal scripts on my system:

$ rpm -ql git | grep libexec
/usr/libexec/git-core/git-contacts
/usr/libexec/git-core/git-credential-netrc
/usr/libexec/git-core/git-filter-branch
/usr/libexec/git-core/git-request-pull

We need to figure out what other distributions are using for internal scripts.

I would also say, instead of container engine specific annotations, which are almost not used at all (the only important one if name for CRI-O at least, but we can also change that), let's own the annotations in this package. We should also move the annotation definition to lib, so that all projects reading the annotations can easily import them from checkpointctl. Maybe start almost all annotation with checkpointctl.annotation.

@rst0git do you remember which annotation are actually used by Podman? CRI-O is just looking for io.kubernetes.cri-o.annotations.checkpoint.name.

We currently use the runtime name to identify an OCI image that contains checkpoint. This functionality needs be extended (in Podman and CRI-O) to verify the comparability of the system before running criu restore. For example, we need to make sure that CRIU has the same version or newer, the CPU architecture is the same, the kernel version is compatible, etc.

We currently use the runtime name to identify an OCI image that contains checkpoint. This functionality needs be extended (in Podman and CRI-O) to verify the comparability of the system before running criu restore. For example, we need to make sure that CRIU has the same version or newer, the CPU architecture is the same, the kernel version is compatible, etc.

Right. But are any of the annotations already verified today or is that just an idea for the future?

Right. But are any of the annotations already verified today or is that just an idea for the future?

In Podman we currently use only IsCheckpointImage to check annotations.

checkpointRuntimeName, found := imgData[i].Annotations[define.CheckpointAnnotationRuntimeName]
if !found {
	return false, fmt.Errorf("image is not a checkpoint: %s", imgID)
}
if hostInfo.Host.OCIRuntime.Name != checkpointRuntimeName {
	return false, fmt.Errorf("container image \"%s\" requires runtime: \"%s\"", imgID, checkpointRuntimeName)
}

I would also say, instead of container engine specific annotations, which are almost not used at all (the only important one if name for CRI-O at least, but we can also change that), let's own the annotations in this package. We should also move the annotation definition to lib, so that all projects reading the annotations can easily import them from checkpointctl. Maybe start almost all annotation with checkpointctl.annotation.

This is a good idea.
@Parthiba-Hazra would you like to open a separate pull request that adds the annotations from Podman and CRI-O? Once this PR is merged, we can update Podman and CRI-O to use the annotations from checkpointctl.

For the destination directory of the script I am currently thinking about /usr/libexec/.... That seems to be Fedora specific, but the git package for example uses it also extensively for internal scripts on my system:

$ rpm -ql git | grep libexec
/usr/libexec/git-core/git-contacts
/usr/libexec/git-core/git-credential-netrc
/usr/libexec/git-core/git-filter-branch
/usr/libexec/git-core/git-request-pull

We need to figure out what other distributions are using for internal scripts.

I thought /usr/libexec is debian specific cause somewhere I see that it can be present under usr/local/libexec, maybe both possible .
well let me check this.

@Parthiba-Hazra /usr/libexec is defined in the Filesystem Hierarchy Standard (FHS) as follows:

/usr/libexec includes internal binaries that are not intended to be executed directly by users or shell scripts. Applications may use a single subdirectory under /usr/libexec.

https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s07.html

/usr/local/ is used for local data that is specific to the host.

I would also say, instead of container engine specific annotations, which are almost not used at all (the only important one if name for CRI-O at least, but we can also change that), let's own the annotations in this package. We should also move the annotation definition to lib, so that all projects reading the annotations can easily import them from checkpointctl. Maybe start almost all annotation with checkpointctl.annotation.

This is a good idea. @Parthiba-Hazra would you like to open a separate pull request that adds the annotations from Podman and CRI-O? Once this PR is merged, we can update Podman and CRI-O to use the annotations from checkpointctl.

@rst0git @adrianreber do we need to keep those annotation's format unchanged or change it to similar what we are doing here(checkpointctl.annotation)?

I would also say, instead of container engine specific annotations, which are almost not used at all (the only important one if name for CRI-O at least, but we can also change that), let's own the annotations in this package. We should also move the annotation definition to lib, so that all projects reading the annotations can easily import them from checkpointctl. Maybe start almost all annotation with checkpointctl.annotation.

This is a good idea. @Parthiba-Hazra would you like to open a separate pull request that adds the annotations from Podman and CRI-O? Once this PR is merged, we can update Podman and CRI-O to use the annotations from checkpointctl.

@rst0git @adrianreber do we need to keep those annotation's format unchanged or change it to similar what we are doing here(checkpointctl.annotation)?

We talked a bit about it and we are not sure. Naming things is hard 😉 .

As this is all about CRIU I think we could go with something like org.criu.checkpointctl.name. I would not include annotation in the annotation.

I would also say, instead of container engine specific annotations, which are almost not used at all (the only important one if name for CRI-O at least, but we can also change that), let's own the annotations in this package. We should also move the annotation definition to lib, so that all projects reading the annotations can easily import them from checkpointctl. Maybe start almost all annotation with checkpointctl.annotation.

This is a good idea. @Parthiba-Hazra would you like to open a separate pull request that adds the annotations from Podman and CRI-O? Once this PR is merged, we can update Podman and CRI-O to use the annotations from checkpointctl.

@rst0git @adrianreber do we need to keep those annotation's format unchanged or change it to similar what we are doing here(checkpointctl.annotation)?

We talked a bit about it and we are not sure. Naming things is hard 😉 .

As this is all about CRIU I think we could go with something like org.criu.checkpointctl.name. I would not include annotation in the annotation.

yes, this looks way better and reasonable - org.criu.checkpointctl.name.

@adrianreber @rst0git Any update on the annotation's name? Is this the final format - org.criu.checkpointctl.checkpoint.name?

Also, shouldn't this annotation's name be org.criu.checkpointctl.checkpoint.container instead of checkpoint.name?

        // CheckpointAnnotationName is used when creating an OCI image
	// from a checkpoint archive to specify the name of the checkpoint.
	CheckpointAnnotationName = "checkpointctl.annotation.checkpoint.name"

Any update on the annotation's name? Is this the final format - org.criu.checkpointctl.checkpoint.name?

@Parthiba-Hazra If you open another pull request with the updated annotations, we can discuss the format there.