Review changes

Author: Harvey Lowndes

cmd/oci/main.go

@@ -18,6 +18,9 @@ import (
 	"fmt"
 	"log"
 	"os"
+	"path"
+	"strings"
+	"sync"
 
 	"github.com/oracle/oci-flexvolume-driver/pkg/flexvolume"
 	"github.com/oracle/oci-flexvolume-driver/pkg/oci/driver"
@@ -27,6 +30,12 @@ import (
 var version string
 var build string
 
+// All registered drivers.
+var (
+	driversMutex sync.Mutex
+	drivers      = make(map[string]flexvolume.Driver)
+)
+
 // GetLogPath returns the default path to the driver log file.
 func GetLogPath() string {
 	path := os.Getenv("OCI_FLEXD_DRIVER_LOG_DIR")
@@ -51,9 +60,57 @@ func main() {
 
 	log.Printf("OCI FlexVolume Driver version: %s (%s)", version, build)
 
-	drivers := map[string]flexvolume.Driver{
-		"oci-bvs": &driver.OCIFlexvolumeDriver{},
+	RegisterDriver("oci-bvs", &driver.OCIFlexvolumeDriver{})
+
+	driver, err := getDriver(drivers)
+	if err != nil {
+		log.Fatalf(err.Error())
+	}
+
+	flexvolume.ExitWithResult(flexvolume.ExecDriver(driver, os.Args))
+}
+
+func getDriver(drivers map[string]flexvolume.Driver) (flexvolume.Driver, error) {
+	driver, err := GetRegisteredDriver(flexvolume.DefaultSymlinkDirectory) //Block volume is default
+	if err != nil {
+		return nil, err
 	}
 
-	flexvolume.ExecDriver(drivers, os.Args)
+	dir := path.Base(os.Args[0])
+	dir = string(strings.TrimPrefix(dir, "oracle~"))
+
+	if dir != "oci" && dir != flexvolume.DefaultSymlinkDirectory {
+		driver, err = GetRegisteredDriver(dir)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	log.Printf("Using %s driver", dir)
+
+	return driver, nil
+}
+
+// GetRegisteredDriver returns an instance of the named driver, or nil if
+// the name is unknown.
+func GetRegisteredDriver(name string) (flexvolume.Driver, error) {
+	driversMutex.Lock()
+	defer driversMutex.Unlock()
+	f, found := drivers[name]
+	if !found {
+		return nil, fmt.Errorf("No driver found for %s", name)
+	}
+	return f, nil
+}
+
+// RegisterDriver registers a flexvolume.Driver by name.  This
+// is expected to happen during app startup.
+func RegisterDriver(name string, driver flexvolume.Driver) {
+	driversMutex.Lock()
+	defer driversMutex.Unlock()
+	if _, found := drivers[name]; found {
+		log.Fatalf("Driver %q was registered twice", name)
+	}
+	log.Printf("Registered driver %q", name)
+	drivers[name] = driver
 }

pkg/flexvolume/flexvolume.go

@@ -20,8 +20,6 @@ import (
 	"io"
 	"log"
 	"os"
-	"path"
-	"strings"
 )
 
 // Defined to enable overriding in tests.
@@ -86,6 +84,26 @@ type Driver interface {
 	Unmount(mountDir string) DriverStatus
 }
 
+// ExitWithResult outputs the given Result and exits with the appropriate exit
+// code.
+func ExitWithResult(result DriverStatus) {
+	code := 1
+	if result.Status == StatusSuccess || result.Status == StatusNotSupported {
+		code = 0
+	}
+
+	res, err := json.Marshal(result)
+	if err != nil {
+		log.Printf("Error marshaling result: %v", err)
+		fmt.Fprintln(out, `{"status":"Failure","message":"Error marshaling result to JSON"}`)
+	} else {
+		s := string(res)
+		log.Printf("Command result: %s", s)
+		fmt.Fprintln(out, s)
+	}
+	exit(code)
+}
+
 // Fail creates a StatusFailure Result with a given message.
 func Fail(a ...interface{}) DriverStatus {
 	msg := fmt.Sprint(a...)
@@ -144,29 +162,13 @@ func processOpts(optsStr string) (Options, error) {
 
 // ExecDriver executes the appropriate FlexvolumeDriver command based on
 // recieved call-out.
-func ExecDriver(drivers map[string]Driver, args []string) DriverStatus {
+func ExecDriver(driver Driver, args []string) DriverStatus {
 	if len(args) < 2 {
 		return Failf("Expected at least one argument")
 	}
 
 	log.Printf("'%s %s' called with %s", args[0], args[1], args[2:])
 
-	driver := drivers[DefaultSymlinkDirectory] //Block volume is default
-
-	dir := path.Base(args[0])
-	dir = string(strings.TrimPrefix(dir, "oracle~"))
-
-	if dir != "oci" && dir != DefaultSymlinkDirectory {
-		driver = drivers[dir]
-	}
-
-	// Moved outside the above if to catch errors in code.
-	if driver == nil {
-		return Failf("No driver found for %s", dir)
-	}
-
-	log.Printf("Using %s driver", dir)
-
 	switch args[1] {
 	// <driver executable> init
 	case "init":

pkg/flexvolume/flexvolume_test.go

@@ -34,7 +34,8 @@ func assertFailure(t *testing.T, expected DriverStatus, status DriverStatus) {
 }
 
 func TestInit(t *testing.T) {
-	status := ExecDriver(map[string]Driver{"oci-bvs": mockFlexvolumeDriver{}},
+	status := ExecDriver(
+		mockFlexvolumeDriver{},
 		[]string{"oci", "init"})
 
 	expected := DriverStatus{Status: "Success"}
@@ -46,16 +47,19 @@ func TestInit(t *testing.T) {
 // StatusNotSupported as the call-out is broken as of the latest stable Kube
 // release (1.6.4).
 func TestGetVolumeName(t *testing.T) {
-	status := ExecDriver(map[string]Driver{"oci-bvs": mockFlexvolumeDriver{}},
-		[]string{"oci", "getvolumename", defaultTestOps})
+	status := ExecDriver(
+		mockFlexvolumeDriver{},
+		[]string{"oci", "getvolumename", defaultTestOps},
+	)
 
 	expected := DriverStatus{Status: "Not supported", Message: "getvolumename is broken as of kube 1.6.4"}
 
 	assertFailure(t, expected, status)
 }
 
 func TestNoVolumeIDDispatch(t *testing.T) {
-	status := ExecDriver(map[string]Driver{"oci-bvs": mockFlexvolumeDriver{}},
+	status := ExecDriver(
+		mockFlexvolumeDriver{},
 		[]string{"oci", "attach", noVolIDTestOps, "nodeName"})
 
 	expected := DriverStatus{
@@ -65,20 +69,10 @@ func TestNoVolumeIDDispatch(t *testing.T) {
 }
 
 func TestAttachUnsuported(t *testing.T) {
-	status := ExecDriver(map[string]Driver{"oci-bvs": mockFlexvolumeDriver{}},
+	status := ExecDriver(
+		mockFlexvolumeDriver{},
 		[]string{"oci", "attach", defaultTestOps, "nodeName"})
 
 	expected := DriverStatus{Status: "Not supported"}
 	assertFailure(t, expected, status)
 }
-
-func TestInvalidSymlink(t *testing.T) {
-	status := ExecDriver(map[string]Driver{"oci-bvs": mockFlexvolumeDriver{}},
-		[]string{"oci-abc", "init", defaultTestOps, "nodeName"})
-
-	expected := DriverStatus{
-		Status:  "Failure",
-		Message: "No driver found for oci-abc",
-	}
-	assertFailure(t, expected, status)
-}