Fix gometalinter issues

Signed-off-by: Madhu Rajanna <mrajanna@redhat.com>

.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

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())
 	}
-
+	return nil
-	glog.Infof("cephfs: successfully mounted volume %s to %s", volID, stagingTargetPath)
-
-	return &csi.NodeStageVolumeResponse{}, nil
 }
 
 // NodePublishVolume mounts the volume mounted to the staging path to the target

pkg/cephfs/volumeoptions.go

@@ -96,7 +96,6 @@ func validateMounter(m string) error {
 func newVolumeOptions(volOptions, secret map[string]string) (*volumeOptions, error) {
 	var (
 		opts volumeOptions
-		provisionVolumeBool string
 		err  error
 	)
 
@@ -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
+}

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 := parseVolCreateRequest(req)
-
-	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
+	volSizeGB := int(rbdVol.VolSize / 1024 / 1024 / 1024)
-	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)
 
 	// 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,
+			VolumeId:      rbdVol.VolID,
-			CapacityBytes: volSizeBytes,
+			CapacityBytes: rbdVol.VolSize,
 			VolumeContext: req.GetParameters(),
 		},
 	}, nil

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 err
-	return &csi.NodeUnpublishVolumeResponse{}, nil
 }
-
 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
 	}

pkg/rbd/rbd_attach.go

@@ -137,23 +137,34 @@ func getNbdDevFromImageAndPool(pool string, image string) (string, bool) {
 
 	for i := 0; i < maxNbds; i++ {
 		nbdPath := basePath + strconv.Itoa(i)
+		devicePath, err := getnbdDevicePath(nbdPath, imgPath, i)
+		if err != nil {
+			continue
+		}
+		return devicePath, true
+	}
+	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)
-			continue
+		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)
-			continue
+		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)
-			continue
+		return "", err
 	}
 	cmdlineArgs := strings.FieldsFunc(string(rawCmdline), func(r rune) bool {
 		return r == '\u0000'
@@ -163,21 +174,20 @@ func getNbdDevFromImageAndPool(pool string, image string) (string, bool) {
 	// 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
+		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])
-			continue
+		return "", err
 	}
-		devicePath := path.Join("/dev", "nbd"+strconv.Itoa(i))
+	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)
-			continue
+		return "", err
 	}
-		return devicePath, true
+	return devicePath, nil
-	}
-	return "", false
 }
 
 // Stat a path, if it doesn't exist, retry maxRetries times.
@@ -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)
+
+	}
+	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, credentials)
+	key, err := getRBDKey(userID, creds)
 	if err != nil {
 		return "", err
 	}
-		output, err = execCommand(cmdName, []string{
+
+	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(volOptions.Pool, image, 10, useNBD)
+	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
 }
 

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{}

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,20 +60,35 @@ 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 {
+		err = decodeObj(path, pattern, file, destObj)
+		if err == errDec {
+			continue
+		} else if err == nil {
+			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 {
-			continue
+		return errDec
 	}
 	if !strings.HasSuffix(file.Name(), ".json") {
-			continue
+		return errDec
 	}
 	// #nosec
-		fp, err := os.Open(path.Join(nc.BasePath, cacheDir, file.Name()))
+	fp, err := os.Open(path.Join(filepath, file.Name()))
 	if err != nil {
 		glog.Infof("node-cache: open file: %s err %v", file.Name(), err)
-			continue
+		return errDec
 	}
 	decoder := json.NewDecoder(fp)
 	if err = decoder.Decode(destObj); err != nil {
@@ -81,11 +98,8 @@ func (nc *NodeCache) ForAll(pattern string, destObj interface{}, f ForAllFunc) e
 		}
 		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 nil
+
 }
 
 // Create creates the metadata file in cache directory with identifier name