KEP-2008: Graduate "Forensic Container Checkpointing" to Beta

[adrianreber]

As defined in the existing KEP the steps to graduate from Alpha to Beta are

At least one container engine has to have implemented the
corresponding CRI APIs to introduce e2e test for checkpointing.

• Enable the feature per default
• No major bugs reported in the previous cycle

CRI-O implemented the corresponding CRI RPC and no major bugs have been reported since the initial release in 1.25.

• One-line PR description: Graduate "Forensic Container Checkpointing" to Beta

• Issue link: Forensic Container Checkpointing #2008

[k8s-ci-robot]

Hi @adrianreber. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[mikebrow]

a. should we move beyond forensics use case(s) before moving to beta? b. should we add scenarios for managing security/encryption (contents include live memory in container..) c. how do we manage the checkpoints (rm/gc) d. do we enable use cases that require restore first?

https://github.com/kubernetes/enhancements/pull/4288/files#diff-240948f6b9e24b79601915d2508930149c894411df0d623ac8c01c46d9cc57eaR87-R92

[bart0sh]

/ok-to-test

[kannon92]

a. should we move beyond forensics use case(s) before moving to beta? b. should we add scenarios for managing security/encryption (contents include live memory in container..) c. how do we manage the checkpoints (rm/gc) d. do we enable use cases that require restore first?

https://github.com/kubernetes/enhancements/pull/4288/files#diff-240948f6b9e24b79601915d2508930149c894411df0d623ac8c01c46d9cc57eaR87-R92

So @mrunalp and I discussed this.

I think that the scope of this KEP will drastically change if we try to support restore as part of this KEP. There is a lot of interest in restore but I think there needs to be a different design for that case. I know there were security implications and it warrants a discussion. Could we consider promoting this KEP to beta (with gc questions answered)? And draft a new KEP to cover checkpoint/restore in more details?

For promotion to beta, I think we should answer the question how do we gc checkpoints and how is storage monitored for it?

One suggestion we have is to maybe consider an operator for managing checkpoints rather than putting it in upstream kubelet.

[kannon92]

@adrianreber Where are we on garbage collecting old container checkpoints?

[adrianreber]

@adrianreber Where are we on garbage collecting old container checkpoints?

I understood it that it should be done in an operator. That sounded to me like it might be independent of this PR.

[kannon92]

@adrianreber Where are we on garbage collecting old container checkpoints?

I understood it that it should be done in an operator. That sounded to me like it might be independent of this PR.

So even if its done in an operator, would we require mention of the operator? My main concern is that kubelet is not going to monitor checkpoints and if they fill up the disk then we effectively take down the node. I don't think we want gc in the main repo (as this is an optional feature) but we may want to have some kind of suggestion or operator that people could use?

We could document this as a known issue and suggest ways to mitigate it if your disk fills up due to checkpoints.

cc @mrunalp @SergeyKanzhelev

[adrianreber]

@kannon92 I added a short paragraph concerning garbage collection and a possible operator. Something like that?

[kannon92]

@adrianreber the other thing that is crucial for beta is to fill out the PRR questions in this KEP. I think you are missing quite a few since this feature was created.

I think everyone one of this questions needs a response to push to beta.

https://github.com/kubernetes/enhancements/tree/master/keps/NNNN-kep-template#production-readiness-review-questionnaire

[adrianreber]

I created an operator which replicates the functionality I tried to bring into the kubelet: https://github.com/checkpoint-restore/checkpoint-restore-operator

At this point it is really simple. It has a parameter to define the maximum number of checkpoints for a container and if more checkpoints are created older checkpoints are deleted.

Maybe we could add a function to eviction manager in case of disk pressure to remove all checkpoints (if feature is enabled).

At first I liked the idea, but unconditionally deleting all checkpoint archives seems a bit harsh and maybe unexpected. Which would mean we need some logic to decide which checkpoint archive to keep. Not sure we want that. If people agree that this would be a good idea I am happy to implement it. At this point I am not 100% convinced it should be done.

@adrianreber the other thing that is crucial for beta is to fill out the PRR questions in this KEP. I think you are missing quite a few since this feature was created.

Thanks for pointing that out. I will look into that.

[deads2k]

Several PRR sections are missing: https://github.com/kubernetes/enhancements/blame/master/keps/NNNN-kep-template/README.md#L454

Please copy/paste the new template and fill out all the alpha and beta questions.

[adrianreber]

@kannon92 Again, thanks a lot for your suggestions. Makes it easy for me to finally understand how this needs to work. It also makes updating the text in the PR easy. Thanks.

[deads2k]

These two need updates to indicate that the kubelet will add (or does have) metrics for the kubelet endpoints indicating usage counts and failure counts. Prior to going to beta, the exact metric names must be added.

[adrianreber]

Reworded as suggested

[adrianreber]

@deads2k Reworked based on your suggestions.

[kannon92]

/lgtm
/assign @deads2k @mrunalp

[deads2k]

PRR is complete for beta.

/approve

[dims]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianreber, deads2k, dims

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:

• keps/prod-readiness/OWNERS [deads2k]
• keps/sig-node/OWNERS [dims]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mikebrow]

LGTM.. would like to talk about documenting how an admin can direct these checkpoints to a secure/encrypted mount the keys for which can be managed by another party. Will be watching for the changes and help get it into containerd cheers!

[rst0git]

would like to talk about documenting how an admin can direct these checkpoints to a secure/encrypted mount the keys for which can be managed by another party.

@mikebrow we are currently working on enabling built-in support for encryption in CRIU (checkpoint-restore/criu#2297). In the current implementation, we use a certificate with a public key to encrypt the content of the checkpoint.

[adrianreber]

I want to thank everyone involved for all the feedback and quick turnaround to get this merged in time. Thanks!