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

feat: Introduced storage/size based global, container, pod, and namespace-level policies #6

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Parthiba-Hazra
Copy link
Contributor

@Parthiba-Hazra Parthiba-Hazra commented Jun 24, 2024

Introduced storage/size based global, container, pod, and namespace-level policies for checkpoint retention.

@snprajwal
Copy link
Member

I noticed that a lot of the code in the controller was repetitive, barring slight differences (container vs pod vs namespace). I think it would make sense for us to generate the file instead of writing it by hand, both to preserve developer sanity and to enable easier verification of the behaviour. In go-criu, we generate the bindings for CRIU's magic values here. Maybe something similar can be used here? Also, I would pick Python over Go for this, since it's just simpler.

@Parthiba-Hazra Parthiba-Hazra force-pushed the storage-quota branch 17 times, most recently from 63947e4 to 3e1543a Compare August 6, 2024 17:22
@Parthiba-Hazra Parthiba-Hazra force-pushed the storage-quota branch 4 times, most recently from 5f3bc75 to 7a799f3 Compare August 10, 2024 05:38
@Parthiba-Hazra Parthiba-Hazra changed the title Storage quota feat: Introduced storage/size based global, container, pod, and namespace-level policies Aug 10, 2024
@Parthiba-Hazra Parthiba-Hazra marked this pull request as ready for review August 10, 2024 06:00
@rst0git
Copy link
Member

rst0git commented Aug 13, 2024

@Parthiba-Hazra Would you be able to rebase this pull request on the main branch?

@Parthiba-Hazra Parthiba-Hazra force-pushed the storage-quota branch 3 times, most recently from 0f4f3aa to 8c53d87 Compare August 14, 2024 07:33
# containerPolicies:
# - namespace: <namespace>
# pod: <pod_name>
# container: <container_name>
# maxCheckpoints: 5
# maxCheckpointSize: 10
# maxTotalSize: 100
Copy link
Member

Choose a reason for hiding this comment

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

It might be better to use format similar to other resource limits in Kubernetes:

https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/#meaning-of-memory

Limits and requests for memory are measured in bytes. You can express memory as a plain integer or as a fixed-point number using one of these quantity suffixes: E, P, T, G, M, k. You can also use the power-of-two equivalents: Ei, Pi, Ti, Gi, Mi, Ki.

- Introduced global, container, pod, and namespace-level
  policies for checkpoint retention, based on storage/size limits.
- Updated CRD definitions to store the storage/size based policies.
- Updated the sample configuration of CheckpointRestoreOperator with
  storage/checkpoint-size based policies

Signed-off-by: Parthiba-Hazra <[email protected]>
- Enhance generate_checkpoint_tar.sh to optionally
  generate tar files larger than 5MB
- Update GitHub Actions workflow to test storage quota
  garbage collection policies

Signed-off-by: Parthiba-Hazra <[email protected]>
…ntions

- Updated size policies from int-based values to resource.Quantity
  for consistency with Kubernetes resource limits.
- Enabled the use of units such as Ki, Mi, Gi for max checkpoint
  size and total size policies.

Signed-off-by: Parthiba-Hazra <[email protected]>
Comment on lines +66 to +69
maxCheckpointSize resource.Quantity = resource.MustParse("1Ei")
maxTotalSizePerPod resource.Quantity = resource.MustParse("1Ei")
maxTotalSizePerContainer resource.Quantity = resource.MustParse("1Ei")
maxTotalSizePerNamespace resource.Quantity = resource.MustParse("1Ei")
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if 1 exabyte is a very sane default value. We could potentially restrict this to something a little more reasonable, like 100 gigabytes or so? cc @rst0git

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.

3 participants