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

add support for incremental snapshots of Azure disks #52

Merged
merged 1 commit into from
Jul 15, 2020

Conversation

stephanwehr
Copy link

@stephanwehr stephanwehr commented Jun 14, 2020

Closes #25

Passed the following test cases:

Test: Omit parameter "incremental"
Result: Full snapshot taken

Test: Set parameter "incremental" to "true"
Result: Incremental snapshot taken

Test: Set parameter "incremental" to "test value"
Result: Backup fails partially, no snapshot taken, error message in log

@stephanwehr stephanwehr force-pushed the support-incremental-snapshots branch from e0ebaab to ce50ac0 Compare June 14, 2020 14:12
@stephanwehr
Copy link
Author

signed off the commit

@ashish-amarnath
Copy link
Member

@stephanwehr Thank you for your PR.
Can you please confirm if you've been able to run this PR through any tests and tell us what those tests were?
I am trying to think how this should be tested apart from having no regressions in behavior.

@ashish-amarnath
Copy link
Member

closing and reopening to re-trigger DCO

@ashish-amarnath
Copy link
Member

That didn't seem to work.
Can you please follow the instructions from https://github.com/vmware-tanzu/velero-plugin-for-microsoft-azure/pull/52/checks?check_run_id=770044678 to sign your commits again? It may have something to do with a missing new-line before the Signed-off:...

Copy link
Member

@ashish-amarnath ashish-amarnath left a comment

Choose a reason for hiding this comment

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

Waiting on the testing info and DCO.

@stephanwehr stephanwehr force-pushed the support-incremental-snapshots branch from ce50ac0 to 3d038eb Compare June 24, 2020 09:08
@stephanwehr
Copy link
Author

stephanwehr commented Jun 24, 2020

Completed DCO

The tests mentioned in the 1st comment were done manually against an AKS cluster.
@Juhibhadviya19 Could you please confirm the behavior of the 3 test cases?

Test 1
Omit the parameter "incremental" inside VolumeSnapshotLocation:

spec:
  config:
    apiTimeout: 10m
    resourceGroup: myname-rg
    subscriptionId: 00000000-0000-0000-0000-000000000000

Create a backup manually or via schedule.
Expected result: Full snapshot taken

Test 2
Set the parameter "incremental" to "true" inside VolumeSnapshotLocation:

spec:
  config:
    apiTimeout: 10m
    incremental: "true"
    resourceGroup: myname-rg
    subscriptionId: 00000000-0000-0000-0000-000000000000

Create a backup manually or via schedule.
Expected result: Incremental snapshot taken

Test 3
Configure the Velero VolumeSnapshotLocation as follows:

spec:
  config:
    apiTimeout: 10m
    incremental: test value
    resourceGroup: myname-rg
    subscriptionId: 00000000-0000-0000-0000-000000000000

Create a backup, the backup should fail with status "PartiallyFailed (N error(s))"
Verify by running:
velero backup describe <name-of-the-failed-backup> --details
Expected output:

Phase: PartiallyFailed (run velero backup logs <name-of-the-failed-backup> for more information)
...
Velero-Native Snapshots: <none included>

To get the error message run:
velero backup logs <name-of-the-failed-backup>
Expected log message:

time="2020-06-24T09:56:02Z" level=error msg="Error getting volume snapshotter for volume snapshot location" backup=velero/<name-of-the-failed-backup> error="rpc error: code = Unknown desc = unable to parse value "test value" for config key "incremental" (expected a boolean value): strconv.ParseBool: parsing "test value": invalid syntax" error.file="/go/src/velero-plugin-for-microsoft-azure/velero-plugin-for-microsoft-azure/volume_snapshotter.go:142" error.function="main.(*VolumeSnapshotter).Init" logSource="pkg/backup/item_backupper.go:437" name=pvc-00000000-0000-0000-0000-000000000000 namespace= persistentVolume=pvc-00000000-0000-0000-0000-000000000000 resource=persistentvolumes volumeSnapshotLocation=default

Make sure no snapshot is taken if the backup fails by listing Azure resources.

Check if Velero can delete the failed backup by running:
velero delete backup <name-of-the-failed-backup>
The K8s resources backups should be deleted from the blob storage.

CAUTION: Backups created by schedules will fail if the parameter incremental is set to a non boolean value and need to be cleaned up by deleting the failed backups.

@Juhibhadviya19
Copy link

@stephanwehr All the 3 test cases behaved as mentioned by you

@skriss skriss removed their request for review July 6, 2020 16:18
Copy link
Member

@ashish-amarnath ashish-amarnath left a comment

Choose a reason for hiding this comment

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

@stephanwehr Thanks for putting up this PR and @Juhibhadviya19 thanks for the confirmation on the testing.
LGTM! 🚀

@nrb and @carlisia PTAL

Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

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

👍

@nrb
Copy link
Contributor

nrb commented Jul 15, 2020

@stephanwehr and @Juhibhadviya19 thanks for the testing!

When restoring from an incremental snapshot, is there any difference in usage, or does Azure do the association under the covers?

@ashish-amarnath
Copy link
Member

There is no difference in the usage. The Azure sdk takes care of this for us. From Velero's perspective, it is the usual velero restore command. One scenario I forgot to verify is:

  1. Create a backup with incremental snapshot enabled.
  2. Restore from that backup with incremental snapshotting disabled.

Is this a scenario that you were able to verify @Juhibhadviya19 ?

@ashish-amarnath
Copy link
Member

ashish-amarnath commented Jul 15, 2020

EDIT:
I think something is not right. Give me a few min to figure out what's going on.

False alarm. My bad. removing hold.

I was also able to run backup with incremental=true and restore with incremental=nil (same as incremental=false) and it works as expected and surprisingly seamlessly! 🎉

@ashish-amarnath
Copy link
Member

@nrb this is ready for merge, unless you have other questions or comments

@nrb
Copy link
Contributor

nrb commented Jul 15, 2020

Thanks for testing that all @ashish-amarnath!

@nrb nrb merged commit 55f3ba1 into vmware-tanzu:master Jul 15, 2020
@stephanwehr stephanwehr deleted the support-incremental-snapshots branch August 3, 2020 09:36
@Sathishkunisai
Copy link

Sathishkunisai commented Aug 4, 2020

thanks. tentative time for releasing Incremental backup support for AKS PVC, with the Release Label Please.

@ashish-amarnath
Copy link
Member

@Sathishkunisai We should have a release with this change end of august, at the same time as Velero 1.5.
However, you can start using this feature using the velero/velero-plugin-for-microsoft-azure:main image for the plugin.

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.

support incremental snapshots of Azure disks
6 participants