From 25642fe404fb2656abb7489f8578c8e4e83f2a40 Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Mon, 28 Jan 2019 17:17:06 +0530 Subject: [PATCH 01/11] Add method comments Signed-off-by: Madhu Rajanna --- cmd/rbd/main.go | 2 +- pkg/cephfs/controllerserver.go | 7 +++++++ pkg/cephfs/driver.go | 16 +++++++++++--- pkg/cephfs/identityserver.go | 3 +++ pkg/cephfs/nodeserver.go | 12 +++++++---- pkg/rbd/controllerserver.go | 16 ++++++++++++++ pkg/rbd/identityserver.go | 3 +++ pkg/rbd/nodeserver.go | 38 ++++++++++++++++------------------ pkg/rbd/rbd.go | 9 +++++++- pkg/util/cachepersister.go | 4 ++++ pkg/util/k8scmcache.go | 8 +++++++ pkg/util/nodecache.go | 6 ++++++ 12 files changed, 95 insertions(+), 29 deletions(-) diff --git a/cmd/rbd/main.go b/cmd/rbd/main.go index 86346ed12..c89092845 100644 --- a/cmd/rbd/main.go +++ b/cmd/rbd/main.go @@ -56,7 +56,7 @@ func main() { os.Exit(1) } - driver := rbd.GetDriver() + driver := rbd.NewDriver() driver.Run(*driverName, *nodeID, *endpoint, *containerized, cp) os.Exit(0) diff --git a/pkg/cephfs/controllerserver.go b/pkg/cephfs/controllerserver.go index 0bb027f31..f42780114 100644 --- a/pkg/cephfs/controllerserver.go +++ b/pkg/cephfs/controllerserver.go @@ -28,6 +28,8 @@ import ( "github.com/ceph/ceph-csi/pkg/util" ) +// ControllerServer struct of CEPH CSI driver with supported methods of CSI +// controller server spec. type ControllerServer struct { *csicommon.DefaultControllerServer MetadataStore util.CachePersister @@ -38,6 +40,7 @@ type controllerCacheEntry struct { VolumeID volumeID } +// CreateVolume creates the volume in backend and store the volume metadata func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) (*csi.CreateVolumeResponse, error) { if err := cs.validateCreateVolumeRequest(req); err != nil { glog.Errorf("CreateVolumeRequest validation failed: %v", err) @@ -102,6 +105,8 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol }, nil } +// DeleteVolume deletes the volume in backend and removes the volume metadata +// from store func (cs *ControllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest) (*csi.DeleteVolumeResponse, error) { if err := cs.validateDeleteVolumeRequest(); err != nil { glog.Errorf("DeleteVolumeRequest validation failed: %v", err) @@ -159,6 +164,8 @@ func (cs *ControllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVol return &csi.DeleteVolumeResponse{}, nil } +// ValidateVolumeCapabilities checks whether the volume capabilities requested +// are supported. func (cs *ControllerServer) ValidateVolumeCapabilities( ctx context.Context, req *csi.ValidateVolumeCapabilitiesRequest) (*csi.ValidateVolumeCapabilitiesResponse, error) { diff --git a/pkg/cephfs/driver.go b/pkg/cephfs/driver.go index ed98ec44b..28cbaea83 100644 --- a/pkg/cephfs/driver.go +++ b/pkg/cephfs/driver.go @@ -26,10 +26,13 @@ import ( ) const ( + // PluginFolder defines the location of ceph plugin PluginFolder = "/var/lib/kubelet/plugins/csi-cephfsplugin" - Version = "1.0.0" + // version of ceph driver + version = "1.0.0" ) +// Driver contains the default identity,node and controller struct type Driver struct { cd *csicommon.CSIDriver @@ -39,19 +42,23 @@ type Driver struct { } var ( + // DefaultVolumeMounter for mounting volumes DefaultVolumeMounter string ) +// NewDriver returns new ceph driver func NewDriver() *Driver { return &Driver{} } +// NewIdentityServer initialize a identity server for ceph CSI driver func NewIdentityServer(d *csicommon.CSIDriver) *IdentityServer { return &IdentityServer{ DefaultIdentityServer: csicommon.NewDefaultIdentityServer(d), } } +// NewControllerServer initialize a controller server for ceph CSI driver func NewControllerServer(d *csicommon.CSIDriver, cachePersister util.CachePersister) *ControllerServer { return &ControllerServer{ DefaultControllerServer: csicommon.NewDefaultControllerServer(d), @@ -59,14 +66,17 @@ func NewControllerServer(d *csicommon.CSIDriver, cachePersister util.CachePersis } } +// NewNodeServer initialize a node server for ceph CSI driver. func NewNodeServer(d *csicommon.CSIDriver) *NodeServer { return &NodeServer{ DefaultNodeServer: csicommon.NewDefaultNodeServer(d), } } +// Run start a non-blocking grpc controller,node and identityserver for +// ceph CSI driver which can serve multiple parallel requests func (fs *Driver) Run(driverName, nodeID, endpoint, volumeMounter string, cachePersister util.CachePersister) { - glog.Infof("Driver: %v version: %v", driverName, Version) + glog.Infof("Driver: %v version: %v", driverName, version) // Configuration @@ -91,7 +101,7 @@ func (fs *Driver) Run(driverName, nodeID, endpoint, volumeMounter string, cacheP // Initialize default library driver - fs.cd = csicommon.NewCSIDriver(driverName, Version, nodeID) + fs.cd = csicommon.NewCSIDriver(driverName, version, nodeID) if fs.cd == nil { glog.Fatalln("Failed to initialize CSI driver") } diff --git a/pkg/cephfs/identityserver.go b/pkg/cephfs/identityserver.go index 5f0a60e76..9f3a6b4fd 100644 --- a/pkg/cephfs/identityserver.go +++ b/pkg/cephfs/identityserver.go @@ -23,10 +23,13 @@ import ( "github.com/kubernetes-csi/drivers/pkg/csi-common" ) +// IdentityServer struct of ceph CSI driver with supported methods of CSI +// identity server spec. type IdentityServer struct { *csicommon.DefaultIdentityServer } +// GetPluginCapabilities returns available capabilities of the ceph driver func (is *IdentityServer) GetPluginCapabilities(ctx context.Context, req *csi.GetPluginCapabilitiesRequest) (*csi.GetPluginCapabilitiesResponse, error) { return &csi.GetPluginCapabilitiesResponse{ Capabilities: []*csi.PluginCapability{ diff --git a/pkg/cephfs/nodeserver.go b/pkg/cephfs/nodeserver.go index 4b6309169..2c7c3b658 100644 --- a/pkg/cephfs/nodeserver.go +++ b/pkg/cephfs/nodeserver.go @@ -29,6 +29,8 @@ import ( "github.com/kubernetes-csi/drivers/pkg/csi-common" ) +// NodeServer struct of ceph CSI driver with supported methods of CSI +// node server spec. type NodeServer struct { *csicommon.DefaultNodeServer } @@ -77,6 +79,7 @@ func getCredentialsForVolume(volOptions *volumeOptions, volID volumeID, req *csi return userCr, nil } +// NodeStageVolume mounts the volume to a staging path on the node. func (ns *NodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolumeRequest) (*csi.NodeStageVolumeResponse, error) { if err := validateNodeStageVolumeRequest(req); err != nil { return nil, status.Error(codes.InvalidArgument, err.Error()) @@ -149,6 +152,8 @@ func (ns *NodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStageVol return &csi.NodeStageVolumeResponse{}, nil } +// NodePublishVolume mounts the volume mounted to the staging path to the target +// path func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolumeRequest) (*csi.NodePublishVolumeResponse, error) { if err := validateNodePublishVolumeRequest(req); err != nil { return nil, status.Error(codes.InvalidArgument, err.Error()) @@ -190,6 +195,7 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis return &csi.NodePublishVolumeResponse{}, nil } +// NodeUnpublishVolume unmounts the volume from the target path func (ns *NodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpublishVolumeRequest) (*csi.NodeUnpublishVolumeResponse, error) { if err := validateNodeUnpublishVolumeRequest(req); err != nil { return nil, status.Error(codes.InvalidArgument, err.Error()) @@ -209,6 +215,7 @@ func (ns *NodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpu return &csi.NodeUnpublishVolumeResponse{}, nil } +// NodeUnstageVolume unstages the volume from the staging path func (ns *NodeServer) NodeUnstageVolume(ctx context.Context, req *csi.NodeUnstageVolumeRequest) (*csi.NodeUnstageVolumeResponse, error) { if err := validateNodeUnstageVolumeRequest(req); err != nil { return nil, status.Error(codes.InvalidArgument, err.Error()) @@ -228,6 +235,7 @@ func (ns *NodeServer) NodeUnstageVolume(ctx context.Context, req *csi.NodeUnstag return &csi.NodeUnstageVolumeResponse{}, nil } +// NodeGetCapabilities returns the supported capabilities of the node server func (ns *NodeServer) NodeGetCapabilities(ctx context.Context, req *csi.NodeGetCapabilitiesRequest) (*csi.NodeGetCapabilitiesResponse, error) { return &csi.NodeGetCapabilitiesResponse{ Capabilities: []*csi.NodeServiceCapability{ @@ -241,7 +249,3 @@ func (ns *NodeServer) NodeGetCapabilities(ctx context.Context, req *csi.NodeGetC }, }, nil } - -func (ns *NodeServer) NodeGetInfo(ctx context.Context, req *csi.NodeGetInfoRequest) (*csi.NodeGetInfoResponse, error) { - return ns.DefaultNodeServer.NodeGetInfo(ctx, req) -} diff --git a/pkg/rbd/controllerserver.go b/pkg/rbd/controllerserver.go index f886d819f..7d5fb8205 100644 --- a/pkg/rbd/controllerserver.go +++ b/pkg/rbd/controllerserver.go @@ -39,6 +39,8 @@ const ( oneGB = 1073741824 ) +// ControllerServer struct of rbd CSI driver with supported methods of CSI +// controller server spec. type ControllerServer struct { *csicommon.DefaultControllerServer MetadataStore util.CachePersister @@ -49,6 +51,8 @@ var ( rbdSnapshots = map[string]*rbdSnapshot{} ) +// LoadExDataFromMetadataStore loads the rbd volume and snapshot +// info from metadata store func (cs *ControllerServer) LoadExDataFromMetadataStore() error { vol := &rbdVolume{} cs.MetadataStore.ForAll("csi-rbd-vol-", vol, func(identifier string) error { @@ -80,6 +84,7 @@ func (cs *ControllerServer) validateVolumeReq(req *csi.CreateVolumeRequest) erro return nil } +// CreateVolume creates the volume in backend and store the volume metadata func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) (*csi.CreateVolumeResponse, error) { if err := cs.validateVolumeReq(req); err != nil { @@ -193,6 +198,8 @@ func (cs *ControllerServer) checkSnapshot(req *csi.CreateVolumeRequest, rbdVol * return nil } +// DeleteVolume deletes the volume in backend and removes the volume metadata +// from store func (cs *ControllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest) (*csi.DeleteVolumeResponse, error) { if err := cs.Driver.ValidateControllerServiceRequest(csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME); err != nil { glog.Warningf("invalid delete volume req: %v", req) @@ -227,6 +234,8 @@ func (cs *ControllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVol return &csi.DeleteVolumeResponse{}, nil } +// ValidateVolumeCapabilities checks whether the volume capabilities requested +// are supported. func (cs *ControllerServer) ValidateVolumeCapabilities(ctx context.Context, req *csi.ValidateVolumeCapabilitiesRequest) (*csi.ValidateVolumeCapabilitiesResponse, error) { for _, cap := range req.VolumeCapabilities { if cap.GetAccessMode().GetMode() != csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER { @@ -240,14 +249,18 @@ func (cs *ControllerServer) ValidateVolumeCapabilities(ctx context.Context, req }, nil } +// ControllerUnpublishVolume returns success response func (cs *ControllerServer) ControllerUnpublishVolume(ctx context.Context, req *csi.ControllerUnpublishVolumeRequest) (*csi.ControllerUnpublishVolumeResponse, error) { return &csi.ControllerUnpublishVolumeResponse{}, nil } +// ControllerPublishVolume returns success response func (cs *ControllerServer) ControllerPublishVolume(ctx context.Context, req *csi.ControllerPublishVolumeRequest) (*csi.ControllerPublishVolumeResponse, error) { return &csi.ControllerPublishVolumeResponse{}, nil } +// CreateSnapshot creates the snapshot in backend and stores metadata +// in store func (cs *ControllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshotRequest) (*csi.CreateSnapshotResponse, error) { if err := cs.Driver.ValidateControllerServiceRequest(csi.ControllerServiceCapability_RPC_CREATE_DELETE_SNAPSHOT); err != nil { glog.Warningf("invalid create snapshot req: %v", req) @@ -371,6 +384,8 @@ func (cs *ControllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS }, nil } +// DeleteSnapshot deletes the snapshot in backend and removes the +//snapshot metadata from store func (cs *ControllerServer) DeleteSnapshot(ctx context.Context, req *csi.DeleteSnapshotRequest) (*csi.DeleteSnapshotResponse, error) { if err := cs.Driver.ValidateControllerServiceRequest(csi.ControllerServiceCapability_RPC_CREATE_DELETE_SNAPSHOT); err != nil { glog.Warningf("invalid delete snapshot req: %v", req) @@ -410,6 +425,7 @@ func (cs *ControllerServer) DeleteSnapshot(ctx context.Context, req *csi.DeleteS return &csi.DeleteSnapshotResponse{}, nil } +// ListSnapshots lists the snapshots in the store func (cs *ControllerServer) ListSnapshots(ctx context.Context, req *csi.ListSnapshotsRequest) (*csi.ListSnapshotsResponse, error) { if err := cs.Driver.ValidateControllerServiceRequest(csi.ControllerServiceCapability_RPC_LIST_SNAPSHOTS); err != nil { glog.Warningf("invalid list snapshot req: %v", req) diff --git a/pkg/rbd/identityserver.go b/pkg/rbd/identityserver.go index 759903aa6..856bc50a8 100644 --- a/pkg/rbd/identityserver.go +++ b/pkg/rbd/identityserver.go @@ -23,10 +23,13 @@ import ( "github.com/kubernetes-csi/drivers/pkg/csi-common" ) +// IdentityServer struct of rbd CSI driver with supported methods of CSI +// identity server spec. type IdentityServer struct { *csicommon.DefaultIdentityServer } +// GetPluginCapabilities returns available capabilities of the rbd driver func (is *IdentityServer) GetPluginCapabilities(ctx context.Context, req *csi.GetPluginCapabilitiesRequest) (*csi.GetPluginCapabilitiesResponse, error) { return &csi.GetPluginCapabilitiesResponse{ Capabilities: []*csi.PluginCapability{ diff --git a/pkg/rbd/nodeserver.go b/pkg/rbd/nodeserver.go index e5696f5d5..835728ad7 100644 --- a/pkg/rbd/nodeserver.go +++ b/pkg/rbd/nodeserver.go @@ -35,11 +35,28 @@ import ( "github.com/kubernetes-csi/drivers/pkg/csi-common" ) +// NodeServer struct of ceph rbd driver with supported methods of CSI +// node server spec type NodeServer struct { *csicommon.DefaultNodeServer mounter mount.Interface } +//TODO remove both stage and unstage methods +//once https://github.com/kubernetes-csi/drivers/pull/145 is merged + +// NodeStageVolume returns unimplemented response +func (ns *NodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolumeRequest) (*csi.NodeStageVolumeResponse, error) { + return nil, status.Error(codes.Unimplemented, "") +} + +// NodeUnstageVolume returns unimplemented response +func (ns *NodeServer) NodeUnstageVolume(ctx context.Context, req *csi.NodeUnstageVolumeRequest) (*csi.NodeUnstageVolumeResponse, error) { + return nil, status.Error(codes.Unimplemented, "") +} + +// NodePublishVolume mounts the volume mounted to the device path to the target +// path func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolumeRequest) (*csi.NodePublishVolumeResponse, error) { targetPath := req.GetTargetPath() targetPathMutex.LockKey(targetPath) @@ -132,6 +149,7 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis return &csi.NodePublishVolumeResponse{}, nil } +// NodeUnpublishVolume unmounts the volume from the target path func (ns *NodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpublishVolumeRequest) (*csi.NodeUnpublishVolumeResponse, error) { targetPath := req.GetTargetPath() targetPathMutex.LockKey(targetPath) @@ -202,26 +220,6 @@ func (ns *NodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpu return &csi.NodeUnpublishVolumeResponse{}, nil } -func (ns *NodeServer) NodeStageVolume( - ctx context.Context, - req *csi.NodeStageVolumeRequest) ( - *csi.NodeStageVolumeResponse, error) { - - return nil, status.Error(codes.Unimplemented, "") -} - -func (ns *NodeServer) NodeUnstageVolume( - ctx context.Context, - req *csi.NodeUnstageVolumeRequest) ( - *csi.NodeUnstageVolumeResponse, error) { - - return nil, status.Error(codes.Unimplemented, "") -} - -func (ns *NodeServer) NodeGetInfo(ctx context.Context, req *csi.NodeGetInfoRequest) (*csi.NodeGetInfoResponse, error) { - return ns.DefaultNodeServer.NodeGetInfo(ctx, req) -} - func resolveBindMountedBlockDevice(mountPath string) (string, error) { cmd := exec.Command("findmnt", "-n", "-o", "SOURCE", "--first-only", "--target", mountPath) out, err := cmd.CombinedOutput() diff --git a/pkg/rbd/rbd.go b/pkg/rbd/rbd.go index abe2bdff4..79c44c846 100644 --- a/pkg/rbd/rbd.go +++ b/pkg/rbd/rbd.go @@ -35,6 +35,7 @@ const ( rbdDefaultUserID = rbdDefaultAdminID ) +// Driver contains the default identity,node and controller struct type Driver struct { cd *csicommon.CSIDriver @@ -47,16 +48,19 @@ var ( version = "1.0.0" ) -func GetDriver() *Driver { +// NewDriver returns new rbd driver +func NewDriver() *Driver { return &Driver{} } +// NewIdentityServer initialize a identity server for rbd CSI driver func NewIdentityServer(d *csicommon.CSIDriver) *IdentityServer { return &IdentityServer{ DefaultIdentityServer: csicommon.NewDefaultIdentityServer(d), } } +// NewControllerServer initialize a controller server for rbd CSI driver func NewControllerServer(d *csicommon.CSIDriver, cachePersister util.CachePersister) *ControllerServer { return &ControllerServer{ DefaultControllerServer: csicommon.NewDefaultControllerServer(d), @@ -64,6 +68,7 @@ func NewControllerServer(d *csicommon.CSIDriver, cachePersister util.CachePersis } } +// NewNodeServer initialize a node server for rbd CSI driver. func NewNodeServer(d *csicommon.CSIDriver, containerized bool) (*NodeServer, error) { mounter := mount.New("") if containerized { @@ -79,6 +84,8 @@ func NewNodeServer(d *csicommon.CSIDriver, containerized bool) (*NodeServer, err }, nil } +// Run start a non-blocking grpc controller,node and identityserver for +// rbd CSI driver which can serve multiple parallel requests func (r *Driver) Run(driverName, nodeID, endpoint string, containerized bool, cachePersister util.CachePersister) { var err error glog.Infof("Driver: %v version: %v", driverName, version) diff --git a/pkg/util/cachepersister.go b/pkg/util/cachepersister.go index c764cc026..3795cebf0 100644 --- a/pkg/util/cachepersister.go +++ b/pkg/util/cachepersister.go @@ -23,11 +23,14 @@ import ( ) const ( + //PluginFolder defines location of plugins PluginFolder = "/var/lib/kubelet/plugins" ) +// ForAllFunc stores metdata with identifier type ForAllFunc func(identifier string) error +// CachePersister interface implemented for store type CachePersister interface { Create(identifier string, data interface{}) error Get(identifier string, data interface{}) error @@ -35,6 +38,7 @@ type CachePersister interface { Delete(identifier string) error } +// NewCachePersister returns CachePersister based on store func NewCachePersister(metadataStore, driverName string) (CachePersister, error) { if metadataStore == "k8s_configmap" { glog.Infof("cache-perister: using kubernetes configmap as metadata cache persister") diff --git a/pkg/util/k8scmcache.go b/pkg/util/k8scmcache.go index 7f5190026..bdcb75d9d 100644 --- a/pkg/util/k8scmcache.go +++ b/pkg/util/k8scmcache.go @@ -33,6 +33,7 @@ import ( "k8s.io/client-go/tools/clientcmd" ) +// K8sCMCache to store metadata type K8sCMCache struct { Client *k8s.Clientset Namespace string @@ -47,6 +48,8 @@ const ( csiMetadataLabelAttr = "com.ceph.ceph-csi/metadata" ) +// GetK8sNamespace returns pod namespace. if pod namespace is empty +// it returns default namespace func GetK8sNamespace() string { namespace := os.Getenv("POD_NAMESPACE") if namespace == "" { @@ -55,6 +58,7 @@ func GetK8sNamespace() string { return namespace } +// NewK8sClient create kubernetes client func NewK8sClient() *k8s.Clientset { var cfg *rest.Config var err error @@ -88,6 +92,7 @@ func (k8scm *K8sCMCache) getMetadataCM(resourceID string) (*v1.ConfigMap, error) return cm, nil } +//ForAll list the metadata in configmaps and filters outs based on the pattern func (k8scm *K8sCMCache) ForAll(pattern string, destObj interface{}, f ForAllFunc) error { listOpts := metav1.ListOptions{LabelSelector: fmt.Sprintf("%s=%s", csiMetadataLabelAttr, cmLabel)} cms, err := k8scm.Client.CoreV1().ConfigMaps(k8scm.Namespace).List(listOpts) @@ -114,6 +119,7 @@ func (k8scm *K8sCMCache) ForAll(pattern string, destObj interface{}, f ForAllFun return nil } +// Create stores the metadata in configmaps with identifier name func (k8scm *K8sCMCache) Create(identifier string, data interface{}) error { cm, err := k8scm.getMetadataCM(identifier) if cm != nil && err == nil { @@ -149,6 +155,7 @@ func (k8scm *K8sCMCache) Create(identifier string, data interface{}) error { return nil } +// Get retrieves the metadata in configmaps with identifier name func (k8scm *K8sCMCache) Get(identifier string, data interface{}) error { cm, err := k8scm.getMetadataCM(identifier) if err != nil { @@ -161,6 +168,7 @@ func (k8scm *K8sCMCache) Get(identifier string, data interface{}) error { return nil } +// Delete deletes the metadata in configmaps with identifier name func (k8scm *K8sCMCache) Delete(identifier string) error { err := k8scm.Client.CoreV1().ConfigMaps(k8scm.Namespace).Delete(identifier, nil) if err != nil { diff --git a/pkg/util/nodecache.go b/pkg/util/nodecache.go index 85092d582..10cf1dc45 100644 --- a/pkg/util/nodecache.go +++ b/pkg/util/nodecache.go @@ -29,12 +29,14 @@ import ( "github.com/pkg/errors" ) +// NodeCache to store metadata type NodeCache struct { BasePath string } var cacheDir = "controller" +// EnsureCacheDirectory creates cache directory if not present func (nc *NodeCache) EnsureCacheDirectory(cacheDir string) error { fullPath := path.Join(nc.BasePath, cacheDir) if _, err := os.Stat(fullPath); os.IsNotExist(err) { @@ -45,6 +47,7 @@ func (nc *NodeCache) EnsureCacheDirectory(cacheDir string) error { return nil } +//ForAll list the metadata in Nodecache and filters outs based on the pattern func (nc *NodeCache) ForAll(pattern string, destObj interface{}, f ForAllFunc) error { err := nc.EnsureCacheDirectory(cacheDir) if err != nil { @@ -80,6 +83,7 @@ func (nc *NodeCache) ForAll(pattern string, destObj interface{}, f ForAllFunc) e return nil } +// Create creates the metadata file in cache directory with identifier name func (nc *NodeCache) Create(identifier string, data interface{}) error { file := path.Join(nc.BasePath, cacheDir, identifier+".json") fp, err := os.Create(file) @@ -95,6 +99,7 @@ func (nc *NodeCache) Create(identifier string, data interface{}) error { return nil } +// Get retrieves the metadata from cache directory with identifier name func (nc *NodeCache) Get(identifier string, data interface{}) error { file := path.Join(nc.BasePath, cacheDir, identifier+".json") fp, err := os.Open(file) @@ -111,6 +116,7 @@ func (nc *NodeCache) Get(identifier string, data interface{}) error { return nil } +// Delete deletes the metadata file from cache directory with identifier name func (nc *NodeCache) Delete(identifier string) error { file := path.Join(nc.BasePath, cacheDir, identifier+".json") err := os.Remove(file) From d625eb7f54c8ae88897f887fc01782295653cc74 Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Mon, 28 Jan 2019 17:28:11 +0530 Subject: [PATCH 02/11] update travis to add gometalinter static check Signed-off-by: Madhu Rajanna --- .travis.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index a411ce0c2..75612b9bb 100644 --- a/.travis.yml +++ b/.travis.yml @@ -9,13 +9,16 @@ branches: go: 1.10.x +install: + - curl -L https://git.io/vp6lP | sh + before_script: - GO_FILES=$(find . -iname '*.go' -type f | grep -v /vendor/) - go get -u golang.org/x/lint/golint #go get github.com/golang/lint/golint script: + - gometalinter --deadline=10m -j 4 --disable=gocyclo --enable=gosimple --enable=misspell --enable=unused --enable=unparam --enable=staticcheck --vendor ./... - test -z $(gofmt -s -l $GO_FILES) - - go vet -v $(go list ./... | grep -v /vendor/) - make rbdplugin - make cephfsplugin From 58997ecbd8e25393ccaa44ad3ad133d2f58dc174 Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Mon, 28 Jan 2019 17:38:16 +0530 Subject: [PATCH 03/11] update go version to 1.11.x Signed-off-by: Madhu Rajanna --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 75612b9bb..4df5abc8b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -7,7 +7,7 @@ branches: - master - csi-v1.0 -go: 1.10.x +go: 1.11.x install: - curl -L https://git.io/vp6lP | sh From c0182c5881ead92675b7e47957096b32081c6841 Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Mon, 28 Jan 2019 17:45:35 +0530 Subject: [PATCH 04/11] Add megacheck for gometalinter Signed-off-by: Madhu Rajanna --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 4df5abc8b..5681ba437 100644 --- a/.travis.yml +++ b/.travis.yml @@ -17,7 +17,7 @@ before_script: - go get -u golang.org/x/lint/golint #go get github.com/golang/lint/golint script: - - gometalinter --deadline=10m -j 4 --disable=gocyclo --enable=gosimple --enable=misspell --enable=unused --enable=unparam --enable=staticcheck --vendor ./... + - gometalinter --deadline=10m -j 4 --disable=gocyclo --enable=megacheck --enable=misspell --enable=unparam --vendor ./... - test -z $(gofmt -s -l $GO_FILES) - make rbdplugin - make cephfsplugin From 008c82c1e70a099b0ee5723115cfb2f1851b8e7d Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Mon, 28 Jan 2019 17:48:19 +0530 Subject: [PATCH 05/11] Fix gometalinter issues Signed-off-by: Madhu Rajanna --- pkg/cephfs/cephconf.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/cephfs/cephconf.go b/pkg/cephfs/cephconf.go index 9277e63a4..f122332a2 100644 --- a/pkg/cephfs/cephconf.go +++ b/pkg/cephfs/cephconf.go @@ -37,8 +37,10 @@ const cephKeyring = `[client.{{.UserID}}] key = {{.Key}} ` +// gosec const cephSecret = `{{.Key}}` +// gosec const ( cephConfigRoot = "/etc/ceph" cephConfigFileNameFmt = "ceph.share.%s.conf" @@ -74,6 +76,7 @@ type cephConfigData struct { } func writeCephTemplate(fileName string, m os.FileMode, t *template.Template, data interface{}) error { + // gosec if err := os.MkdirAll(cephConfigRoot, 0755); err != nil { return err } From 7a0c233c27aabd96d51a44ab8957fa26c542cddc Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Mon, 28 Jan 2019 19:29:16 +0530 Subject: [PATCH 06/11] Fix issues found in gometalinter Signed-off-by: Madhu Rajanna --- cmd/cephfs/main.go | 5 ++++- cmd/rbd/main.go | 8 ++++++-- pkg/cephfs/cephconf.go | 8 +++++++- pkg/cephfs/cephuser.go | 16 +++++++++++++--- pkg/cephfs/controllerserver.go | 3 ++- pkg/cephfs/nodeserver.go | 18 ++++++++++++------ pkg/cephfs/volume.go | 19 +++++++++++++++---- pkg/cephfs/volumeoptions.go | 5 ++++- pkg/rbd/controllerserver.go | 31 +++++++++++++++++++++++++++---- pkg/rbd/nodeserver.go | 29 +++++++++++++++++++---------- pkg/rbd/rbd.go | 5 ++++- pkg/rbd/rbd_attach.go | 7 ++++++- pkg/util/nodecache.go | 20 +++++++++++++++++--- 13 files changed, 136 insertions(+), 38 deletions(-) diff --git a/cmd/cephfs/main.go b/cmd/cephfs/main.go index 3d54b87c4..ece731eac 100644 --- a/cmd/cephfs/main.go +++ b/cmd/cephfs/main.go @@ -27,7 +27,10 @@ import ( ) func init() { - flag.Set("logtostderr", "true") + if err := flag.Set("logtostderr", "true"); err != nil { + glog.Errorf("failed to set logtostderr flag: %v", err) + os.Exit(1) + } } var ( diff --git a/cmd/rbd/main.go b/cmd/rbd/main.go index c89092845..9bcd8a3d8 100644 --- a/cmd/rbd/main.go +++ b/cmd/rbd/main.go @@ -27,7 +27,10 @@ import ( ) func init() { - flag.Set("logtostderr", "true") + if err := flag.Set("logtostderr", "true"); err != nil { + glog.Errorf("failed to set logtostderr flag: %v", err) + os.Exit(1) + } } var ( @@ -64,10 +67,11 @@ func main() { func createPersistentStorage(persistentStoragePath string) error { if _, err := os.Stat(persistentStoragePath); os.IsNotExist(err) { - if err := os.MkdirAll(persistentStoragePath, os.FileMode(0755)); err != nil { + if err = os.MkdirAll(persistentStoragePath, os.FileMode(0755)); err != nil { return err } } else { + return err } return nil } diff --git a/pkg/cephfs/cephconf.go b/pkg/cephfs/cephconf.go index f122332a2..d9a0cebbd 100644 --- a/pkg/cephfs/cephconf.go +++ b/pkg/cephfs/cephconf.go @@ -21,6 +21,8 @@ import ( "os" "path" "text/template" + + "github.com/golang/glog" ) const cephConfig = `[global] @@ -89,7 +91,11 @@ func writeCephTemplate(fileName string, m os.FileMode, t *template.Template, dat return err } - defer f.Close() + defer func() { + if err := f.Close(); err != nil { + glog.Errorf("failed to close file %s with error %s", f.Name(), err) + } + }() return t.Execute(f, data) } diff --git a/pkg/cephfs/cephuser.go b/pkg/cephfs/cephuser.go index 48675e054..9536b4483 100644 --- a/pkg/cephfs/cephuser.go +++ b/pkg/cephfs/cephuser.go @@ -21,6 +21,8 @@ import ( "encoding/json" "fmt" "os" + + "github.com/golang/glog" ) const ( @@ -111,12 +113,20 @@ func deleteCephUser(adminCr *credentials, volID volumeID) error { "auth", "rm", cephEntityClientPrefix + userID, } - if err := execCommandAndValidate("ceph", args[:]...); err != nil { + var err error + if err = execCommandAndValidate("ceph", args[:]...); err != nil { return err } - os.Remove(getCephKeyringPath(volID, userID)) - os.Remove(getCephSecretPath(volID, userID)) + keyringPath := getCephKeyringPath(volID, userID) + if err = os.Remove(keyringPath); err != nil { + glog.Errorf("failed to remove keyring file %s with error %s", keyringPath, err) + } + + secretPath := getCephSecretPath(volID, userID) + if err = os.Remove(secretPath); err != nil { + glog.Errorf("failed to remove secret file %s with error %s", secretPath, err) + } return nil } diff --git a/pkg/cephfs/controllerserver.go b/pkg/cephfs/controllerserver.go index f42780114..20b6fe729 100644 --- a/pkg/cephfs/controllerserver.go +++ b/pkg/cephfs/controllerserver.go @@ -132,7 +132,8 @@ func (cs *ControllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVol // mons may have changed since create volume, // retrieve the latest mons and override old mons secret := req.GetSecrets() - if mon, err := getMonValFromSecret(secret); err == nil && len(mon) > 0 { + mon := "" + if mon, err = getMonValFromSecret(secret); err == nil && len(mon) > 0 { glog.Infof("override old mons [%q] with [%q]", ce.VolOptions.Monitors, mon) ce.VolOptions.Monitors = mon } diff --git a/pkg/cephfs/nodeserver.go b/pkg/cephfs/nodeserver.go index 2c7c3b658..412196929 100644 --- a/pkg/cephfs/nodeserver.go +++ b/pkg/cephfs/nodeserver.go @@ -197,18 +197,21 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis // NodeUnpublishVolume unmounts the volume from the target path func (ns *NodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpublishVolumeRequest) (*csi.NodeUnpublishVolumeResponse, error) { - if err := validateNodeUnpublishVolumeRequest(req); err != nil { + var err error + if err = validateNodeUnpublishVolumeRequest(req); err != nil { return nil, status.Error(codes.InvalidArgument, err.Error()) } targetPath := req.GetTargetPath() // Unmount the bind-mount - if err := unmountVolume(targetPath); err != nil { + if err = unmountVolume(targetPath); err != nil { return nil, status.Error(codes.Internal, err.Error()) } - os.Remove(targetPath) + if err = os.Remove(targetPath); err != nil { + return nil, status.Error(codes.Internal, err.Error()) + } glog.Infof("cephfs: successfully unbinded volume %s from %s", req.GetVolumeId(), targetPath) @@ -217,18 +220,21 @@ func (ns *NodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpu // NodeUnstageVolume unstages the volume from the staging path func (ns *NodeServer) NodeUnstageVolume(ctx context.Context, req *csi.NodeUnstageVolumeRequest) (*csi.NodeUnstageVolumeResponse, error) { - if err := validateNodeUnstageVolumeRequest(req); err != nil { + var err error + if err = validateNodeUnstageVolumeRequest(req); err != nil { return nil, status.Error(codes.InvalidArgument, err.Error()) } stagingTargetPath := req.GetStagingTargetPath() // Unmount the volume - if err := unmountVolume(stagingTargetPath); err != nil { + if err = unmountVolume(stagingTargetPath); err != nil { return nil, status.Error(codes.Internal, err.Error()) } - os.Remove(stagingTargetPath) + if err = os.Remove(stagingTargetPath); err != nil { + return nil, status.Error(codes.Internal, err.Error()) + } glog.Infof("cephfs: successfully umounted volume %s from %s", req.GetVolumeId(), stagingTargetPath) diff --git a/pkg/cephfs/volume.go b/pkg/cephfs/volume.go index ce6f01a17..51d290dae 100644 --- a/pkg/cephfs/volume.go +++ b/pkg/cephfs/volume.go @@ -20,6 +20,8 @@ import ( "fmt" "os" "path" + + "github.com/golang/glog" ) const ( @@ -70,8 +72,7 @@ func createVolume(volOptions *volumeOptions, adminCr *credentials, volID volumeI } defer func() { - unmountVolume(cephRoot) - os.Remove(cephRoot) + umountAndRemove(cephRoot) }() volOptions.RootPath = getVolumeRootPathCeph(volID) @@ -123,8 +124,7 @@ func purgeVolume(volID volumeID, adminCr *credentials, volOptions *volumeOptions } defer func() { - unmountVolume(volRoot) - os.Remove(volRoot) + umountAndRemove(volRoot) }() if err := os.Rename(volRoot, volRootDeleting); err != nil { @@ -137,3 +137,14 @@ func purgeVolume(volID volumeID, adminCr *credentials, volOptions *volumeOptions return nil } + +func umountAndRemove(mountPoint string) { + var err error + if err = unmountVolume(mountPoint); err != nil { + glog.Errorf("failed to unmount %s with error %s", mountPoint, err) + } + + if err = os.Remove(mountPoint); err != nil { + glog.Errorf("failed to remove %s with error %s", mountPoint, err) + } +} diff --git a/pkg/cephfs/volumeoptions.go b/pkg/cephfs/volumeoptions.go index 8b7028991..ce59d9c2a 100644 --- a/pkg/cephfs/volumeoptions.go +++ b/pkg/cephfs/volumeoptions.go @@ -102,7 +102,8 @@ func newVolumeOptions(volOptions, secret map[string]string) (*volumeOptions, err // extract mon from secret first if err = extractOption(&opts.MonValueFromSecret, "monValueFromSecret", volOptions); err == nil { - if mon, err := getMonValFromSecret(secret); err == nil && len(mon) > 0 { + mon := "" + if mon, err = getMonValFromSecret(secret); err == nil && len(mon) > 0 { opts.Monitors = mon } } @@ -131,6 +132,8 @@ func newVolumeOptions(volOptions, secret map[string]string) (*volumeOptions, err } // This field is optional, don't check for its presence + // nolint: errcheck + // (skip errcheck as this is optional) extractOption(&opts.Mounter, "mounter", volOptions) if err = opts.validate(); err != nil { diff --git a/pkg/rbd/controllerserver.go b/pkg/rbd/controllerserver.go index 7d5fb8205..ab17f4c66 100644 --- a/pkg/rbd/controllerserver.go +++ b/pkg/rbd/controllerserver.go @@ -55,16 +55,19 @@ var ( // info from metadata store func (cs *ControllerServer) LoadExDataFromMetadataStore() error { vol := &rbdVolume{} + // nolint: errcheck cs.MetadataStore.ForAll("csi-rbd-vol-", vol, func(identifier string) error { rbdVolumes[identifier] = vol return nil }) snap := &rbdSnapshot{} + // nolint: errcheck cs.MetadataStore.ForAll("csi-rbd-(.*)-snap-", snap, func(identifier string) error { rbdSnapshots[identifier] = snap return nil }) + glog.Infof("Loaded %d volumes and %d snapshots from metadata store", len(rbdVolumes), len(rbdSnapshots)) return nil } @@ -91,7 +94,11 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol return nil, err } volumeNameMutex.LockKey(req.GetName()) - defer volumeNameMutex.UnlockKey(req.GetName()) + defer func() { + if err := volumeNameMutex.UnlockKey(req.GetName()); err != nil { + glog.Warningf("failed to unlock mutex volume:%s %v", req.GetName(), err) + } + }() // Need to check for already existing volume name, and if found // check for the requested capacity and already allocated capacity @@ -208,7 +215,13 @@ func (cs *ControllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVol // For now the image get unconditionally deleted, but here retention policy can be checked volumeID := req.GetVolumeId() volumeIDMutex.LockKey(volumeID) - defer volumeIDMutex.UnlockKey(volumeID) + + defer func() { + if err := volumeIDMutex.UnlockKey(volumeID); err != nil { + glog.Warningf("failed to unlock mutex volume:%s %v", volumeID, err) + } + }() + rbdVol := &rbdVolume{} if err := cs.MetadataStore.Get(volumeID, rbdVol); err != nil { if os.IsNotExist(errors.Cause(err)) { @@ -276,7 +289,12 @@ func (cs *ControllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS } snapshotNameMutex.LockKey(req.GetName()) - defer snapshotNameMutex.UnlockKey(req.GetName()) + + defer func() { + if err := snapshotNameMutex.UnlockKey(req.GetName()); err != nil { + glog.Warningf("failed to unlock mutex snapshot:%s %v", req.GetName(), err) + } + }() // Need to check for already existing snapshot name, and if found // check for the requested source volume id and already allocated source volume id @@ -397,7 +415,12 @@ func (cs *ControllerServer) DeleteSnapshot(ctx context.Context, req *csi.DeleteS return nil, status.Error(codes.InvalidArgument, "Snapshot ID cannot be empty") } snapshotIDMutex.LockKey(snapshotID) - defer snapshotIDMutex.UnlockKey(snapshotID) + + defer func() { + if err := snapshotIDMutex.UnlockKey(snapshotID); err != nil { + glog.Warningf("failed to unlock mutex snapshot:%s %v", snapshotID, err) + } + }() rbdSnap := &rbdSnapshot{} if err := cs.MetadataStore.Get(snapshotID, rbdSnap); err != nil { diff --git a/pkg/rbd/nodeserver.go b/pkg/rbd/nodeserver.go index 835728ad7..a54bf7058 100644 --- a/pkg/rbd/nodeserver.go +++ b/pkg/rbd/nodeserver.go @@ -60,7 +60,12 @@ func (ns *NodeServer) NodeUnstageVolume(ctx context.Context, req *csi.NodeUnstag func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolumeRequest) (*csi.NodePublishVolumeResponse, error) { targetPath := req.GetTargetPath() targetPathMutex.LockKey(targetPath) - defer targetPathMutex.UnlockKey(targetPath) + + defer func() { + if err := targetPathMutex.UnlockKey(targetPath); err != nil { + glog.Warningf("failed to unlock mutex targetpath:%s %v", targetPath, err) + } + }() var volName string isBlock := req.GetVolumeCapability().GetBlock() != nil @@ -84,12 +89,12 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis if os.IsNotExist(err) { if isBlock { // create an empty file - targetPathFile, err := os.OpenFile(targetPath, os.O_CREATE|os.O_RDWR, 0750) - if err != nil { + targetPathFile, e := os.OpenFile(targetPath, os.O_CREATE|os.O_RDWR, 0750) + if e != nil { glog.V(4).Infof("Failed to create targetPath:%s with error: %v", targetPath, err) - return nil, status.Error(codes.Internal, err.Error()) + return nil, status.Error(codes.Internal, e.Error()) } - if err := targetPathFile.Close(); err != nil { + if err = targetPathFile.Close(); err != nil { glog.V(4).Infof("Failed to close targetPath:%s with error: %v", targetPath, err) return nil, status.Error(codes.Internal, err.Error()) } @@ -153,7 +158,12 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis func (ns *NodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpublishVolumeRequest) (*csi.NodeUnpublishVolumeResponse, error) { targetPath := req.GetTargetPath() targetPathMutex.LockKey(targetPath) - defer targetPathMutex.UnlockKey(targetPath) + + defer func() { + if err := targetPathMutex.UnlockKey(targetPath); err != nil { + glog.Warningf("failed to unlock mutex targetpath:%s %v", targetPath, err) + } + }() notMnt, err := ns.mounter.IsNotMountPoint(targetPath) if err != nil { @@ -177,7 +187,6 @@ func (ns *NodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpu // Bind mounted device needs to be resolved by using resolveBindMountedBlockDevice if devicePath == "devtmpfs" { - var err error devicePath, err = resolveBindMountedBlockDevice(targetPath) if err != nil { return nil, status.Error(codes.Internal, err.Error()) @@ -206,13 +215,13 @@ func (ns *NodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpu } // Unmapping rbd device - if err := detachRBDDevice(devicePath); err != nil { + if err = detachRBDDevice(devicePath); err != nil { glog.V(3).Infof("failed to unmap rbd device: %s with error: %v", devicePath, err) return nil, err } // Remove targetPath - if err := os.RemoveAll(targetPath); err != nil { + if err = os.RemoveAll(targetPath); err != nil { glog.V(3).Infof("failed to remove targetPath: %s with error: %v", targetPath, err) return nil, err } @@ -240,7 +249,7 @@ func parseFindMntResolveSource(out string) (string, error) { return match[1], nil } // Check if out is a block device - reBlk := regexp.MustCompile("^devtmpfs\\[(/[^/]+(?:/[^/]*)*)\\]$") + reBlk := regexp.MustCompile(`^devtmpfs\\[(/[^/]+(?:/[^/]*)*)\\]$`) if match := reBlk.FindStringSubmatch(out); match != nil { return fmt.Sprintf("/dev%s", match[1]), nil } diff --git a/pkg/rbd/rbd.go b/pkg/rbd/rbd.go index 79c44c846..ba83b325f 100644 --- a/pkg/rbd/rbd.go +++ b/pkg/rbd/rbd.go @@ -112,7 +112,10 @@ func (r *Driver) Run(driverName, nodeID, endpoint string, containerized bool, ca } r.cs = NewControllerServer(r.cd, cachePersister) - r.cs.LoadExDataFromMetadataStore() + + if err = r.cs.LoadExDataFromMetadataStore(); err != nil { + glog.Fatalf("failed to load metadata from store, err %v\n", err) + } s := csicommon.NewNonBlockingGRPCServer() s.Start(endpoint, r.ids, r.cs, r.ns) diff --git a/pkg/rbd/rbd_attach.go b/pkg/rbd/rbd_attach.go index ee7b39a2c..1cca3f8b7 100644 --- a/pkg/rbd/rbd_attach.go +++ b/pkg/rbd/rbd_attach.go @@ -229,7 +229,12 @@ func attachRBDImage(volOptions *rbdVolume, userID string, credentials map[string devicePath, found := waitForPath(volOptions.Pool, image, 1, useNBD) if !found { attachdetachMutex.LockKey(imagePath) - defer attachdetachMutex.UnlockKey(imagePath) + + defer func() { + if err = attachdetachMutex.UnlockKey(imagePath); err != nil { + glog.Warningf("failed to unlock mutex imagepath:%s %v", imagePath, err) + } + }() _, err = execCommand("modprobe", []string{moduleName}) if err != nil { diff --git a/pkg/util/nodecache.go b/pkg/util/nodecache.go index 10cf1dc45..f4a1c566f 100644 --- a/pkg/util/nodecache.go +++ b/pkg/util/nodecache.go @@ -73,7 +73,10 @@ func (nc *NodeCache) ForAll(pattern string, destObj interface{}, f ForAllFunc) e } decoder := json.NewDecoder(fp) if err = decoder.Decode(destObj); err != nil { - fp.Close() + if err = fp.Close(); err != nil { + return errors.Wrapf(err, "failed to close file %s", file.Name()) + + } return errors.Wrapf(err, "node-cache: couldn't decode file %s", file.Name()) } if err := f(strings.TrimSuffix(file.Name(), filepath.Ext(file.Name()))); err != nil { @@ -90,7 +93,13 @@ func (nc *NodeCache) Create(identifier string, data interface{}) error { if err != nil { return errors.Wrapf(err, "node-cache: failed to create metadata storage file %s\n", file) } - defer fp.Close() + + defer func() { + if err = fp.Close(); err != nil { + glog.Warningf("failed to close file:%s %v", fp.Name(), err) + } + }() + encoder := json.NewEncoder(fp) if err = encoder.Encode(data); err != nil { return errors.Wrapf(err, "node-cache: failed to encode metadata for file: %s\n", file) @@ -106,7 +115,12 @@ func (nc *NodeCache) Get(identifier string, data interface{}) error { if err != nil { return errors.Wrapf(err, "node-cache: open error for %s", file) } - defer fp.Close() + + defer func() { + if err = fp.Close(); err != nil { + glog.Warningf("failed to close file:%s %v", fp.Name(), err) + } + }() decoder := json.NewDecoder(fp) if err = decoder.Decode(data); err != nil { From ca2e4752962a45835fff94727780cac3b9817fcd Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Tue, 29 Jan 2019 01:25:10 +0530 Subject: [PATCH 07/11] Fix gometalinter issues Signed-off-by: Madhu Rajanna --- pkg/cephfs/cephconf.go | 8 +- pkg/cephfs/util.go | 2 +- pkg/cephfs/volumemounter.go | 2 + pkg/cephfs/volumeoptions.go | 4 +- pkg/rbd/controllerserver.go | 177 +++++++++++++++++++++--------------- pkg/rbd/nodeserver.go | 113 ++++++++++++++--------- pkg/rbd/rbd_attach.go | 38 +++++--- pkg/rbd/rbd_util.go | 3 +- pkg/util/nodecache.go | 3 + 9 files changed, 214 insertions(+), 136 deletions(-) diff --git a/pkg/cephfs/cephconf.go b/pkg/cephfs/cephconf.go index d9a0cebbd..d690d7eaa 100644 --- a/pkg/cephfs/cephconf.go +++ b/pkg/cephfs/cephconf.go @@ -39,15 +39,13 @@ const cephKeyring = `[client.{{.UserID}}] key = {{.Key}} ` -// gosec -const cephSecret = `{{.Key}}` +const cephSecret = `{{.Key}}` // #nosec -// gosec const ( cephConfigRoot = "/etc/ceph" cephConfigFileNameFmt = "ceph.share.%s.conf" cephKeyringFileNameFmt = "ceph.share.%s.client.%s.keyring" - cephSecretFileNameFmt = "ceph.share.%s.client.%s.secret" + cephSecretFileNameFmt = "ceph.share.%s.client.%s.secret" // #nosec ) var ( @@ -78,7 +76,7 @@ type cephConfigData struct { } func writeCephTemplate(fileName string, m os.FileMode, t *template.Template, data interface{}) error { - // gosec + // #nosec if err := os.MkdirAll(cephConfigRoot, 0755); err != nil { return err } diff --git a/pkg/cephfs/util.go b/pkg/cephfs/util.go index 44b93cc13..fe5eca6fe 100644 --- a/pkg/cephfs/util.go +++ b/pkg/cephfs/util.go @@ -40,7 +40,7 @@ func makeVolumeID(volName string) volumeID { func execCommand(command string, args ...string) ([]byte, error) { glog.V(4).Infof("cephfs: EXEC %s %s", command, args) - cmd := exec.Command(command, args...) + cmd := exec.Command(command, args...) // #nosec return cmd.CombinedOutput() } diff --git a/pkg/cephfs/volumemounter.go b/pkg/cephfs/volumemounter.go index 58ff48abd..16c2ad27c 100644 --- a/pkg/cephfs/volumemounter.go +++ b/pkg/cephfs/volumemounter.go @@ -36,7 +36,9 @@ var ( // Load available ceph mounters installed on system into availableMounters // Called from driver.go's Run() func loadAvailableMounters() error { + // #nosec fuseMounterProbe := exec.Command("ceph-fuse", "--version") + // #nosec kernelMounterProbe := exec.Command("mount.ceph") if fuseMounterProbe.Run() == nil { diff --git a/pkg/cephfs/volumeoptions.go b/pkg/cephfs/volumeoptions.go index ce59d9c2a..1dbb55122 100644 --- a/pkg/cephfs/volumeoptions.go +++ b/pkg/cephfs/volumeoptions.go @@ -132,8 +132,8 @@ func newVolumeOptions(volOptions, secret map[string]string) (*volumeOptions, err } // This field is optional, don't check for its presence - // nolint: errcheck - // (skip errcheck as this is optional) + // nolint + // (skip errcheck and gosec as this is optional) extractOption(&opts.Mounter, "mounter", volOptions) if err = opts.validate(); err != nil { diff --git a/pkg/rbd/controllerserver.go b/pkg/rbd/controllerserver.go index ab17f4c66..e79cf4258 100644 --- a/pkg/rbd/controllerserver.go +++ b/pkg/rbd/controllerserver.go @@ -55,14 +55,14 @@ var ( // info from metadata store func (cs *ControllerServer) LoadExDataFromMetadataStore() error { vol := &rbdVolume{} - // nolint: errcheck + // nolint cs.MetadataStore.ForAll("csi-rbd-vol-", vol, func(identifier string) error { rbdVolumes[identifier] = vol return nil }) snap := &rbdSnapshot{} - // nolint: errcheck + // nolint cs.MetadataStore.ForAll("csi-rbd-(.*)-snap-", snap, func(identifier string) error { rbdSnapshots[identifier] = snap return nil @@ -104,10 +104,10 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol // check for the requested capacity and already allocated capacity if exVol, err := getRBDVolumeByName(req.GetName()); err == nil { // Since err is nil, it means the volume with the same name already exists - // need to check if the size of exisiting volume is the same as in new + // need to check if the size of existing volume is the same as in new // request if exVol.VolSize >= req.GetCapacityRange().GetRequiredBytes() { - // exisiting volume is compatible with new request and should be reused. + // existing volume is compatible with new request and should be reused. // TODO (sbezverk) Do I need to make sure that RBD volume still exists? return &csi.CreateVolumeResponse{ Volume: &csi.Volume{ @@ -145,22 +145,9 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol volSizeGB := int(volSizeBytes / 1024 / 1024 / 1024) // Check if there is already RBD image with requested name - found, _, _ := rbdStatus(rbdVol, rbdVol.UserID, req.GetSecrets()) - if !found { - // if VolumeContentSource is not nil, this request is for snapshot - if req.VolumeContentSource != nil { - if err = cs.checkSnapshot(req, rbdVol); err != nil { - return nil, err - } - } else { - err = createRBDImage(rbdVol, volSizeGB, rbdVol.AdminID, req.GetSecrets()) - if err != nil { - glog.Warningf("failed to create volume: %v", err) - return nil, err - } - - glog.V(4).Infof("create volume %s", volName) - } + err = cs.checkrbdStatus(rbdVol, req, volSizeGB) + if err != nil { + return nil, err } if createErr := cs.MetadataStore.Create(volumeID, rbdVol); createErr != nil { glog.Warningf("failed to store volume metadata with error: %v", err) @@ -181,6 +168,28 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol }, nil } +func (cs *ControllerServer) checkrbdStatus(rbdVol *rbdVolume, req *csi.CreateVolumeRequest, volSizeGB int) error { + var err error + // Check if there is already RBD image with requested name + found, _, _ := rbdStatus(rbdVol, rbdVol.UserID, req.GetSecrets()) // #nosec + if !found { + // if VolumeContentSource is not nil, this request is for snapshot + if req.VolumeContentSource != nil { + if err = cs.checkSnapshot(req, rbdVol); err != nil { + return err + } + } else { + err = createRBDImage(rbdVol, volSizeGB, rbdVol.AdminID, req.GetSecrets()) + if err != nil { + glog.Warningf("failed to create volume: %v", err) + return err + } + + glog.V(4).Infof("create volume %s", rbdVol.VolName) + } + } + return nil +} func (cs *ControllerServer) checkSnapshot(req *csi.CreateVolumeRequest, rbdVol *rbdVolume) error { snapshot := req.VolumeContentSource.GetSnapshot() if snapshot == nil { @@ -275,19 +284,10 @@ func (cs *ControllerServer) ControllerPublishVolume(ctx context.Context, req *cs // CreateSnapshot creates the snapshot in backend and stores metadata // in store func (cs *ControllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshotRequest) (*csi.CreateSnapshotResponse, error) { - if err := cs.Driver.ValidateControllerServiceRequest(csi.ControllerServiceCapability_RPC_CREATE_DELETE_SNAPSHOT); err != nil { - glog.Warningf("invalid create snapshot req: %v", req) + + if err := cs.validateSnapshotReq(req); err != nil { return nil, err } - - // Check sanity of request Snapshot Name, Source Volume Id - if len(req.Name) == 0 { - return nil, status.Error(codes.InvalidArgument, "Snapshot Name cannot be empty") - } - if len(req.SourceVolumeId) == 0 { - return nil, status.Error(codes.InvalidArgument, "Source Volume ID cannot be empty") - } - snapshotNameMutex.LockKey(req.GetName()) defer func() { @@ -338,54 +338,16 @@ func (cs *ControllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS rbdSnap.SourceVolumeID = req.GetSourceVolumeId() rbdSnap.SizeBytes = rbdVolume.VolSize - err = createSnapshot(rbdSnap, rbdSnap.AdminID, req.GetSecrets()) + err = cs.doSnapshot(rbdSnap, req.GetSecrets()) // if we already have the snapshot, return the snapshot if err != nil { - if exitErr, ok := err.(*exec.ExitError); ok { - if status, ok := exitErr.Sys().(syscall.WaitStatus); ok { - if status.ExitStatus() == int(syscall.EEXIST) { - glog.Warningf("Snapshot with the same name: %s, we return this.", req.GetName()) - } else { - glog.Warningf("failed to create snapshot: %v", err) - return nil, err - } - } else { - glog.Warningf("failed to create snapshot: %v", err) - return nil, err - } - } else { - glog.Warningf("failed to create snapshot: %v", err) - return nil, err - } - } else { - glog.V(4).Infof("create snapshot %s", snapName) - err = protectSnapshot(rbdSnap, rbdSnap.AdminID, req.GetSecrets()) - - if err != nil { - err = deleteSnapshot(rbdSnap, rbdSnap.AdminID, req.GetSecrets()) - if err != nil { - return nil, fmt.Errorf("snapshot is created but failed to protect and delete snapshot: %v", err) - } - return nil, fmt.Errorf("Snapshot is created but failed to protect snapshot") - } + return nil, err } rbdSnap.CreatedAt = ptypes.TimestampNow().GetSeconds() - if createErr := cs.MetadataStore.Create(snapshotID, rbdSnap); createErr != nil { - - glog.Warningf("rbd: failed to store snapInfo with error: %v", err) - // Unprotect snapshot - err = unprotectSnapshot(rbdSnap, rbdSnap.AdminID, req.GetSecrets()) - if err != nil { - return nil, status.Errorf(codes.Unknown, "This Snapshot should be removed but failed to unprotect snapshot: %s/%s with error: %v", rbdSnap.Pool, rbdSnap.SnapName, err) - } - // Deleting snapshot - glog.V(4).Infof("deleting Snaphot %s", rbdSnap.SnapName) - if err = deleteSnapshot(rbdSnap, rbdSnap.AdminID, req.GetSecrets()); err != nil { - return nil, status.Errorf(codes.Unknown, "This Snapshot should be removed but failed to delete snapshot: %s/%s with error: %v", rbdSnap.Pool, rbdSnap.SnapName, err) - } - return nil, createErr + if err = cs.storeSnapMetadata(rbdSnap, req.GetSecrets()); err != nil { + return nil, err } rbdSnapshots[snapshotID] = rbdSnap @@ -402,6 +364,75 @@ func (cs *ControllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS }, nil } +func (cs *ControllerServer) storeSnapMetadata(rbdSnap *rbdSnapshot, secret map[string]string) error { + errCreate := cs.MetadataStore.Create(rbdSnap.SnapID, rbdSnap) + if errCreate != nil { + glog.Warningf("rbd: failed to store snapInfo with error: %v", errCreate) + // Unprotect snapshot + err := unprotectSnapshot(rbdSnap, rbdSnap.AdminID, secret) + if err != nil { + return status.Errorf(codes.Unknown, "This Snapshot should be removed but failed to unprotect snapshot: %s/%s with error: %v", rbdSnap.Pool, rbdSnap.SnapName, err) + } + // Deleting snapshot + glog.V(4).Infof("deleting Snaphot %s", rbdSnap.SnapName) + if err = deleteSnapshot(rbdSnap, rbdSnap.AdminID, secret); err != nil { + return status.Errorf(codes.Unknown, "This Snapshot should be removed but failed to delete snapshot: %s/%s with error: %v", rbdSnap.Pool, rbdSnap.SnapName, err) + } + } + return errCreate +} + +func (cs *ControllerServer) validateSnapshotReq(req *csi.CreateSnapshotRequest) error { + if err := cs.Driver.ValidateControllerServiceRequest(csi.ControllerServiceCapability_RPC_CREATE_DELETE_SNAPSHOT); err != nil { + glog.Warningf("invalid create snapshot req: %v", req) + return err + } + + // Check sanity of request Snapshot Name, Source Volume Id + if len(req.Name) == 0 { + return status.Error(codes.InvalidArgument, "Snapshot Name cannot be empty") + } + if len(req.SourceVolumeId) == 0 { + return status.Error(codes.InvalidArgument, "Source Volume ID cannot be empty") + } + return nil +} + +func (cs *ControllerServer) doSnapshot(rbdSnap *rbdSnapshot, secret map[string]string) error { + err := createSnapshot(rbdSnap, rbdSnap.AdminID, secret) + // if we already have the snapshot, return the snapshot + if err != nil { + if exitErr, ok := err.(*exec.ExitError); ok { + if status, ok := exitErr.Sys().(syscall.WaitStatus); ok { + if status.ExitStatus() == int(syscall.EEXIST) { + glog.Warningf("Snapshot with the same name: %s, we return this.", rbdSnap.SnapName) + } else { + glog.Warningf("failed to create snapshot: %v", err) + return err + } + } else { + glog.Warningf("failed to create snapshot: %v", err) + return err + } + } else { + glog.Warningf("failed to create snapshot: %v", err) + return err + } + } else { + glog.V(4).Infof("create snapshot %s", rbdSnap.SnapName) + err = protectSnapshot(rbdSnap, rbdSnap.AdminID, secret) + + if err != nil { + err = deleteSnapshot(rbdSnap, rbdSnap.AdminID, secret) + if err != nil { + return fmt.Errorf("snapshot is created but failed to protect and delete snapshot: %v", err) + } + return fmt.Errorf("Snapshot is created but failed to protect snapshot") + } + } + return nil +} + // DeleteSnapshot deletes the snapshot in backend and removes the //snapshot metadata from store func (cs *ControllerServer) DeleteSnapshot(ctx context.Context, req *csi.DeleteSnapshotRequest) (*csi.DeleteSnapshotResponse, error) { diff --git a/pkg/rbd/nodeserver.go b/pkg/rbd/nodeserver.go index a54bf7058..632d42a30 100644 --- a/pkg/rbd/nodeserver.go +++ b/pkg/rbd/nodeserver.go @@ -67,47 +67,16 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis } }() - var volName string - isBlock := req.GetVolumeCapability().GetBlock() != nil - - if isBlock { - // Get volName from targetPath - s := strings.Split(targetPath, "/") - volName = s[len(s)-1] - } else { - // Get volName from targetPath - if !strings.HasSuffix(targetPath, "/mount") { - return nil, fmt.Errorf("rbd: malformed the value of target path: %s", targetPath) - } - s := strings.Split(strings.TrimSuffix(targetPath, "/mount"), "/") - volName = s[len(s)-1] + volName, err := ns.getVolumeName(req) + if err != nil { + return nil, err } + isBlock := req.GetVolumeCapability().GetBlock() != nil // Check if that target path exists properly - notMnt, err := ns.mounter.IsNotMountPoint(targetPath) + notMnt, err := ns.createTargetPath(targetPath, isBlock) if err != nil { - if os.IsNotExist(err) { - if isBlock { - // create an empty file - targetPathFile, e := os.OpenFile(targetPath, os.O_CREATE|os.O_RDWR, 0750) - if e != nil { - glog.V(4).Infof("Failed to create targetPath:%s with error: %v", targetPath, err) - return nil, status.Error(codes.Internal, e.Error()) - } - if err = targetPathFile.Close(); err != nil { - glog.V(4).Infof("Failed to close targetPath:%s with error: %v", targetPath, err) - return nil, status.Error(codes.Internal, err.Error()) - } - } else { - // Create a directory - if err = os.MkdirAll(targetPath, 0750); err != nil { - return nil, status.Error(codes.Internal, err.Error()) - } - } - notMnt = true - } else { - return nil, status.Error(codes.Internal, err.Error()) - } + return nil, err } if !notMnt { @@ -125,11 +94,41 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis } glog.V(4).Infof("rbd image: %s/%s was successfully mapped at %s\n", req.GetVolumeId(), volOptions.Pool, devicePath) + // Publish Path + err = ns.mountVolume(req, devicePath) + if err != nil { + return nil, err + } + return &csi.NodePublishVolumeResponse{}, nil +} + +func (ns *NodeServer) getVolumeName(req *csi.NodePublishVolumeRequest) (string, error) { + var volName string + isBlock := req.GetVolumeCapability().GetBlock() != nil + targetPath := req.GetTargetPath() + if isBlock { + // Get volName from targetPath + s := strings.Split(targetPath, "/") + volName = s[len(s)-1] + } else { + // Get volName from targetPath + if !strings.HasSuffix(targetPath, "/mount") { + return "", fmt.Errorf("rbd: malformed the value of target path: %s", targetPath) + } + s := strings.Split(strings.TrimSuffix(targetPath, "/mount"), "/") + volName = s[len(s)-1] + } + return volName, nil +} + +func (ns *NodeServer) mountVolume(req *csi.NodePublishVolumeRequest, devicePath string) error { // Publish Path fsType := req.GetVolumeCapability().GetMount().GetFsType() readOnly := req.GetReadonly() attrib := req.GetVolumeContext() mountFlags := req.GetVolumeCapability().GetMount().GetMountFlags() + isBlock := req.GetVolumeCapability().GetBlock() != nil + targetPath := req.GetTargetPath() glog.V(4).Infof("target %v\nisBlock %v\nfstype %v\ndevice %v\nreadonly %v\nattributes %v\n mountflags %v\n", targetPath, isBlock, fsType, devicePath, readOnly, attrib, mountFlags) @@ -138,7 +137,7 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis if isBlock { options := []string{"bind"} if err := diskMounter.Mount(devicePath, targetPath, fsType, options); err != nil { - return nil, err + return err } } else { options := []string{} @@ -147,11 +146,42 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis } if err := diskMounter.FormatAndMount(devicePath, targetPath, fsType, options); err != nil { - return nil, err + return err } } + return nil +} + +func (ns *NodeServer) createTargetPath(targetPath string, isBlock bool) (bool, error) { + // Check if that target path exists properly + notMnt, err := ns.mounter.IsNotMountPoint(targetPath) + if err != nil { + if os.IsNotExist(err) { + if isBlock { + // create an empty file + // #nosec + targetPathFile, e := os.OpenFile(targetPath, os.O_CREATE|os.O_RDWR, 0750) + if e != nil { + glog.V(4).Infof("Failed to create targetPath:%s with error: %v", targetPath, err) + return notMnt, status.Error(codes.Internal, e.Error()) + } + if err = targetPathFile.Close(); err != nil { + glog.V(4).Infof("Failed to close targetPath:%s with error: %v", targetPath, err) + return notMnt, status.Error(codes.Internal, err.Error()) + } + } else { + // Create a directory + if err = os.MkdirAll(targetPath, 0750); err != nil { + return notMnt, status.Error(codes.Internal, err.Error()) + } + } + notMnt = true + } else { + return false, status.Error(codes.Internal, err.Error()) + } + } + return notMnt, err - return &csi.NodePublishVolumeResponse{}, nil } // NodeUnpublishVolume unmounts the volume from the target path @@ -230,6 +260,7 @@ func (ns *NodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpu } func resolveBindMountedBlockDevice(mountPath string) (string, error) { + // #nosec cmd := exec.Command("findmnt", "-n", "-o", "SOURCE", "--first-only", "--target", mountPath) out, err := cmd.CombinedOutput() if err != nil { @@ -244,7 +275,7 @@ func parseFindMntResolveSource(out string) (string, error) { // cut trailing newline out = strings.TrimSuffix(out, "\n") // Check if out is a mounted device - reMnt := regexp.MustCompile("^(/[^/]+(?:/[^/]*)*)$") + reMnt := regexp.MustCompile(`^(/[^/]+(?:/[^/]*)*)$`) if match := reMnt.FindStringSubmatch(out); match != nil { return match[1], nil } diff --git a/pkg/rbd/rbd_attach.go b/pkg/rbd/rbd_attach.go index 1cca3f8b7..6a6505266 100644 --- a/pkg/rbd/rbd_attach.go +++ b/pkg/rbd/rbd_attach.go @@ -61,6 +61,7 @@ func getRbdDevFromImageAndPool(pool string, image string) (string, bool) { name := f.Name() // First match pool, then match name. poolFile := path.Join(sysPath, name, "pool") + // #nosec poolBytes, err := ioutil.ReadFile(poolFile) if err != nil { glog.V(4).Infof("error reading %s: %v", poolFile, err) @@ -71,6 +72,7 @@ func getRbdDevFromImageAndPool(pool string, image string) (string, bool) { continue } imgFile := path.Join(sysPath, name, "name") + // #nosec imgBytes, err := ioutil.ReadFile(imgFile) if err != nil { glog.V(4).Infof("error reading %s: %v", imgFile, err) @@ -140,12 +142,14 @@ func getNbdDevFromImageAndPool(pool string, image string) (string, bool) { glog.V(4).Infof("error reading nbd info directory %s: %v", nbdPath, err) continue } + // #nosec pidBytes, err := ioutil.ReadFile(path.Join(nbdPath, "pid")) if err != nil { glog.V(5).Infof("did not find valid pid file in dir %s: %v", nbdPath, err) continue } cmdlineFileName := path.Join(hostRootFS, "/proc", strings.TrimSpace(string(pidBytes)), "cmdline") + // #nosec rawCmdline, err := ioutil.ReadFile(cmdlineFileName) if err != nil { glog.V(4).Infof("failed to read cmdline file %s: %v", cmdlineFileName, err) @@ -246,22 +250,11 @@ func attachRBDImage(volOptions *rbdVolume, userID string, credentials map[string Factor: rbdImageWatcherFactor, Steps: rbdImageWatcherSteps, } - err := wait.ExponentialBackoff(backoff, func() (bool, error) { - used, rbdOutput, err := rbdStatus(volOptions, userID, credentials) - if err != nil { - return false, fmt.Errorf("fail to check rbd image status with: (%v), rbd output: (%s)", err, rbdOutput) - } - return !used, nil - }) - // return error if rbd image has not become available for the specified timeout - if err == wait.ErrWaitTimeout { - return "", fmt.Errorf("rbd image %s is still being used", imagePath) - } - // return error if any other errors were encountered during wating for the image to become available + err := waitForrbdImage(backoff, volOptions, userID, credentials) + if err != nil { return "", err } - mon, err := getMon(volOptions, credentials) if err != nil { return "", err @@ -287,6 +280,25 @@ func attachRBDImage(volOptions *rbdVolume, userID string, credentials map[string return devicePath, nil } +func waitForrbdImage(backoff wait.Backoff, volOptions *rbdVolume, userID string, credentials map[string]string) error { + image := volOptions.VolName + imagePath := fmt.Sprintf("%s/%s", volOptions.Pool, image) + + err := wait.ExponentialBackoff(backoff, func() (bool, error) { + used, rbdOutput, err := rbdStatus(volOptions, userID, credentials) + if err != nil { + return false, fmt.Errorf("fail to check rbd image status with: (%v), rbd output: (%s)", err, rbdOutput) + } + return !used, nil + }) + // return error if rbd image has not become available for the specified timeout + if err == wait.ErrWaitTimeout { + return fmt.Errorf("rbd image %s is still being used", imagePath) + } + // return error if any other errors were encountered during waiting for the image to become available + return err +} + func detachRBDDevice(devicePath string) error { var err error var output []byte diff --git a/pkg/rbd/rbd_util.go b/pkg/rbd/rbd_util.go index abff869d8..b2583f7a2 100644 --- a/pkg/rbd/rbd_util.go +++ b/pkg/rbd/rbd_util.go @@ -145,7 +145,7 @@ func createRBDImage(pOpts *rbdVolume, volSz int, adminID string, credentials map } // rbdStatus checks if there is watcher on the image. -// It returns true if there is a watcher onthe image, otherwise returns false. +// It returns true if there is a watcher on the image, otherwise returns false. func rbdStatus(pOpts *rbdVolume, userID string, credentials map[string]string) (bool, string, error) { var output string var cmd []byte @@ -221,6 +221,7 @@ func deleteRBDImage(pOpts *rbdVolume, adminID string, credentials map[string]str } func execCommand(command string, args []string) ([]byte, error) { + // #nosec cmd := exec.Command(command, args...) return cmd.CombinedOutput() } diff --git a/pkg/util/nodecache.go b/pkg/util/nodecache.go index f4a1c566f..e32c77513 100644 --- a/pkg/util/nodecache.go +++ b/pkg/util/nodecache.go @@ -40,6 +40,7 @@ var cacheDir = "controller" func (nc *NodeCache) EnsureCacheDirectory(cacheDir string) error { fullPath := path.Join(nc.BasePath, cacheDir) if _, err := os.Stat(fullPath); os.IsNotExist(err) { + // #nosec if err := os.Mkdir(fullPath, 0755); err != nil { return errors.Wrapf(err, "node-cache: failed to create %s folder with error: %v", fullPath, err) } @@ -66,6 +67,7 @@ func (nc *NodeCache) ForAll(pattern string, destObj interface{}, f ForAllFunc) e if !strings.HasSuffix(file.Name(), ".json") { continue } + // #nosec fp, err := os.Open(path.Join(nc.BasePath, cacheDir, file.Name())) if err != nil { glog.Infof("node-cache: open file: %s err %v", file.Name(), err) @@ -111,6 +113,7 @@ func (nc *NodeCache) Create(identifier string, data interface{}) error { // Get retrieves the metadata from cache directory with identifier name func (nc *NodeCache) Get(identifier string, data interface{}) error { file := path.Join(nc.BasePath, cacheDir, identifier+".json") + // #nosec fp, err := os.Open(file) if err != nil { return errors.Wrapf(err, "node-cache: open error for %s", file) From 50ba8ed446a5939832f1bcd952fa7165bc4eb119 Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Tue, 29 Jan 2019 11:19:16 +0530 Subject: [PATCH 08/11] Fix gometalinter issues Signed-off-by: Madhu Rajanna --- .travis.yml | 2 +- pkg/cephfs/nodeserver.go | 22 ++++-- pkg/cephfs/volumeoptions.go | 57 ++++++++------ pkg/rbd/controllerserver.go | 56 ++++++++------ pkg/rbd/nodeserver.go | 27 ++++--- pkg/rbd/rbd_attach.go | 143 +++++++++++++++++++++--------------- pkg/rbd/rbd_util.go | 8 +- pkg/util/nodecache.go | 58 +++++++++------ 8 files changed, 227 insertions(+), 146 deletions(-) diff --git a/.travis.yml b/.travis.yml index 5681ba437..489c10c0a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -17,7 +17,7 @@ before_script: - go get -u golang.org/x/lint/golint #go get github.com/golang/lint/golint script: - - gometalinter --deadline=10m -j 4 --disable=gocyclo --enable=megacheck --enable=misspell --enable=unparam --vendor ./... + - gometalinter --deadline=10m -j 4 --enable=megacheck --enable=misspell --vendor ./... - test -z $(gofmt -s -l $GO_FILES) - make rbdplugin - make cephfsplugin diff --git a/pkg/cephfs/nodeserver.go b/pkg/cephfs/nodeserver.go index 412196929..d140e4c17 100644 --- a/pkg/cephfs/nodeserver.go +++ b/pkg/cephfs/nodeserver.go @@ -128,28 +128,38 @@ func (ns *NodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStageVol } // It's not, mount now + if err = ns.mount(volOptions, req); err != nil { + return nil, err + } + + glog.Infof("cephfs: successfully mounted volume %s to %s", volID, stagingTargetPath) + + return &csi.NodeStageVolumeResponse{}, nil +} + +func (*NodeServer) mount(volOptions *volumeOptions, req *csi.NodeStageVolumeRequest) error { + stagingTargetPath := req.GetStagingTargetPath() + volID := volumeID(req.GetVolumeId()) cr, err := getCredentialsForVolume(volOptions, volID, req) if err != nil { glog.Errorf("failed to get ceph credentials for volume %s: %v", volID, err) - return nil, status.Error(codes.Internal, err.Error()) + return status.Error(codes.Internal, err.Error()) } m, err := newMounter(volOptions) if err != nil { glog.Errorf("failed to create mounter for volume %s: %v", volID, err) + return status.Error(codes.Internal, err.Error()) } glog.V(4).Infof("cephfs: mounting volume %s with %s", volID, m.name()) if err = m.mount(stagingTargetPath, cr, volOptions, volID); err != nil { glog.Errorf("failed to mount volume %s: %v", volID, err) - return nil, status.Error(codes.Internal, err.Error()) + return status.Error(codes.Internal, err.Error()) } - - glog.Infof("cephfs: successfully mounted volume %s to %s", volID, stagingTargetPath) - - return &csi.NodeStageVolumeResponse{}, nil + return nil } // NodePublishVolume mounts the volume mounted to the staging path to the target diff --git a/pkg/cephfs/volumeoptions.go b/pkg/cephfs/volumeoptions.go index 1dbb55122..7ce6e86b3 100644 --- a/pkg/cephfs/volumeoptions.go +++ b/pkg/cephfs/volumeoptions.go @@ -95,9 +95,8 @@ func validateMounter(m string) error { func newVolumeOptions(volOptions, secret map[string]string) (*volumeOptions, error) { var ( - opts volumeOptions - provisionVolumeBool string - err error + opts volumeOptions + err error ) // extract mon from secret first @@ -113,32 +112,44 @@ func newVolumeOptions(volOptions, secret map[string]string) (*volumeOptions, err return nil, fmt.Errorf("either monitors or monValueFromSecret should be set") } } - if err = extractOption(&provisionVolumeBool, "provisionVolume", volOptions); err != nil { + + if err = extractNewVolOpt(&opts, volOptions); err != nil { return nil, err } - if opts.ProvisionVolume, err = strconv.ParseBool(provisionVolumeBool); err != nil { - return nil, fmt.Errorf("Failed to parse provisionVolume: %v", err) - } - - if opts.ProvisionVolume { - if err = extractOption(&opts.Pool, "pool", volOptions); err != nil { - return nil, err - } - } else { - if err = extractOption(&opts.RootPath, "rootPath", volOptions); err != nil { - return nil, err - } - } - - // This field is optional, don't check for its presence - // nolint - // (skip errcheck and gosec as this is optional) - extractOption(&opts.Mounter, "mounter", volOptions) - if err = opts.validate(); err != nil { return nil, err } return &opts, nil } + +func extractNewVolOpt(opts *volumeOptions, volOpt map[string]string) error { + var ( + provisionVolumeBool string + err error + ) + if err = extractOption(&provisionVolumeBool, "provisionVolume", volOpt); err != nil { + return err + } + + if opts.ProvisionVolume, err = strconv.ParseBool(provisionVolumeBool); err != nil { + return fmt.Errorf("Failed to parse provisionVolume: %v", err) + } + + if opts.ProvisionVolume { + if err = extractOption(&opts.Pool, "pool", volOpt); err != nil { + return err + } + } else { + if err = extractOption(&opts.RootPath, "rootPath", volOpt); err != nil { + return err + } + } + + // This field is optional, don't check for its presence + // nolint + // (skip errcheck and gosec as this is optional) + extractOption(&opts.Mounter, "mounter", volOpt) + return nil +} diff --git a/pkg/rbd/controllerserver.go b/pkg/rbd/controllerserver.go index e79cf4258..c69a6ccc4 100644 --- a/pkg/rbd/controllerserver.go +++ b/pkg/rbd/controllerserver.go @@ -87,6 +87,33 @@ func (cs *ControllerServer) validateVolumeReq(req *csi.CreateVolumeRequest) erro return nil } +func parseVolCreateRequest(req *csi.CreateVolumeRequest) (*rbdVolume, error) { + // TODO (sbezverk) Last check for not exceeding total storage capacity + + rbdVol, err := getRBDVolumeOptions(req.GetParameters()) + if err != nil { + return nil, err + } + + // Generating Volume Name and Volume ID, as according to CSI spec they MUST be different + volName := req.GetName() + uniqueID := uuid.NewUUID().String() + if len(volName) == 0 { + volName = rbdVol.Pool + "-dynamic-pvc-" + uniqueID + } + rbdVol.VolName = volName + volumeID := "csi-rbd-vol-" + uniqueID + rbdVol.VolID = volumeID + // Volume Size - Default is 1 GiB + volSizeBytes := int64(oneGB) + if req.GetCapacityRange() != nil { + volSizeBytes = req.GetCapacityRange().GetRequiredBytes() + } + rbdVol.VolSize = volSizeBytes + + return rbdVol, nil +} + // CreateVolume creates the volume in backend and store the volume metadata func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) (*csi.CreateVolumeResponse, error) { @@ -120,36 +147,19 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol return nil, status.Errorf(codes.AlreadyExists, "Volume with the same name: %s but with different size already exist", req.GetName()) } - // TODO (sbezverk) Last check for not exceeding total storage capacity - - rbdVol, err := getRBDVolumeOptions(req.GetParameters()) + rbdVol, err := parseVolCreateRequest(req) if err != nil { return nil, err } - // Generating Volume Name and Volume ID, as according to CSI spec they MUST be different - volName := req.GetName() - uniqueID := uuid.NewUUID().String() - if len(volName) == 0 { - volName = rbdVol.Pool + "-dynamic-pvc-" + uniqueID - } - rbdVol.VolName = volName - volumeID := "csi-rbd-vol-" + uniqueID - rbdVol.VolID = volumeID - // Volume Size - Default is 1 GiB - volSizeBytes := int64(oneGB) - if req.GetCapacityRange() != nil { - volSizeBytes = req.GetCapacityRange().GetRequiredBytes() - } - rbdVol.VolSize = volSizeBytes - volSizeGB := int(volSizeBytes / 1024 / 1024 / 1024) + volSizeGB := int(rbdVol.VolSize / 1024 / 1024 / 1024) // Check if there is already RBD image with requested name err = cs.checkrbdStatus(rbdVol, req, volSizeGB) if err != nil { return nil, err } - if createErr := cs.MetadataStore.Create(volumeID, rbdVol); createErr != nil { + if createErr := cs.MetadataStore.Create(rbdVol.VolID, rbdVol); createErr != nil { glog.Warningf("failed to store volume metadata with error: %v", err) if err = deleteRBDImage(rbdVol, rbdVol.AdminID, req.GetSecrets()); err != nil { glog.V(3).Infof("failed to delete rbd image: %s/%s with error: %v", rbdVol.Pool, rbdVol.VolName, err) @@ -158,11 +168,11 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol return nil, createErr } - rbdVolumes[volumeID] = rbdVol + rbdVolumes[rbdVol.VolID] = rbdVol return &csi.CreateVolumeResponse{ Volume: &csi.Volume{ - VolumeId: volumeID, - CapacityBytes: volSizeBytes, + VolumeId: rbdVol.VolID, + CapacityBytes: rbdVol.VolSize, VolumeContext: req.GetParameters(), }, }, nil diff --git a/pkg/rbd/nodeserver.go b/pkg/rbd/nodeserver.go index 632d42a30..5db7c53d4 100644 --- a/pkg/rbd/nodeserver.go +++ b/pkg/rbd/nodeserver.go @@ -215,11 +215,20 @@ func (ns *NodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpu return nil, status.Error(codes.Internal, err.Error()) } + if err = ns.unmount(targetPath, devicePath, cnt); err != nil { + return nil, err + } + + return &csi.NodeUnpublishVolumeResponse{}, nil +} + +func (ns *NodeServer) unmount(targetPath, devicePath string, cnt int) error { + var err error // Bind mounted device needs to be resolved by using resolveBindMountedBlockDevice if devicePath == "devtmpfs" { devicePath, err = resolveBindMountedBlockDevice(targetPath) if err != nil { - return nil, status.Error(codes.Internal, err.Error()) + return status.Error(codes.Internal, err.Error()) } glog.V(4).Infof("NodeUnpublishVolume: devicePath: %s, (original)cnt: %d\n", devicePath, cnt) // cnt for GetDeviceNameFromMount is broken for bind mouted device, @@ -235,30 +244,27 @@ func (ns *NodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpu err = ns.mounter.Unmount(targetPath) if err != nil { glog.V(3).Infof("failed to unmount targetPath: %s with error: %v", targetPath, err) - return nil, status.Error(codes.Internal, err.Error()) + return status.Error(codes.Internal, err.Error()) } cnt-- if cnt != 0 { // TODO should this be fixed not to success, so that driver can retry unmounting? - return &csi.NodeUnpublishVolumeResponse{}, nil + return nil } // Unmapping rbd device if err = detachRBDDevice(devicePath); err != nil { glog.V(3).Infof("failed to unmap rbd device: %s with error: %v", devicePath, err) - return nil, err + return err } // Remove targetPath if err = os.RemoveAll(targetPath); err != nil { glog.V(3).Infof("failed to remove targetPath: %s with error: %v", targetPath, err) - return nil, err } - - return &csi.NodeUnpublishVolumeResponse{}, nil + return err } - func resolveBindMountedBlockDevice(mountPath string) (string, error) { // #nosec cmd := exec.Command("findmnt", "-n", "-o", "SOURCE", "--first-only", "--target", mountPath) @@ -275,12 +281,13 @@ func parseFindMntResolveSource(out string) (string, error) { // cut trailing newline out = strings.TrimSuffix(out, "\n") // Check if out is a mounted device - reMnt := regexp.MustCompile(`^(/[^/]+(?:/[^/]*)*)$`) + reMnt := regexp.MustCompile("^(/[^/]+(?:/[^/]*)*)$") if match := reMnt.FindStringSubmatch(out); match != nil { return match[1], nil } // Check if out is a block device - reBlk := regexp.MustCompile(`^devtmpfs\\[(/[^/]+(?:/[^/]*)*)\\]$`) + // nolint + reBlk := regexp.MustCompile("^devtmpfs\\[(/[^/]+(?:/[^/]*)*)\\]$") if match := reBlk.FindStringSubmatch(out); match != nil { return fmt.Sprintf("/dev%s", match[1]), nil } diff --git a/pkg/rbd/rbd_attach.go b/pkg/rbd/rbd_attach.go index 6a6505266..a93e7c379 100644 --- a/pkg/rbd/rbd_attach.go +++ b/pkg/rbd/rbd_attach.go @@ -137,42 +137,8 @@ func getNbdDevFromImageAndPool(pool string, image string) (string, bool) { for i := 0; i < maxNbds; i++ { nbdPath := basePath + strconv.Itoa(i) - _, err := os.Lstat(nbdPath) + devicePath, err := getnbdDevicePath(nbdPath, imgPath, i) if err != nil { - glog.V(4).Infof("error reading nbd info directory %s: %v", nbdPath, err) - continue - } - // #nosec - pidBytes, err := ioutil.ReadFile(path.Join(nbdPath, "pid")) - if err != nil { - glog.V(5).Infof("did not find valid pid file in dir %s: %v", nbdPath, err) - continue - } - cmdlineFileName := path.Join(hostRootFS, "/proc", strings.TrimSpace(string(pidBytes)), "cmdline") - // #nosec - rawCmdline, err := ioutil.ReadFile(cmdlineFileName) - if err != nil { - glog.V(4).Infof("failed to read cmdline file %s: %v", cmdlineFileName, err) - continue - } - cmdlineArgs := strings.FieldsFunc(string(rawCmdline), func(r rune) bool { - return r == '\u0000' - }) - // Check if this process is mapping a rbd device. - // Only accepted pattern of cmdline is from execRbdMap: - // rbd-nbd map pool/image ... - if len(cmdlineArgs) < 3 || cmdlineArgs[0] != rbdTonbd || cmdlineArgs[1] != "map" { - glog.V(4).Infof("nbd device %s is not used by rbd", nbdPath) - continue - } - if cmdlineArgs[2] != imgPath { - glog.V(4).Infof("rbd-nbd device %s did not match expected image path: %s with path found: %s", - nbdPath, imgPath, cmdlineArgs[2]) - continue - } - devicePath := path.Join("/dev", "nbd"+strconv.Itoa(i)) - if _, err := os.Lstat(devicePath); err != nil { - glog.Warningf("Stat device %s for imgpath %s failed %v", devicePath, imgPath, err) continue } return devicePath, true @@ -180,6 +146,50 @@ func getNbdDevFromImageAndPool(pool string, image string) (string, bool) { return "", false } +func getnbdDevicePath(nbdPath, imgPath string, count int) (string, error) { + + _, err := os.Lstat(nbdPath) + if err != nil { + glog.V(4).Infof("error reading nbd info directory %s: %v", nbdPath, err) + return "", err + } + // #nosec + pidBytes, err := ioutil.ReadFile(path.Join(nbdPath, "pid")) + if err != nil { + glog.V(5).Infof("did not find valid pid file in dir %s: %v", nbdPath, err) + return "", err + } + cmdlineFileName := path.Join(hostRootFS, "/proc", strings.TrimSpace(string(pidBytes)), "cmdline") + // #nosec + rawCmdline, err := ioutil.ReadFile(cmdlineFileName) + if err != nil { + glog.V(4).Infof("failed to read cmdline file %s: %v", cmdlineFileName, err) + return "", err + } + cmdlineArgs := strings.FieldsFunc(string(rawCmdline), func(r rune) bool { + return r == '\u0000' + }) + // Check if this process is mapping a rbd device. + // Only accepted pattern of cmdline is from execRbdMap: + // rbd-nbd map pool/image ... + if len(cmdlineArgs) < 3 || cmdlineArgs[0] != rbdTonbd || cmdlineArgs[1] != "map" { + glog.V(4).Infof("nbd device %s is not used by rbd", nbdPath) + return "", err + + } + if cmdlineArgs[2] != imgPath { + glog.V(4).Infof("rbd-nbd device %s did not match expected image path: %s with path found: %s", + nbdPath, imgPath, cmdlineArgs[2]) + return "", err + } + devicePath := path.Join("/dev", "nbd"+strconv.Itoa(count)) + if _, err := os.Lstat(devicePath); err != nil { + glog.Warningf("Stat device %s for imgpath %s failed %v", devicePath, imgPath, err) + return "", err + } + return devicePath, nil +} + // Stat a path, if it doesn't exist, retry maxRetries times. func waitForPath(pool, image string, maxRetries int, useNbdDriver bool) (string, bool) { for i := 0; i < maxRetries; i++ { @@ -216,21 +226,18 @@ func checkRbdNbdTools() bool { func attachRBDImage(volOptions *rbdVolume, userID string, credentials map[string]string) (string, error) { var err error - var output []byte image := volOptions.VolName imagePath := fmt.Sprintf("%s/%s", volOptions.Pool, image) useNBD := false - cmdName := rbd moduleName := rbd if volOptions.Mounter == rbdTonbd && hasNBD { useNBD = true - cmdName = rbdTonbd moduleName = nbd } - devicePath, found := waitForPath(volOptions.Pool, image, 1, useNBD) + _, found := waitForPath(volOptions.Pool, image, 1, useNBD) if !found { attachdetachMutex.LockKey(imagePath) @@ -243,6 +250,7 @@ func attachRBDImage(volOptions *rbdVolume, userID string, credentials map[string _, err = execCommand("modprobe", []string{moduleName}) if err != nil { glog.Warningf("rbd: failed to load rbd kernel module:%v", err) + return "", err } backoff := wait.Backoff{ @@ -250,33 +258,50 @@ func attachRBDImage(volOptions *rbdVolume, userID string, credentials map[string Factor: rbdImageWatcherFactor, Steps: rbdImageWatcherSteps, } - err := waitForrbdImage(backoff, volOptions, userID, credentials) + err = waitForrbdImage(backoff, volOptions, userID, credentials) - if err != nil { - return "", err - } - mon, err := getMon(volOptions, credentials) if err != nil { return "", err } - glog.V(5).Infof("rbd: map mon %s", mon) - key, err := getRBDKey(userID, credentials) - if err != nil { - return "", err - } - output, err = execCommand(cmdName, []string{ - "map", imagePath, "--id", userID, "-m", mon, "--key=" + key}) - if err != nil { - glog.Warningf("rbd: map error %v, rbd output: %s", err, string(output)) - return "", fmt.Errorf("rbd: map failed %v, rbd output: %s", err, string(output)) - } - devicePath, found = waitForPath(volOptions.Pool, image, 10, useNBD) - if !found { - return "", fmt.Errorf("Could not map image %s, Timeout after 10s", imagePath) - } + } + devicePath, err := createPath(volOptions, userID, credentials) + + return devicePath, err +} + +func createPath(volOpt *rbdVolume, userID string, creds map[string]string) (string, error) { + image := volOpt.VolName + imagePath := fmt.Sprintf("%s/%s", volOpt.Pool, image) + + mon, err := getMon(volOpt, creds) + if err != nil { + return "", err } + glog.V(5).Infof("rbd: map mon %s", mon) + key, err := getRBDKey(userID, creds) + if err != nil { + return "", err + } + + useNBD := false + cmdName := rbd + if volOpt.Mounter == rbdTonbd && hasNBD { + useNBD = true + cmdName = rbdTonbd + } + + output, err := execCommand(cmdName, []string{ + "map", imagePath, "--id", userID, "-m", mon, "--key=" + key}) + if err != nil { + glog.Warningf("rbd: map error %v, rbd output: %s", err, string(output)) + return "", fmt.Errorf("rbd: map failed %v, rbd output: %s", err, string(output)) + } + devicePath, found := waitForPath(volOpt.Pool, image, 10, useNBD) + if !found { + return "", fmt.Errorf("Could not map image %s, Timeout after 10s", imagePath) + } return devicePath, nil } diff --git a/pkg/rbd/rbd_util.go b/pkg/rbd/rbd_util.go index b2583f7a2..3e1b6f770 100644 --- a/pkg/rbd/rbd_util.go +++ b/pkg/rbd/rbd_util.go @@ -259,6 +259,12 @@ func getRBDVolumeOptions(volOptions map[string]string) (*rbdVolume, error) { } } + getCredsFromVol(rbdVol, volOptions) + return rbdVol, nil +} + +func getCredsFromVol(rbdVol *rbdVolume, volOptions map[string]string) { + var ok bool rbdVol.AdminID, ok = volOptions["adminid"] if !ok { rbdVol.AdminID = rbdDefaultAdminID @@ -271,9 +277,7 @@ func getRBDVolumeOptions(volOptions map[string]string) (*rbdVolume, error) { if !ok { rbdVol.Mounter = rbdDefaultMounter } - return rbdVol, nil } - func getRBDSnapshotOptions(snapOptions map[string]string) (*rbdSnapshot, error) { var ok bool rbdSnap := &rbdSnapshot{} diff --git a/pkg/util/nodecache.go b/pkg/util/nodecache.go index e32c77513..50a72c040 100644 --- a/pkg/util/nodecache.go +++ b/pkg/util/nodecache.go @@ -36,6 +36,8 @@ type NodeCache struct { var cacheDir = "controller" +var errDec = errors.New("file not found") + // EnsureCacheDirectory creates cache directory if not present func (nc *NodeCache) EnsureCacheDirectory(cacheDir string) error { fullPath := path.Join(nc.BasePath, cacheDir) @@ -58,36 +60,48 @@ func (nc *NodeCache) ForAll(pattern string, destObj interface{}, f ForAllFunc) e if err != nil { return errors.Wrapf(err, "node-cache: failed to read %s folder", nc.BasePath) } - + path := path.Join(nc.BasePath, cacheDir) for _, file := range files { - match, err := regexp.MatchString(pattern, file.Name()) - if err != nil || !match { + err = decodeObj(path, pattern, file, destObj) + if err == errDec { continue - } - if !strings.HasSuffix(file.Name(), ".json") { - continue - } - // #nosec - fp, err := os.Open(path.Join(nc.BasePath, cacheDir, file.Name())) - if err != nil { - glog.Infof("node-cache: open file: %s err %v", file.Name(), err) - continue - } - decoder := json.NewDecoder(fp) - if err = decoder.Decode(destObj); err != nil { - if err = fp.Close(); err != nil { - return errors.Wrapf(err, "failed to close file %s", file.Name()) - + } else if err == nil { + if err = f(strings.TrimSuffix(file.Name(), filepath.Ext(file.Name()))); err != nil { + return err } - return errors.Wrapf(err, "node-cache: couldn't decode file %s", file.Name()) - } - if err := f(strings.TrimSuffix(file.Name(), filepath.Ext(file.Name()))); err != nil { - return err } + return err + } return nil } +func decodeObj(filepath, pattern string, file os.FileInfo, destObj interface{}) error { + match, err := regexp.MatchString(pattern, file.Name()) + if err != nil || !match { + return errDec + } + if !strings.HasSuffix(file.Name(), ".json") { + return errDec + } + // #nosec + fp, err := os.Open(path.Join(filepath, file.Name())) + if err != nil { + glog.Infof("node-cache: open file: %s err %v", file.Name(), err) + return errDec + } + decoder := json.NewDecoder(fp) + if err = decoder.Decode(destObj); err != nil { + if err = fp.Close(); err != nil { + return errors.Wrapf(err, "failed to close file %s", file.Name()) + + } + return errors.Wrapf(err, "node-cache: couldn't decode file %s", file.Name()) + } + return nil + +} + // Create creates the metadata file in cache directory with identifier name func (nc *NodeCache) Create(identifier string, data interface{}) error { file := path.Join(nc.BasePath, cacheDir, identifier+".json") From 03d93219d7510c4c9ff578f3bb6c2f2a6d0ce612 Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Tue, 29 Jan 2019 11:37:03 +0530 Subject: [PATCH 09/11] Fix metalinter issue Signed-off-by: Madhu Rajanna --- pkg/rbd/rbd_attach.go | 5 ++--- pkg/util/cachepersister.go | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/rbd/rbd_attach.go b/pkg/rbd/rbd_attach.go index a93e7c379..45bc22792 100644 --- a/pkg/rbd/rbd_attach.go +++ b/pkg/rbd/rbd_attach.go @@ -237,7 +237,7 @@ func attachRBDImage(volOptions *rbdVolume, userID string, credentials map[string moduleName = nbd } - _, found := waitForPath(volOptions.Pool, image, 1, useNBD) + devicePath, found := waitForPath(volOptions.Pool, image, 1, useNBD) if !found { attachdetachMutex.LockKey(imagePath) @@ -263,9 +263,8 @@ func attachRBDImage(volOptions *rbdVolume, userID string, credentials map[string if err != nil { return "", err } - + devicePath, err = createPath(volOptions, userID, credentials) } - devicePath, err := createPath(volOptions, userID, credentials) return devicePath, err } diff --git a/pkg/util/cachepersister.go b/pkg/util/cachepersister.go index 3795cebf0..63a5c0f42 100644 --- a/pkg/util/cachepersister.go +++ b/pkg/util/cachepersister.go @@ -27,7 +27,7 @@ const ( PluginFolder = "/var/lib/kubelet/plugins" ) -// ForAllFunc stores metdata with identifier +// ForAllFunc stores metadata with identifier type ForAllFunc func(identifier string) error // CachePersister interface implemented for store From ec6cc5228364c01962068d1997929f755520c7b0 Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Tue, 29 Jan 2019 18:17:04 +0530 Subject: [PATCH 10/11] remove golint from travis.yml Signed-off-by: Madhu Rajanna --- .travis.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 489c10c0a..3b12f179e 100644 --- a/.travis.yml +++ b/.travis.yml @@ -14,7 +14,6 @@ install: before_script: - GO_FILES=$(find . -iname '*.go' -type f | grep -v /vendor/) - - go get -u golang.org/x/lint/golint #go get github.com/golang/lint/golint script: - gometalinter --deadline=10m -j 4 --enable=megacheck --enable=misspell --vendor ./... From 74796bd57bd9c511940a04e60b0e20d0a1734065 Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Tue, 29 Jan 2019 18:29:07 +0530 Subject: [PATCH 11/11] Fix review comment Signed-off-by: Madhu Rajanna --- pkg/rbd/controllerserver.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/rbd/controllerserver.go b/pkg/rbd/controllerserver.go index c69a6ccc4..334f2f3a0 100644 --- a/pkg/rbd/controllerserver.go +++ b/pkg/rbd/controllerserver.go @@ -155,7 +155,7 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol volSizeGB := int(rbdVol.VolSize / 1024 / 1024 / 1024) // Check if there is already RBD image with requested name - err = cs.checkrbdStatus(rbdVol, req, volSizeGB) + err = cs.checkRBDStatus(rbdVol, req, volSizeGB) if err != nil { return nil, err } @@ -178,7 +178,7 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol }, nil } -func (cs *ControllerServer) checkrbdStatus(rbdVol *rbdVolume, req *csi.CreateVolumeRequest, volSizeGB int) error { +func (cs *ControllerServer) checkRBDStatus(rbdVol *rbdVolume, req *csi.CreateVolumeRequest, volSizeGB int) error { var err error // Check if there is already RBD image with requested name found, _, _ := rbdStatus(rbdVol, rbdVol.UserID, req.GetSecrets()) // #nosec