WIP: Support using symlinks to find correct driver #130

harveylowndes · 2018-06-25T08:57:28Z

Addresses oracle/oci-volume-provisioner#131

The volume provisioner has been patched to now not use names to dispatch custom logic. A storage class can now use it's provisioner property to determine what driver should be used. At the moment only block volume storage is supported provisioner: oracle.com/oci-bvs with aim that file storage will soon be supported also oracle/oci-volume-provisioner#90. Backwards compatible so that existing storage classes using provisioner: oracle.com/oci will default to using the block volume storage however this should be marked as deprecated and removed in a future release.

owainlewis · 2018-06-25T14:56:35Z

cmd/oci/main.go

+
+// RegisterDriver registers a flexvolume.Driver by name.  This
+// is expected to happen during app startup.
+func RegisterDriver(name string, driver flexvolume.Driver) {


Can this be private? RegisterDriver => registerDriver

owainlewis · 2018-06-25T14:57:09Z

cmd/oci/main.go

+
+// GetRegisteredDriver returns an instance of the named driver, or nil if
+// the name is unknown. An error is thrown if the named driver is not found.
+func GetRegisteredDriver(name string) (flexvolume.Driver, error) {


Make private? GetRegisteredDriver => getRegisteredDriver

owainlewis · 2018-06-26T15:45:20Z

pkg/flexvolume/flexvolume.go


 	// <driver executable> attach <json options> <node name>
 	case "attach":
 		if len(args) != 4 {
-			ExitWithResult(Fail("attach expected exactly 4 arguments; got ", args))
+			Failf("attach expected exactly 4 arguments; got ", args)


Should this be

Failf("attach expected exactly 4 arguments; got %d ", len(args))

owainlewis · 2018-06-26T15:48:02Z

cmd/oci/main.go

@@ -27,6 +30,12 @@ import (
 var version string
 var build string

+// All registered drivers.
+var (


Since we already know the drivers ahead of time it seems a complex way to do this. Why do we need to have a mutex and a mutable map of drivers? Could we not just create a static map and avoid this registration logic entirely?

drivers = map[string]flexvolume.Driver{ "oci-bvs": driver.OCIFlexvolume, } func getDriverDir(path string) string { dir := path.Base(path) return strings.TrimPrefix(dir, "oracle~") } driverDir := getDriverDir(os.Args[0]) driver, ok := drivers[driverDir] if !ok { return nil, fmt.Errorf("No driver found with name %s", name) }

owainlewis · 2018-06-26T15:53:51Z

cmd/oci/main.go

@@ -27,6 +30,12 @@ import (
 var version string
 var build string

+// All registered drivers.
+var (
+	driversMutex sync.Mutex


Since there are no concurrent threads of execution why do we need a mutex?

prydie

Some changes but overall great work 👍

prydie · 2018-07-03T11:16:41Z

cmd/oci/main.go

+	}
+
+	dir := path.Base(os.Args[0])
+	dir = string(strings.TrimPrefix(dir, "oracle~"))


Cast to string here is not required:

func TrimPrefix(s, prefix string) string

prydie · 2018-07-03T11:18:34Z

cmd/oci/main.go

+		return nil, err
+	}
+
+	dir := path.Base(os.Args[0])


driverName := strings.TrimPrefix(path.Base(os.Args[0]), "oracle~")

Additionally in the (very unlikely) event that len(os.Args) == 0 we should load the default driver to prevent a panic on os.Args[0].

prydie · 2018-07-03T11:56:13Z

pkg/flexvolume/flexvolume_test.go

-	defer func() { exit = osexit }()
-
-	ExecDriver(mockFlexvolumeDriver{}, []string{"oci", "attach", defaultTestOps, "nodeName"})
+	status := ExecDriver(


status := ExecDriver(mockFlexvolumeDriver{}, []string{"oci", "attach", defaultTestOps, "nodeName"})

prydie · 2018-07-03T11:56:51Z

pkg/flexvolume/flexvolume_test.go

-		t.Fatalf(`Expected '{"status":"Not supported","message":"getvolumename is broken as of kube 1.6.4"}}'; got %s`, out.(*bytes.Buffer).String())
-	}
+func TestNoVolumeIDDispatch(t *testing.T) {
+	status := ExecDriver(


status := ExecDriver(mockFlexvolumeDriver{}, []string{"oci", "attach", noVolIDTestOps, "nodeName"})

prydie · 2018-07-03T11:57:24Z

pkg/flexvolume/flexvolume_test.go

-	bak := out
-	out = new(bytes.Buffer)
-	defer func() { out = bak }()
+	status := ExecDriver(


status := ExecDriver(mockFlexvolumeDriver{}, []string{"oci", "getvolumename", defaultTestOps})

prydie · 2018-07-03T12:18:43Z

deploy.sh

 if [ -f "$CONFIG_FILE" ]; then
  cp  "$CONFIG_FILE"  "$driver_dir/$config_file_name"
 fi

 while : ; do
  touch $LOG_FILE
  tail -f $LOG_FILE
-done
+done


Missing trailing \n

prydie · 2018-07-03T12:20:29Z

pkg/flexvolume/flexvolume.go

@@ -67,6 +67,8 @@ const (
 	OptionKeyPodUID       = "kubernetes.io/pod.uid"

 	OptionKeyServiceAccountName = "kubernetes.io/serviceAccount.name"
+
+	DefaultSymlinkDirectory = "oci-bvs"


pkg/flexvolume is intended to be vendor agnostic and should not contain OCI specific code. Suggest moving to cmd/oci. Additionally, I'd suggest DefaultDriver would be a more appropriate name for this constant.

prydie · 2018-07-03T12:20:48Z

pkg/flexvolume/flexvolume.go

@@ -143,127 +162,127 @@ func processOpts(optsStr string) (Options, error) {

 // ExecDriver executes the appropriate FlexvolumeDriver command based on
 // recieved call-out.
-func ExecDriver(driver Driver, args []string) {
+func ExecDriver(driver Driver, args []string) DriverStatus {


prydie · 2018-07-03T12:31:33Z

cmd/oci/main.go

+}
+
+func getDriver(name string) (flexvolume.Driver, error) {
+	driver := drivers[name]


driver, ok := drivers[name] if !ok {

prydie · 2018-07-03T12:33:27Z

cmd/oci/main.go

+
+func getDriver(name string) (flexvolume.Driver, error) {
+	driver := drivers[name]
+	if driver == nil {


AFAIK this will only be true if drivers["foo"] is explicitly assigned to nil because nil is not the zero value for flexvolume.Driver (and drivers is map[string]flexvolume.Driver). Hence the need for checking ok.

owainlewis · 2018-07-04T13:42:24Z

test/integration/attach_nonexistent_volume_test.go

 )

 // TestAttachNonexistentVolume tests that attach fails for invalid volume OCIDs.
 func TestAttachNonexistentVolume(t *testing.T) {
-	d := &driver.OCIFlexvolumeDriver{}
+	d := &block.OCIFlexvolumeDriver{}


I'd prefer a constructor of some kind here to make code easy to read. Not necessary in this PR just a thought.

driver := block.New()

prydie

A few naming tweaks but otherwise LGTM 🎉

prydie · 2018-07-04T13:44:25Z

pkg/oci/driver/block/block_driver.go

+)
+
+// OCIFlexvolumeDriver implements the flexvolume.Driver interface for OCI.
+type OCIFlexvolumeDriver struct{}


s/OCIFlexvolumeDriver/Driver/ (as now will be consumed as block.Driver).

prydie · 2018-07-04T13:44:36Z

pkg/oci/driver/block/block_driver.go

+type OCIFlexvolumeDriver struct{}
+
+func init() {
+	driver.RegisterDriver("oci-bvs", &OCIFlexvolumeDriver{})


prydie · 2018-07-04T13:45:56Z

pkg/oci/driver/driver.go

-// OCIFlexvolumeDriver implements the flexvolume.Driver interface for OCI.
-type OCIFlexvolumeDriver struct{}
+// RegisterDriver adds a driver to the drivers map.
+func RegisterDriver(name string, driver flexvolume.Driver) {


s/RegisterDriver/Register/ (calling code will reference as driver.Register())

prydie · 2018-07-04T13:46:15Z

pkg/oci/driver/driver.go

+}
+
+// GetDriver returns a driver from the map associated with a given key.
+func GetDriver(name string) (flexvolume.Driver, error) {


s/GetDriver/Get/ (calling code will reference as driver.Get())

prydie · 2018-07-04T13:47:07Z

pkg/oci/driver/driver.go

+}
+
+// GetDriverName returns the name (map key) of the driver from a given path.
+func GetDriverName(args []string) string {


s/GetDriverName/NameFromArgs/ (calling code will reference as driver.NameFromArgs())

MadalinaPatrichi · 2018-08-17T13:01:23Z

Note: Only needed if FSS Dynamic Shared feature is implemented. There’s only one driver currently so it’s not needed. If we need to use the driver as part of the phase 2 FSS work we’ll need the above or another flex driver binary

harveylowndes requested review from owainlewis and prydie June 25, 2018 09:14

owainlewis self-assigned this Jun 25, 2018

owainlewis added the enhancement label Jun 25, 2018

owainlewis reviewed Jun 25, 2018

View reviewed changes

harveylowndes force-pushed the symlink-flexvol-driver-provisioner branch from e5663ef to 19346b2 Compare June 26, 2018 09:13

owainlewis reviewed Jun 26, 2018

View reviewed changes

harveylowndes force-pushed the symlink-flexvol-driver-provisioner branch from 9793dd6 to fc9d00b Compare June 27, 2018 09:30

prydie suggested changes Jul 3, 2018

View reviewed changes

Support using symlinks in storage class to find correct driver

98ada0a

harveylowndes force-pushed the symlink-flexvol-driver-provisioner branch from fc9d00b to 98ada0a Compare July 4, 2018 08:49

Review changes

605d7d5

harveylowndes force-pushed the symlink-flexvol-driver-provisioner branch from 845aba5 to 4b9e908 Compare July 4, 2018 13:32

owainlewis reviewed Jul 4, 2018

View reviewed changes

prydie suggested changes Jul 4, 2018

View reviewed changes

harveylowndes force-pushed the symlink-flexvol-driver-provisioner branch from 4b9e908 to 9f09609 Compare July 4, 2018 14:23

Restructure workspace and driver packages

78d66b3

harveylowndes force-pushed the symlink-flexvol-driver-provisioner branch from 9f09609 to 78d66b3 Compare July 4, 2018 14:40

harveylowndes changed the title ~~Support using symlinks to find correct driver~~ WIP: Support using symlinks to find correct driver Jul 6, 2018

owainlewis closed this Sep 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Support using symlinks to find correct driver #130

WIP: Support using symlinks to find correct driver #130

harveylowndes commented Jun 25, 2018

owainlewis Jun 25, 2018

owainlewis Jun 25, 2018

owainlewis Jun 26, 2018

owainlewis Jun 26, 2018 •

edited

Loading

owainlewis Jun 26, 2018

prydie left a comment

prydie Jul 3, 2018

prydie Jul 3, 2018

prydie Jul 3, 2018

prydie Jul 3, 2018

prydie Jul 3, 2018

prydie Jul 3, 2018

prydie Jul 3, 2018

prydie Jul 3, 2018

prydie Jul 3, 2018

prydie Jul 3, 2018

prydie Jul 3, 2018

owainlewis Jul 4, 2018 •

edited

Loading

prydie left a comment

prydie Jul 4, 2018

prydie Jul 4, 2018

prydie Jul 4, 2018

prydie Jul 4, 2018

prydie Jul 4, 2018

MadalinaPatrichi commented Aug 17, 2018

WIP: Support using symlinks to find correct driver #130

WIP: Support using symlinks to find correct driver #130

Conversation

harveylowndes commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owainlewis Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owainlewis Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

prydie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MadalinaPatrichi commented Aug 17, 2018

owainlewis Jun 26, 2018 •

edited

Loading

owainlewis Jul 4, 2018 •

edited

Loading