Repository: kubernetes-csi/external-snapshotter
Pull request #211 (2019-12-17 01:46:13)

Remove createSnapshotContentRetryCount and createSnapshotContentInterval
**What type of PR is this?**
/kind cleanup

**What this PR does / why we need it**:
Removes createSnapshotContentRetryCount and createSnapshotContentInterval
from command line options.

**Which issue(s) this PR fixes**:
Fixes #

**Special notes for your reviewer**:
This was brought up during the review of the README PR (https://github.com/kubernetes-csi/external-snapshotter/pull/206) that these parameters should just be internal variables, not command line options.

**Does this PR introduce a user-facing change?**:
```release-note
Removes createSnapshotContentRetryCount and createSnapshotContentInterval
from command line options.
```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-csi/external-snapshotter/pull/211#" title="Author self-approved">xing-yang</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-csi%2Fexternal-snapshotter).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-csi/external-snapshotter/blob/master/OWNERS)~~ [xing-yang]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
xing-yang: /assign @yuxiangqian 

Patch:
diff --git a/README.md b/README.md
index 0d1e2d885..7d83293c7 100644
--- a/README.md
+++ b/README.md
@@ -1,150 +1,72 @@
-[![Build Status](https://travis-ci.org/kubernetes-csi/external-snapshotter.svg?branch=master)](https://travis-ci.org/kubernetes-csi/external-snapshotter)
-
 # CSI Snapshotter
 
-The CSI snapshotter is part of Kubernetes implementation of [Container Storage Interface (CSI)](https://github.com/container-storage-interface/spec).
-
-The volume snapshot feature supports CSI v1.0 and higher. It was introduced as an Alpha feature in Kubernetes v1.12 and has been promoted to an Beta feature in Kubernetes 1.17.
+The CSI external-snapshotter is part of Kubernetes implementation of [Container Storage Interface (CSI)](https://github.com/container-storage-interface/spec).
 
+The volume snapshot feature supports CSI v1.0 and it has been an Alpha feature in Kubernetes since v1.12.
 
 ## Overview
 
-With the promotion of Volume Snapshot to beta, the feature is now enabled by default on standard Kubernetes deployments instead of being opt-in.
-
-The move of the Kubernetes Volume Snapshot feature to beta also means:
-* A revamp of volume snapshot APIs.
-* The CSI external-snapshotter sidecar is split into two controllers, a snapshot controller and a CSI external-snapshotter sidecar.
-
-The snapshot controller is deployed by the Kubernetes distributions and is responsible for watching the VolumeSnapshot CRD objects and manges the creation and deletion lifecycle of snapshots.
-
-The CSI external-snapshotter sidecar watches Kubernetes VolumeSnapshotContent CRD objects and triggers CreateSnapshot/DeleteSnapshot against a CSI endpoint.
-
-Blog post for the beta feature can be found [here](https://kubernetes.io/blog/2019/12/09/kubernetes-1-17-feature-cis-volume-snapshot-beta)
-
-
-## Compatibility
-
-This information reflects the head of this branch.
-
-| Compatible with CSI Version                                                                | Container Image             | Min K8s Version | Snapshot CRD version |
-| ------------------------------------------------------------------------------------------ | ----------------------------| --------------- | -------------------- |
-| [CSI Spec v1.0.0](https://github.com/container-storage-interface/spec/releases/tag/v1.0.0) | quay.io/k8scsi/csi-snapshotter | 1.17         | v1beta1              |
-| [CSI Spec v1.0.0](https://github.com/container-storage-interface/spec/releases/tag/v1.0.0) | quay.io/k8scsi/snapshot-controller | 1.17     | v1beta1              |
-
-
-## Feature Status
-
-The `VolumeSnapshotDataSource` feature gate was introduced in Kubernetes 1.12 and it is enabled by default in Kubernetes 1.17 when the volume snapshot feature is promoted to beta.
-
+CSI Snapshotter is an external controller that watches Kubernetes Snapshot CRD objects and triggers CreateSnapshot/DeleteSnapshot against a CSI endpoint. Full design can be found at Kubernetes proposal at [here](https://github.com/kubernetes/community/blob/master/contributors/design-proposals/storage/csi-snapshot.md)
 
 ## Design
 
-Both the snapshot controller and CSI external-snapshotter sidecar follow [controller](https://github.com/kubernetes/community/blob/master/contributors/devel/controllers.md) pattern and uses informers to watch for events. The snapshot controller watches for `VolumeSnapshot` and `VolumeSnapshotContent` create/update/delete events.
-
-The CSI external-snapshotter sidecar only watches for `VolumeSnapshotContent` create/update/delete events. It filters out these objects with `Driver==<CSI driver name>` specified in the associated VolumeSnapshotClass object and then processes these events in workqueues with exponential backoff.
-
-The CSI external-snapshotter sidecar talks to CSI over socket (/run/csi/socket by default, configurable by -csi-address).
-
-### Hightlights in the snapshot v1beta1 APIs
-
-* DeletionPolicy is a required field in both VolumeSnapshotClass and VolumeSnapshotContent. This way the user has to explicitly specify it, leaving no room for confusion.
-* VolumeSnapshotSpec has a required Source field. Source may be either a PersistentVolumeClaimName (if dynamically provisioning a snapshot) or VolumeSnapshotContentName (if pre-provisioning a snapshot).
-* VolumeSnapshotContentSpec has a required Source field. This Source may be either a VolumeHandle (if dynamically provisioning a snapshot) or a SnapshotHandle (if pre-provisioning volume snapshots).
-* VolumeSnapshot contains a Status to indicate the current state of the volume snapshot. It has a field BoundVolumeSnapshotContentName to indicate the VolumeSnapshot object is bound to a VolumeSnapshotContent.
-* VolumeSnapshotContent contains a Status to indicate the current state of the volume snapshot content. It has a field SnapshotHandle to indicate that the VolumeSnapshotContent represents a snapshot on the storage system.
-
-
-## Usage
-
-The Volume Snapshot feature now depends on a new, volume snapshot controller in addition to the volume snapshot CRDs. Both the volume snapshot controller and the CRDs are independent of any CSI driver. Regardless of the number CSI drivers deployed on the cluster, there must be only one instance of the volume snapshot controller running and one set of volume snapshot CRDs installed per cluster.
-
-Therefore, it is strongly recommended that Kubernetes distributors bundle and deploy the controller and CRDs as part of their Kubernetes cluster management process (independent of any CSI Driver).
+External snapshotter follows [controller](https://github.com/kubernetes/community/blob/master/contributors/devel/controllers.md) pattern and uses informers to watch for `VolumeSnapshot` and `VolumeSnapshotContent` create/update/delete events. It filters out these objects with `Snapshotter==<CSI driver name>` specified in the associated VolumeSnapshotClass object and then processes these events in workqueues with exponential backoff.
 
-If your cluster does not come pre-installed with the correct components, you may manually install these components by executing the following steps.
+### Snapshotter
 
-Install Snapshot Beta CRDs:
-* kubectl create -f config/crd
-* https://github.com/kubernetes-csi/external-snapshotter/tree/master/config/crd
-* Do this once per cluster
+Snapshotter talks to CSI over socket (/run/csi/socket by default, configurable by -csi-address). The snapshotter then:
 
-Install Common Snapshot Controller:
-* kubectl create -f deploy/kubernetes/snapshot-controller
-* https://github.com/kubernetes-csi/external-snapshotter/tree/master/deploy/kubernetes/snapshot-controller
-* Do this once per cluster
+* Discovers the supported snapshotter name by `GetDriverName` call.
 
-Install CSI Driver:
-* Follow instructions provided by your CSI Driver vendor.
-* Here is an example to install the sample hostpath CSI driver
-  * kubectl create -f deploy/kubernetes/csi-snapshotter
-  * https://github.com/kubernetes-csi/external-snapshotter/tree/master/deploy/kubernetes/csi-snapshotter
+* Uses ControllerGetCapabilities for find out if CSI driver supports `ControllerServiceCapability_RPC_CREATE_DELETE_SNAPSHOT` and `ControllerServiceCapability_RPC_LIST_SNAPSHOTS` calls. Otherwise, the controller will not start.
 
-### Snapshot controller command line options
+* Processes new/updated/deleted `VolumeSnapshots`: The snapshotter only processes `VolumeSnapshot` that has `snapshotter` specified in its `VolumeSnapshotClass` matches its driver name. The process workflow is as follows
+  * If the snapshot status is `Ready`, the controller checks whether the snapshot and its content still binds correctly. If there is any problem with the binding (e.g., snapshot points to a non-exist snapshot content), update the snapshot status and emit event.
+  * If the snapshot status is not ready, there are two cases.
+    * `SnapshotContentName` is not empty: the controller verifies whether the snapshot content exists and also binds to the snapshot. If verification passes, the controller binds the snapshot and its content objects and marks it is ready. Otherwise, it updates the error status of the snapshot.
+    * `SnapshotContentName` is set empty: the controller will first check whether there is already a content object which binds the snapshot correctly with snapshot uid (`VolumeSnapshotRef.UID`) specified. If so, the controller binds these two objects. Otherwise, the controller issues a create snapshot operation. Please note that if the error status shows that snapshot creation already failed before, it will not try to create snapshot again.
 
-#### Important optional arguments that are highly recommended to be used
-* `--leader-election`: Enables leader election. This is useful when there are multiple replicas of the same snapshot controller running for the same Kubernetes deployment. Only one of them may be active (=leader). A new leader will be re-elected when current leader dies or becomes unresponsive for ~15 seconds.
+* Processes new/updated/deleted `VolumeSnapshotContents`: The snapshotter only processes `VolumeSnapshotContent` in which the CSI driver specified in the spec matches the controller's driver name.
+  * If the `VolumeSnapshotRef` is set to nil, skip this content since it is not bound to any snapshot object.
+  * Otherwise, the controller verifies whether the content object is correctly bound to a snapshot object. In case the `VolumeSnapshotRef.UID` is set but it does not match its snapshot object or snapshot no long exists, the content object and its associated snapshot will be deleted.
 
-* `--leader-election-namespace <namespace>`: The namespace where the leader election resource exists. Defaults to the pod namespace if not set.
-
-#### Other recognized arguments
-* `--kubeconfig <path>`: Path to Kubernetes client configuration that the snapshot controller uses to connect to Kubernetes API server. When omitted, default token provided by Kubernetes will be used. This option is useful only when the snapshot controller does not run as a Kubernetes pod, e.g. for debugging.
-
-* `--resync-period <duration>`: Internal resync interval when the snapshot controller re-evaluates all existing `VolumeSnapshot` instances and tries to fulfill them, i.e. create / delete corresponding snapshots. It does not affect re-tries of failed calls! It should be used only when there is a bug in Kubernetes watch logic. Default is 60 seconds.
-
-* `--version`: Prints current snapshot controller version and quits.
-
-* All glog / klog arguments are supported, such as `-v <log level>` or `-alsologtostderr`.
-
-### CSI external snapshotter sidecar command line options
-
-#### Important optional arguments that are highly recommended to be used
-* `--csi-address <path to CSI socket>`: This is the path to the CSI driver socket inside the pod that the external-snapshotter container will use to issue CSI operations (`/run/csi/socket` is used by default).
-
-* `--leader-election`: Enables leader election. This is useful when there are multiple replicas of the same external-snapshotter running for one CSI driver. Only one of them may be active (=leader). A new leader will be re-elected when current leader dies or becomes unresponsive for ~15 seconds.
-
-* `--leader-election-namespace <namespace>`: The namespace where the leader election resource exists. Defaults to the pod namespace if not set.
-
-* `--timeout <duration>`: Timeout of all calls to CSI driver. It should be set to value that accommodates majority of `CreateSnapshot`, `DeleteSnapshot`, and `ListSnapshots` calls. 1 minute is used by default.
-
-* `snapshot-name-prefix`: Prefix to apply to the name of a created snapshot. Default is `snapshot`.
-
-* `snapshot-name-uuid-length`: Length in characters for the generated uuid of a created snapshot. Defaults behavior is to NOT truncate.
-
-#### Other recognized arguments
-* `--kubeconfig <path>`: Path to Kubernetes client configuration that the CSI external-snapshotter uses to connect to Kubernetes API server. When omitted, default token provided by Kubernetes will be used. This option is useful only when the external-snapshotter does not run as a Kubernetes pod, e.g. for debugging.
+## Usage
 
-* `--resync-period <duration>`: Internal resync interval when the CSI external-snapshotter re-evaluates all existing `VolumeSnapshotContent` instances and tries to fulfill them, i.e. update / delete corresponding snapshots. It does not affect re-tries of failed CSI calls! It should be used only when there is a bug in Kubernetes watch logic. Default is 60 seconds.
+### Running on command line
 
-* `--version`: Prints current CSI external-snapshotter version and quits.
+For debugging, it is possible to run snapshotter on command line. For example,
 
-* All glog / klog arguments are supported, such as `-v <log level>` or `-alsologtostderr`.
+```bash
+csi-snapshotter -kubeconfig ~/.kube/config -v 5 -csi-address /run/csi/socket
+```
 
+### Running in a statefulset
 
-## Upgrade from v1alpha1 to v1beta1
+It is necessary to create a new service account and give it enough privileges to run the snapshotter. We provide .yaml files that deploy for use together with the hostpath example driver. A real production deployment must customize them:
 
-The change from v1alpha1 to v1beta1 snapshot APIs is not backward compatible.
+```bash
+for i in $(find deploy/kubernetes -name '*.yaml'); do kubectl create -f $i; done
+```
 
-If you have already deployed v1alpha1 snapshot APIs and external-snapshotter sidecar controller and want to upgrade to v1beta1, you need to do the following:
-* Delete volume snapshots created using v1alpha1 snapshot CRDs and external-snapshotter sidecar controller.
-* Uninstall v1alpha1 snapshot CRDs, external-snapshotter sidecar controller, and CSI driver.
-* Install v1beta1 snapshot CRDs, snapshot controller, CSI external-snapshotter sidecar and CSI driver.
+### Running with Leader Election
 
+If you want to run external-snapshotter with higher availability, you can enable resource based leader election. To enable this, set the following flags:
+```bash
+--leader-election=true
+```
 
 ## Testing
 
 Running Unit Tests:
 
 ```bash
-go test -timeout 30s  github.com/kubernetes-csi/external-snapshotter/pkg/common-controller
-
-go test -timeout 30s  github.com/kubernetes-csi/external-snapshotter/pkg/sidecar-controller
+go test -timeout 30s  github.com/kubernetes-csi/external-snapshotter/pkg/controller
 ```
 
-
 ## Dependency Management
 
 external-snapshotter uses [go modules](https://blog.golang.org/using-go-modules).
 
-
 ## Community, discussion, contribution, and support
 
 Learn how to engage with the Kubernetes community on the [community page](http://kubernetes.io/community/).
diff --git a/cmd/snapshot-controller/main.go b/cmd/snapshot-controller/main.go
index 88d91a7aa..cbe92f0fe 100644
--- a/cmd/snapshot-controller/main.go
+++ b/cmd/snapshot-controller/main.go
@@ -46,11 +46,9 @@ const (
 
 // Command line flags
 var (
-	kubeconfig                      = flag.String("kubeconfig", "", "Absolute path to the kubeconfig file. Required only when running out of cluster.")
-	createSnapshotContentRetryCount = flag.Int("create-snapshotcontent-retrycount", 5, "Number of retries when we create a snapshot content object for a snapshot.")
-	createSnapshotContentInterval   = flag.Duration("create-snapshotcontent-interval", 10*time.Second, "Interval between retries when we create a snapshot content object for a snapshot.")
-	resyncPeriod                    = flag.Duration("resync-period", 60*time.Second, "Resync interval of the controller.")
-	showVersion                     = flag.Bool("version", false, "Show version.")
+	kubeconfig   = flag.String("kubeconfig", "", "Absolute path to the kubeconfig file. Required only when running out of cluster.")
+	resyncPeriod = flag.Duration("resync-period", 60*time.Second, "Resync interval of the controller.")
+	showVersion  = flag.Bool("version", false, "Show version.")
 
 	leaderElection          = flag.Bool("leader-election", false, "Enables leader election.")
 	leaderElectionNamespace = flag.String("leader-election-namespace", "", "The namespace where the leader election resource exists. Defaults to the pod namespace if not set.")
@@ -96,7 +94,7 @@ func main() {
 	// Add Snapshot types to the defualt Kubernetes so events can be logged for them
 	snapshotscheme.AddToScheme(scheme.Scheme)
 
-	klog.V(2).Infof("Start NewCSISnapshotController with kubeconfig [%s] createSnapshotContentRetryCount [%d] createSnapshotContentInterval [%d] resyncPeriod [%+v]", *kubeconfig, *createSnapshotContentRetryCount, *createSnapshotContentInterval, *resyncPeriod)
+	klog.V(2).Infof("Start NewCSISnapshotController with kubeconfig [%s] resyncPeriod [%+v]", *kubeconfig, *resyncPeriod)
 
 	ctrl := controller.NewCSISnapshotCommonController(
 		snapClient,
@@ -105,8 +103,6 @@ func main() {
 		factory.Snapshot().V1beta1().VolumeSnapshotContents(),
 		factory.Snapshot().V1beta1().VolumeSnapshotClasses(),
 		coreFactory.Core().V1().PersistentVolumeClaims(),
-		*createSnapshotContentRetryCount,
-		*createSnapshotContentInterval,
 		*resyncPeriod,
 	)
 
diff --git a/pkg/common-controller/snapshot_controller_base.go b/pkg/common-controller/snapshot_controller_base.go
index 958f03fae..2518dad85 100644
--- a/pkg/common-controller/snapshot_controller_base.go
+++ b/pkg/common-controller/snapshot_controller_base.go
@@ -42,6 +42,12 @@ import (
 	"k8s.io/kubernetes/pkg/util/goroutinemap"
 )
 
+// Number of retries when we create a VolumeSnapshotContent object
+const createSnapshotContentRetryCount = 5
+
+// Interval between retries when we create a VolumeSnapshotContent object
+const createSnapshotContentInterval = 10 * time.Second
+
 type csiSnapshotCommonController struct {
 	clientset     clientset.Interface
 	client        kubernetes.Interface
@@ -77,8 +83,6 @@ func NewCSISnapshotCommonController(
 	volumeSnapshotContentInformer storageinformers.VolumeSnapshotContentInformer,
 	volumeSnapshotClassInformer storageinformers.VolumeSnapshotClassInformer,
 	pvcInformer coreinformers.PersistentVolumeClaimInformer,
-	createSnapshotContentRetryCount int,
-	createSnapshotContentInterval time.Duration,
 	resyncPeriod time.Duration,
 ) *csiSnapshotCommonController {
 	broadcaster := record.NewBroadcaster()


Test patch:
diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go
index 82531eae5..1aaf51ee7 100644
--- a/pkg/common-controller/framework_test.go
+++ b/pkg/common-controller/framework_test.go
@@ -761,8 +761,6 @@ func newTestController(kubeClient kubernetes.Interface, clientset clientset.Inte
 		informerFactory.Snapshot().V1beta1().VolumeSnapshotContents(),
 		informerFactory.Snapshot().V1beta1().VolumeSnapshotClasses(),
 		coreFactory.Core().V1().PersistentVolumeClaims(),
-		3,
-		5*time.Millisecond,
 		60*time.Second,
 	)