Support disk image snapshots

[mattcary]

I believe the right documentation is https://cloud.google.com/compute/docs/images/create-delete-deprecate-private-images for creating, and https://cloud.google.com/compute/docs/disks/create-disk-from-source.

To create the disk image, a parameter could be added to the VolumeSnapshotClass. For restoring, the existing volume cloning support (create from VolumeSource) may just work, or additional plumbing may be required.

It's also necessary to look into the zonal vs regional behavior of disk images and see if anything needs to be done around that.

[luohao]

@mattcary I think the following changes need to be made to enable disk image snapshots:

• add disk image create/delete/list/get function to CloudProvider
• add a parameter to VolumeSnapshotClass and update CreateSnapshot() in CSI driver to create snapshots/images based on the VolumeSnapshotClass config
• update InsertDisk() in CloudProvider to check snapshot type and set source appropriately in the disk creation request. The existing VolumeSource is sufficient to support clones from images and we can tell what the source is from the snapshot ID.
• update ListSnapshots() in CSI driver to query both images and snapshots and concatenate the results.

I have only found regional/multi-regional disk images in the doc and the only difference is the storage locations(e.g., ['us-central-1'] v.s. ['us']. I think no extra change is required to support both.

I have two questions:

• the ListSnapshots API in CSI driver returns nextToken for pagination. Since we list the snapshots from two sources(snapshots and images), should we concatenate the results and return our own token? Is there a better way of implementing it?
• I am adding tests(unit, sanity, e2e). Is there a test infra that I can use to run e2e tests before I post the PR? I looked at testgrid and it says tests are triggered after you create pull request.

[mattcary]

- add disk image *create/delete/list/get* function to CloudProvider - add a parameter to VolumeSnapshotClass and update CreateSnapshot() in CSI driver to create snapshots/images based on the VolumeSnapshotClass config - update InsertDisk() in CloudProvider to check snapshot type and set source appropriately in the disk creation request. The existing VolumeSource is sufficient to support clones from images and we can tell what the source is from the snapshot ID. - update ListSnapshots() in CSI driver to query both images and snapshots and concatenate the results. That looks reasonable.

I have only found regional/multi-regional disk images in the doc and the

only difference is the storage locations(e.g., ['us-central-1'] v.s. ['us']. I think no extra change is required to support both. That makes sense, thanks for looking into it. I have two questions: - the ListSnapshots API in CSI driver returns nextToken for pagination. Since we list the snapshots from two sources(snapshots and images), should we concatenate the results and return our own token? Is there a better way of implementing it? I think that's fine. See for example here

<https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/blob/master/pkg/gce-cloud-provider/compute/gce-compute.go#L104> where we drop the page token entirely and just concatenate all responses into a single list. That might work too. Can you check the call sites of ListSnapshots to see if they just always concatencate the pages together immediately? If that's so we may as well drop the pagination like we do in ListDisks.

- I am adding tests(unit, sanity, e2e). Is there a test infra that I can use to run e2e tests before I post the PR? I looked at testgrid and it says tests are triggered after you create pull request. To run the e2e tests manually you'd need an account that can create

basically arbitrary gce resources; I think that might be hard to get to you. If the iteration isn't too slow for you it might be easiest to create a WIP PR and then manually run tests by putting /test in a comment (or just waiting after you push changes as they'll be run automatically by the presubmit). The e2e tests I think can just be run off of your workstation. The harder ones are the k8s-integration test (which, confusingly, run the k/k e2e test suite). We may want to add a config in the cloud-provider-gcp test config.... but that shouldn't block your PR. Let's get this implemented and we can worry about the integration tests later. Thank you again for your help on this! I feel guilty :-)

…

— Reply to this email directly, view it on GitHub <#924 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIJCBAE5S3ESMESITPNMIYLU6EKNLANCNFSM5PVJC2EQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: <kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/issues/924/1058429070 @github.com>

[luohao]

Thanks @mattcary for the pointers, they are super helpful.

I'll post a PR and we can iterate on that.

/assign @luohao

[luohao]

@mattcary I created a PR(#926), but the tests are not running. Looks like it needs verification from maintainers. Can you help with that?

[mattcary]

Sure thing. Hadn't gotten that far down in my inbox yet :-) Feel free to ping me on slack in the future if you need things like this moved forward.

…

On Thu, Mar 10, 2022 at 9:16 AM Hao Luo ***@***.***> wrote: @mattcary <https://github.com/mattcary> I created a PR(#926 <#926>), but the tests are not running. Looks like it needs verification from maintainers. Can you help with that? — Reply to this email directly, view it on GitHub <#924 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIJCBACIE67ZNT3WZZ6QRRTU7IU7HANCNFSM5PVJC2EQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: <kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/issues/924/1064304316 @github.com>

[mattcary]

implementation PR looks basically good, the next thing to think about after it gets submitted is getting support in k/k e2e tests (which is done here through test/k8s-integration). I think we can just add a new snapshot class to the test config, but it needs digging into.

[luohao]

Thanks @mattcary.

I have another patch that adds new test cases in e2e and integration tests. Should I push to the existing PR or create a new PR?

In my patch, I added a new snapshot class that doubles the test configs the test will run(and potentially double the time it runs). Would that be OK or do we need to just create a single test config for disk image snapshot?

[mattcary]

Let's do the e2e & integration tests in a new PR.

I think the new snapshot class that runs all the tests through disk images is the right thing to do---anyway, let's discuss in that PR so we can be concrete, I'm not quite sure what you mean by a single test config here.