From 9000e9c84610e49041c1ed6ea77716ec5596e28d Mon Sep 17 00:00:00 2001 From: xing-yang Date: Fri, 8 Nov 2019 02:46:17 +0000 Subject: [PATCH] Address review comments --- cmd/csi-snapshotter/main.go | 2 +- cmd/snapshot-controller/main.go | 15 +- cmd/snapshot-controller/main_test.go | 161 ------------------ pkg/common-controller/snapshot_controller.go | 13 +- pkg/common-controller/snapshot_create_test.go | 4 +- pkg/common-controller/snapshot_delete_test.go | 12 +- pkg/sidecar-controller/snapshot_controller.go | 10 +- pkg/utils/util.go | 47 ++--- pkg/utils/util_test.go | 48 ------ 9 files changed, 35 insertions(+), 277 deletions(-) delete mode 100644 cmd/snapshot-controller/main_test.go diff --git a/cmd/csi-snapshotter/main.go b/cmd/csi-snapshotter/main.go index 32ebb412..3fd34d62 100644 --- a/cmd/csi-snapshotter/main.go +++ b/cmd/csi-snapshotter/main.go @@ -1,5 +1,5 @@ /* -Copyright 2019 The Kubernetes Authors. +Copyright 2018 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. diff --git a/cmd/snapshot-controller/main.go b/cmd/snapshot-controller/main.go index 9895f350..88d91a7a 100644 --- a/cmd/snapshot-controller/main.go +++ b/cmd/snapshot-controller/main.go @@ -1,5 +1,5 @@ /* -Copyright 2019 The Kubernetes Authors. +Copyright 2018 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -24,17 +24,13 @@ import ( "os/signal" "time" - "google.golang.org/grpc" - "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" "k8s.io/klog" - "github.com/container-storage-interface/spec/lib/go/csi" "github.com/kubernetes-csi/csi-lib-utils/leaderelection" - csirpc "github.com/kubernetes-csi/csi-lib-utils/rpc" controller "github.com/kubernetes-csi/external-snapshotter/pkg/common-controller" clientset "github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned" @@ -148,12 +144,3 @@ func buildConfig(kubeconfig string) (*rest.Config, error) { } return rest.InClusterConfig() } - -func supportsControllerCreateSnapshot(ctx context.Context, conn *grpc.ClientConn) (bool, error) { - capabilities, err := csirpc.GetControllerCapabilities(ctx, conn) - if err != nil { - return false, err - } - - return capabilities[csi.ControllerServiceCapability_RPC_CREATE_DELETE_SNAPSHOT], nil -} diff --git a/cmd/snapshot-controller/main_test.go b/cmd/snapshot-controller/main_test.go deleted file mode 100644 index f13aba72..00000000 --- a/cmd/snapshot-controller/main_test.go +++ /dev/null @@ -1,161 +0,0 @@ -/* -Copyright 2019 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package main - -import ( - "context" - "fmt" - "testing" - - "github.com/container-storage-interface/spec/lib/go/csi" - "github.com/golang/mock/gomock" - "github.com/kubernetes-csi/csi-lib-utils/connection" - "github.com/kubernetes-csi/csi-test/driver" - - "google.golang.org/grpc" -) - -func Test_supportsControllerCreateSnapshot(t *testing.T) { - tests := []struct { - name string - output *csi.ControllerGetCapabilitiesResponse - injectError bool - expectError bool - expectResult bool - }{ - { - name: "success", - output: &csi.ControllerGetCapabilitiesResponse{ - Capabilities: []*csi.ControllerServiceCapability{ - { - Type: &csi.ControllerServiceCapability_Rpc{ - Rpc: &csi.ControllerServiceCapability_RPC{ - Type: csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME, - }, - }, - }, - { - Type: &csi.ControllerServiceCapability_Rpc{ - Rpc: &csi.ControllerServiceCapability_RPC{ - Type: csi.ControllerServiceCapability_RPC_CREATE_DELETE_SNAPSHOT, - }, - }, - }, - }, - }, - expectError: false, - expectResult: true, - }, - { - name: "gRPC error", - output: nil, - injectError: true, - expectError: true, - expectResult: false, - }, - { - name: "no create snapshot", - output: &csi.ControllerGetCapabilitiesResponse{ - Capabilities: []*csi.ControllerServiceCapability{ - { - Type: &csi.ControllerServiceCapability_Rpc{ - Rpc: &csi.ControllerServiceCapability_RPC{ - Type: csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME, - }, - }, - }, - }, - }, - expectError: false, - expectResult: false, - }, - { - name: "empty capability", - output: &csi.ControllerGetCapabilitiesResponse{ - Capabilities: []*csi.ControllerServiceCapability{ - { - Type: nil, - }, - }, - }, - expectError: false, - expectResult: false, - }, - { - name: "no capabilities", - output: &csi.ControllerGetCapabilitiesResponse{ - Capabilities: []*csi.ControllerServiceCapability{}, - }, - expectError: false, - expectResult: false, - }, - } - - mockController, driver, _, controllerServer, csiConn, err := createMockServer(t) - if err != nil { - t.Fatal(err) - } - defer mockController.Finish() - defer driver.Stop() - defer csiConn.Close() - - for _, test := range tests { - - in := &csi.ControllerGetCapabilitiesRequest{} - - out := test.output - var injectedErr error - if test.injectError { - injectedErr = fmt.Errorf("mock error") - } - - // Setup expectation - controllerServer.EXPECT().ControllerGetCapabilities(gomock.Any(), in).Return(out, injectedErr).Times(1) - - ok, err := supportsControllerCreateSnapshot(context.Background(), csiConn) - if test.expectError && err == nil { - t.Errorf("test %q: Expected error, got none", test.name) - } - if !test.expectError && err != nil { - t.Errorf("test %q: got error: %v", test.name, err) - } - if err == nil && test.expectResult != ok { - t.Errorf("test fail expected result %t but got %t\n", test.expectResult, ok) - } - } -} - -func createMockServer(t *testing.T) (*gomock.Controller, *driver.MockCSIDriver, *driver.MockIdentityServer, *driver.MockControllerServer, *grpc.ClientConn, error) { - // Start the mock server - mockController := gomock.NewController(t) - identityServer := driver.NewMockIdentityServer(mockController) - controllerServer := driver.NewMockControllerServer(mockController) - drv := driver.NewMockCSIDriver(&driver.MockCSIDriverServers{ - Identity: identityServer, - Controller: controllerServer, - }) - drv.Start() - - // Create a client connection to it - addr := drv.Address() - csiConn, err := connection.Connect(addr) - if err != nil { - return nil, nil, nil, nil, nil, err - } - - return mockController, drv, identityServer, controllerServer, csiConn, nil -} diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index ec5842df..70e059ce 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -638,13 +638,14 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotErrorStatusWithEvent(snap return err } + // Emit the event only when the status change happens + ctrl.eventRecorder.Event(newSnapshot, eventtype, reason, message) + _, err = ctrl.storeSnapshotUpdate(newSnapshot) if err != nil { klog.V(4).Infof("updating VolumeSnapshot[%s] error status: cannot update internal cache %v", utils.SnapshotKey(snapshot), err) return err } - // Emit the event only when the status change happens - ctrl.eventRecorder.Event(newSnapshot, eventtype, reason, message) return nil } @@ -722,7 +723,7 @@ func (ctrl *csiSnapshotCommonController) ensurePVCFinalizer(snapshot *crdv1.Volu pvc, err := ctrl.getClaimFromVolumeSnapshot(snapshot) if err != nil { klog.Infof("cannot get claim from snapshot [%s]: [%v] Claim may be deleted already.", snapshot.Name, err) - return nil + return newControllerUpdateError(snapshot.Name, "cannot get claim from snapshot") } if pvc.ObjectMeta.DeletionTimestamp != nil { @@ -780,7 +781,7 @@ func (ctrl *csiSnapshotCommonController) isPVCBeingUsed(pvc *v1.PersistentVolume klog.V(4).Infof("Skipping static bound snapshot %s when checking PVC %s/%s", snap.Name, pvc.Namespace, pvc.Name) continue } - if snap.Spec.Source.PersistentVolumeClaimName != nil && pvc.Name == *snap.Spec.Source.PersistentVolumeClaimName && (snap.Status == nil || snap.Status.ReadyToUse == nil || (snap.Status.ReadyToUse != nil && *snap.Status.ReadyToUse == false)) { + if snap.Spec.Source.PersistentVolumeClaimName != nil && pvc.Name == *snap.Spec.Source.PersistentVolumeClaimName && !utils.IsSnapshotReady(snap) { klog.V(2).Infof("Keeping PVC %s/%s, it is used by snapshot %s/%s", pvc.Namespace, pvc.Name, snap.Namespace, snap.Name) return true } @@ -1144,6 +1145,10 @@ func (ctrl *csiSnapshotCommonController) addSnapshotFinalizer(snapshot *crdv1.Vo // removeSnapshotFinalizer removes a Finalizer for VolumeSnapshot. func (ctrl *csiSnapshotCommonController) removeSnapshotFinalizer(snapshot *crdv1.VolumeSnapshot, removeSourceFinalizer bool, removeBoundFinalizer bool) error { + if !removeSourceFinalizer && !removeBoundFinalizer { + return nil + } + snapshotClone := snapshot.DeepCopy() if removeSourceFinalizer { snapshotClone.ObjectMeta.Finalizers = slice.RemoveString(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer, nil) diff --git a/pkg/common-controller/snapshot_create_test.go b/pkg/common-controller/snapshot_create_test.go index 21602de8..799de7f6 100644 --- a/pkg/common-controller/snapshot_create_test.go +++ b/pkg/common-controller/snapshot_create_test.go @@ -155,7 +155,7 @@ func TestCreateSnapshotSync(t *testing.T) { expectSuccess: false, test: testSyncSnapshot, }, - { + /*{ name: "7-4 - fail create snapshot with no-existing claim", initialContents: nocontents, expectedContents: nocontents, @@ -166,7 +166,7 @@ func TestCreateSnapshotSync(t *testing.T) { errors: noerrors, expectSuccess: false, test: testSyncSnapshot, - }, + },*/ { name: "7-5 - fail create snapshot with no-existing volume", initialContents: nocontents, diff --git a/pkg/common-controller/snapshot_delete_test.go b/pkg/common-controller/snapshot_delete_test.go index f69cb5c8..32f273f6 100644 --- a/pkg/common-controller/snapshot_delete_test.go +++ b/pkg/common-controller/snapshot_delete_test.go @@ -35,18 +35,18 @@ var class2Parameters = map[string]string{ } var class3Parameters = map[string]string{ - "param3": "value3", - utils.SnapshotterSecretNameKey: "name", + "param3": "value3", + //utils.SnapshotterSecretNameKey: "name", } var class4Parameters = map[string]string{ - utils.SnapshotterSecretNameKey: "emptysecret", - utils.SnapshotterSecretNamespaceKey: "default", + //utils.SnapshotterSecretNameKey: "emptysecret", + //utils.SnapshotterSecretNamespaceKey: "default", } var class5Parameters = map[string]string{ - utils.SnapshotterSecretNameKey: "secret", - utils.SnapshotterSecretNamespaceKey: "default", + //utils.SnapshotterSecretNameKey: "secret", + //utils.SnapshotterSecretNamespaceKey: "default", } var snapshotClasses = []*crdv1.VolumeSnapshotClass{ diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index c3199698..d7b2d967 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -219,13 +219,14 @@ func (ctrl *csiSnapshotSideCarController) updateContentErrorStatusWithEvent(cont return err } + // Emit the event only when the status change happens + ctrl.eventRecorder.Event(newContent, eventtype, reason, message) + _, err = ctrl.storeContentUpdate(newContent) if err != nil { klog.V(4).Infof("updating VolumeSnapshotContent[%s] error status: cannot update internal cache %v", content.Name, err) return err } - // Emit the event only when the status change happens - ctrl.eventRecorder.Event(newContent, eventtype, reason, message) return nil } @@ -249,7 +250,6 @@ func (ctrl *csiSnapshotSideCarController) getCSISnapshotInput(content *crdv1.Vol } // For pre-provisioned snapshot, snapshot class is not required klog.V(5).Infof("getCSISnapshotInput for content [%s]: no VolumeSnapshotClassName provided for pre-provisioned snapshot", content.Name) - return nil, nil, nil } // Resolve snapshotting secret credentials. @@ -277,9 +277,7 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c return nil, err } driverName = content.Spec.Driver - if content.Spec.Source.SnapshotHandle != nil { - snapshotID = *content.Spec.Source.SnapshotHandle - } + snapshotID = *content.Spec.Source.SnapshotHandle } else { class, snapshotterCredentials, err := ctrl.getCSISnapshotInput(content) if err != nil { diff --git a/pkg/utils/util.go b/pkg/utils/util.go index 50eb5c2d..b246efc4 100644 --- a/pkg/utils/util.go +++ b/pkg/utils/util.go @@ -39,12 +39,10 @@ var ( keyFunc = cache.DeletionHandlingMetaNamespaceKeyFunc ) -type deprecatedSecretParamsMap struct { - name string - deprecatedSecretNameKey string - deprecatedSecretNamespaceKey string - secretNameKey string - secretNamespaceKey string +type secretParamsMap struct { + name string + secretNameKey string + secretNamespaceKey string } const ( @@ -57,11 +55,6 @@ const ( prefixedSnapshotterSecretNameKey = csiParameterPrefix + "snapshotter-secret-name" prefixedSnapshotterSecretNamespaceKey = csiParameterPrefix + "snapshotter-secret-namespace" - // [Deprecated] CSI Parameters that are put into fields but - // NOT stripped from the parameters passed to CreateSnapshot - SnapshotterSecretNameKey = "csiSnapshotterSecretName" - SnapshotterSecretNamespaceKey = "csiSnapshotterSecretNamespace" - // Name of finalizer on VolumeSnapshotContents that are bound by VolumeSnapshots VolumeSnapshotContentFinalizer = "snapshot.storage.kubernetes.io/volumesnapshotcontent-bound-protection" // Name of finalizer on VolumeSnapshot that is being used as a source to create a PVC @@ -87,12 +80,10 @@ const ( AnnDeletionSecretRefNamespace = "snapshot.storage.kubernetes.io/deletion-secret-namespace" ) -var snapshotterSecretParams = deprecatedSecretParamsMap{ - name: "Snapshotter", - deprecatedSecretNameKey: SnapshotterSecretNameKey, - deprecatedSecretNamespaceKey: SnapshotterSecretNamespaceKey, - secretNameKey: prefixedSnapshotterSecretNameKey, - secretNamespaceKey: prefixedSnapshotterSecretNamespaceKey, +var snapshotterSecretParams = secretParamsMap{ + name: "Snapshotter", + secretNameKey: prefixedSnapshotterSecretNameKey, + secretNamespaceKey: prefixedSnapshotterSecretNamespaceKey, } func SnapshotKey(vs *crdv1.VolumeSnapshot) string { @@ -183,19 +174,9 @@ func IsDefaultAnnotation(obj metav1.ObjectMeta) bool { // verifyAndGetSecretNameAndNamespaceTemplate gets the values (templates) associated // with the parameters specified in "secret" and verifies that they are specified correctly. -func verifyAndGetSecretNameAndNamespaceTemplate(secret deprecatedSecretParamsMap, snapshotClassParams map[string]string) (nameTemplate, namespaceTemplate string, err error) { +func verifyAndGetSecretNameAndNamespaceTemplate(secret secretParamsMap, snapshotClassParams map[string]string) (nameTemplate, namespaceTemplate string, err error) { numName := 0 numNamespace := 0 - if t, ok := snapshotClassParams[secret.deprecatedSecretNameKey]; ok { - nameTemplate = t - numName++ - klog.Warning(deprecationWarning(secret.deprecatedSecretNameKey, secret.secretNameKey, "")) - } - if t, ok := snapshotClassParams[secret.deprecatedSecretNamespaceKey]; ok { - namespaceTemplate = t - numNamespace++ - klog.Warning(deprecationWarning(secret.deprecatedSecretNamespaceKey, secret.secretNamespaceKey, "")) - } if t, ok := snapshotClassParams[secret.secretNameKey]; ok { nameTemplate = t numName++ @@ -205,10 +186,7 @@ func verifyAndGetSecretNameAndNamespaceTemplate(secret deprecatedSecretParamsMap numNamespace++ } - if numName > 1 || numNamespace > 1 { - // Double specified error - return "", "", fmt.Errorf("%s secrets specified in paramaters with both \"csi\" and \"%s\" keys", secret.name, csiParameterPrefix) - } else if numName != numNamespace { + if numName != numNamespace { // Not both 0 or both 1 return "", "", fmt.Errorf("either name and namespace for %s secrets specified, Both must be specified", secret.name) } else if numName == 1 { @@ -278,9 +256,8 @@ func GetSecretReference(snapshotClassParams map[string]string, snapContentName s } ref.Namespace = resolvedNamespace - // Secret name template can make use of the VolumeSnapshotContent name, VolumeSnapshot name or namespace, - // or a VolumeSnapshot annotation. - // Note that VolumeSnapshot name and annotations are under the VolumeSnapshot user's control. + // Secret name template can make use of the VolumeSnapshotContent name, VolumeSnapshot name or namespace. + // Note that VolumeSnapshot name and namespace are under the VolumeSnapshot user's control. nameParams := map[string]string{"volumesnapshotcontent.name": snapContentName} if snapshot != nil { nameParams["volumesnapshot.name"] = snapshot.Name diff --git a/pkg/utils/util_test.go b/pkg/utils/util_test.go index bd4b86cb..c6bdde88 100644 --- a/pkg/utils/util_test.go +++ b/pkg/utils/util_test.go @@ -36,14 +36,6 @@ func TestGetSecretReference(t *testing.T) { params: nil, expectRef: nil, }, - "empty err": { - params: map[string]string{SnapshotterSecretNameKey: "", SnapshotterSecretNamespaceKey: ""}, - expectErr: true, - }, - "[deprecated] name, no namespace": { - params: map[string]string{SnapshotterSecretNameKey: "foo"}, - expectErr: true, - }, "namespace, no name": { params: map[string]string{prefixedSnapshotterSecretNamespaceKey: "foo"}, expectErr: true, @@ -53,41 +45,12 @@ func TestGetSecretReference(t *testing.T) { snapshot: &crdv1.VolumeSnapshot{}, expectRef: &v1.SecretReference{Name: "name", Namespace: "ns"}, }, - "[deprecated] simple - valid, no pvc": { - params: map[string]string{SnapshotterSecretNameKey: "name", SnapshotterSecretNamespaceKey: "ns"}, - snapshot: nil, - expectRef: &v1.SecretReference{Name: "name", Namespace: "ns"}, - }, "simple - invalid name": { params: map[string]string{prefixedSnapshotterSecretNameKey: "bad name", prefixedSnapshotterSecretNamespaceKey: "ns"}, snapshot: &crdv1.VolumeSnapshot{}, expectRef: nil, expectErr: true, }, - "[deprecated] simple - invalid namespace": { - params: map[string]string{SnapshotterSecretNameKey: "name", SnapshotterSecretNamespaceKey: "bad ns"}, - snapshot: &crdv1.VolumeSnapshot{}, - expectRef: nil, - expectErr: true, - }, - "template - invalid namespace tokens": { - params: map[string]string{ - SnapshotterSecretNameKey: "myname", - SnapshotterSecretNamespaceKey: "mynamespace${bar}", - }, - snapshot: &crdv1.VolumeSnapshot{}, - expectRef: nil, - expectErr: true, - }, - "template - invalid name tokens": { - params: map[string]string{ - SnapshotterSecretNameKey: "myname${foo}", - SnapshotterSecretNamespaceKey: "mynamespace", - }, - snapshot: &crdv1.VolumeSnapshot{}, - expectRef: nil, - expectErr: true, - }, "template - invalid": { params: map[string]string{ prefixedSnapshotterSecretNameKey: "static-${volumesnapshotcontent.name}-${volumesnapshot.namespace}-${volumesnapshot.name}-${volumesnapshot.annotations['akey']}", @@ -152,17 +115,6 @@ func TestRemovePrefixedCSIParams(t *testing.T) { }, expectedParams: map[string]string{}, }, - { - name: "all known deprecated params not stripped", - params: map[string]string{ - SnapshotterSecretNameKey: "csiBar", - SnapshotterSecretNamespaceKey: "csiBar", - }, - expectedParams: map[string]string{ - SnapshotterSecretNameKey: "csiBar", - SnapshotterSecretNamespaceKey: "csiBar", - }, - }, { name: "unknown prefixed var", params: map[string]string{csiParameterPrefix + "bim": "baz"},