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 Fast Snapshot Restores #1554

Merged

Conversation

torredil
Copy link
Member

Is this a bug fix or adding new feature?

  • New feature.

What is this PR about? / Why do we need it?

  • This PR adds support for Fast Snapshot Restores via the fastSnapshotRestoreAvailabilityZones VolumeSnapshotClass parameter. Availability zones are specified as a comma-separated list.

Example

apiVersion: snapshot.storage.k8s.io/v1
kind: VolumeSnapshotClass
metadata:
  name: csi-aws-vsc
driver: ebs.csi.aws.com
deletionPolicy: Delete
parameters:
  fastSnapshotRestoreAvailabilityZones: "us-east-1a, us-east-1b"
  • The driver must be given permission to access the EnableFastSnapshotRestores EC2 API. This example snippet can be used in an IAM policy to grant access to EnableFastSnapshotRestores:
{
  "Effect": "Allow",
  "Action": [
    "ec2:EnableFastSnapshotRestores"
  ],
  "Resource": "*"
}

Failure Mode

  • The driver will attempt to check if the availability zones provided are supported for fast snapshot restore via DescribeAvailabilityZones before attempting to create the snapshot. If the EnableFastSnapshotRestores API call fails, the driver will hard-fail the request and delete the snapshot. This is to ensure that the snapshot is not left in an inconsistent state.

Issues

What testing is done?

$ make test

Ran 119 of 119 Specs in 0.418 seconds
SUCCESS! -- 119 Passed | 0 Failed | 0 Pending | 0 Skipped
  • manifests
apiVersion: snapshot.storage.k8s.io/v1
kind: VolumeSnapshotClass
metadata:
  name: csi-aws-vsc
driver: ebs.csi.aws.com
deletionPolicy: Delete
parameters:
  fastSnapshotRestoreAvailabilityZones: "us-east-1a,us-east-1b,us-east-1c"
---
kind: StorageClass
apiVersion: storage.k8s.io/v1
metadata:
  name: ebs-sc
provisioner: ebs.csi.aws.com
volumeBindingMode: WaitForFirstConsumer
---
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: ebs-claim
spec:
  accessModes:
    - ReadWriteOnce
  storageClassName: ebs-sc
  resources:
    requests:
      storage: 4Gi
---
apiVersion: v1
kind: Pod
metadata:
  name: app
spec:
  containers:
  - name: app
    image: centos
    command: ["/bin/sh"]
    args: ["-c", "while true; do echo $(date -u) >> /data/out.txt; sleep 5; done"]
    volumeMounts:
    - name: persistent-storage
      mountPath: /data
  volumes:
  - name: persistent-storage
    persistentVolumeClaim:
      claimName: ebs-claim
---
apiVersion: snapshot.storage.k8s.io/v1
kind: VolumeSnapshot
metadata:
  name: ebs-volume-snapshot
spec:
  volumeSnapshotClassName: csi-aws-vsc
  source:
    persistentVolumeClaimName: ebs-claim
---

Screen Shot 2023-03-31 at 10 52 32 AM

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 31, 2023
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 31, 2023
@torredil torredil force-pushed the fast-snapshot-restores branch from c289ae2 to 177fc68 Compare March 31, 2023 15:07
Copy link
Contributor

@ConnorJC3 ConnorJC3 left a comment

Choose a reason for hiding this comment

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

Largely lgtm, great work 🎉

In addition to the nits below, can we add a test case for the controller when cloud.AvailabilityZones indicates the request is invalid?

pkg/driver/controller.go Outdated Show resolved Hide resolved
pkg/driver/controller.go Outdated Show resolved Hide resolved
@torredil torredil force-pushed the fast-snapshot-restores branch from 177fc68 to 193d771 Compare March 31, 2023 21:08
pkg/cloud/cloud_test.go Outdated Show resolved Hide resolved
pkg/cloud/cloud_test.go Outdated Show resolved Hide resolved
pkg/driver/controller.go Outdated Show resolved Hide resolved
@hanyuel
Copy link
Contributor

hanyuel commented Mar 31, 2023

Overall LGTM. Left a few minor comments

@torredil torredil force-pushed the fast-snapshot-restores branch from 193d771 to 578f86d Compare March 31, 2023 21:39
Copy link
Contributor

@ConnorJC3 ConnorJC3 left a comment

Choose a reason for hiding this comment

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

small mistake on new tests, otherwise lgtm

pkg/driver/controller_test.go Outdated Show resolved Hide resolved
@torredil torredil force-pushed the fast-snapshot-restores branch from 578f86d to de20e06 Compare April 3, 2023 21:05
@ConnorJC3
Copy link
Contributor

/retest

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 10, 2023
@torredil torredil force-pushed the fast-snapshot-restores branch from de20e06 to 1250b7d Compare April 12, 2023 14:35
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 12, 2023
@ConnorJC3
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2023
@ConnorJC3
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ConnorJC3

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 14, 2023
@k8s-ci-robot k8s-ci-robot merged commit 5d4d310 into kubernetes-sigs:master Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Support initialization of EBS volumes created from Snapshots Fast Snapshot Restore Support
4 participants