Address review comments

Author: xing-yang

keps/sig-storage/20190709-csi-snapshot.md

@@ -96,15 +96,15 @@ The rest of the document includes required information missing from the original
 
 ### API Changes
 
-A number of changes were made to the Kubernetes volume snapshot API between alpha to beta. These changes are not backward compatible and the alpha API is no longer supported. The purpose of these changes was to make API definitions clear and easier to use.
+A number of changes were made to the Kubernetes volume snapshot API between alpha to beta. These changes are not backward compatible and the alpha API is no longer supported. The purpose of these changes was to make API definitions more clear and easier to use.
 
-The following changes are made:
+The following changes have been made from the Alpha API:
 
 * DeletionPolicy is now a required field rather than optional in both VolumeSnapshotClass and VolumeSnapshotContent. This way the user has to explicitly specify it, leaving no room for confusion.
 * VolumeSnapshotSpec has a new required Source field. Source may be either a PersistentVolumeClaimName (if dynamically provisioning a snapshot) or VolumeSnapshotContentName (if pre-provisioning a snapshot).
 * VolumeSnapshotContentSpec also has a new required Source field. This Source may be either a VolumeHandle (if dynamically provisioning a snapshot) or a SnapshotHandle (if pre-provisioning volume snapshots).
 * VolumeSnapshotStatus now contains a BoundVolumeSnapshotContentName to indicate the VolumeSnapshot object is bound to a VolumeSnapshotContent.
-* VolumeSnapshotContentnow contains a Status to indicate the current state of the content. It has a field SnapshotHandle to indicate that the VolumeSnapshotContent represents a snapshot on the storage system.
+* VolumeSnapshotContent now contains a Status to indicate the current state of the content. It has a field SnapshotHandle which is the unique identifier of a snapshot on the storage system.
 
 The beta VolumeSnapshot API object is as follows:
 
@@ -198,17 +198,19 @@ type VolumeSnapshotClass struct {
 
 ### Controller Split
 
-When Volume Snapshot is promoted to Beta in Kubernetes 1.17, the CSI external-snapshotter sidecar controller is split into two controllers: a snapshot-controller and a CSI external-snapshotter sidecar.
+Along with VolumeSnapshot being promoted to Beta in Kubernetes 1.17, the CSI external-snapshotter sidecar controller has been split into two controllers: a snapshot-controller and a CSI external-snapshotter sidecar.
 
-The snapshot controller will be watching the Kubernetes API server for `VolumeSnapshot` and `VolumeSnapshotContent` CRD objects. The CSI `external-snapshotter` sidecar only watches the Kubernetes API server for `VolumeSnapshotContent` CRD objects.
+The snapshot controller will be watching the Kubernetes API server for `VolumeSnapshot`, `VolumeSnapshotContent`, and `VolumeSnapshotClass` CRD objects. The CSI `external-snapshotter` sidecar watches the Kubernetes API server for `VolumeSnapshotContent` and `VolumeSnapshotClass` CRD objects.
 
-The creation of a new `VolumeSnapshot` object referencing a `SnapshotClass` CRD object corresponding to this driver causes the snapshot controller to trigger the creation of a Kubernetes `VolumeSnapshotContent` object to represent the to-be-created new snapshot.
+For dynamic provisioning, the creation of a new `VolumeSnapshot` object referencing a `VolumeSnapshotClass` CRD object corresponding to this driver causes the snapshot controller to trigger the creation of a Kubernetes `VolumeSnapshotContent` object to represent the to-be-created new snapshot.
 
 The creation of a new `VolumeSnapshotContent` object causes the sidecar container to trigger a `CreateSnapshot` operation against the specified CSI endpoint to provision a new snapshot. When a new snapshot is successfully provisioned, the sidecar container updates the status field of the `VolumeSnapshotContent` object to represent the new snapshot.
 
-The snapshot controller will be updating the status field of the `VolumeSnapshot` object accordingly based on the status field of the `VolumeSnapshotContent` object to indicate the new snapshot is ready to be used.
+The snapshot controller will be updating the status field of the `VolumeSnapshot` object accordingly based on the status field of the `VolumeSnapshotContent` object to indicate the new snapshot is ready to be used or failed.
 
-The deletion event of a `VolumeSnapshot` object bound to a `VolumeSnapshotContent` corresponding to this driver with a `delete` deletion policy causes the snapshot controller to start deleting the `VolumeSnapshotContent` object and add an annotation to the object to indicate it is being deleted. Note that both the `VolumeSnapshot` object and the `VolumeSnapshotContent` object will not be deleted immediately due to the finalizers. When the sidecar container detects this update on the `VolumeSnapshotContent` object, it triggers a `DeleteSnapshot` operation against the specified CSI endpoint to delete the snapshot. Once the snapshot is successfully deleted, the sidecar container removes the finalizer on the `VolumeSnapshotContent` object which leads to the deletion of the object from Kubernetes. The snapshot controller then removes the finalizer on the `VolumeSnapshot` object and as a result the object will be deleted from Kubernetes.
+The deletion event of a `VolumeSnapshot` object bound to a `VolumeSnapshotContent` corresponding to this driver with a `delete` deletion policy causes the snapshot controller to start deleting the `VolumeSnapshotContent` object and add an annotation to the object to indicate it is being deleted. Note that both the `VolumeSnapshot` object and the `VolumeSnapshotContent` object will not be deleted immediately due to the finalizers. When the sidecar container detects this update on the `VolumeSnapshotContent` object, it triggers a `DeleteSnapshot` operation against the specified CSI endpoint to delete the snapshot. Once the snapshot is successfully deleted, the sidecar container removes the finalizer on the `VolumeSnapshotContent` object which leads to the deletion of the object from Kubernetes. The snapshot controller then removes the finalizer on the `VolumeSnapshot` object and as a result the object will be deleted from Kubernetes. If a user deletes a bound `VolumeSnapshotContent` object directly, it will have a deletion timestamp set however will persist in API server until its corresponding `VolumeSnapshot` object also gets a deletion timestamp set from a deletion request.
+
+If the deletion policy is `retain` when deleting a `VolumeSnapshot` object bound to a `VolumeSnapshotContent`, the finalizers will be removed from both objects, the `VolumeSnapshot` object will be deleted from Kubernetes, but the `VolumeSnapshotContent` and the snapshot on the storage system will remain.
 
 ### Other Changes Implemented
 
@@ -219,7 +221,7 @@ Here are the changes since the original design proposal:
 * Lease based Leader Election support is added.
 * Added `VolumeSnapshotContent` deletion policy which is also specified in `VolumeSnapshotClass`.
 * Added Finalizer on the snapshot source PVC to prevent it from being deleted when a snapshot is being created from it.
-* Added Finalizer on the `VolumeSnapshotContent` object to prevent it from being deleted when it is bound to the `VolumeSnapshot` object.
+* Added Finalizer on the `VolumeSnapshotContent` object to prevent it from being deleted directly from API server when it is bound to the `VolumeSnapshot` object.
 * Added Finalizer on the `VolumeSnapshot` object to prevent it from being deleted when it is being used as a source to create a PVC.
 * Added Finalizer on the `VolumeSnapshot` object to prevent it from being deleted when it is bound to the `VolumeSnapshotContent` object.
 * Added check to see whether ListSnapshots is supported by the CSI driver. If it is supported, ListSnapshots will be called to find out the status of a snapshot during static binding; otherwise it is assumed the snapshot ID provided by the admin is valid.