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: add snapshot max count and size #2140

Merged
merged 3 commits into from
Jan 3, 2024

Conversation

FrankYang0529
Copy link
Contributor

@FrankYang0529 FrankYang0529 commented Sep 12, 2023

ref: longhorn/longhorn#6563

rely on:

Test Plan:

snapshot-max-count setting

  • Test the default value is 250.
  • Test its value should be integer and between 2 to 250.
  • Update the setting to 5 and create a Volume without snapshotMaxCount. The default snapshotMaxCount on the Volume will be 5.

snapshotMaxCount in Volume & updateSnapshotMaxCount action

  • Test we can't create a volume with snapshotMaxCount which is lower than 2 or more than 250.
  • Create a volume with snapshotMaxCount=3 and snapshotMaxSize=0.
  • Attach the volume with maintenance mode.
  • Create two snapshots.
  • Revert to the first snapshot. (volume-head will be on the second branch)
  • Create the third snapshot.
  • Revert to the first snapshot. (volume-head will be on the third branch)
  • Should not be able to create a new snapshot, because we already have 3 snapshots.
  • Use updateSnapshotMaxCount action to update snapshotMaxCount to 4.
  • Create the fourth snapshot.
  • Should not be able to use updateSnapshotMaxCount action to update snapshotMaxCount to 3, because we can't make snapshotMaxCount be smaller than current usage.
  • Delete a snapshot.
  • Use updateSnapshotMaxCount action to update snapshotMaxCount to 3.

snapshotMaxSize in Volume & updateSnapshotMaxSize action

  • Test we can't create a volume with snapshotMaxSize which is more than 0 and lower than 2 * volume.spec.size.
  • Create a volume with size=1073741824 (1G), snapshotMaxCount=250 and snapshotMaxSize=2357198848 (2.2G).
  • Create a related PV/PVC.
  • Use a pod to mount volume.
  • Create file1 with 950M in the volume.
  • Create the first snapshot.
  • Remove the file1 and create file2 with 950M in the volume.
  • Create the second snapshot.
  • Remove the file2 and create file3 with 950M in the volume.
  • Should not be able to create a new snapshot, because the total snapshot size will be more than 2.2G.
  • Use updateSnapshotMaxSize action to update snapshotMaxSize to 3430940672 (3.2G).
  • Create the third snapshot.
  • Should not be able to use updateSnapshotMaxSize action to update snapshotMaxSize to 2357198848 (2.2G), because we can't make snapshotMaxSize be smaller than current usage.
  • Delete a snapshot.
  • Use updateSnapshotMaxSize action to update snapshotMaxSize to 2357198848 (2.2G).

upgrade test

  • Create a volume in LH v1.5.x.
  • Upgrade to v1.6.0-dev and check there is no error on the volume.

SnapshotMaxCount int `json:"snapshotMaxCount"`
// +kubebuilder:validation:Type=string
// +optional
SnapshotMaxSize int64 `json:"snapshotMaxSize,string"`
Copy link
Member

Choose a reason for hiding this comment

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

Why kubebuilder:validation:Type=string instead of int?

Copy link
Contributor Author

@FrankYang0529 FrankYang0529 Dec 28, 2023

Choose a reason for hiding this comment

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

I followed VolumeSpec.Size. @derekbit, could you share the idea to use string validation? This was added from #1136. Thank you.

type VolumeSpec struct {
// +kubebuilder:validation:Type=string
// +optional
Size int64 `json:"size,string"`

controller/engine_controller.go Outdated Show resolved Hide resolved
engineapi/instance_manager.go Show resolved Hide resolved
webhook/resources/volume/validator.go Outdated Show resolved Hide resolved
webhook/resources/volume/validator.go Show resolved Hide resolved
webhook/resources/volume/validator.go Show resolved Hide resolved
webhook/resources/volume/validator.go Outdated Show resolved Hide resolved
Copy link

mergify bot commented Dec 28, 2023

This pull request is now in conflicts. Could you fix it @FrankYang0529? 🙏

Copy link

mergify bot commented Dec 28, 2023

This pull request is now in conflicts. Could you fix it @FrankYang0529? 🙏

Copy link

mergify bot commented Dec 29, 2023

This pull request is now in conflicts. Could you fix it @FrankYang0529? 🙏

Copy link
Member

@innobead innobead left a comment

Choose a reason for hiding this comment

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

A few comments.

controller/volume_controller.go Outdated Show resolved Hide resolved
webhook/resources/volume/mutator.go Outdated Show resolved Hide resolved
webhook/resources/volume/validator.go Show resolved Hide resolved
webhook/resources/volume/mutator.go Show resolved Hide resolved
Signed-off-by: PoAn Yang <poan.yang@suse.com>
Signed-off-by: PoAn Yang <poan.yang@suse.com>
Signed-off-by: PoAn Yang <poan.yang@suse.com>
@innobead innobead merged commit 30befe5 into longhorn:master Jan 3, 2024
5 checks passed
@FrankYang0529 FrankYang0529 deleted the LH-6563 branch January 4, 2024 00:13
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.

2 participants