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.

Sign up for GitHub

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

Jump to bottom

Expiry storage volume backups #12029

Merged
merged 5 commits into from
Jul 20, 2023
Merged

Expiry storage volume backups #12029

merged 5 commits into from
Jul 20, 2023

Conversation

monstermunchkin
Copy link
Contributor

This adds an hourly task which removes expired storage volume snapshots.

roosterfish
roosterfish reviewed Jul 17, 2023
View reviewed changes
Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

Looks good, just a question above.

lxd/backup.go Outdated Show resolved Hide resolved
@monstermunchkin monstermunchkin force-pushed the issues/12028-expired-storage-volume-backups branch from 1abf1aa to f0a461f Compare July 17, 2023 14:37
@tomponline
Copy link
Member

Can we have a test for this process?

@monstermunchkin
Copy link
Contributor Author

Can we have a test for this process?

So, we cannot have the test wait for an hour which is why we don't have tests for the instance backup removal either. But we could in theory start LXD, create a storage volume backup,restart LXD, and hope that the backup gets cleaned up.

@tomponline
Copy link
Member

Can we have a test for this process?

So, we cannot have the test wait for an hour which is why we don't have tests for the instance backup removal either. But we could in theory start LXD, create a storage volume backup,restart LXD, and hope that the backup gets cleaned up.

Yes I think that is what the other snapshot expiry tests do.

@monstermunchkin monstermunchkin force-pushed the issues/12028-expired-storage-volume-backups branch from f0a461f to 78c1524 Compare July 18, 2023 08:12
tomponline
tomponline reviewed Jul 18, 2023
View reviewed changes
lxd/db/backups.go Outdated Show resolved Hide resolved
tomponline
tomponline reviewed Jul 18, 2023
View reviewed changes
lxd/backup.go Outdated Show resolved Hide resolved
@monstermunchkin monstermunchkin force-pushed the issues/12028-expired-storage-volume-backups branch from 78c1524 to 67d375c Compare July 18, 2023 12:58
tomponline
tomponline reviewed Jul 18, 2023
View reviewed changes
lxd/db/backups.go Outdated Show resolved Hide resolved
tomponline
tomponline reviewed Jul 18, 2023
View reviewed changes
lxd/backup.go Outdated Show resolved Hide resolved
@monstermunchkin monstermunchkin force-pushed the issues/12028-expired-storage-volume-backups branch from 67d375c to 975bd13 Compare July 19, 2023 05:45
tomponline
tomponline approved these changes Jul 19, 2023
View reviewed changes
Copy link
Member

@tomponline tomponline 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

tomponline
tomponline requested changes Jul 19, 2023
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

Tests are failing though

@monstermunchkin
Copy link
Contributor Author

Tests are failing because GetStoragePoolVolumeWithID doesn't set StorageVolumeArgs.NodeID.

@monstermunchkin monstermunchkin force-pushed the issues/12028-expired-storage-volume-backups branch from 975bd13 to 1ffd204 Compare July 19, 2023 07:30
@tomponline
Copy link
Member

@monstermunchkin ceph tests failing

@tomponline
Copy link
Member

Amy idea what's causing ceph to fail?

@tomponline
Copy link
Member

Will need a rebase to fix curl in tests too

@monstermunchkin
Copy link
Contributor Author

I'm currently investigating the ceph issue.

@monstermunchkin
Copy link
Contributor Author

Ah, I know why this is failing. For ceph volumes, we set node_id to nil as the pool is not tied to any node.

@monstermunchkin monstermunchkin force-pushed the issues/12028-expired-storage-volume-backups branch from 1ffd204 to 77453dd Compare July 19, 2023 18:22
@monstermunchkin
lxd/db: Return nodeID in GetStoragePoolVolumeWithID
9f738f2 
Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>
@monstermunchkin
lxd/db: Add function to get expired storage volume backups
cdaa9e6 
Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>
@monstermunchkin
lxd: Add hourly job pruning expired storage volume backups
f28e9bd 
Fixes canonical#12028

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>
@monstermunchkin
operationtype: Change description of BackupsExpire
fec375c 
Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>
@monstermunchkin
test: Check backup volume expiry
63d8be8 
Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>
@monstermunchkin monstermunchkin force-pushed the issues/12028-expired-storage-volume-backups branch from 77453dd to 63d8be8 Compare July 20, 2023 05:50
@monstermunchkin
Copy link
Contributor Author

@tomponline I fixed the ceph issue, and all tests should be good now.

@tomponline tomponline merged commit fc1c870 into canonical:main Jul 20, 2023
24 of 25 checks passed
@tomponline
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomponline tomponline tomponline approved these changes

@roosterfish roosterfish roosterfish approved these changes

@gabrielmougard gabrielmougard Awaiting requested review from gabrielmougard

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@monstermunchkin @tomponline @roosterfish